From 3925839f2a5fe047dbe9497eddb745a3407acf01 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 09:09:20 -0400 Subject: [PATCH 1/6] test(#4652): failing-first coverage for the four unconfined boundaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of epic #4636, absorbing #4327 and #4354. Tests only; no fix. These MUST fail. Four CLI boundaries join externally-supplied input to a managed root with no containment validation. Each was driven through the real CLI and confirmed unconfined before the assertions were written: todo complete src/commands.cts cmdTodoComplete check predicate --phase-dir check-command-router cmdCheckPredicate check decision-coverage-plan check-command-router resolvePath check gap-analysis.plan-post check-command-router Boundary 1 is worse than the issue describes. #4327 reports that a traversal name "resolves outside the todos root", which reads as an information leak. Measured, it is destructive: `todo complete ../../../../b1out/leak.md` exited 0, MOVED the outside file into completed/, and unlinked the original. The file was gone. cmdTodoComplete ends in fs.unlinkSync(sourcePath), so an unconfined name does not merely read across the boundary, it consumes across it. Boundary 2 reproduces #4354 exactly: a BLOCKING gate returned {"block":false,"details":{"match":true}} sourced entirely from a SECURITY.md in a directory the caller chose, outside the project. Boundaries 3 and 4 are not named in the epic. Both accepted an outside phase dir and exited 0. Rows that exist because they are the ones nobody enumerates: - ORDERING. A real file is created outside the todos root, then the traversal name targeting it is asserted rejected AND the outside file asserted still present and unmoved. #4327 notes the existence check and the move target BOTH follow the unvalidated join, so a rejection that lands after the read has already leaked — and, per the finding above, after the unlink has already destroyed. - `a/../../b.md` — looks balanced, resolves outside. - --dry-run must reject too; a preview must not leak a resolved outside path. - ${PHASE_DIR} interpolation into a command-exit-zero predicate is the SECOND predicate kind, which a fix inside gate-predicate-evaluator.cts would miss. - An absolute path INSIDE the project must still be accepted at every boundary — absolute is not a synonym for escaping. Cross-boundary rows loop over one shared list of escaping inputs and assert all four reject with the same shape, so four sites adopting one predicate cannot drift into four rejection contracts. Property tests cover BOTH directions — outside is always rejected, inside is always accepted. A property asserting only rejection is satisfied by a predicate that rejects everything, which is the degenerate-implementation trap found in Phase 1's review. Both are seeded. Regressions fold into the owning module suites rather than a new tests/fix-NNNN-*.test.cjs, per scripts/lint-regression-test-names.cjs. Co-Authored-By: Claude Opus 5 --- .../check-gap-analysis-plan-post-e2e.test.cjs | 134 ++++++++++++++++ tests/check-predicate.test.cjs | 125 ++++++++++++++- tests/commands.test.cjs | 146 ++++++++++++++++++ tests/security.test.cjs | 139 ++++++++++++++++- 4 files changed, 542 insertions(+), 2 deletions(-) diff --git a/tests/check-gap-analysis-plan-post-e2e.test.cjs b/tests/check-gap-analysis-plan-post-e2e.test.cjs index 039918f04..c298e9c8d 100644 --- a/tests/check-gap-analysis-plan-post-e2e.test.cjs +++ b/tests/check-gap-analysis-plan-post-e2e.test.cjs @@ -20,6 +20,7 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const os = require('os'); const { spawnSync } = require('child_process'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); @@ -717,3 +718,136 @@ describe('resolveLoopHooks plan:post — pure function against real registry', ( assert.strictEqual(entry.gates[0].capId, 'gap-analysis'); }); }); + +// ─── #4652: containment boundaries — resolvePath (check-command-router.cts:92) +// and `check gap-analysis.plan-post ` ─────────────────────────────── +// +// Boundary 3: `check decision-coverage-plan ` resolves the phase-dir +// positional via `resolvePath()`, which just does +// `path.isAbsolute(p) ? p : path.join(projectDir, p)` — no containment check. +// Boundary 4: `check gap-analysis.plan-post ` takes `args[2]` +// unconfined and joins it directly in `runGapAnalysis` (gap-checker.cts). + +function runDecisionCoveragePlan(extraFlags, phaseDir, contextPath, cwd) { + return runGsdTools(['query', 'check.decision-coverage-plan', ...extraFlags, phaseDir, contextPath], cwd); +} + +describe('resolvePath / check decision-coverage-plan — containment boundary (#4652)', () => { + let tmpDir; + let phaseDir; + let outsideDir; + + beforeEach(() => { + tmpDir = createTempProject(); + phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(phaseDir, { recursive: true }); + outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-decision-outside-')); + }); + + afterEach(() => { + cleanup(tmpDir); + cleanup(outsideDir); + }); + + test('[RED #4652] an outside phase-dir is rejected (currently resolves and proceeds unconfined)', () => { + const contextPath = path.join(phaseDir, 'CONTEXT.md'); + fs.writeFileSync( + contextPath, + '# Phase Context\n\n\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n\n', + ); + fs.writeFileSync(path.join(outsideDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n'); + const relOutside = path.relative(tmpDir, outsideDir); + + const result = runGsdTools( + ['--json-errors', 'query', 'check.decision-coverage-plan', relOutside, contextPath], + tmpDir, + ); + + assert.strictEqual( + result.success, + false, + `an outside phase-dir must be rejected before evaluating plan coverage ` + + `(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + }); + + test('[regression] a valid in-project relative phase-dir still proceeds', () => { + const contextPath = path.join(phaseDir, 'CONTEXT.md'); + fs.writeFileSync( + contextPath, + '# Phase Context\n\n\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n\n', + ); + fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n'); + const relPhaseDir = path.relative(tmpDir, phaseDir); + + const result = runDecisionCoveragePlan([], relPhaseDir, contextPath, tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + assert.strictEqual(out.passed, true, 'in-project phase-dir with covered decision must pass'); + }); + + test('[regression] an absolute path INSIDE the project is accepted', () => { + const contextPath = path.join(phaseDir, 'CONTEXT.md'); + fs.writeFileSync( + contextPath, + '# Phase Context\n\n\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n\n', + ); + fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n'); + + const result = runDecisionCoveragePlan([], phaseDir, contextPath, tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + assert.strictEqual(out.passed, true, 'absolute in-project phase-dir must be accepted'); + }); +}); + +describe('check gap-analysis.plan-post — containment boundary (#4652)', () => { + let tmpDir; + let phaseDir; + let outsideDir; + + beforeEach(() => { + tmpDir = createTempProject(); + phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(phaseDir, { recursive: true }); + const init = runGsdTools('config-ensure-section', tmpDir); + assert.ok(init.success, `config-ensure-section failed: ${init.error}`); + outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-gap-outside-')); + }); + + afterEach(() => { + cleanup(tmpDir); + cleanup(outsideDir); + }); + + test('[RED #4652] an outside phase-dir is rejected (currently resolves and proceeds unconfined)', () => { + fs.writeFileSync(path.join(outsideDir, '01-PLAN.md'), '# Plan\n\nSome content.\n'); + const relOutside = path.relative(tmpDir, outsideDir); + + const result = runGsdTools(['--json-errors', 'check', 'gap-analysis.plan-post', relOutside, '--raw'], tmpDir); + + assert.strictEqual( + result.success, + false, + `an outside phase-dir must be rejected ` + + `(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + }); + + test('[regression] a valid phase-dir still proceeds (advisory, block:false)', () => { + writeRequirements(path.join(tmpDir, '.planning'), ['REQ-01']); + writePlan(phaseDir, '01', '# Plan\n\nImplements REQ-01.\n'); + + const r = runGapCheck([phaseDir], tmpDir); + assert.ok(r.success, `check failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.strictEqual(out.block, false, 'gap-analysis is always advisory'); + }); + + test('[regression] a missing phase-dir argument still gives the existing SDK_MISSING_ARG error', () => { + const result = runGsdTools(['--json-errors', 'check', 'gap-analysis.plan-post', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'must fail when phaseDir omitted'); + const parsed = JSON.parse(result.error); + assert.strictEqual(parsed.reason, 'sdk_missing_arg', 'must keep the existing SDK_MISSING_ARG reason'); + }); +}); diff --git a/tests/check-predicate.test.cjs b/tests/check-predicate.test.cjs index f35d7bec0..6dd35181c 100644 --- a/tests/check-predicate.test.cjs +++ b/tests/check-predicate.test.cjs @@ -13,11 +13,15 @@ * (a 100ms timeout killing `sleep 1`), so there is no orphan/leak risk. */ -const { describe, test } = require('node:test'); +const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); const { evaluatePredicate } = require('../gsd-core/bin/lib/gate-predicate-evaluator.cjs'); const { buildPredicateDeps, parsePredicateFlags } = require('../gsd-core/bin/lib/check-command-router.cjs'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); // ─── buildPredicateDeps: real subprocess exit mapping ───────────────────────── @@ -153,3 +157,122 @@ describe('partitionPredicateArgs (#4130 follow-up)', () => { assert.deepEqual(positionals, ['p1', 'p2', 'p3']); }); }); + +// ─── #4354: `check predicate --phase-dir` containment boundary ─────────────── +// +// cmdCheckPredicate passes the `--phase-dir` flag VERBATIM into PredicateContext +// (src/check-command-router.cts) with no containment validation. Both predicate +// kinds read/interpolate that value: `artifact-frontmatter-equals` resolves it +// as `targetDir` for `findPhaseArtifact`, and `command-exit-zero` interpolates +// it into `${PHASE_DIR}` in the shelled-out command. These tests reproduce the +// issue's exact repro and prove the boundary is currently unconfined. + +describe('check predicate --phase-dir — containment boundary (#4354)', () => { + let projDir; + let outsideDir; + + beforeEach(() => { + projDir = createTempProject(); + fs.mkdirSync(path.join(projDir, '.planning', 'phases', '05-x'), { recursive: true }); + outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-predicate-outside-')); + }); + + afterEach(() => { + cleanup(projDir); + cleanup(outsideDir); + }); + + test('[RED #4354] the issue\'s exact repro: artifact-frontmatter-equals against a foreign SECURITY.md via an outside --phase-dir must be rejected, not evaluated', () => { + fs.writeFileSync( + path.join(outsideDir, 'SECURITY.md'), + '---\nstatus: passed\n---\n# Security\n', + ); + + 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', outsideDir, '--raw'], + projDir, + ); + + // CURRENT BUG (documented, not asserted as desired): this command today + // succeeds and prints {"block":false,...} — a BLOCKING gate passing on + // foreign evidence read from OUTSIDE the project. REQUIRED behavior: + // the outside --phase-dir must be rejected before evaluation. + assert.strictEqual( + result.success, + false, + `an outside --phase-dir must be rejected before evaluating the predicate ` + + `(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + }); + + test('[RED #4354] a command-exit-zero predicate interpolating ${PHASE_DIR} with an outside --phase-dir must also be rejected', () => { + fs.writeFileSync(path.join(outsideDir, 'marker.txt'), 'outside-marker\n'); + + const predicate = JSON.stringify({ + kind: 'command-exit-zero', + command: 'test -f "${PHASE_DIR}/marker.txt"', + }); + + const result = runGsdTools( + ['--json-errors', 'check', 'predicate', '--predicate', predicate, '--phase-dir', outsideDir, '--raw'], + projDir, + ); + + assert.strictEqual( + result.success, + false, + `a command-exit-zero predicate interpolating an outside --phase-dir must be rejected ` + + `(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + }); + + test('[regression] a valid in-project --phase-dir still evaluates', () => { + const phaseDir = path.join(projDir, '.planning', 'phases', '05-x'); + fs.writeFileSync( + path.join(phaseDir, 'SECURITY.md'), + '---\nstatus: passed\n---\n# Security\n', + ); + const predicate = JSON.stringify({ + kind: 'artifact-frontmatter-equals', + artifact: 'SECURITY.md', + field: 'status', + equals: 'passed', + }); + + const result = runGsdTools( + ['check', 'predicate', '--predicate', predicate, '--phase-dir', phaseDir, '--raw'], + projDir, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.block, false, 'in-project phase-dir evaluation must still pass'); + }); + + test('[regression] no --phase-dir at all still falls back to cwd and evaluates', () => { + fs.writeFileSync( + path.join(projDir, 'SECURITY.md'), + '---\nstatus: passed\n---\n# Security\n', + ); + const predicate = JSON.stringify({ + kind: 'artifact-frontmatter-equals', + artifact: 'SECURITY.md', + field: 'status', + equals: 'passed', + }); + + const result = runGsdTools( + ['check', 'predicate', '--predicate', predicate, '--raw'], + projDir, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.block, false, 'cwd-fallback evaluation must still pass'); + }); +}); diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 94f30e8ed..30280193c 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -13,6 +13,7 @@ const { test, describe, after, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const os = require('os'); const { runGsdTools, createTempProject, createTempDir, cleanup } = require('./helpers.cjs'); const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); @@ -798,6 +799,151 @@ describe('todo complete command', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// todo complete — containment boundary (#4327) +// +// cmdTodoComplete joins the externally-supplied `filename` into +// todosDir(cwd)/pending with NO containment validation (src/commands.cts). +// These tests prove the boundary is currently unconfined — a traversal name +// is neither rejected before the existence check nor before the move. +// ───────────────────────────────────────────────────────────────────────────── + +describe('todo complete — containment boundary (#4327)', () => { + let tmpDir; + let pendingDir; + + beforeEach(() => { + tmpDir = createTempProject(); + pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // ── Regressions: normal completion must keep working ───────────────────── + + test('[regression] a valid existing todo name completes and the file moves to completed/', () => { + fs.writeFileSync(path.join(pendingDir, 'ok-name.md'), '---\nstatus: pending\n---\n'); + const result = runGsdTools(['todo', 'complete', 'ok-name.md'], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + assert.ok(!fs.existsSync(path.join(pendingDir, 'ok-name.md')), 'removed from pending'); + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'todos', 'completed', 'ok-name.md')), + 'present in completed', + ); + }); + + test('[regression] a name with dots like "2026-09-12.some.todo.md" completes', () => { + fs.writeFileSync(path.join(pendingDir, '2026-09-12.some.todo.md'), '---\nstatus: pending\n---\n'); + const result = runGsdTools(['todo', 'complete', '2026-09-12.some.todo.md'], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'todos', 'completed', '2026-09-12.some.todo.md')), + 'dotted-name todo completes', + ); + }); + + test('[regression] a missing name still produces the existing "Todo not found" error', () => { + const result = runGsdTools(['todo', 'complete', 'does-not-exist.md'], tmpDir); + assert.ok(!result.success, 'must still fail'); + assert.ok(result.error.includes('not found'), 'error must still mention "not found"'); + }); + + // ── MUST BE REJECTED — currently unconfined (RED) ───────────────────────── + + const ESCAPING_NAMES = ['../../escaped', '../sibling.md', 'sub/name.md', 'a/../../b.md']; + + for (const name of ESCAPING_NAMES) { + test(`[RED #4327] "todo complete ${name}" must be rejected (currently unconfined)`, () => { + const targetPath = path.join(pendingDir, name); + fs.mkdirSync(path.dirname(targetPath), { recursive: true }); + fs.writeFileSync(targetPath, '---\nstatus: pending\n---\nSENTINEL\n'); + + const result = runGsdTools(['todo', 'complete', name], tmpDir); + assert.strictEqual( + result.success, + false, + `"${name}" must be rejected as an escaping/invalid todo name (currently ` + + `${result.success ? 'SUCCEEDED — unconfined join, no containment check' : 'failed for an unrelated reason'})`, + ); + }); + } + + test('[RED #4327] an absolute path outside the project is rejected', () => { + 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 result = runGsdTools(['todo', 'complete', outsideFile], tmpDir); + assert.strictEqual(result.success, false, 'an absolute path outside the project must be rejected'); + } finally { + cleanup(outsideDir); + } + }); + + // ── CRITICAL ORDERING (#4327): the existence check AND the move target ──── + // both follow the unvalidated join, so a rejection that happens after the + // read has already leaked. Prove the outside file is neither read-through + // nor moved/deleted by a (today, absent) rejection. + + test('[RED #4327] CRITICAL ORDERING: a traversal name resolving to a real outside file is rejected WITHOUT the outside file being moved or deleted', () => { + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); + try { + const outsideFile = path.join(outsideDir, 'leak-target.md'); + const sentinel = '---\nstatus: pending\n---\nSENTINEL-LEAK\n'; + fs.writeFileSync(outsideFile, sentinel); + const relName = path.relative(pendingDir, outsideFile); + + const result = runGsdTools(['todo', 'complete', relName], tmpDir); + + assert.strictEqual( + result.success, + false, + `traversal name "${relName}" resolving to ${outsideFile} must be rejected`, + ); + assert.ok( + fs.existsSync(outsideFile), + 'the outside file must still exist — a rejected completion must not move/delete it', + ); + assert.strictEqual( + fs.readFileSync(outsideFile, 'utf-8'), + sentinel, + 'the outside file content must be byte-for-byte untouched', + ); + const completedDir = path.join(tmpDir, '.planning', 'todos', 'completed'); + if (fs.existsSync(completedDir)) { + assert.ok( + !fs.readdirSync(completedDir).includes('leak-target.md'), + 'the outside file must never land inside completed/', + ); + } + } finally { + cleanup(outsideDir); + } + }); + + test('[RED #4327] "todo complete --dry-run" must be rejected — a dry run must not leak a resolved outside path', () => { + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); + try { + const outsideFile = path.join(outsideDir, 'dry-leak.md'); + fs.writeFileSync(outsideFile, '---\nstatus: pending\n---\n'); + const relName = path.relative(pendingDir, outsideFile); + + const result = runGsdTools(['todo', 'complete', relName, '--dry-run'], tmpDir); + + assert.strictEqual( + result.success, + false, + `dry-run completion of traversal name "${relName}" resolving to ${outsideFile} must be rejected`, + ); + } finally { + cleanup(outsideDir); + } + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // todo match-phase command // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/security.test.cjs b/tests/security.test.cjs index e75f0944e..8467bdd2c 100644 --- a/tests/security.test.cjs +++ b/tests/security.test.cjs @@ -9,7 +9,8 @@ const assert = require('node:assert/strict'); const path = require('path'); const os = require('os'); const fs = require('fs'); -const { cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const fc = require('./helpers/fast-check-setup.cjs'); const { validatePath, @@ -1209,3 +1210,139 @@ describe('SECURE: ASVS level scaling (#1627)', () => { }); }); } + +// ─── #4652: validatePath property tests (both directions) ──────────────────── +// +// PR1: any relative path with `..` segments that resolves OUTSIDE the root is +// ALWAYS rejected. PR2: any path that resolves INSIDE the root is ALWAYS +// accepted. Both directions are required — a predicate rejecting everything +// would vacuously satisfy PR1 alone. + +describe('validatePath — containment properties (#4652)', () => { + // A path SEGMENT: letters/digits/dash/underscore, non-empty, never '.' or '..' + // by construction so every generated escaping path is escaping ONLY via the + // deliberately-injected `..` components below (never an accidental one). + const segmentArb = fc + .stringMatching(/^[A-Za-z0-9_-]+$/) + .filter((s) => s.length > 0 && s !== '.' && s !== '..'); + + test('PR1: a relative path with enough leading ".." segments to resolve OUTSIDE the root is ALWAYS rejected', () => { + fc.assert(fc.property( + fc.array(segmentArb, { minLength: 1, maxLength: 4 }), // base depth below an anchor + fc.integer({ min: 1, max: 8 }), // extra ".." beyond the base depth + fc.array(segmentArb, { minLength: 0, maxLength: 3 }), // trailing segments after escaping + (baseSegments, extraUp, tailSegments) => { + // Root sits `baseSegments.length` levels below a stable anchor. + const anchor = path.resolve('/gsd-root-anchor'); + const root = path.join(anchor, ...baseSegments); + // Enough ".." to exit past the anchor itself, guaranteeing the resolved + // path is OUTSIDE root (and outside the anchor) regardless of anchor + // depth on this OS. + const upCount = baseSegments.length + extraUp; + const traversal = path.join(...Array(upCount).fill('..'), ...tailSegments, 'target'); + + const result = validatePath(traversal, root); + assert.strictEqual( + result.safe, + false, + `traversal ${JSON.stringify(traversal)} against root ${root} must be rejected, got: ${JSON.stringify(result)}`, + ); + }, + )); + }); + + test('PR2: a path that resolves INSIDE the root (no traversal beyond it) is ALWAYS accepted', () => { + fc.assert(fc.property( + fc.array(segmentArb, { minLength: 1, maxLength: 5 }), + (segments) => { + const root = path.resolve('/gsd-root-anchor-in'); + const relPath = path.join(...segments); + + const result = validatePath(relPath, root); + assert.strictEqual( + result.safe, + true, + `in-root path ${JSON.stringify(relPath)} against root ${root} must be accepted, got: ${JSON.stringify(result)}`, + ); + assert.strictEqual(result.resolved, path.resolve(root, relPath)); + }, + )); + }); +}); + +// ─── #4652: cross-boundary containment — same escaping inputs, all four +// boundaries, same rejection shape ──────────────────────────────────────────── +// +// One shared list of escaping inputs is driven through all four containment +// boundaries named in #4652 (todo complete, check predicate --phase-dir, +// check decision-coverage-plan's resolvePath, check gap-analysis.plan-post). +// Each boundary is asserted to reject with the SAME error shape: +// `{ ok: false, reason: 'usage', message }` (ERROR_REASON.USAGE) under +// `--json-errors`. None of these boundaries validate today, so every row is +// expected to FAIL until the fix lands (RED). + +describe('cross-boundary containment — shared escaping inputs, same rejection shape (#4652)', () => { + const ESCAPING_INPUTS = ['../../escaped', '../sibling', 'a/../../b']; + + function setupProject() { + const tmpDir = createTempProject(); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '05-x'); + fs.mkdirSync(phaseDir, { recursive: true }); + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + return { tmpDir, phaseDir, pendingDir }; + } + + function assertUsageRejection(result, label) { + assert.strictEqual( + result.success, + false, + `${label} must be rejected (currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`, + ); + let parsed = null; + try { parsed = JSON.parse(result.error); } catch (_) { /* not JSON — also a failure to fix */ } + assert.ok(parsed, `${label}: stderr must be JSON under --json-errors, got: ${result.error}`); + assert.strictEqual(parsed.ok, false, `${label}: parsed.ok must be false`); + assert.strictEqual(parsed.reason, 'usage', `${label}: reason must be ERROR_REASON.USAGE ("usage"), got: ${parsed.reason}`); + } + + for (const escaping of ESCAPING_INPUTS) { + test(`[RED #4652] "${escaping}" is rejected identically (reason: usage) at all four boundaries`, () => { + const { tmpDir } = setupProject(); + try { + // Boundary 1: todo complete + const todoResult = runGsdTools(['--json-errors', 'todo', 'complete', escaping], tmpDir); + assertUsageRejection(todoResult, `todo complete "${escaping}"`); + + // Boundary 2: check predicate --phase-dir + const predicate = JSON.stringify({ + kind: 'command-exit-zero', + command: 'true', + }); + const predicateResult = runGsdTools( + ['--json-errors', 'check', 'predicate', '--predicate', predicate, '--phase-dir', escaping, '--raw'], + tmpDir, + ); + assertUsageRejection(predicateResult, `check predicate --phase-dir "${escaping}"`); + + // Boundary 3: check decision-coverage-plan + const contextPath = path.join(tmpDir, 'CONTEXT.md'); + fs.writeFileSync(contextPath, '# Context\n\n\n## Implementation Decisions\n\n- **D-01:** x\n\n'); + const decisionResult = runGsdTools( + ['--json-errors', 'query', 'check.decision-coverage-plan', escaping, contextPath], + tmpDir, + ); + assertUsageRejection(decisionResult, `check decision-coverage-plan "${escaping}"`); + + // Boundary 4: check gap-analysis.plan-post + const gapResult = runGsdTools( + ['--json-errors', 'check', 'gap-analysis.plan-post', escaping, '--raw'], + tmpDir, + ); + assertUsageRejection(gapResult, `check gap-analysis.plan-post "${escaping}"`); + } finally { + cleanup(tmpDir); + } + }); + } +}); From 374300da17fb7b180c071c60c00cdf143d641c85 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 09:33:32 -0400 Subject: [PATCH 2/6] fix(#4652): confine every boundary that joins argv to a managed root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of epic #4636, absorbing #4327 and #4354. Implements ADR-4650 decision 3: containment is a boundary concern — the predicate runs where external input enters, not at whichever interior call site remembered. Four boundaries now validate against their managed root and reject with a USAGE-shaped error before touching the filesystem: todo complete -> todosDir(cwd) check predicate --phase-dir -> projectDir check decision-coverage-plan -> projectDir (via resolvePath) check gap-analysis.plan-post -> projectDir #4327 understated its own severity. It reports that a traversal name "resolves outside the todos root", which reads as an information leak. Measured, it was destructive: the command exited 0, MOVED the outside file into completed/, and unlinked the original. cmdTodoComplete ends in fs.unlinkSync(sourcePath), so an unconfined name consumed across the boundary rather than merely reading across it. Validation now precedes every fs call — existsSync, readFileSync, ensureDir, writeSync, unlinkSync — and both halves of the move are confined, so neither source nor destination can land outside the root. --dry-run is rejected on the same terms; a preview must not leak a resolved outside path either. #4354 reproduces exactly: a BLOCKING gate returned block:false sourced entirely from a SECURITY.md in a caller-chosen directory outside the project. THE HARDER HALF, found by the isolated adversarial review of the first attempt: validating a path and then using a DIFFERENT one closes nothing. The first fix validated `--phase-dir` joined against `--cwd`, then passed the RAW unjoined value into the predicate context. gate-predicate-evaluator uses it as-is and findPhaseArtifact resolves a relative path against the REAL process cwd — so validation and the read used two different roots whenever process.cwd() differed from --cwd. Reproduced: running from a directory holding a plan with `secret_field: LEAKED_VALUE`, a predicate declared against an empty --cwd project exited 0 and returned "actual":"LEAKED_VALUE". The rule now applied at all three router sites: **use the validated resolved path, never the raw input.** Independently re-verified after the fix — the lookup resolves in the --cwd project and no value leaks. gate-predicate-evaluator.cts is untouched and still imports no fs. Confining in the router is what keeps that pure-leaf contract intact AND covers ${PHASE_DIR} interpolation into command-exit-zero, which an evaluator-local fix would have missed entirely. Also fixed, same review: `todo complete .` and `..` passed containment (they resolve to the pending dir, which IS inside the root) and then threw an uncaught EISDIR with an absolute-path stack trace. Now a clean USAGE rejection naming the real reason — "todo name is not a file" — rather than borrowing the escape message, which would have stated something false. Ripples discharged BEFORE the verification checkpoint rather than after, per the Phase 1 retrospective: docs/reference/gate-predicates.md and docs/CLI-TOOLS.md document the new constraints, CONTEXT.md records why containment lives at the router rather than the evaluator, the changeset is written, and the install-tree goldens were regenerated to confirm unchanged (no new shipped file) rather than assumed. Co-Authored-By: Claude Opus 5 --- .changeset/sharp-tigers-sing.md | 5 +++ CONTEXT.md | 2 +- docs/CLI-TOOLS.md | 8 ++++ docs/reference/gate-predicates.md | 9 +++++ src/check-command-router.cts | 34 +++++++++++++++-- src/commands.cts | 20 +++++++++- .../check-gap-analysis-plan-post-e2e.test.cjs | 4 +- tests/check-predicate.test.cjs | 38 +++++++++++++++++++ tests/commands.test.cjs | 19 ++++++++++ 9 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 .changeset/sharp-tigers-sing.md diff --git a/.changeset/sharp-tigers-sing.md b/.changeset/sharp-tigers-sing.md new file mode 100644 index 000000000..ecda1e774 --- /dev/null +++ b/.changeset/sharp-tigers-sing.md @@ -0,0 +1,5 @@ +--- +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) diff --git a/CONTEXT.md b/CONTEXT.md index 294ae108c..00e53adab 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -357,7 +357,7 @@ A bundle delivering one optional GSD feature, toggled as a unit at install or af Generated description of what the five-step loop (Discuss → Plan → Execute → Verify → Ship) exposes as extension points: per-step loop points, agent roles, and core artifacts. Sourced from structured `` HTML-comment markers embedded near the top of each of the five step workflow files (`discuss-phase.md`, `plan-phase.md`, `execute-phase.md`, `verify-work.md`, `ship.md`). Generated by `scripts/gen-loop-host-contract.cjs` → `gsd-core/bin/lib/loop-host-contract.cjs` (ADR-894 §3 phase 3a-impl-2). Covers exactly the 12 canonical points (discuss:pre/post, plan:pre/post, execute:pre/wave:pre/wave:post/post, verify:pre/post, ship:pre/post). The generator enforces a drift guard: every declared non-orchestrator agent role must correspond to an actual agent reference in the workflow file. Consumed by `gen-capability-registry.cjs` (replaces the former inline `LOOP_HOST_CONTRACT` constant). Run `node scripts/gen-loop-host-contract.cjs --write` after editing a workflow step marker. ### Gate Predicate Evaluator Module -Pure, deps-injected evaluator for capability gate `check.predicate` blocks (#2008, ADR-2008). Prior to #2008 the registry validator accepted `check.predicate` (one of exactly-one-of `query`/`predicate`/`agentVerdict`) and the loop-resolver rendered it, but nothing EVALUATED a declared predicate — only `check.query` was enforced (dispatched via `gsd_run check `), and built-in gates like `security` worked only via hard-coded `capId` prose branches in `ship.md`/`execute-phase.md`/`verify-work.md`. This module is the generic evaluation path: `evaluatePredicate(predicate, context, deps) → { block, message, details? }` dispatches by `predicate.kind` through a `KIND_TABLE`. Built-in kinds: `command-exit-zero` runs a declared command in a bounded `sh -c` subprocess (production binding: `shell-command-projection.execTool`) at the project root, inheriting env; exit 0 ⇒ pass, non-zero ⇒ block, timeout (SIGTERM) ⇒ block; `artifact-frontmatter-equals` resolves a phase or project-level Markdown artifact through injected dependencies and compares a frontmatter field by scalar string value. Command predicates interpolate `${PHASE_NUMBER}`/`${PHASE_DIR}`/`${PHASE_REQ_IDS}` from gate context. A THROWN error (malformed predicate, non-positive/non-finite timeout, command >4096 chars, missing dependencies, unknown kind) maps at the CLI seam to a non-zero check-command exit, which the workflow's two-step gate contract treats as a step-1 command failure routed per `onError` — so an evaluator bug is never conflated with a legitimate block decision. Leaf pure module (no fs/child_process/config — subprocess and artifact seams injected). CLI entry: `gsd_run check predicate --predicate '' [--phase-dir …] [--phase-number …] [--phase-req-ids …] --raw`, wired into `check-command-router.cts:cmdCheckPredicate`; the four generic workflow gate-dispatch sites (`execute:wave:post`, `execute:post`, `plan:post`, and `ship:pre` — the last wired by #3559, which had resolved gates generically but enforced only two hardcoded `capId`s) branch on `check` shape (`query` vs `predicate`). Source of truth: `src/gate-predicate-evaluator.cts`. Docs: `docs/reference/gate-predicates.md`, `docs/how-to/command-exit-zero-gate.md`. +Pure, deps-injected evaluator for capability gate `check.predicate` blocks (#2008, ADR-2008). Prior to #2008 the registry validator accepted `check.predicate` (one of exactly-one-of `query`/`predicate`/`agentVerdict`) and the loop-resolver rendered it, but nothing EVALUATED a declared predicate — only `check.query` was enforced (dispatched via `gsd_run check `), and built-in gates like `security` worked only via hard-coded `capId` prose branches in `ship.md`/`execute-phase.md`/`verify-work.md`. This module is the generic evaluation path: `evaluatePredicate(predicate, context, deps) → { block, message, details? }` dispatches by `predicate.kind` through a `KIND_TABLE`. Built-in kinds: `command-exit-zero` runs a declared command in a bounded `sh -c` subprocess (production binding: `shell-command-projection.execTool`) at the project root, inheriting env; exit 0 ⇒ pass, non-zero ⇒ block, timeout (SIGTERM) ⇒ block; `artifact-frontmatter-equals` resolves a phase or project-level Markdown artifact through injected dependencies and compares a frontmatter field by scalar string value. Command predicates interpolate `${PHASE_NUMBER}`/`${PHASE_DIR}`/`${PHASE_REQ_IDS}` from gate context. A THROWN error (malformed predicate, non-positive/non-finite timeout, command >4096 chars, missing dependencies, unknown kind) maps at the CLI seam to a non-zero check-command exit, which the workflow's two-step gate contract treats as a step-1 command failure routed per `onError` — so an evaluator bug is never conflated with a legitimate block decision. Leaf pure module (no fs/child_process/config — subprocess and artifact seams injected). CLI entry: `gsd_run check predicate --predicate '' [--phase-dir …] [--phase-number …] [--phase-req-ids …] --raw`, wired into `check-command-router.cts:cmdCheckPredicate`. `--phase-dir` is CONFINED to the project root at that router boundary (#4354, epic #4636 Phase 2): an unconfined value let a BLOCKING gate return `block: false` on an artifact in a directory the caller chose, and the same value interpolates into `${PHASE_DIR}` for `command-exit-zero`, so confining at the boundary covers BOTH kinds while keeping this module's fs-free leaf contract intact — a fix inside the evaluator would have required giving a pure module a filesystem dependency and would still have missed the interpolation path. The four generic workflow gate-dispatch sites (`execute:wave:post`, `execute:post`, `plan:post`, and `ship:pre` — the last wired by #3559, which had resolved gates generically but enforced only two hardcoded `capId`s) branch on `check` shape (`query` vs `predicate`). Source of truth: `src/gate-predicate-evaluator.cts`. Docs: `docs/reference/gate-predicates.md`, `docs/how-to/command-exit-zero-gate.md`. ### Capability Registry Generated central manifest projecting all co-located Capability declarations into one validated artifact for runtime resolution and for the install, surface, config, and loop-extension adapters. Mirrors the research-profiles / package-identity generation pattern (co-located source → generated central file). Generated by `scripts/gen-capability-registry.cjs` → `gsd-core/bin/lib/capability-registry.cjs` (ADR-894 §5 phase 3a-impl). Role-partitioned indexes: `bySkill`, `byAgent`, `byLoopPoint` (hook ordering materialized), `configKeys` (ownership map: key→capId), `configSchema` (full per-key schema: key→{ owner, type, default, description }), `runtimes`, `requiresClosure(id)`. Each feature capability's entry in `capabilities` now includes the optional `activationKey` field (the dotted config key that gates the whole capability, e.g. `"graphify.enabled"`; absent means no config gate). ADR-857 phase 3b adds `configSchema` with validated type/default/description per key, sourced from each capability's `.config` slice. ADR-857 phase 4a adds two derived views: `capabilityClusters` (`{ : [] }` — each cap's skills array, sorted, derived from the capability's `skills` declaration; consistency-gated against the hand-authored `CLUSTERS`) and `profileMembership` (`{ : { tier, profiles: [...] } }` — the tier-derived index: suffix of `PROFILE_RANK` starting at the capability's tier). Both views cover the same capability set: only capabilities that own skills (non-empty `skills` array). The generator enforces a HARD gate (throws) if a capId matching a `CLUSTERS` key has a mismatched skill set, and emits SOFT `⚠ pending-reconciliation` warnings to stderr (never to the file) for skills not yet in the hand-authored profile at the capability's tier. `install` and `surface` are UNTOUCHED (still read hand-authored constants; derived views are emitted and tested but unconsumed until cutover). Validated against the Loop Host Contract (12 points; generated by `gen-loop-host-contract.cjs` from workflow markers, phase 3a-impl-2). Run `node scripts/gen-capability-registry.cjs --write` after editing any `capabilities//capability.json`. diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 5f03fcfdb..84719e689 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1177,6 +1177,14 @@ from `todos/pending/` to `todos/completed/` and upserts `completed:` and `status: completed` inside the file's frontmatter block. Unknown flags are 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. + ```bash # UAT audit — scan all phases for unresolved items node gsd-tools.cjs audit-uat diff --git a/docs/reference/gate-predicates.md b/docs/reference/gate-predicates.md index 1e44a0d4a..6c3fa9138 100644 --- a/docs/reference/gate-predicates.md +++ b/docs/reference/gate-predicates.md @@ -81,6 +81,15 @@ the gate context; all others are left untouched for `sh` to interpret: An undefined placeholder interpolates to the empty string. +**`--phase-dir` is confined to the project.** The value is validated to resolve +inside the project root before any predicate is evaluated; one that escapes is +rejected as a usage error rather than evaluated. This applies to both kinds — +`artifact-frontmatter-equals` resolves its artifact under that directory, and +`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. + **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 — there is no sandbox boundary vs. the user's own shell. See ADR-2008 "Trust diff --git a/src/check-command-router.cts b/src/check-command-router.cts index 9a6508996..008c3352a 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -90,7 +90,12 @@ function readIfExists(filePath: string): string { } function resolvePath(inputPath: string, projectDir: string): string { - return path.isAbsolute(inputPath) ? inputPath : path.join(projectDir, inputPath); + const candidate = path.isAbsolute(inputPath) ? inputPath : path.join(projectDir, inputPath); + const check = validatePath(candidate, projectDir, { allowAbsolute: true }); + if (!check.safe) { + error(`path escapes its allowed directory: ${inputPath}`, ERROR_REASON.USAGE); + } + return check.resolved; } interface WorkflowConfig { @@ -1194,8 +1199,17 @@ 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 phaseReqIds = args[3] ?? undefined; - const result = runGapAnalysis(projectDir, phaseDir, { phaseReqIds }); + const result = runGapAnalysis(projectDir, phaseDirCheck.resolved, { 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 @@ -1362,10 +1376,24 @@ function cmdCheckPredicate(projectDir: string, args: string[], raw: boolean): vo error('predicate --predicate value must be valid JSON', ERROR_REASON.USAGE); return; } + 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; + } const ctx = { cwd: projectDir, phaseNumber: flags['phase-number'], - phaseDir: flags['phase-dir'], + phaseDir: resolvedPhaseDir, phaseReqIds: flags['phase-req-ids'], }; let result; diff --git a/src/commands.cts b/src/commands.cts index 39b8c70be..fdefd0938 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -11,7 +11,7 @@ import path from 'node:path'; import { normalizeEol } from './text-lines.cjs'; import { execGit, platformWriteSync, platformReadSync, platformEnsureDir, isSpawnTimeout, retryRenameSync } from './shell-command-projection.cjs'; import { escapeRegex } from './pattern.cjs'; -import { requireSafePath, sanitizeForDisplay } from './security.cjs'; +import { requireSafePath, sanitizeForDisplay, validatePath } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error, ERROR_REASON } = ioMod; @@ -3491,11 +3491,29 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod const pendingDir = path.join(todosRoot, 'pending'); const completedDir = path.join(todosRoot, 'completed'); const sourcePath = path.join(pendingDir, filename as string); + const targetPath = path.join(completedDir, filename as string); + + const sourceCheck = validatePath(sourcePath, todosRoot, { allowAbsolute: true }); + if (!sourceCheck.safe) { + error(`todo file escapes its allowed directory: ${filename as string}`, ERROR_REASON.USAGE); + } + const targetCheck = validatePath(targetPath, todosRoot, { allowAbsolute: true }); + if (!targetCheck.safe) { + error(`todo file escapes its allowed directory: ${filename as string}`, ERROR_REASON.USAGE); + } if (!fs.existsSync(sourcePath)) { error(`Todo not found: ${filename as string}`); } + // #4652: `.` and `..` resolve to the pending dir itself (which IS inside + // 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()) { + error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE); + } + const content = fs.readFileSync(sourcePath, 'utf-8'); const today = realClock.localToday(); diff --git a/tests/check-gap-analysis-plan-post-e2e.test.cjs b/tests/check-gap-analysis-plan-post-e2e.test.cjs index c298e9c8d..3a3044539 100644 --- a/tests/check-gap-analysis-plan-post-e2e.test.cjs +++ b/tests/check-gap-analysis-plan-post-e2e.test.cjs @@ -777,7 +777,7 @@ describe('resolvePath / check decision-coverage-plan — containment boundary (# contextPath, '# Phase Context\n\n\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n\n', ); - fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n'); + fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\n## tasks\n\n- D-01: Use pattern X\n'); const relPhaseDir = path.relative(tmpDir, phaseDir); const result = runDecisionCoveragePlan([], relPhaseDir, contextPath, tmpDir); @@ -792,7 +792,7 @@ describe('resolvePath / check decision-coverage-plan — containment boundary (# contextPath, '# Phase Context\n\n\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n\n', ); - fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n'); + fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\n## tasks\n\n- D-01: Use pattern X\n'); const result = runDecisionCoveragePlan([], phaseDir, contextPath, tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); diff --git a/tests/check-predicate.test.cjs b/tests/check-predicate.test.cjs index 6dd35181c..2f3a2b035 100644 --- a/tests/check-predicate.test.cjs +++ b/tests/check-predicate.test.cjs @@ -255,6 +255,44 @@ describe('check predicate --phase-dir — containment boundary (#4354)', () => { assert.strictEqual(parsed.block, false, 'in-project phase-dir evaluation must still pass'); }); + test('[regression #4652] a relative --phase-dir must resolve against --cwd, not the real process cwd, and must not leak the outside file', () => { + // The real process cwd (outsideDir) contains a foreign SECURITY.md; the + // CLI is told --cwd projDir with a relative --phase-dir '.'. Before #4652, + // cmdCheckPredicate validated the joined (projDir + '.') path but passed + // the RAW, un-joined '.' into ctx.phaseDir, which findPhaseArtifact then + // resolved against the real process cwd (outsideDir) — leaking foreign + // frontmatter. The fix must reject this, and the leaked value must never + // appear in the output. + fs.writeFileSync( + path.join(outsideDir, 'SECURITY.md'), + '---\nstatus: LEAKED_VALUE\n---\n# Security\n', + ); + + const predicate = JSON.stringify({ + kind: 'artifact-frontmatter-equals', + artifact: 'SECURITY.md', + field: 'status', + equals: 'NOPE', + }); + + const result = runGsdTools( + ['--json-errors', 'check', 'predicate', '--cwd', projDir, '--predicate', predicate, '--phase-dir', '.', '--raw'], + outsideDir, + ); + + const combinedOutput = `${result.output || ''}${result.error || ''}`; + assert.ok( + !combinedOutput.includes('LEAKED_VALUE'), + `the outside file's frontmatter value must never leak into the output (got: ${combinedOutput})`, + ); + assert.strictEqual( + result.success && JSON.parse(result.output).block === false, + false, + `a relative --phase-dir must not resolve against the real process cwd and must not pass ` + + `(currently: ${combinedOutput})`, + ); + }); + 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 30280193c..c075ba618 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -871,6 +871,25 @@ describe('todo complete — containment boundary (#4327)', () => { }); } + // '.' and '..' resolve to the pending dir itself (which IS inside the + // root, so containment passes) but are not a todo name — before #4652 + // this fell through to an uncaught EISDIR with an absolute-path stack + // trace instead of a clean rejection. + for (const name of ['.', '..']) { + test(`[regression #4652] "todo complete ${name}" is rejected cleanly (no uncaught EISDIR / stack trace)`, () => { + const result = runGsdTools(['todo', 'complete', name], tmpDir); + assert.strictEqual(result.success, false, `"${name}" must be rejected`); + assert.ok( + !/at\s+\S+\s+\(.*\.c?ts?:\d+/.test(result.error || ''), + `rejection must not leak a stack trace (got: ${result.error})`, + ); + assert.ok( + !(result.error || '').includes('EISDIR'), + `rejection must be a clean USAGE error, not an uncaught EISDIR (got: ${result.error})`, + ); + }); + } + test('[RED #4327] an absolute path outside the project is rejected', () => { const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); try { From 9c20b7b40a4502bcbb9d2a5d2b8853dcd0c136e3 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 09:43:36 -0400 Subject: [PATCH 3/6] 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 }); }); }); From c7956c162b0c7d3cb842d636b2603ce8c2066031 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 10:07:40 -0400 Subject: [PATCH 4/6] =?UTF-8?q?fix(#4652):=20a=20todo=20name=20must=20be?= =?UTF-8?q?=20a=20basename=20=E2=80=94=20containment=20alone=20cannot=20sa?= =?UTF-8?q?y=20that?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The verification checkpoint caught a real design gap, not a flaky test. Confining sourcePath/targetPath within todosRoot correctly rejects `../../escaped`, which leaves the root. It does NOT reject these, because they all land inside it: ../sibling.md -> /sibling.md escapes pending/, not the root a/../../b.md -> /b.md same sub/name.md -> /sub/name.md inside pending/, but nested Three committed tests asserted these must be rejected and were right: the design says "a todo name is a basename, not a path", and #4327 requires the resolved path stay inside "the todos root (pending and completed subdirs)". Containment against a root is structurally incapable of expressing "basename" — it answers "is this inside?", and all three are. The wrong tool was reaching for the wrong question. A basename guard now runs BEFORE any path is joined: reject on a `/` or `\` separator, on a path.basename / path.win32.basename mismatch, on `.` / `..`, and on a NUL byte. Both separators are checked explicitly because on POSIX a literal backslash is an ordinary filename character to path.basename but not to path.win32.basename or to the user's intent — this repo has a documented bug class for exactly that asymmetry. Same predicate shape as findPhaseArtifact in check-command-router.cts, so the two agree. Containment is kept as defense-in-depth rather than replaced. The basename guard is the specific rule; containment is the backstop. Message wording matters here and is deliberate: `sub/name.md` does NOT escape its allowed directory, so reusing the escape message would have stated something false. It now says the name must be a plain filename, not a path. docs/CLI-TOOLS.md corrected again, in the opposite direction from last time. The previous revision said an absolute filename is "folded under the root" and 404s — true then, false now: the basename guard rejects it before any join happens. Two corrections to one paragraph in one phase is the cost of documenting behavior while it is still moving; the paragraph now matches the shipped code. Verified through the real CLI, not by calling the built function directly: all eight rejection cases produce the new USAGE message; `ok.md` still completes and moves to completed/; `missing.md` still gives "Todo not found". Also corrected the now-stale comment above the isFile() check — it described `.`/`..` reaching that line, which the basename guard now prevents. The check itself stays: a bare basename can still name a directory, FIFO or socket in pending/. Co-Authored-By: Claude Opus 5 --- docs/CLI-TOOLS.md | 25 +++++++++++++------------ src/commands.cts | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index ff979dbf0..0d863d6fe 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1177,18 +1177,19 @@ from `todos/pending/` to `todos/completed/` and upserts `completed:` and `status: completed` inside the file's frontmatter block. Unknown flags are rejected loudly. -`` is a **basename inside the todos root**, not a path. A value that -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. +`` is a **basename inside the todos root**, not a path. A basename +guard runs first, before `` is joined onto any directory: a value +containing an embedded separator (either `/` or `\`, e.g. `sub/name.md` or +`sub\name.md`), a value whose own basename differs from itself (e.g. +`a/../../b.md`, `../sibling.md`), a bare `.` or `..`, an absolute path (e.g. +`/etc/passwd`), or a NUL byte is rejected as a usage error **before** any file +is read or moved (#4327, #4652). A traversal that only escapes the `pending`/ +`completed` subdirectory without leaving the todos root (`../sibling.md`) is +caught by this same guard, not by containment. Containment against the todos +root still runs afterward as defense-in-depth for the resolved source and +target paths, so neither half of the move can land outside the root. The +check covers both halves of the move, 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/src/commands.cts b/src/commands.cts index 362fffd3b..663e02b39 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -3490,6 +3490,30 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod const todosRoot = todosDir(cwd); const pendingDir = path.join(todosRoot, 'pending'); const completedDir = path.join(todosRoot, 'completed'); + + // #4652: containment against todosRoot only rejects paths that leave the + // root — it cannot express "a todo name is a basename, not a path" (see + // #4327). `../sibling.md`, `a/../../b.md`, and `sub/name.md` all resolve + // to a location inside todosRoot (or inside pending/) and would pass + // containment, yet none of them is a bare filename. Reject on basename + // shape FIRST, before any path is even joined — same predicate shape as + // findPhaseArtifact in check-command-router.cts. Checking both `/` and + // `\` explicitly (not just path.basename) matters on POSIX, where a + // literal backslash is just an ordinary filename character to + // path.basename but not to path.win32.basename or to the user's intent. + const rawFilename = filename as string; + if ( + rawFilename === '.' || + rawFilename === '..' || + rawFilename.includes('\0') || + rawFilename.includes('/') || + rawFilename.includes('\\') || + path.basename(rawFilename) !== rawFilename || + path.win32.basename(rawFilename) !== rawFilename + ) { + error(`todo name must be a plain filename inside the pending directory, not a path: ${rawFilename}`, ERROR_REASON.USAGE); + } + const sourcePath = path.join(pendingDir, filename as string); const targetPath = path.join(completedDir, filename as string); @@ -3509,10 +3533,13 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod error(`Todo not found: ${filename as string}`); } - // #4652: `.` and `..` resolve to the pending dir itself (which IS inside - // 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. + // #4652: a name that IS a bare basename can still resolve to something that + // is not a regular file — a directory, symlink-to-directory, FIFO or socket + // sitting in pending/ under an ordinary-looking name. `.` and `..` no longer + // reach here (the basename guard above rejects them first), so this is not + // about traversal; it stops fs.readFileSync from throwing an uncaught EISDIR + // with an absolute-path stack trace where every sibling case gives a clean + // USAGE rejection. if (!fs.statSync(resolvedSource).isFile()) { error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE); } From f64f8a0e7b01fa1d938aa1e4036f9233c5f7e536 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 10:24:48 -0400 Subject: [PATCH 5/6] test(#4652): correct the absolute-filename test to match the basename guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test asserted that an absolute filename is folded under the pending dir and fails as "not found" rather than being rejected. That held for exactly one commit. The basename guard rejects any name containing a separator before any join happens, so an absolute path never reaches containment or the filesystem at all. Now asserts the USAGE rejection the CLI actually emits, verified by running it. All four outside-file protections are kept unchanged — the file still exists, its content is byte-identical, it never lands in completed/, and cleanup runs in finally. Those are the assertions that carry the security value; only the claim about HOW the rejection happens was stale. Co-Authored-By: Claude Opus 5 --- tests/commands.test.cjs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index dc97f7360..a03efb6b9 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -890,25 +890,24 @@ describe('todo complete — containment boundary (#4327)', () => { }); } - 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. + test('[#4327] an absolute filename is rejected as a non-basename before any join — the outside file is untouched', () => { + // A basename guard added since #4327 rejects any filename containing `/` + // or `\` BEFORE it is ever joined against pendingDir — so an absolute + // path never reaches path.join, containment, or the filesystem at all. + // It is a USAGE rejection, not a containment/escape check and not a + // plain "not found". This test also pins the thing that actually + // matters: the real outside file is never read, moved, or deleted. const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-')); try { const outsideFile = path.join(outsideDir, 'evil.md'); const sentinel = '---\nstatus: pending\n---\nSENTINEL\n'; fs.writeFileSync(outsideFile, sentinel); - const result = runGsdTools(['todo', 'complete', outsideFile], tmpDir); + const result = runGsdTools(['--json-errors', 'todo', 'complete', outsideFile], tmpDir); - 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.strictEqual(result.success, false, 'the command must fail (a filename containing a separator is rejected)'); + const parsed = JSON.parse(result.error); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'usage'); assert.ok(fs.existsSync(outsideFile), 'the real outside file must still exist'); assert.strictEqual( fs.readFileSync(outsideFile, 'utf-8'), From e1f169a378b163aefa0259e64490e40d89964f4b Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 10:41:11 -0400 Subject: [PATCH 6/6] chore(#4652): backfill changeset PR number to 4666 Co-Authored-By: Claude Opus 5 --- .changeset/sharp-tigers-sing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/sharp-tigers-sing.md b/.changeset/sharp-tigers-sing.md index 6e1ad8f45..840c17164 100644 --- a/.changeset/sharp-tigers-sing.md +++ b/.changeset/sharp-tigers-sing.md @@ -1,5 +1,5 @@ --- type: Security -pr: 0 +pr: 4666 --- **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)