From 374300da17fb7b180c071c60c00cdf143d641c85 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 09:33:32 -0400 Subject: [PATCH] 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 {