diff --git a/.changeset/sharp-tigers-sing.md b/.changeset/sharp-tigers-sing.md new file mode 100644 index 000000000..840c17164 --- /dev/null +++ b/.changeset/sharp-tigers-sing.md @@ -0,0 +1,5 @@ +--- +type: Security +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) 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..0d863d6fe 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1177,6 +1177,20 @@ 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 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 node gsd-tools.cjs audit-uat diff --git a/docs/reference/gate-predicates.md b/docs/reference/gate-predicates.md index 1e44a0d4a..ba6091ebd 100644 --- a/docs/reference/gate-predicates.md +++ b/docs/reference/gate-predicates.md @@ -81,6 +81,18 @@ 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. `${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 — there is no sandbox boundary vs. the user's own shell. See ADR-2008 "Trust 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 9a6508996..576116a57 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,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 resolvedPhaseDir = resolvePath(phaseDir, projectDir); const phaseReqIds = args[3] ?? undefined; - const result = runGapAnalysis(projectDir, phaseDir, { 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 @@ -1362,10 +1368,15 @@ 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 !== '') { + resolvedPhaseDir = resolvePath(rawPhaseDir, projectDir); + } 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..663e02b39 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; @@ -3490,13 +3490,61 @@ 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'); - const sourcePath = path.join(pendingDir, filename as string); - if (!fs.existsSync(sourcePath)) { + // #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); + + 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); + } + + const resolvedSource = sourceCheck.resolved; + const resolvedTarget = targetCheck.resolved; + + if (!fs.existsSync(resolvedSource)) { error(`Todo not found: ${filename as string}`); } - const content = fs.readFileSync(sourcePath, 'utf-8'); + // #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); + } + + const content = fs.readFileSync(resolvedSource, 'utf-8'); const today = realClock.localToday(); // #4096: --dry-run mirrors `milestone complete --dry-run` (#2118) — every @@ -3509,8 +3557,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); @@ -3523,8 +3571,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-gap-analysis-plan-post-e2e.test.cjs b/tests/check-gap-analysis-plan-post-e2e.test.cjs index 039918f04..3a3044539 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\n## tasks\n\n- D-01: Use pattern X\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\n## tasks\n\n- D-01: Use pattern X\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..c890e7bbd 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,219 @@ 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 #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('[#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'), + '---\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..a03efb6b9 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,226 @@ 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'})`, + ); + }); + } + + // '.' 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('[#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(['--json-errors', 'todo', 'complete', outsideFile], tmpDir); + + 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'), + 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); + } + }); + + // ── 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); + } + }); + + 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 */ } + } + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // todo match-phase command // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/security.test.cjs b/tests/security.test.cjs index e75f0944e..f7e34d523 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)}`, + ); + }, + ), { seed: 4652, numRuns: 200 }); + }); + + 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)); + }, + ), { seed: 4652, numRuns: 200 }); + }); +}); + +// ─── #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); + } + }); + } +});