mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(electron): restrict loadBackupData IPC to the backup directory (#8206)
* test(e2e): retry supersync time-estimate dialog until input binds
The fill('10m') could fire its `input` event before the duration input's
value-accessor (an `input` HostListener) was wired, so the typed value
never reached the model and submit() saved an empty time — the panel
stayed "-/-" and toContainText('10m') timed out (scheduled run
27197862391, SuperSync 1/6). Retry the whole open→fill→submit cycle so a
fill that didn't stick is re-applied once the control binds.
* fix(electron): restrict loadBackupData IPC to the backup directory
The BACKUP_LOAD_DATA handler passed the renderer-supplied path straight
to readFileSync with no validation. Because plugin background scripts run
via `new Function` in the renderer, any installed plugin (or an XSS
payload in task content) could read arbitrary files through
window.ea.loadBackupData('/etc/shadow').
Constrain the path to BACKUP_DIR / BACKUP_DIR_WINSTORE via a new
isPathInsideDir guard. It uses path.relative (not a startsWith string
compare) so `..` traversal is collapsed and a name-prefixed sibling
directory (backups vs backups-evil) is not mistaken for a child. The only
legitimate caller already passes paths built from BACKUP_DIR, and
getBackupPath() is display-only, so backup restore is unaffected.
Adds electron/file-path-guard.test.cjs (node --test) covering traversal
escape, prefix-sibling, absolute-outside, and normalize-stays-inside.
Refs GHSA-x937-wf3j-88q3
* docs(electron): note lexical-only containment in file-path-guard
Clarify that isPathInsideDir does string-based containment (no fs.realpath),
so a symlink planted inside the dir is out of scope — it requires local
filesystem write access, outside this guard's renderer-input threat model.
Surfaced during multi-agent review of the GHSA-x937-wf3j-88q3 fix.
* test(electron): load guard via computed path for asar require check
The electron-smoke job runs tools/verify-electron-requires.js over the
packaged app.asar, which flags literal relative require() calls that can't
resolve in the package. The new guard test had require('./file-path-guard.ts'),
but .ts source is excluded from app.asar, so the check failed. Load the module
via a computed path (require(path.resolve(__dirname, ...))) — the same pattern
the other electron *.test.cjs files use; the scanner skips computed requires.
Also reworded the comment, which had reintroduced the literal pattern (the
scanner matches raw text, comments included).
This commit is contained in:
parent
4c869c2e15
commit
b729fd941c
3 changed files with 93 additions and 2 deletions
|
|
@ -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' });
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
50
electron/file-path-guard.test.cjs
Normal file
50
electron/file-path-guard.test.cjs
Normal file
|
|
@ -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);
|
||||
});
|
||||
27
electron/file-path-guard.ts
Normal file
27
electron/file-path-guard.ts
Normal file
|
|
@ -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);
|
||||
};
|
||||
Loading…
Add table
Add a link
Reference in a new issue