diff --git a/electron/local-file-sync.test.cjs b/electron/local-file-sync.test.cjs index fd2ad5a020..1883af8206 100644 --- a/electron/local-file-sync.test.cjs +++ b/electron/local-file-sync.test.cjs @@ -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']( diff --git a/electron/local-file-sync.ts b/electron/local-file-sync.ts index 0d648f0e01..e1e8d154c3 100644 --- a/electron/local-file-sync.ts +++ b/electron/local-file-sync.ts @@ -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 | null = null; + +const _loadSyncFolderFromDisk = async (): Promise => { + const all = await loadSimpleStoreAll(); + const raw = all[SYNC_FOLDER_KEY]; + return typeof raw === 'string' && raw.length > 0 ? raw : null; +}; + +const getSyncFolderPath = async (): Promise => { + 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 => { + // 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 => { @@ -124,8 +134,11 @@ export const initLocalFileSyncAdapter = (): void => { }, ): Promise => { 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 => { 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 => { 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 => { 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 => { - 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 => { 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 => { - 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; +}; diff --git a/electron/sync-folder-store.test.cjs b/electron/sync-folder-store.test.cjs deleted file mode 100644 index 60c37c10b1..0000000000 --- a/electron/sync-folder-store.test.cjs +++ /dev/null @@ -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'); -}); diff --git a/electron/sync-folder-store.ts b/electron/sync-folder-store.ts deleted file mode 100644 index 705541b54d..0000000000 --- a/electron/sync-folder-store.ts +++ /dev/null @@ -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 | null = null; - -const _loadIntoCache = async (): Promise => { - 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 => { - 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 => { - 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; -}; diff --git a/src/app/features/config/form-cfgs/sync-form.const.ts b/src/app/features/config/form-cfgs/sync-form.const.ts index 97ad1a4cdd..4c7a6ed51e 100644 --- a/src/app/features/config/form-cfgs/sync-form.const.ts +++ b/src/app/features/config/form-cfgs/sync-form.const.ts @@ -196,8 +196,10 @@ export const SYNC_FORM: ConfigFormSection = { }, }, { + // 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 = { return (localProvider as LocalFileSyncPicker | undefined)?.pickDirectory(); }, }, - expressions: { - 'props.required': (field: FormlyFieldConfig) => - field?.parent?.parent?.model?.syncProvider === SyncProviderId.LocalFile, - }, }, ], }, diff --git a/src/app/imex/sync/sync-config.service.ts b/src/app/imex/sync/sync-config.service.ts index 31075806d4..2d8c5d420b 100644 --- a/src/app/imex/sync/sync-config.service.ts +++ b/src/app/imex/sync/sync-config.service.ts @@ -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]: {