diff --git a/electron/backup.ts b/electron/backup.ts index 3251eeb50d..4a27a74169 100644 --- a/electron/backup.ts +++ b/electron/backup.ts @@ -15,6 +15,7 @@ import { error, log } from 'electron-log/main'; import type { AppDataCompleteLegacy } from '../src/app/imex/sync/sync.model'; import type { AppDataComplete } from '../src/app/op-log/model/model-config'; import { getBackupTimestamp } from './shared-with-frontend/get-backup-timestamp'; +import { isPathInsideDir } from './file-path-guard'; import { DEFAULT_MAX_BACKUP_FILES, selectBackupFilesToDelete, @@ -63,8 +64,21 @@ export function initBackupAdapter(): void { // RESTORE_BACKUP ipcMain.handle(IPC.BACKUP_LOAD_DATA, (ev, backupPath: string): string => { - log('Reading backup file: ', backupPath); - return readFileSync(backupPath, { encoding: 'utf8' }); + // `backupPath` comes from the renderer, which runs untrusted plugin code, + // so it must be constrained to the backup directory. Otherwise any plugin + // (or XSS payload) could read arbitrary files via window.ea.loadBackupData. + // See GHSA-x937-wf3j-88q3. Both the regular and the Windows-Store backup + // dirs are accepted; the legitimate caller only ever passes paths built + // from BACKUP_DIR (see IPC.BACKUP_IS_AVAILABLE above). + if ( + !isPathInsideDir(BACKUP_DIR, backupPath) && + !isPathInsideDir(BACKUP_DIR_WINSTORE, backupPath) + ) { + throw new Error('BACKUP_LOAD_DATA: refused path outside backup directory'); + } + const resolved = path.resolve(backupPath); + log('Reading backup file: ', resolved); + return readFileSync(resolved, { encoding: 'utf8' }); }); } diff --git a/electron/file-path-guard.test.cjs b/electron/file-path-guard.test.cjs new file mode 100644 index 0000000000..331dc40cfd --- /dev/null +++ b/electron/file-path-guard.test.cjs @@ -0,0 +1,50 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +require('ts-node/register/transpile-only'); + +// Resolve the module via a computed path rather than a literal relative +// require of the .ts file. The .ts source is excluded from the packaged +// app.asar, and tools/verify-electron-requires.js flags literal relative +// requires that cannot resolve in the package (it scans raw text). A computed +// require is skipped by that static check and matches the pattern the other +// electron *.test.cjs files use. The test still runs from source via ts-node. +const { isPathInsideDir } = require(path.resolve(__dirname, 'file-path-guard.ts')); + +const DIR = path.resolve('/home/user/.config/superProductivity/backups'); + +test('accepts a file directly inside the directory', () => { + assert.equal(isPathInsideDir(DIR, path.join(DIR, '2026-01-01.json')), true); +}); + +test('accepts a file in a nested subdirectory', () => { + assert.equal(isPathInsideDir(DIR, path.join(DIR, 'sub', 'a.json')), true); +}); + +test('collapses traversal that escapes the directory', () => { + assert.equal(isPathInsideDir(DIR, path.join(DIR, '..', '..', 'secret.txt')), false); +}); + +test('rejects an absolute path outside the directory', () => { + assert.equal(isPathInsideDir(DIR, '/etc/passwd'), false); +}); + +test('rejects a sibling directory that shares a name prefix', () => { + // `backups-evil` must not be treated as inside `backups`. + assert.equal(isPathInsideDir(DIR, DIR + '-evil/x.json'), false); +}); + +test('rejects the directory itself (no file to read)', () => { + assert.equal(isPathInsideDir(DIR, DIR), false); +}); + +test('rejects empty / non-string input', () => { + assert.equal(isPathInsideDir(DIR, ''), false); + assert.equal(isPathInsideDir(DIR, undefined), false); + assert.equal(isPathInsideDir(DIR, null), false); +}); + +test('accepts a path that needs normalization but stays inside', () => { + assert.equal(isPathInsideDir(DIR, path.join(DIR, 'sub', '..', 'a.json')), true); +}); diff --git a/electron/file-path-guard.ts b/electron/file-path-guard.ts new file mode 100644 index 0000000000..f1fe132ee7 --- /dev/null +++ b/electron/file-path-guard.ts @@ -0,0 +1,27 @@ +import * as path from 'path'; + +/** + * Returns true if `targetPath` resolves to a location strictly inside `dir`. + * + * Security boundary: several IPC handlers receive file paths from the renderer, + * which executes untrusted plugin code (plugin scripts run via `new Function` + * in `src/app/plugins/plugin-runner.ts`). Without constraining the path, a + * plugin could read/write arbitrary files via the exposed `window.ea` bridge. + * See GHSA-x937-wf3j-88q3. + * + * `path.relative` is used instead of a `startsWith` string compare so that + * `..` traversal is collapsed and a sibling directory sharing a name prefix + * (e.g. `backups` vs `backups-evil`) is not mistaken for a child. + * + * Containment is purely lexical (no `fs.realpath`): a symlink planted *inside* + * `dir` pointing outside would pass. That requires pre-existing local + * filesystem write access, which is outside the threat model here (untrusted + * renderer/plugin-supplied path strings), so symlink resolution is omitted. + */ +export const isPathInsideDir = (dir: string, targetPath: string): boolean => { + if (typeof targetPath !== 'string' || targetPath.length === 0) { + return false; + } + const rel = path.relative(path.resolve(dir), path.resolve(targetPath)); + return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); +};