diff --git a/packages/core/src/agents/workflow-snapshot.test.ts b/packages/core/src/agents/workflow-snapshot.test.ts index 9a8472f329f..212a5ef7183 100644 --- a/packages/core/src/agents/workflow-snapshot.test.ts +++ b/packages/core/src/agents/workflow-snapshot.test.ts @@ -139,7 +139,10 @@ describe('writeWorkflowSnapshot + listWorkflowSnapshots', () => { await fs.mkdir(`${dir}/${runId}`, { recursive: true }); await fs.writeFile(`${dir}/${runId}/journal.jsonl`, '{}\n', 'utf8'); // Distinct runId per write; startTime ascending. Each write prunes. - await writeWorkflowSnapshot(config, task({ runId, startTime: 1_000 + i })); + await writeWorkflowSnapshot( + config, + task({ runId, startTime: 1_000 + i }), + ); } const entries = await fs.readdir(dir); const files = entries.filter((f) => f.endsWith('.json')); @@ -148,4 +151,48 @@ describe('writeWorkflowSnapshot + listWorkflowSnapshots', () => { // The pruned runs' journal directories are gone too (no orphan leak). expect(journalDirs.length).toBe(MAX_RETAINED_SNAPSHOTS); }); + + // Security: prune derives `runId` from the snapshot filename and feeds it to + // a recursive `fs.rm`. A crafted `.json` name must NOT let that delete + // anything but a well-formed `wf_` run dir — a file named `...json` + // yields `runId = ".."` (parent dir), `notarun.json` yields a sibling dir. + it('does not recursively delete via a crafted snapshot filename (path traversal)', async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + await fs.mkdir(dir, { recursive: true }); + + // Canary in the runs dir's PARENT — a `..` traversal would delete it. + const canary = path.join(dir, '..', 'CANARY.txt'); + await fs.writeFile(canary, 'keep', 'utf8'); + // A non-run sibling dir INSIDE the runs dir — a `notarun.json` stem targets it. + await fs.mkdir(path.join(dir, 'notarun'), { recursive: true }); + await fs.writeFile(path.join(dir, 'notarun', 'keep.txt'), 'keep', 'utf8'); + + // Fill to the cap with legit run snapshots (no prune yet at == cap). + for (let i = 0; i < MAX_RETAINED_SNAPSHOTS; i++) { + await writeWorkflowSnapshot( + config, + task({ runId: `wf_${i.toString(16)}`, startTime: 10_000 + i }), + ); + } + // Plant two malicious snapshot files as the OLDEST (pruned first): + // `...json` → stem `..` → would rm the parent (project root) + // `notarun.json` → stem `notarun` → would rm the sibling dir + for (const name of ['...json', 'notarun.json']) { + const p = path.join(dir, name); + await fs.writeFile(p, '{}', 'utf8'); + await fs.utimes(p, new Date(0), new Date(0)); // oldest → selected to prune + } + // One more legit write tips the count over the cap and triggers prune. + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_ff', startTime: 99_999 }), + ); + + // The guard spared both the parent canary and the non-run sibling dir. + await expect(fs.access(canary)).resolves.toBeUndefined(); + await expect( + fs.access(path.join(dir, 'notarun', 'keep.txt')), + ).resolves.toBeUndefined(); + }); }); diff --git a/packages/core/src/agents/workflow-snapshot.ts b/packages/core/src/agents/workflow-snapshot.ts index 9b0797b8caf..9d317bc6854 100644 --- a/packages/core/src/agents/workflow-snapshot.ts +++ b/packages/core/src/agents/workflow-snapshot.ts @@ -162,13 +162,32 @@ async function pruneSnapshots(dir: string): Promise { // resume journal). Removing only the `.json` snapshot would leave // those journal dirs to grow without bound, so prune both together. const runId = s.f.replace(/\.json$/, ''); + // ...but gate the recursive delete on a well-formed run id. The list is a + // plain `.json` glob, so a file named `...json` yields `runId = ".."` and + // `fs.rm(`${dir}/..`, {recursive,force})` would delete the runs dir's + // PARENT; `notarun.json` would delete a sibling `notarun/`. A malicious + // repo could ship such a file and trip it once pruning kicks in. Only the + // generated `wf_` shape (mirrors workflow.ts's resumeFromRunId guard) + // may drive `fs.rm`. The `.json` unlink stays unconditional — it removes + // exactly that one file, never a directory. + const isRunDir = /^wf_[0-9a-f]+$/.test(runId); return Promise.all([ - fs.unlink(`${dir}/${s.f}`).catch((e) => - debugLogger.warn(`prune unlink failed for ${s.f}: ${e}`), - ), - fs.rm(`${dir}/${runId}`, { recursive: true, force: true }).catch((e) => - debugLogger.warn(`prune journal dir failed for ${runId}: ${e}`), - ), + fs + .unlink(`${dir}/${s.f}`) + .catch((e) => + debugLogger.warn(`prune unlink failed for ${s.f}: ${e}`), + ), + ...(isRunDir + ? [ + fs + .rm(`${dir}/${runId}`, { recursive: true, force: true }) + .catch((e) => + debugLogger.warn( + `prune journal dir failed for ${runId}: ${e}`, + ), + ), + ] + : []), ]); }), );