Mirrors/super-productivity
1
0
Fork 0
mirror of https://github.com/johannesjo/super-productivity.git synced 2026-07-17 16:37:43 +00:00
Code Issues Projects Releases Wiki Activity

refactor(electron): simplify sync folder ownership + stop renderer writes

Browse source 
Post-review of Phase 2 (#8228). The reviewer flagged one real bug and
several pieces of premature scaffolding.

Real bug — finding #1: the sync form was still writing the sync folder
path back into the renderer credential store. Phase 2 made the IPCs
take a relative path and main own the canonical root, but the Formly
button at sync-form.const.ts:200 had `key: 'syncFolderPath'`, which
caused FormlyBtn.onClick to setValue(picker-return-string) on the
form model. That flowed into privateCfg.syncFolderPath on save. The
"renderer no longer holds the authoritative copy" claim was true in
the IPC contract but untrue at runtime. A compromised renderer could
keep mutating its own privateCfg.syncFolderPath; nothing on master
reads it after Phase 2, but any future code path that does would
re-open the hole. Fix:
- drop `syncFolderPath` from PROVIDER_FIELD_DEFAULTS[LocalFile] so it
  is not in the LocalFile credential-store defaults at all
- drop `key: 'syncFolderPath'` from the LocalFile picker button so the
  picker's return value is not bound to the form model

KISS simplifications (a/b/d from review):
- delete electron/sync-folder-store.ts and its test; inline the
  load/cache/persist helpers at the top of local-file-sync.ts as
  ~25 LOC. The dedicated module was carrying an init/get/set ceremony
  and a Promise singleton for a single string that is read at most a
  few times per second in the worst case. simple-store is the durable
  layer either way.
- drop the `_resolveOrThrow` typeof precheck; resolveSyncPath already
  type-checks `relativePath` and throws the same opaque error
- inline `resolveSyncPath(...).absolutePath` into each handler; the
  helper was the third layer doing the same thing

Other findings addressed:
- finding #2 (PICK_DIRECTORY silently undefined on persist failure):
  PICK_DIRECTORY now returns a safe Error via createSafeIpcError when
  realpath/persist fails, distinct from `undefined` for user-cancel.
  New tests cover both branches plus the "cache must not be poisoned
  with a non-existent path" invariant.
- finding #4 (set-without-canonicalize): the inlined
  setSyncFolderPath now `realpathSync.native`s the picked path before
  persisting, so a relative or symlinked picker result is rejected
  early and the on-disk form matches what resolveSyncPath compares
  against on read.
- finding #5 (fire-and-forget init that was also being awaited per
  handler): dropped the fire-and-forget; handlers just `await
  getSyncFolderPath()`, which lazy-loads from disk on first call and
  caches.

All 52 electron security tests still pass; lint clean; tsc clean.
This commit is contained in:
johannesjo 2026-06-09 23:15:53 +02:00
parent 0c21649fde
commit 00e71b407f
6 changed files with 133 additions and 294 deletions
Download patch file Download diff file Expand all files Collapse all files

74
electron/local-file-sync.test.cjs
View file

@ -2,18 +2,9 @@
* Security tests for the local file-sync IPC handlers.
*
* Post-issue-#8228 the handlers only accept a `relativePath`; the renderer
* never supplies an absolute path. Main owns the sync folder via
* `sync-folder-store` and resolves the relative path against it. These tests
* cover:
*
* - happy path inside a user-chosen sync folder
* - relative-path traversal (`..`) escape attempts
* - sync folder pointed at userData (must be denied at config time so the
* renderer cannot forge a grant file via the sync API)
* - absent sync folder
* - unchanged image-inlining and to-file-url guards
*
* Run via the standard electron .test.cjs runner.
* never supplies an absolute path. The sync folder is owned and persisted
* main-side (the cache lives at the top of `local-file-sync.ts`) and the
* relative path is resolved against it via `sync-path-resolver`.
*/
const test = require('node:test');
const assert = require('node:assert/strict');
@ -26,22 +17,22 @@ require('ts-node/register/transpile-only');
const modPath = path.resolve(__dirname, 'local-file-sync.ts');
const simpleStorePath = path.resolve(__dirname, 'simple-store.ts');
const syncFolderStorePath = path.resolve(__dirname, 'sync-folder-store.ts');
const syncPathResolverPath = path.resolve(__dirname, 'sync-path-resolver.ts');
const originalModuleLoad = Module._load;
let handlers = {};
let userDataDir;
let externalDir;
let nextDialogResult = { canceled: true, filePaths: [] };
const installMocks = (overrides = {}) => {
const installMocks = () => {
Module._load = function patchedLoad(request, parent, isMain) {
if (request === 'electron') {
return {
ipcMain: { handle: (channel, fn) => (handlers[channel] = fn) },
app: { getPath: () => userDataDir },
dialog: overrides.dialog || {
showOpenDialog: async () => ({ canceled: true, filePaths: [] }),
dialog: {
showOpenDialog: async () => nextDialogResult,
},
};
}
@ -58,7 +49,6 @@ const installMocks = (overrides = {}) => {
const resetCaches = () => {
delete require.cache[modPath];
delete require.cache[simpleStorePath];
delete require.cache[syncFolderStorePath];
delete require.cache[syncPathResolverPath];
handlers = {};
};
@ -69,17 +59,17 @@ const load = () => {
};
const configureSyncFolder = async (folder) => {
const store = require(syncFolderStorePath);
await store.setSyncFolderPath(folder);
// Drive the real PICK_DIRECTORY path so the test exercises the same
// canonicalize-and-persist code production uses.
nextDialogResult = { canceled: false, filePaths: [folder] };
const result = await handlers['PICK_DIRECTORY']({});
if (result instanceof Error) throw result;
return result;
};
test.beforeEach(() => {
userDataDir = fs.realpathSync.native(
fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ud-')),
);
externalDir = fs.realpathSync.native(
fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ext-')),
);
userDataDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ud-')));
externalDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ext-')));
installMocks();
load();
});
@ -225,25 +215,35 @@ test('GET_SYNC_FOLDER_PATH returns null when unconfigured, then the configured p
});
test('PICK_DIRECTORY persists the selection main-side', async () => {
// Re-install mocks with a non-cancelling dialog.
Module._load = originalModuleLoad;
const picked = externalDir;
installMocks({
dialog: {
showOpenDialog: async () => ({ canceled: false, filePaths: [picked] }),
},
});
load();
nextDialogResult = { canceled: false, filePaths: [externalDir] };
const returned = await handlers['PICK_DIRECTORY']({});
assert.equal(returned, picked);
assert.equal(returned, externalDir);
assert.equal(
await handlers['GET_SYNC_FOLDER_PATH']({}),
picked,
externalDir,
'main-side store is updated; renderer does not have to echo back',
);
});
test('PICK_DIRECTORY surfaces a safe error when persistence fails', async () => {
// Picker returns a path that no longer exists on disk → realpath throws
// → PICK_DIRECTORY must return a distinct Error, not undefined, so the
// renderer doesn't confuse persist-failure with user-cancel.
const gone = fs.mkdtempSync(path.join(os.tmpdir(), 'sp-gone-'));
fs.rmSync(gone, { recursive: true, force: true });
nextDialogResult = { canceled: false, filePaths: [gone] };
const result = await handlers['PICK_DIRECTORY']({});
assert.ok(result instanceof Error);
// Cache must not be poisoned with a non-existent path.
assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), null);
});
test('PICK_DIRECTORY returns undefined on user-cancel (distinct from error)', async () => {
nextDialogResult = { canceled: true, filePaths: [] };
const result = await handlers['PICK_DIRECTORY']({});
assert.equal(result, undefined);
});
test('READ_LOCAL_IMAGE_AS_DATA_URL refuses an image inside userData', async () => {
fs.writeFileSync(path.join(userDataDir, 'secret.png'), 'not-really-png');
const result = await handlers['READ_LOCAL_IMAGE_AS_DATA_URL'](

152
electron/local-file-sync.ts
View file

@ -2,6 +2,7 @@ import { IPC } from './shared-with-frontend/ipc-events.const';
import {
readdirSync,
readFileSync,
realpathSync,
renameSync,
statSync,
writeFileSync,
@ -13,11 +14,7 @@ import { getWin } from './main-window';
import { fileURLToPath, pathToFileURL } from 'url';
import { assertPathOutside } from './file-path-guard';
import { resolveSyncPath } from './sync-path-resolver';
import {
getSyncFolderPath,
initSyncFolderStore,
setSyncFolderPath,
} from './sync-folder-store';
import { loadSimpleStoreAll, saveSimpleStore } from './simple-store';
// SECURITY: file-sync must never read/write/list inside the app's private dir,
// which holds settings/grants/db — touching it is a privilege-escalation
@ -26,29 +23,42 @@ import {
// changed at startup (--user-data-dir, Snap). See file-path-guard.ts.
const getAppPrivateDir = (): string => app.getPath('userData');
// Resolve a renderer-supplied relativePath against the main-owned sync folder.
// Throws a generic, path-free error so the offending input never round-trips
// to the renderer via createSafeIpcError.
const _resolveOrThrow = (relativePath: unknown): string => {
if (typeof relativePath !== 'string') {
const e = new Error('Path not allowed for the sync folder');
e.name = 'PathNotAllowedError';
delete (e as { stack?: string }).stack;
throw e;
// Main-owned sync folder path. Backed by simple-store under SYNC_FOLDER_KEY;
// cached in-memory after first load so each FS IPC doesn't re-read the file.
// The canonical form is stored so subsequent realpath comparisons in
// resolveSyncPath line up regardless of symlink/case-fold drift.
const SYNC_FOLDER_KEY = 'syncFolderPath';
let _cachedSyncFolder: string | null | undefined = undefined;
let _loadPromise: Promise<string | null> | null = null;
const _loadSyncFolderFromDisk = async (): Promise<string | null> => {
const all = await loadSimpleStoreAll();
const raw = all[SYNC_FOLDER_KEY];
return typeof raw === 'string' && raw.length > 0 ? raw : null;
};
const getSyncFolderPath = async (): Promise<string | null> => {
if (_cachedSyncFolder !== undefined) return _cachedSyncFolder;
if (!_loadPromise) {
_loadPromise = _loadSyncFolderFromDisk().then((v) => {
_cachedSyncFolder = v;
return v;
});
}
return resolveSyncPath(
getSyncFolderPath() ?? undefined,
relativePath,
getAppPrivateDir(),
).absolutePath;
return _loadPromise;
};
const setSyncFolderPath = async (rawPath: string): Promise<string> => {
// Canonicalize at write time so the persisted value is the form
// resolveSyncPath will compare against, and so a relative or
// symlinked path is rejected before it gets stored.
const canonical = realpathSync.native(rawPath);
await saveSimpleStore(SYNC_FOLDER_KEY, canonical);
_cachedSyncFolder = canonical;
return canonical;
};
export const initLocalFileSyncAdapter = (): void => {
// Fire-and-forget: subsequent IPC handlers also `await initSyncFolderStore()`
// defensively (it is idempotent via _loadOnce), so we don't need to gate
// handler registration on this promise resolving.
void initSyncFolderStore();
ipcMain.handle(
IPC.READ_LOCAL_IMAGE_AS_DATA_URL,
async (_, filePathOrUrl: string): Promise<string | null> => {
@ -124,8 +134,11 @@ export const initLocalFileSyncAdapter = (): void => {
},
): Promise<string | Error> => {
try {
await initSyncFolderStore();
const filePath = _resolveOrThrow(relativePath);
const filePath = resolveSyncPath(
(await getSyncFolderPath()) ?? undefined,
relativePath,
getAppPrivateDir(),
).absolutePath;
log(IPC.FILE_SYNC_SAVE, {
dataLength: dataStr.length,
hasData: dataStr.length > 0,
@ -159,8 +172,11 @@ export const initLocalFileSyncAdapter = (): void => {
},
): Promise<{ rev: string; dataStr: string | undefined } | Error> => {
try {
await initSyncFolderStore();
const filePath = _resolveOrThrow(relativePath);
const filePath = resolveSyncPath(
(await getSyncFolderPath()) ?? undefined,
relativePath,
getAppPrivateDir(),
).absolutePath;
log(IPC.FILE_SYNC_LOAD, {
hasLocalRev: !!localRev,
});
@ -190,8 +206,11 @@ export const initLocalFileSyncAdapter = (): void => {
},
): Promise<void | Error> => {
try {
await initSyncFolderStore();
const filePath = _resolveOrThrow(relativePath);
const filePath = resolveSyncPath(
(await getSyncFolderPath()) ?? undefined,
relativePath,
getAppPrivateDir(),
).absolutePath;
log(IPC.FILE_SYNC_REMOVE);
unlinkSync(filePath);
return;
@ -213,9 +232,13 @@ export const initLocalFileSyncAdapter = (): void => {
},
): Promise<true | Error> => {
try {
await initSyncFolderStore();
// Default to operating on the sync root itself (relativePath = '').
const dirPath = _resolveOrThrow(relativePath ?? '');
// Default to the sync root itself (relativePath = '') so the legacy
// "is the configured sync folder reachable?" check still works.
const dirPath = resolveSyncPath(
(await getSyncFolderPath()) ?? undefined,
relativePath ?? '',
getAppPrivateDir(),
).absolutePath;
const dirEntries = readdirSync(dirPath);
log(IPC.CHECK_DIR_EXISTS, {
dirEntryCount: dirEntries.length,
@ -244,8 +267,11 @@ export const initLocalFileSyncAdapter = (): void => {
},
): Promise<string[] | Error> => {
try {
await initSyncFolderStore();
const dirPath = _resolveOrThrow(relativePath ?? '');
const dirPath = resolveSyncPath(
(await getSyncFolderPath()) ?? undefined,
relativePath ?? '',
getAppPrivateDir(),
).absolutePath;
return readdirSync(dirPath);
} catch (e) {
error('Local file sync list files failed', getSafeErrorMeta(e));
@ -259,39 +285,35 @@ export const initLocalFileSyncAdapter = (): void => {
},
);
ipcMain.handle(IPC.PICK_DIRECTORY, async (): Promise<string | undefined> => {
const { canceled, filePaths } = (await dialog.showOpenDialog(getWin(), {
title: 'Select sync folder',
buttonLabel: 'Select Folder',
properties: [
'openDirectory',
'createDirectory',
'promptToCreate',
'dontAddToRecent',
],
})) as unknown as { canceled: boolean; filePaths: string[] };
if (canceled) {
return undefined;
}
const selected = filePaths[0];
if (!selected) {
return undefined;
}
// Persist main-side immediately so the renderer never holds the
// authoritative copy of the sync folder path. The renderer may still
// receive the path for display, but a subsequent IPC will look up the
// stored value rather than trusting whatever the renderer echoes back.
ipcMain.handle(IPC.PICK_DIRECTORY, async (): Promise<string | Error | undefined> => {
try {
await setSyncFolderPath(selected);
const { canceled, filePaths } = (await dialog.showOpenDialog(getWin(), {
title: 'Select sync folder',
buttonLabel: 'Select Folder',
properties: [
'openDirectory',
'createDirectory',
'promptToCreate',
'dontAddToRecent',
],
})) as unknown as { canceled: boolean; filePaths: string[] };
if (canceled || !filePaths[0]) {
return undefined;
}
// Persist main-side BEFORE returning the display string. If
// canonicalization or persistence fails (deleted between pick and
// commit, EACCES on userData, etc.), surface a safe error rather than
// a silent undefined — otherwise the renderer cannot distinguish
// failure from user-cancel and the user is left wondering why their
// pick didn't take.
return await setSyncFolderPath(filePaths[0]);
} catch (e) {
error('Failed to persist sync folder selection', getSafeErrorMeta(e));
return undefined;
error('PICK_DIRECTORY failed to persist sync folder', getSafeErrorMeta(e));
return createSafeIpcError(IPC.PICK_DIRECTORY, e);
}
return selected;
});
ipcMain.handle(IPC.GET_SYNC_FOLDER_PATH, async (): Promise<string | null> => {
await initSyncFolderStore();
return getSyncFolderPath();
});
@ -364,3 +386,9 @@ const createSafeIpcError = (operation: IPC, e: unknown): Error => {
return safeError;
};
/** Test-only: clear the in-memory cache so the next read re-loads from disk. */
export const __resetSyncFolderCacheForTests = (): void => {
_cachedSyncFolder = undefined;
_loadPromise = null;
};

102
electron/sync-folder-store.test.cjs
View file

@ -1,102 +0,0 @@
const test = require('node:test');
const assert = require('node:assert/strict');
const os = require('node:os');
const path = require('node:path');
const { promises: fs } = require('node:fs');
const Module = require('node:module');
require('ts-node/register/transpile-only');
const originalModuleLoad = Module._load;
let userDataDir;
const simpleStoreModulePath = path.resolve(__dirname, 'simple-store.ts');
const syncFolderStoreModulePath = path.resolve(__dirname, 'sync-folder-store.ts');
const installMocks = () => {
Module._load = function patchedLoad(request, parent, isMain) {
if (request === 'electron') {
return { app: { getPath: () => userDataDir } };
}
if (request === 'electron-log/main') {
return { log: () => {}, error: () => {} };
}
return originalModuleLoad.call(this, request, parent, isMain);
};
};
const resetModules = () => {
delete require.cache[simpleStoreModulePath];
delete require.cache[syncFolderStoreModulePath];
};
const loadStore = () => {
resetModules();
return require(syncFolderStoreModulePath);
};
test.beforeEach(async () => {
userDataDir = await fs.mkdtemp(path.join(os.tmpdir(), 'sp-sync-folder-store-'));
installMocks();
});
test.afterEach(async () => {
Module._load = originalModuleLoad;
resetModules();
await fs.rm(userDataDir, { recursive: true, force: true });
});
test('getSyncFolderPath returns null on first init (no persisted value)', async () => {
const store = loadStore();
await store.initSyncFolderStore();
assert.equal(store.getSyncFolderPath(), null);
});
test('setSyncFolderPath persists and is readable across reloads', async () => {
const store = loadStore();
await store.initSyncFolderStore();
await store.setSyncFolderPath('/Users/me/sync');
assert.equal(store.getSyncFolderPath(), '/Users/me/sync');
store.__resetSyncFolderCacheForTests();
await store.initSyncFolderStore();
assert.equal(store.getSyncFolderPath(), '/Users/me/sync');
});
test('setSyncFolderPath(null) clears the persisted value', async () => {
const store = loadStore();
await store.initSyncFolderStore();
await store.setSyncFolderPath('/Users/me/sync');
await store.setSyncFolderPath(null);
assert.equal(store.getSyncFolderPath(), null);
store.__resetSyncFolderCacheForTests();
await store.initSyncFolderStore();
assert.equal(store.getSyncFolderPath(), null);
});
test('setSyncFolderPath(empty string) is treated as null', async () => {
const store = loadStore();
await store.initSyncFolderStore();
await store.setSyncFolderPath('/some/path');
await store.setSyncFolderPath('');
assert.equal(store.getSyncFolderPath(), null);
});
test('getSyncFolderPath throws when called before init', async () => {
const store = loadStore();
assert.throws(() => store.getSyncFolderPath(), /initSyncFolderStore/);
});
test('setSyncFolderPath is idempotent (no persist if value unchanged)', async () => {
// Indirectly verified: a no-op set must not corrupt cache state on the
// next read. (We can't observe the file write count without monkey-patching
// simple-store; this guards against the regression where idempotence
// accidentally inverts the cache.)
const store = loadStore();
await store.initSyncFolderStore();
await store.setSyncFolderPath('/x');
await store.setSyncFolderPath('/x');
await store.setSyncFolderPath('/x');
assert.equal(store.getSyncFolderPath(), '/x');
});

87
electron/sync-folder-store.ts
View file

@ -1,87 +0,0 @@
import { loadSimpleStoreAll, saveSimpleStore } from './simple-store';
/**
* Main-owned, persisted location of the user's sync folder.
*
* Background (issue #8228): before this store, the renderer held the sync
* folder path in its own IndexedDB (`privateCfg.syncFolderPath`) and echoed
* it back as an absolute path on every FILE_SYNC_* IPC. That meant a
* compromised renderer could swap the path for any other absolute path
* outside `userData` and get unrestricted read/write. With the path owned
* by main, the renderer only ever sends the *relative* part; main resolves
* against the path it stored itself.
*
* Persistence piggybacks on `simple-store` so we get the same durability
* mechanics (atomic write, corruption quarantine, EISDIR recovery, queue).
* An in-memory cache backs the hot path so per-IPC lookup does not stat
* the simpleSettings file every call.
*/
const SIMPLE_STORE_KEY = 'syncFolderPath';
interface CacheState {
value: string | null;
}
let _cache: CacheState | null = null;
let _loadOnce: Promise<void> | null = null;
const _loadIntoCache = async (): Promise<void> => {
if (_loadOnce) return _loadOnce;
_loadOnce = (async () => {
const all = await loadSimpleStoreAll();
const raw = all[SIMPLE_STORE_KEY];
_cache = { value: typeof raw === 'string' && raw.length > 0 ? raw : null };
})();
return _loadOnce;
};
/**
* Load the persisted value into the in-memory cache. MUST be called once at
* app startup before any IPC handler reads via {@link getSyncFolderPath}.
*/
export const initSyncFolderStore = async (): Promise<void> => {
await _loadIntoCache();
};
/**
* Synchronously return the current sync folder path, or null if none is set.
*
* Callers must have invoked {@link initSyncFolderStore} during app startup.
* Throws — rather than silently returning null — so a missed startup wiring
* is loud during development, not a silent permissive-by-default surprise
* in production.
*/
export const getSyncFolderPath = (): string | null => {
if (!_cache) {
throw new Error(
'sync-folder-store: initSyncFolderStore() must be called before getSyncFolderPath()',
);
}
return _cache.value;
};
/**
* Persist a new sync folder path. Pass null to clear.
*
* The caller is responsible for having validated that `value` is a path the
* user explicitly approved (typically the result of a main-process file
* dialog). This module trusts its input — it is only reachable from main
* code.
*/
export const setSyncFolderPath = async (value: string | null): Promise<void> => {
await _loadIntoCache();
if (!_cache) {
throw new Error('sync-folder-store: cache failed to initialize');
}
const next = typeof value === 'string' && value.length > 0 ? value : null;
if (_cache.value === next) return;
_cache.value = next;
await saveSimpleStore(SIMPLE_STORE_KEY, next);
};
/** Test-only: drop the in-memory cache so a fresh init re-reads disk. */
export const __resetSyncFolderCacheForTests = (): void => {
_cache = null;
_loadOnce = null;
};

8
src/app/features/config/form-cfgs/sync-form.const.ts
View file

@ -196,8 +196,10 @@ export const SYNC_FORM: ConfigFormSection<SyncConfig> = {
},
},
{
// No `key` on purpose: post-#8228 the sync folder path is owned
// main-side. The picker's return value is for display only and
// must not write back into the renderer credential store.
type: 'btn',
key: 'syncFolderPath',
templateOptions: {
text: T.F.SYNC.FORM.LOCAL_FILE.L_SYNC_FOLDER_PATH,
btnStyle: 'stroked',
@ -209,10 +211,6 @@ export const SYNC_FORM: ConfigFormSection<SyncConfig> = {
return (localProvider as LocalFileSyncPicker | undefined)?.pickDirectory();
},
},
expressions: {
'props.required': (field: FormlyFieldConfig) =>
field?.parent?.parent?.model?.syncProvider === SyncProviderId.LocalFile,
},
},
],
},

4
src/app/imex/sync/sync-config.service.ts
View file

@ -94,7 +94,9 @@ const PROVIDER_FIELD_DEFAULTS: Record<
encryptKey: '',
},
[SyncProviderId.LocalFile]: {
syncFolderPath: '',
// syncFolderPath is intentionally omitted: post-#8228 the sync folder
// path is owned main-side (electron/local-file-sync.ts) so a compromised
// renderer cannot rewrite it via the credential store.
encryptKey: '',
},
[SyncProviderId.Dropbox]: {