From 9c20b7b40a4502bcbb9d2a5d2b8853dcd0c136e3 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 09:43:36 -0400 Subject: [PATCH] fix(#4652): use the validated path, collapse the duplication, correct two false claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the two-axis review, all fixed in place. THE ONE THAT MATTERS: cmdTodoComplete validated sourcePath and targetPath and then ran every fs call against the RAW strings — existsSync, statSync, readFileSync, platformWriteSync, unlinkSync, and the dry-run path payload — never sourceCheck.resolved / targetCheck.resolved. That is the exact "validate one path, use another" shape ADR-4650 names as the defect this epic exists to prevent, and it is the same bug this phase had just fixed in check-command-router. Committed inside the fix for it. All I/O now uses the resolved paths; user-facing messages still echo the raw filename, never a resolved absolute path. A VACUOUS TEST, and the false doc claim it was propping up. The test "[RED #4327] an absolute path outside the project is rejected" would have passed with ZERO containment logic: path.join(pendingDir, '/abs/outside/x') yields /abs/outside/x — Node does not let a later absolute segment escape — so the name is FOLDED under the root, passes containment, and simply 404s. The test only ever observed "Todo not found". It now asserts what is actually true and actually valuable: an absolute name is neutralized, and the real outside file is not read, not moved, and still present afterward. docs/CLI-TOOLS.md claimed such a path "is rejected as a usage error", which was false; it now describes the fold-under-root behavior. Traversal and embedded separators ARE rejected, and those claims stand. DUPLICATION THIS EPIC EXISTS TO REMOVE. resolvePath already did isAbsolute-or-join + validatePath + reject; cmdGapAnalysisPlanPost and cmdCheckPredicate each re-inlined the identical triplet in the same file. Both now call resolvePath. Cost, stated rather than hidden: its generic message replaces the two sites' distinct "phase-dir escapes…" wording. The message still names the offending input, and one predicate with one message is the point. SYMLINK COVERAGE was required by #4652's "Done when" and was missing. Added for both the todos root and --phase-dir, skipping cleanly on EPERM so the Windows lanes do not fail where unprivileged symlink creation is disallowed. Both fast-check properties were UNSEEDED. Seeded now. The changeset named "check decision-coverage-plan" as a boundary; that is a caller of the shared resolvePath, which the body never mentioned. Corrected. DISCLOSED, not hidden: ctx.phaseDir is now always the resolved ABSOLUTE path, so ${PHASE_DIR} interpolation and the "not found in " message show an absolute value where a relative --phase-dir previously produced a relative one. That is an observable output change. A test pins it and docs/reference/gate-predicates.md states it. Also regenerated scripts/lib/platform-conformance-tier.generated.cjs and its macos twin — the new tests changed check-predicate.test.cjs's tier classification. Caught by npm run lint:ci locally rather than by a bench run. Co-Authored-By: Claude Opus 5 --- .changeset/sharp-tigers-sing.md | 2 +- docs/CLI-TOOLS.md | 17 +++-- docs/reference/gate-predicates.md | 5 +- .../lib/macos-conformance-tier.generated.cjs | 1 + .../platform-conformance-tier.generated.cjs | 1 + src/check-command-router.cts | 23 +------ src/commands.cts | 17 ++--- tests/check-predicate.test.cjs | 59 +++++++++++++++++ tests/commands.test.cjs | 63 ++++++++++++++++++- tests/security.test.cjs | 4 +- 10 files changed, 152 insertions(+), 40 deletions(-) diff --git a/.changeset/sharp-tigers-sing.md b/.changeset/sharp-tigers-sing.md index ecda1e774..6e1ad8f45 100644 --- a/.changeset/sharp-tigers-sing.md +++ b/.changeset/sharp-tigers-sing.md @@ -2,4 +2,4 @@ type: Security pr: 0 --- -**Path containment at every boundary that takes a directory or filename from the command line** — `todo complete` followed a traversal name outside the todos root and moved the file it found there, `check predicate --phase-dir` let a blocking gate return a passing verdict on evidence from a directory the caller chose, and `check decision-coverage-plan` / `check gap-analysis.plan-post` both accepted a phase directory outside the project. All four now validate against their managed root and reject with a usage error before touching the filesystem. (#4327, #4354) +**Path containment at every boundary that takes a directory or filename from the command line** — `todo complete` followed a traversal name outside the todos root and moved the file it found there, `check predicate --phase-dir` let a blocking gate return a passing verdict on evidence from a directory the caller chose, and the shared `resolvePath` helper — used by `check decision-coverage-plan` and `check gap-analysis.plan-post` — accepted a phase directory outside the project. All boundaries now validate against their managed root and reject with a usage error before touching the filesystem. (#4327, #4354) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 84719e689..ff979dbf0 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1178,12 +1178,17 @@ from `todos/pending/` to `todos/completed/` and upserts `completed:` and rejected loudly. `` is a **basename inside the todos root**, not a path. A value that -resolves outside that root — a traversal like `../../escaped`, an embedded -separator like `sub/name.md`, or an absolute path — is rejected as a usage -error **before** any file is read or moved (#4327). The check covers both halves -of the move, so neither the source nor the destination can land outside the -root, and `--dry-run` is rejected on the same terms rather than previewing a -resolved outside path. +resolves outside that root — a traversal like `../../escaped` or an embedded +separator like `sub/name.md` — is rejected as a usage error **before** any file +is read or moved (#4327). An absolute path is handled differently: it is +**folded under the todos root** (Node's `path.join` does not let a later +absolute segment escape a prior one), so it cannot reach a file outside the +root — it simply fails with the ordinary "Todo not found" error unless a file +of that joined name happens to exist under `todos/pending/`; it is not +rejected as a containment violation. The check covers both halves of the move, +so neither the source nor the destination can land outside the root, and +`--dry-run` is rejected on the same terms rather than previewing a resolved +outside path. ```bash # UAT audit — scan all phases for unresolved items diff --git a/docs/reference/gate-predicates.md b/docs/reference/gate-predicates.md index 6c3fa9138..ba6091ebd 100644 --- a/docs/reference/gate-predicates.md +++ b/docs/reference/gate-predicates.md @@ -88,7 +88,10 @@ rejected as a usage error rather than evaluated. This applies to both kinds — `command-exit-zero` interpolates it into `${PHASE_DIR}` — so an unconfined value would let a **blocking** gate return `block: false` on evidence from a directory the caller chose (#4354). An absolute path inside the project is still accepted; -absolute is not a synonym for escaping. +absolute is not a synonym for escaping. `${PHASE_DIR}` always interpolates the +**resolved absolute path**, even when `--phase-dir` was given as a relative +value — a command relying on `${PHASE_DIR}` staying relative must not assume +that. **Sandbox.** cwd = project root; env = inherited from the GSD process; killed (SIGTERM) on timeout. The command runs as the user, on the user's machine — diff --git a/scripts/lib/macos-conformance-tier.generated.cjs b/scripts/lib/macos-conformance-tier.generated.cjs index 94545e8a6..989afa26a 100644 --- a/scripts/lib/macos-conformance-tier.generated.cjs +++ b/scripts/lib/macos-conformance-tier.generated.cjs @@ -30,6 +30,7 @@ module.exports = { "tests/capability-trust.test.cjs", "tests/changeset-parse.test.cjs", "tests/check-contract-drift.test.cjs", + "tests/check-predicate.test.cjs", "tests/check-ui-safety-gate.test.cjs", "tests/check-update-config-dir.test.cjs", "tests/chunked-planning-parallel.test.cjs", diff --git a/scripts/lib/platform-conformance-tier.generated.cjs b/scripts/lib/platform-conformance-tier.generated.cjs index f0dcf6bee..831bfc718 100644 --- a/scripts/lib/platform-conformance-tier.generated.cjs +++ b/scripts/lib/platform-conformance-tier.generated.cjs @@ -30,6 +30,7 @@ module.exports = { "tests/check-env.test.cjs", "tests/check-gap-analysis-plan-post-e2e.test.cjs", "tests/check-glossary-refs.test.cjs", + "tests/check-predicate.test.cjs", "tests/check-tdd-review-checkpoint-e2e.test.cjs", "tests/check-ui-safety-gate.test.cjs", "tests/check-update-config-dir.test.cjs", diff --git a/src/check-command-router.cts b/src/check-command-router.cts index 008c3352a..576116a57 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -1199,17 +1199,9 @@ function cmdGapAnalysisPlanPost(projectDir: string, args: string[], raw: boolean error('gap-analysis.plan-post requires a phase-dir argument: check gap-analysis.plan-post [phase-req-ids]', ERROR_REASON.SDK_MISSING_ARG); return; } - const phaseDirCheck = validatePath( - path.isAbsolute(phaseDir) ? phaseDir : path.join(projectDir, phaseDir), - projectDir, - { allowAbsolute: true }, - ); - if (!phaseDirCheck.safe) { - error(`phase-dir escapes its allowed directory: ${phaseDir}`, ERROR_REASON.USAGE); - return; - } + const resolvedPhaseDir = resolvePath(phaseDir, projectDir); const phaseReqIds = args[3] ?? undefined; - const result = runGapAnalysis(projectDir, phaseDirCheck.resolved, { phaseReqIds }); + const result = runGapAnalysis(projectDir, resolvedPhaseDir, { phaseReqIds }); // Uniform gate contract: block = false (gap-analysis is always advisory, never blocks). // `message` carries the human-readable gap analysis report so the dispatch's // advisory branch can surface it. --raw emits JSON (rawValue=undefined), not @@ -1379,16 +1371,7 @@ function cmdCheckPredicate(projectDir: string, args: string[], raw: boolean): vo const rawPhaseDir = flags['phase-dir']; let resolvedPhaseDir: string | undefined = rawPhaseDir; if (typeof rawPhaseDir === 'string' && rawPhaseDir !== '') { - const phaseDirCheck = validatePath( - path.isAbsolute(rawPhaseDir) ? rawPhaseDir : path.join(projectDir, rawPhaseDir), - projectDir, - { allowAbsolute: true }, - ); - if (!phaseDirCheck.safe) { - error(`phase-dir escapes its allowed directory: ${rawPhaseDir}`, ERROR_REASON.USAGE); - return; - } - resolvedPhaseDir = phaseDirCheck.resolved; + resolvedPhaseDir = resolvePath(rawPhaseDir, projectDir); } const ctx = { cwd: projectDir, diff --git a/src/commands.cts b/src/commands.cts index fdefd0938..362fffd3b 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -3502,7 +3502,10 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod error(`todo file escapes its allowed directory: ${filename as string}`, ERROR_REASON.USAGE); } - if (!fs.existsSync(sourcePath)) { + const resolvedSource = sourceCheck.resolved; + const resolvedTarget = targetCheck.resolved; + + if (!fs.existsSync(resolvedSource)) { error(`Todo not found: ${filename as string}`); } @@ -3510,11 +3513,11 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod // todosRoot, so containment passes) but are not a todo file — reject them // the same way as any other invalid name instead of letting // fs.readFileSync throw an uncaught EISDIR with an absolute-path stack trace. - if (!fs.statSync(sourcePath).isFile()) { + if (!fs.statSync(resolvedSource).isFile()) { error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE); } - const content = fs.readFileSync(sourcePath, 'utf-8'); + const content = fs.readFileSync(resolvedSource, 'utf-8'); const today = realClock.localToday(); // #4096: --dry-run mirrors `milestone complete --dry-run` (#2118) — every @@ -3527,8 +3530,8 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod file: filename, date: today, would_move: { - source: path.relative(cwd, sourcePath).split(path.sep).join('/'), - target: path.relative(cwd, path.join(completedDir, filename as string)).split(path.sep).join('/'), + source: path.relative(cwd, resolvedSource).split(path.sep).join('/'), + target: path.relative(cwd, resolvedTarget).split(path.sep).join('/'), }, would_set: { completed: today, status: 'completed' }, }, raw); @@ -3541,8 +3544,8 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod const completedContent = upsertTodoCompletionFields(content, today); - platformWriteSync(path.join(completedDir, filename as string), completedContent); - fs.unlinkSync(sourcePath); + platformWriteSync(resolvedTarget, completedContent); + fs.unlinkSync(resolvedSource); output({ completed: true, file: filename, date: today }, raw, 'completed'); } diff --git a/tests/check-predicate.test.cjs b/tests/check-predicate.test.cjs index 2f3a2b035..c890e7bbd 100644 --- a/tests/check-predicate.test.cjs +++ b/tests/check-predicate.test.cjs @@ -293,6 +293,65 @@ describe('check predicate --phase-dir — containment boundary (#4354)', () => { ); }); + test('[#4652] a --phase-dir that is a symlink inside the project resolving outside the project is rejected', (t) => { + fs.writeFileSync( + path.join(outsideDir, 'SECURITY.md'), + '---\nstatus: passed\n---\n# Security\n', + ); + const linkPath = path.join(projDir, '.planning', 'phases', 'linked-out'); + try { + fs.symlinkSync(outsideDir, linkPath, 'dir'); + } catch (e) { + if (e.code === 'EPERM') { + t.skip('symlink creation is not permitted on this platform (EPERM)'); + return; + } + throw e; + } + + const predicate = JSON.stringify({ + kind: 'artifact-frontmatter-equals', + artifact: 'SECURITY.md', + field: 'status', + equals: 'passed', + }); + + const result = runGsdTools( + ['--json-errors', 'check', 'predicate', '--predicate', predicate, '--phase-dir', linkPath, '--raw'], + projDir, + ); + + assert.strictEqual( + result.success, + false, + `a --phase-dir symlink resolving outside the project must be rejected ` + + `(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + }); + + test('[#4652] a relative --phase-dir interpolates ${PHASE_DIR} as the resolved ABSOLUTE path, not the relative value', () => { + const phaseDir = path.join(projDir, '.planning', 'phases', '05-x'); + fs.writeFileSync(path.join(phaseDir, 'marker.txt'), 'marker\n'); + + const predicate = JSON.stringify({ + kind: 'command-exit-zero', + command: 'echo "${PHASE_DIR}" > "${PHASE_DIR}/interpolated.txt"', + }); + + const result = runGsdTools( + ['check', 'predicate', '--predicate', predicate, '--phase-dir', '.planning/phases/05-x', '--raw'], + projDir, + ); + + assert.ok(result.success, `Command failed: ${result.error}`); + const interpolated = fs.readFileSync(path.join(phaseDir, 'interpolated.txt'), 'utf-8').trim(); + assert.strictEqual( + interpolated, + fs.realpathSync(phaseDir), + `${'${PHASE_DIR}'} must interpolate the resolved absolute path, not the relative --phase-dir value`, + ); + }); + test('[regression] no --phase-dir at all still falls back to cwd and evaluates', () => { fs.writeFileSync( path.join(projDir, 'SECURITY.md'), diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index c075ba618..dc97f7360 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -890,13 +890,38 @@ describe('todo complete — containment boundary (#4327)', () => { }); } - test('[RED #4327] an absolute path outside the project is rejected', () => { + test('[#4327] an absolute filename is folded under the pending dir, not rejected as containment violation — the outside file is untouched', () => { + // MEASURED: path.join(pendingDir, '/abs/outside/evil.md') === `${pendingDir}/abs/outside/evil.md` + // — Node's path.join does not let a later absolute segment escape a prior + // one. So an absolute `filename` is folded INSIDE todosRoot, passes + // containment, and simply 404s as "Todo not found" (unless a file of + // that joined name happens to exist under pendingDir). It is NOT + // rejected as a containment/escape violation. const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); try { const outsideFile = path.join(outsideDir, 'evil.md'); - fs.writeFileSync(outsideFile, '---\nstatus: pending\n---\nSENTINEL\n'); + const sentinel = '---\nstatus: pending\n---\nSENTINEL\n'; + fs.writeFileSync(outsideFile, sentinel); const result = runGsdTools(['todo', 'complete', outsideFile], tmpDir); - assert.strictEqual(result.success, false, 'an absolute path outside the project must be rejected'); + + assert.strictEqual(result.success, false, 'the command must fail (the folded path does not exist under pending/)'); + assert.ok( + (result.error || '').includes('not found'), + `must fail as a plain "not found", not a containment rejection (got: ${result.error})`, + ); + assert.ok(fs.existsSync(outsideFile), 'the real outside file must still exist'); + assert.strictEqual( + fs.readFileSync(outsideFile, 'utf-8'), + sentinel, + 'the real outside file must never be read/touched', + ); + const completedDir = path.join(tmpDir, '.planning', 'todos', 'completed'); + if (fs.existsSync(completedDir)) { + assert.ok( + !fs.readdirSync(completedDir).includes('evil.md'), + 'the outside file must never land inside completed/', + ); + } } finally { cleanup(outsideDir); } @@ -961,6 +986,38 @@ describe('todo complete — containment boundary (#4327)', () => { cleanup(outsideDir); } }); + + test('[#4652] a symlink inside pending/ whose target is a real file outside the todos root is rejected — the outside target is untouched', (t) => { + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); + const linkPath = path.join(pendingDir, 'linked.md'); + try { + const outsideFile = path.join(outsideDir, 'real-target.md'); + const sentinel = '---\nstatus: pending\n---\nSENTINEL-SYMLINK\n'; + fs.writeFileSync(outsideFile, sentinel); + try { + fs.symlinkSync(outsideFile, linkPath, 'file'); + } catch (e) { + if (e.code === 'EPERM') { + t.skip('symlink creation is not permitted on this platform (EPERM)'); + return; + } + throw e; + } + + const result = runGsdTools(['todo', 'complete', 'linked.md'], tmpDir); + + assert.strictEqual(result.success, false, 'a symlink pointing outside the todos root must be rejected'); + assert.ok(fs.existsSync(outsideFile), 'the outside symlink target must still exist'); + assert.strictEqual( + fs.readFileSync(outsideFile, 'utf-8'), + sentinel, + 'the outside symlink target content must be byte-for-byte untouched', + ); + } finally { + cleanup(outsideDir); + try { fs.unlinkSync(linkPath); } catch { /* not created, or already gone */ } + } + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/security.test.cjs b/tests/security.test.cjs index 8467bdd2c..f7e34d523 100644 --- a/tests/security.test.cjs +++ b/tests/security.test.cjs @@ -1248,7 +1248,7 @@ describe('validatePath — containment properties (#4652)', () => { `traversal ${JSON.stringify(traversal)} against root ${root} must be rejected, got: ${JSON.stringify(result)}`, ); }, - )); + ), { seed: 4652, numRuns: 200 }); }); test('PR2: a path that resolves INSIDE the root (no traversal beyond it) is ALWAYS accepted', () => { @@ -1266,7 +1266,7 @@ describe('validatePath — containment properties (#4652)', () => { ); assert.strictEqual(result.resolved, path.resolve(root, relPath)); }, - )); + ), { seed: 4652, numRuns: 200 }); }); });