From bbdf7e8e84b965d3a625bcc79b5667fcb59befc5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 12 Sep 2026 22:17:46 -0400 Subject: [PATCH] =?UTF-8?q?chore(#4654):=20add=20local/no-unconfined-path-?= =?UTF-8?q?join=20and=20drain=20it=20to=20zero=20=E2=80=94=20Phase=204=20o?= =?UTF-8?q?f=20#4636=20(#4674)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(#4654): add local/no-unconfined-path-join and drain it to zero Phase 4 of epic #4636 — the ratchet, and the phase that makes the epic hold. THE MEASUREMENT THAT RESHAPED THE PHASE. An AST census (the repo's own parser, not grep) found what the epic never enumerated: ADR-4650 named seven containment implementations; `src/` alone held roughly 24 more hand-rolled gates across ~13 files, several guarding a write or an `fs.rmSync`. Two verified by reading rather than pattern-matching — `research-store.cts` comments its own as "ensure the resolved file path stays inside the store dir" immediately before a write, and `capability-lifecycle.cts` gates `fs.rmSync` with one. So the epic's Done-when "one containment predicate, used at every site" was FALSE when Phase 3 reported it satisfied. It is true now: the rule is clean across src/, scripts/, gsd-core/bin/ and hooks/ with an EMPTY allowlist. WHY NOT THE RULE THE ISSUE PROPOSED. #4654 proposed flagging `path.join` whose first argument is a managed root and whose later arguments derive from argv. That is a taint analysis over 2046 call sites, in ESLint, without type information; "derives from argv" is not locally decidable. Any approximation either floods or is trivially evaded, and a rule that fires on hundreds of correct sites earns an allowlist of hundreds — the opposite of a ratchet. What is actually duplicated is the COMPARISON, not the join, and that has one recognizable shape. Arm 1 X.startsWith(Y + sep) the hand-rolled containment idiom Arm 2 a containment predicate called as a bare statement, answer discarded Arm 2 is the issue's "asserts the result was narrowed, not merely that a helper was called". Its example `validatePath(x, root).resolved` is already structurally impossible — Phase 3 un-exported `validatePath` — so the remaining expressible failure is ignoring the answer, which is the defect that recurred five times in this epic. The census found exactly one live instance (`milestone.cts:1643`); it now returns the proven `ContainedPath` so consumers stop re-deriving the path the comment above it was extracted to stop them re-deriving. The rule deliberately does NOT try to catch validate-one-path-use-another where the answer is used but a different variable flows onward. That needs flow analysis; the branded `ContainedPath` from Phase 3 is the defense there, and the two are complementary. PER-SITE FAMILY CHOICE, NOT A DEFAULT. Phase 3's lesson binds: collapsing a lexical site onto the realpath family broke four tests and was caught only by the matrix. Every migrated site was triaged individually. The six installer-migrations tree-walks and the six capability-lifecycle gates take the LEXICAL family because their operands are already realpath-resolved and they deliberately treat the final component as a link; boundary sites take realpath. TWO SITES WITH AN INVERTED CONTRACT, which a mechanical swap would have broken. `installer-migrations.cts:127` and `runtime-artifact-install-plan.cts:144` REJECT `target === root` by contract, while the canonical comparison ACCEPTS it. Swapped naively, a migration could `rmdir` the user's config root and a third-party descriptor could write at configHome itself. Both keep `=== root` as an explicit additional arm alongside the predicate call — the predicate decides containment, the call site keeps its own extra condition (ADR-4650 decision 6). ONE DUPLICATE DELETED OUTRIGHT: `planning-inspect.cts`'s `isWithinRoot` was byte-identical to `isContainedIn` and said so in its own docstring. `isContainedIn` is now exported for callers that have already resolved both operands and need only the comparison, with a doc note that a caller which has NOT resolved them must use a full predicate instead. THE MARKER, AND WHY IT IS NOT THE ALLOWLIST. Nine sites are justified holdouts and carry `// allow-handrolled-containment: ` with a mandatory, reviewable reason. Two justifications: (a) not a containment decision — an ancestor-walk loop condition, sub-repo grouping, worktree identity matching, declared-path coverage; (b) it IS containment but the canonical predicate is unreachable — `capability-validator.cjs` is a committed pre-build `.cjs` and the compiled `security.cjs` is untracked build output, so requiring it would break a fresh clone. `scripts/lib/drift-scan.cjs` runs under `lint:ci` with the same exposure. The marker was renamed from `allow-lexical-prefix-match` mid-phase because that name asserted only (a) and would have stated something false at the (b) sites. A marker suppresses BEFORE the violation counter increments, so a file whose every occurrence is marked still reports `staleAllowlistEntry` — otherwise a drained entry lingers and silently re-permits the site later. DEMONSTRATED RED, per #4654: a hand-rolled copy reintroduced into a real `src/` file made `npm run lint` fail with the rule's full guidance message; removing it returned the tree to clean. Both halves recorded — red alone proves nothing, since a rule red for an unrelated reason looks identical. DISCLOSED: `defaultRequireFromInstallRoot` (gsd-tools.cjs) previously carried two distinct rejection messages and two manual realpath calls; routing it through `tryWithinRoot` collapses them to one message, and a missing module now surfaces as MODULE_NOT_FOUND rather than ENOENT. No test asserts either message. The security property is preserved and slightly strengthened — the candidate is realpathed and containment re-checked, and the dangling-symlink oracle closure comes along with it. Co-Authored-By: Claude Opus 5 * docs(#4654): record the containment ratchet in CONTEXT.md and the security model Both entries previously described the seam without the thing that keeps it a seam. They now state what the rule bans, and — more usefully for whoever reads this next — what it deliberately does NOT attempt: deciding per path.join call whether an argument came from user input. That question is not locally decidable, and an approximation across ~2000 join sites would earn an exemption list of hundreds, which is the opposite of a ratchet. Also records the marker's two legitimate justifications and that its reason is mandatory, so the escape stays reviewable rather than becoming a mute button. Glossary gate 270 refs exit 0; install-tree goldens and CONTEXT-INDEX.json regenerated and confirmed byte-identical rather than assumed — which also confirms eslint-rules/ is not a shipped path. Co-Authored-By: Claude Opus 5 * fix(#4654): close review findings and the two matrix failures MATRIX FAILURE 1 — a collapsed message broke a negative-proof test, and my evidence for collapsing it was wrong. I searched tests/ for the literal string "resolves outside its install root", found nothing, and reported that no test asserted it. The test matches a REGEX SUBSTRING, /outside its install root/, so the literal search missed it. What broke was "NEGATIVE PROOF: a symlinked module pointing OUTSIDE the install root is not loaded" — the test guarding the exact property I claimed was preserved. defaultRequireFromInstallRoot now does both checks again with both messages byte-identical, each routed through the canonical predicate, which is better than the original since that hand-rolled both comparisons. MATRIX FAILURE 2 — shipped migrations are checksum-locked, and a marker cannot serve there. migrationChecksum hashes plan.toString(), which INCLUDES comments, so a suppression marker inside a plan body drifts the baseline exactly as an edit does. Measured: with markers in place, two of the four still differed from their committed checksums. The four shipped bodies are now byte-identical to next, and the rule's config excludes those four paths BY NAME rather than by a directory wildcard, so a NEW migration is still covered. Six containment comparisons stay un-ratcheted there; that gap is recorded in the rule's Known gaps, in CONTEXT.md and in the security model rather than left implicit. Justification (c) is removed from the marker's documented reasons, because a marker was proven unable to express it. ADVERSARIAL REVIEW — the sharpest finding was that the rule banned the CORRECT shape while permitting the incorrect one: startsWith(root) with no separator is the genuinely unsafe form, since it accepts a sibling such as root-evil, and my own test blessed it as valid. Flagging every bare startsWith would swamp the rule, so that stays a STATED gap rather than a silent one. Closed for real: the template-literal spelling, which the census never saw because it only inspected plus-concatenation — that surfaced TWELVE more sites, now triaged and migrated. A separator reached through a const alias is now resolved via scope analysis. And isContainedIn, exported in Phase 3, was missing from the discarded-result set, so a bare no-op call went unflagged on the one function the epic funnels through. SECURITY REVIEW — the marker could over-suppress two ways: a block comment worked identically to a line comment, and one marker silently covered every violation sharing its line. It now requires a Line comment positioned after the flagged node ends, so it anchors to the node it trails. Four sites had dropped an unreachable-but-deliberate equality rejection against the root; each is restored as the call site's own arm. eslint.config.mjs still documented the OLD marker token, which my rename missed — it would have sent the next author in circles. A FALSE GREEN, recorded because it nearly stuck: lint:ci reported exit 0 from a stale eslint cache while twelve real violations existed. Every lint check here now clears the cache first. Co-Authored-By: Claude Opus 5 * fix(#4654): anchor a suppression marker to the violation it actually trails The matrix caught this; my own test caught it, on its first execution. The case "two violations on one line: trailing marker suppresses only the one it trails" expected 1 error and got 0 — both were suppressed. ROOT CAUSE: the anchoring accepted any Line comment on the node's line whose range started at or after the node's end. A trailing marker at the END of a line sits after EVERY node on that line, so that condition held for all of them. "After the node" does not identify WHICH node the marker trails. The fix reads as correct and is not. FIX: deferred reporting. Violations accumulate during traversal instead of being reported immediately; at Program:exit each marker claims exactly ONE pending violation — the one on its line whose end is nearest before the marker begins — and every unclaimed violation is then counted and reported. One marker, one suppression. An earlier violation sharing the line is still reported, which is the property the security review asked for and the previous attempt only appeared to deliver. The counter now increments at flush time rather than during traversal, so a suppressed occurrence still does not keep an allowlist entry alive. AND A TOOL THAT SHOULD HAVE EXISTED BEFORE THE FIRST MATRIX RUN. `node --test` is hard-blocked here, so this rule's test file could only ever be executed on the remote matrix — which is why a broken anchoring shipped into a run. ESLint's programmatic Linter API is not a test runner, and exercising the rule through it verifies every case locally in seconds. All 24 now pass locally, including the two-on-one-line case that failed remotely. That loop should have been built before the rule was first sent to the matrix rather than after it failed twice. Co-Authored-By: Claude Opus 5 * chore(#4654): backfill PR 4674 into the changeset and complete 70-docs.json The phase gate requires enablementSequence and the Diataxis quadrants; 70-docs now carries both, with the how-to quadrant skipped for a stated reason rather than an empty field. The audience for this deliverable is a contributor who trips the rule, and the task-oriented guidance reaches them in the ESLint message itself — which names the correct predicate, says how to choose between the realpath and lexical families, cites the Phase 3 regression caused by choosing wrong, and gives the marker syntax. A docs/how-to page would be a second, driftable copy read by nobody at the moment of failure. enablementSequence is recorded as what it actually is: a VERIFICATION sequence, not an enablement one. The rule is never off, so there is no off-to-on transition to describe. scripts/lint-docs-required.cjs now passes (ok_docs_updated) — it could not evaluate against the mandated pr:0 placeholder. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/silly-zebras-roar.md | 5 + CONTEXT.md | 2 +- docs/explanation/security-model.md | 28 + .../no-unconfined-path-join.allowlist.json | 1 + eslint-rules/no-unconfined-path-join.cjs | 426 +++++++++++++++ eslint.config.mjs | 48 ++ gsd-core/bin/gsd-tools.cjs | 22 +- gsd-core/bin/lib/capability-validator.cjs | 2 +- gsd-core/bin/verify-reapply-patches.cjs | 2 +- scripts/affected-tests-lib.cjs | 2 +- scripts/diff-touches-shipped-paths.cjs | 2 +- scripts/gen-adr-index.cjs | 10 +- scripts/lib/drift-scan.cjs | 2 +- scripts/lint-source-test-name-collision.cjs | 2 +- src/capability-lifecycle.cts | 16 +- src/check-command-router.cts | 28 +- src/code-review-depth.cts | 4 +- src/commands.cts | 12 +- .../worktree-health.cts | 2 +- src/install-engine.cts | 10 +- src/installer-migrations.cts | 12 +- src/milestone.cts | 29 +- src/planning-inspect.cts | 19 +- src/refactor-trigger-command-router.cts | 8 +- src/research-store.cts | 23 +- src/reviewer-step-dispatch.cts | 10 +- src/runtime-artifact-install-plan.cts | 15 +- src/runtime-artifact-layout.cts | 18 +- src/security.cts | 12 +- src/task-command-router.cts | 18 +- src/verification.cts | 8 +- src/verify-command-grounding.cts | 2 +- src/verify.cts | 10 +- src/worktree-safety.cts | 19 +- tests/no-unconfined-path-join.rule.test.cjs | 499 ++++++++++++++++++ 35 files changed, 1214 insertions(+), 114 deletions(-) create mode 100644 .changeset/silly-zebras-roar.md create mode 100644 eslint-rules/no-unconfined-path-join.allowlist.json create mode 100644 eslint-rules/no-unconfined-path-join.cjs create mode 100644 tests/no-unconfined-path-join.rule.test.cjs diff --git a/.changeset/silly-zebras-roar.md b/.changeset/silly-zebras-roar.md new file mode 100644 index 000000000..e5a1162d5 --- /dev/null +++ b/.changeset/silly-zebras-roar.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 4674 +--- +**Every path-containment check in the tree now routes through one predicate, enforced by lint** — around two dozen hand-rolled containment comparisons were still scattered across installers, capability lifecycle, research storage and command routing; each now takes its decision from the canonical predicate while keeping its own behavior. A new lint rule bans the hand-rolled shape and a discarded containment answer, so a reintroduced copy fails the build. Two rejection messages in capability module loading collapse into one, and a missing module now reports as a module-resolution failure rather than a file-not-found. (#4654) diff --git a/CONTEXT.md b/CONTEXT.md index 102d7036e..652b62806 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -330,7 +330,7 @@ Module owning install-time staging and content-rewrite selection for a pre-resol Narrow, enumerated fs seam for the `installRuntimeArtifacts` call tree (`src/install-engine.cts`) — lands ADR-58's never-shipped `cleanup` rollout step (`registry → adapter → helpers → cleanup`, #2874, epic #2866 Phase 5). `installRuntimeArtifacts` now returns the executed plan it ran (`{ runtime, scope, kinds: [{kind, sourceDir, destDir, preserved}], cleanup: [{dir, ok}], postSteps }`) instead of `void`, including on the `combinedFamilyInstall` (OpenCode/Kilo) early-return path — no path may return `undefined` after this phase. Failure is unchanged: stage/rewrite errors still throw rather than becoming an `ok:false` value, so a caller cannot read success-shaped data off a failure path. Delivery is an ambient single mutable adapter (`current`), swapped for the duration of one synchronous install via `withInstallFs(deps.fs, fn)` and always restored in a `finally` — a `deps` parameter threaded through every function on the call tree (`install-profiles.cts`, `runtime-artifact-conversion.cts`, `commonjs-marker.cts`, `installer-migrations.cts`'s two reachable entry points) was rejected as a dozen+-site touch for no behavioral gain over the ambient swap, extending rather than replacing `createRuntimeArtifactInstallPlan`'s existing `deps` bag precedent (Runtime Artifact Install Plan Module). An injected `deps.fs` is a PARTIAL adapter merged over real `node:fs`; any method it omits — except `realpathSync`, the one method documented to degrade gracefully — now THROWS immediately if actually called, naming the missing method, rather than silently resolving to the real filesystem (#2875 defect fix: the prior silent fall-through let a fake adapter missing e.g. `rmSync` perform real destructive IO unnoticed). **Routes destination IO only, by design**: every write/probe against the install destination (copies, removals, snapshot/restore of preserved skill dirs, the manifest read) is fake-able; locating this package's own source tree (`findInstallSourceRoot`/`findAgentsSourceRoot`'s walk-up-from-`__dirname`, `readGsdCommandNames`) stays real and unrouted — a destination-fake's store starts empty and was never seeded with the repo's own paths, so routing that lookup would make every fake-adapter install throw instead of staging. The symlink-escape guard (`hasExistingSymlinkBetween`) and `assertDestWithinConfigHome` keep their REFUSAL DECISIONS outside this seam — only their `existsSync`/`lstatSync`/`realpathSync` probes route through it, so an injected fake can change what a probe observes but never flip the security decision itself. Writes remain byte-identical to pre-#2874 (AC4/AC5); existing `void`-ignoring callers (`bin/install.js`) are unaffected. Source: `gsd-core/bin/lib/install-fs-adapter.cjs` (generated from `src/install-fs-adapter.cts`). See Runtime Artifact Install Plan Module, ADR-58. ### Path Containment Seam -One predicate answers "does this path resolve inside this root?" for the whole tree (epic #4636, ADR-4650). `src/security.cts` owns the resolution engine — realpath-based, with a closed dangling-symlink existence oracle and ancestor canonicalization so a not-yet-created path under a non-canonical base (macOS `/var` vs `/private/var`) still resolves — and that engine is NOT exported: the module-internal `validatePath` returns `{ safe, resolved, error }` and populates `resolved` with the ESCAPING path on the traversal branch, so a caller who skipped the boolean would get an attacker-controlled value precisely in the dangerous case. The exported surface is three shapes that cannot do that: `assertWithinRoot(candidate, root, label?, policy?)` throws, `tryWithinRoot(candidate, root, policy?)` returns exactly `null`, and `requireSafePath` is a preserved alias of the throwing form. All three return the branded `ContainedPath` — a plain `string` is not assignable to it, so validating one path and then handing a different one to the filesystem is a type error rather than a silent bug. `policy` is the named `PathAcceptance` acceptance policy (`RelativeOnly` | `AbsoluteInsideRoot`) that replaced a per-call-site `{ allowAbsolute: true }` boolean, which read as "containment is relaxed here" when in fact an absolute path outside the root was always rejected. The rejection text `Path escapes allowed directory: is outside ` is an observable CLI contract, surfaced through command `reason` fields and asserted by `tests/quick-batch.test.cjs`. **A wrapper may decide HOW to degrade, never WHETHER a path is contained** (ADR-4650 decision 6): `planning-inspect.cts`'s `isPathContained` takes its decision from the predicate but keeps must-exist as its own separate condition, because the canonical engine deliberately ACCEPTS a not-yet-created path under the root while its callers guard an `fs` read that is about to happen. **Containment is decided in exactly one place, but resolved two ways.** `isContainedIn` — the separator-aware comparison, so a sibling sharing a prefix (`-evil`) is never accepted — is module-internal and is the single decision. Two exported families sit on it and differ ONLY in how a candidate is resolved before that decision: `assertWithinRoot`/`tryWithinRoot` realpath-resolve, and `assertWithinRootLexical`/`tryWithinRootLexical` use `path.resolve` alone and never touch the filesystem (they accept `opts.pathImpl` so win32 separator semantics are testable off Windows). **A lexical check CANNOT SEE A SYMLINK**, so a caller relying on one for a write-confinement guarantee must pair it with its own symlink refusal. Three call sites legitimately need the lexical resolution and route their DECISION through it: `gsd-core/bin/gsd-tools.cjs`'s restore gate, which additionally rejects symlinks OUTRIGHT (the realpath predicate accepts a link resolving inside the root, which for a restore still writes through it) and treats `target === root` as NOT contained via its own extra check; `external-descriptor-trust.cts`'s `isPathConfined`, because two install callers validate a `destSubpath` BEFORE the `mkdirSync` that creates it, where realpath cannot resolve; and `installer-migrations.cts`'s `ensureInsideConfig`, because that module snapshots, restores and backs up a symlinked managed path AS A LINK — resolving it dereferenced exactly those links and rejected them for escaping `configDir`, which the remote matrix caught as four failing tests. `normalizeRelPath` is that one's real pre-gate, throwing on absolute paths and `..` before containment runs. Source of truth: `gsd-core/bin/lib/security.cjs` (generated from `src/security.cts`). See Gate Predicate Evaluator Module, Runtime Artifact Install Plan Module, Real Home Guard Module. +One predicate answers "does this path resolve inside this root?" for the whole tree (epic #4636, ADR-4650). `src/security.cts` owns the resolution engine — realpath-based, with a closed dangling-symlink existence oracle and ancestor canonicalization so a not-yet-created path under a non-canonical base (macOS `/var` vs `/private/var`) still resolves — and that engine is NOT exported: the module-internal `validatePath` returns `{ safe, resolved, error }` and populates `resolved` with the ESCAPING path on the traversal branch, so a caller who skipped the boolean would get an attacker-controlled value precisely in the dangerous case. The exported surface is three shapes that cannot do that: `assertWithinRoot(candidate, root, label?, policy?)` throws, `tryWithinRoot(candidate, root, policy?)` returns exactly `null`, and `requireSafePath` is a preserved alias of the throwing form. All three return the branded `ContainedPath` — a plain `string` is not assignable to it, so validating one path and then handing a different one to the filesystem is a type error rather than a silent bug. `policy` is the named `PathAcceptance` acceptance policy (`RelativeOnly` | `AbsoluteInsideRoot`) that replaced a per-call-site `{ allowAbsolute: true }` boolean, which read as "containment is relaxed here" when in fact an absolute path outside the root was always rejected. The rejection text `Path escapes allowed directory: is outside ` is an observable CLI contract, surfaced through command `reason` fields and asserted by `tests/quick-batch.test.cjs`. **A wrapper may decide HOW to degrade, never WHETHER a path is contained** (ADR-4650 decision 6): `planning-inspect.cts`'s `isPathContained` takes its decision from the predicate but keeps must-exist as its own separate condition, because the canonical engine deliberately ACCEPTS a not-yet-created path under the root while its callers guard an `fs` read that is about to happen. **Containment is decided in exactly one place, but resolved two ways.** `isContainedIn` — the separator-aware comparison, so a sibling sharing a prefix (`-evil`) is never accepted — is module-internal and is the single decision. Two exported families sit on it and differ ONLY in how a candidate is resolved before that decision: `assertWithinRoot`/`tryWithinRoot` realpath-resolve, and `assertWithinRootLexical`/`tryWithinRootLexical` use `path.resolve` alone and never touch the filesystem (they accept `opts.pathImpl` so win32 separator semantics are testable off Windows). **A lexical check CANNOT SEE A SYMLINK**, so a caller relying on one for a write-confinement guarantee must pair it with its own symlink refusal. Three call sites legitimately need the lexical resolution and route their DECISION through it: `gsd-core/bin/gsd-tools.cjs`'s restore gate, which additionally rejects symlinks OUTRIGHT (the realpath predicate accepts a link resolving inside the root, which for a restore still writes through it) and treats `target === root` as NOT contained via its own extra check; `external-descriptor-trust.cts`'s `isPathConfined`, because two install callers validate a `destSubpath` BEFORE the `mkdirSync` that creates it, where realpath cannot resolve; and `installer-migrations.cts`'s `ensureInsideConfig`, because that module snapshots, restores and backs up a symlinked managed path AS A LINK — resolving it dereferenced exactly those links and rejected them for escaping `configDir`, which the remote matrix caught as four failing tests. `normalizeRelPath` is that one's real pre-gate, throwing on absolute paths and `..` before containment runs. **The ratchet (#4654).** `local/no-unconfined-path-join` (`eslint-rules/`) runs under `npm run lint` with an **empty allowlist** and bans two shapes: the hand-rolled comparison `X.startsWith(Y + sep)`, and a containment predicate called as a bare statement so its answer is discarded. It does NOT attempt to flag `path.join` by taint — "this argument derives from argv" is not locally decidable, and an approximation over the repo's 2046 join sites would flood. What is duplicated is the comparison, not the join. The rule cannot see *validate-one-path-use-another* where the answer is used but a different variable flows onward; the branded `ContainedPath` is the defense there, and the two are complementary. A justified holdout carries `// allow-handrolled-containment: ` on the reported line, with a mandatory reason asserting either that the comparison is not a containment decision, or that the predicate is unreachable — the live case being `capability-validator.cjs` and `scripts/lib/drift-scan.cjs`, which run before `npm run build:lib` while the compiled `security.cjs` is untracked build output. A marker suppresses BEFORE the violation counter increments, so a file whose every occurrence is marked still reports `staleAllowlistEntry` and its allowlist entry can be dropped. **A marker cannot cover a checksum-locked shipped body.** The four `src/installer-migrations/*.cts` bodies whose `plan` is hashed against `EXPECTED_CHECKSUMS` (#670) are excluded from the rule ENTIRELY, by exact path in `eslint.config.mjs`'s `ignores` (not a directory wildcard, so a NEW migration file stays linted): `plan.toString()` hashes the function's source text INCLUDING comments, so placing a marker inside the body drifts the checksum exactly like editing the code would — measured directly, both `003-rename-get-shit-done-to-gsd-core.cts` and `004-prune-stale-pristine-snapshots.cts` failed their checksum with the marker in place. This leaves those four files' hand-rolled comparisons permanently un-ratcheted; the only remedy is a fix-forward migration, never an edit to a shipped body. Source of truth: `gsd-core/bin/lib/security.cjs` (generated from `src/security.cts`). See Gate Predicate Evaluator Module, Runtime Artifact Install Plan Module, Real Home Guard Module. ### Real Home Guard Module Refuses an in-process install whose destination would land in the developer's REAL home while a Node test runner is in scope (#3712, ADR-1239/#2088 territory). Interface: `assertTestHomeSandboxed(operation, runtime, kinds, deps?)` (throws), `isTestHomeGuardRefusal(err)`, `SANDBOX_MARKER`. Exists because a runtime kind may declare a global `home` override resolved from `os.homedir()` rather than from the caller's `configDir` — codex's skills kind (`home: ".agents"`) and, since #3738, antigravity's global skills AND agents kinds (`home: ".gemini/config"`, the dir AGY scans for global discovery; configHome stays `~/.gemini/antigravity` for settings/runtime files) are the live cases — so sandboxing `configDir`/`targetDir` does NOT contain it, and `assertDestWithinConfigHome` (Runtime Artifact Install Plan Module) structurally cannot see the class: that gate confines a `destSubpath` to whatever root it is handed, and here the root IS the escaped home. The observed failure was silent destruction — a test file that sandboxed only `targetDir` pruned all 71 `gsd-*` skills from a real `~/.agents/skills` via `_removeGsdEntries` while the suite exited 0 and the manifest still reported a healthy install. **Six writers** resolve a kind `home` and then write or destroy under it and therefore carry a call: `installRuntimeArtifacts`, `uninstallRuntimeArtifacts` (Install Engine Module), `applySurface` (Surface Module), and `migrateLegacyDevPreferencesToSkill` — which CREATES rather than prunes, which is why it was missed on the first pass, and which runs from `_runLegacyInstallMigrations` BEFORE `installRuntimeArtifacts`' own assertion, so it guards the destination it already resolved rather than resolving a second time (generative-fix divergence) — plus `installOpencodeFamilySkills` and `installAgentsKindStandalone`, guarded against a future descriptor change rather than a present escape since no combined-family or agents kind declares a `home` today. Each of the four reachable writers carries an optional `deps: { os?, env? }` tail parameter — the guard's trigger condition is "HOME equals the passwd home", which cannot be simulated without pointing at the developer's real home — so `tests/install-write-confinement.test.cjs` drives the REAL entrypoint for each one and deleting any guard call site turns a row red. `migrateLegacyDevPreferencesToSkill` guards only when `target.hasHomeOverride` — that same resolution's own answer to "did the skills kind declare a `home`?", never inferred from `installRoot !== targetDir`, which is FALSE whenever the override resolves onto `targetDir` itself (a configDir of `$HOME/.agents`, exactly where codex's override points); both directions of that condition are pinned, so widening it to refuse ordinary confined migrations fails a row too. **Gated on `NODE_TEST_CONTEXT`** (set by `node --test`), never on `GSD_TEST_MODE` — several in-process test files, including the one that caused #3712, never set the latter — so ordinary installs outside a Node test context are untouched and codex still installs to `$HOME/.agents` normally. **The predicate asks about the DESTINATION, not about HOME state**, and about the path the write RESOLVES to rather than how it is spelled: a stale layout captured before `sandboxHome()` still names the real `~/.agents` while HOME is sandboxed, so a HOME-state check would wave it through, and an aliased `/.agents` symlink or junction would otherwise redirect an allowed path into the real home. Canonicalization itself fails CLOSED: a component that cannot be resolved (`EACCES`/`EPERM` on a directory whose mode changed, `ELOOP` on a symlink cycle, `EIO` on a failing mount) is REFUSED rather than falling back to the lexical spelling — that fallback was a second, unnamed fail-open, and precisely the ALLOW an unresolvable alias needs, since the sandbox spelling then satisfies the nested-sandbox exemption (Codex review of #3725). Only `ENOENT`/`ENOTDIR` walk up, matching `identify`'s own errno split rather than inventing a second one: both mean "no such path as named", which is the ordinary shape of a destination no install has created yet. A destination inside the real home is exempted only when all three hold — HOME differs from the passwd home by filesystem identity, the passwd home is not itself beneath that HOME (`/Users`, `C:\Users`), and the destination resolves beneath it. That exemption is not a softening: on Windows `os.tmpdir()` is under `%USERPROFILE%`, so EVERY test sandbox is a descendant of the real home and containment alone cannot separate the safe case from the dangerous one — without it the whole Windows matrix refuses. Every containment decision compares by filesystem identity (`st_dev`+`st_ino`), never pathname (the one pathname comparison in the module is `sameDirectory`'s equality shortcut on the marker branch, described below): `path.resolve` resolves neither symlinks nor case and `realpath` returns a canonical pathname two routes to one directory can disagree on. **Fails CLOSED**, with one named exception: where no passwd entry is readable (some CI images) it falls back to a path-valued marker set by `sandboxHome()`/`installSpawnEnv()` in `tests/helpers.cjs`, which must identify, must equal the home in effect, AND must contain every resolved destination. All three are load-bearing: matching HOME attests that a caller sandboxed HOME and says nothing about where these destinations resolve, so on its own the marker waved through the very stale-layout shape the primary branch refuses; and `sameDirectory`'s pathname-equality shortcut can answer yes for two identical UNidentifiable paths, which is not enough to place a destination against. It remains a deliberate weakening, since nothing on such a host can contradict a marker naming the real home. `installSpawnEnv` re-points the marker at an explicitly overridden HOME, so a spawn that supplies its own sandbox is not refused by a marker still naming the helper's default one. Refusals are STAMPED so `bin/install.js` can rethrow them without running `_codexPreConfigRollback()`, which deletes and recreates every snapshotted `gsd-*` directory in the resolved skills root — otherwise the guard's own refusal would provoke the mutation it exists to prevent. **Known limits, stated rather than implied**: a subordinate bind mount of the real `~/.agents` into the sandbox is not closed (a bind mount is not a link, so realpath keeps the mount-point spelling; closing it needs non-portable mount-table introspection), and a cross-process TOCTOU swap between check and write is out of reach. Source: `gsd-core/bin/lib/real-home-guard.cjs` (generated from `src/real-home-guard.cts`). See Install Engine Module, Surface Module, Runtime Artifact Install Plan Module, Runtime Artifact Layout Module. diff --git a/docs/explanation/security-model.md b/docs/explanation/security-model.md index 99a67d377..c1b82d195 100644 --- a/docs/explanation/security-model.md +++ b/docs/explanation/security-model.md @@ -197,6 +197,34 @@ the install root, and a link planted at a capability's own `SKILL.md` was followed by `statSync`, so an outside file's contents were installed as a skill body. +**The ratchet.** A convention saying "remember to use the predicate" is exactly +what produced the unvalidated sites in the first place, so the rule +`local/no-unconfined-path-join` enforces it under `npm run lint` with an empty +allowlist. It bans the hand-rolled comparison `X.startsWith(Y + separator)` and +a containment predicate called as a bare statement with its answer discarded. +It deliberately does not try to decide, for each of the repository's ~2000 +`path.join` calls, whether an argument came from user input — that question is +not answerable locally, and a rule that fires on hundreds of correct sites earns +an exemption list of hundreds. What actually gets copied is the comparison. + +A site that legitimately cannot use the predicate carries a comment +`// allow-handrolled-containment: ` naming why: either the comparison is +not a containment decision (an ancestor-walk loop, identity matching), or the +predicate is unreachable — two files run before the compiled module they would +need to import exists. The reason is mandatory and reviewable; the rule does +not accept an empty one. + +A marker cannot cover a shipped, checksum-locked artifact whose body must not +change: the four `src/installer-migrations/*.cts` bodies hashed against +`EXPECTED_CHECKSUMS` (#670) hash `plan.toString()`, the function's source text +INCLUDING comments, so a marker placed inside the body drifts the checksum +exactly as an edit would — verified directly against the committed baseline. +These four files are instead excluded from the rule entirely, by exact path in +`eslint.config.mjs`'s `ignores` (not a directory wildcard, so a new migration +file is still linted), leaving their hand-rolled comparisons permanently +un-ratcheted; the only remedy is a fix-forward migration, never an edit to a +shipped body. + **Runtime hook: `gsd-prompt-guard.js`.** This hook fires on every Write or Edit call that targets `.planning/` files. It scans the content being written for injection patterns shared with `gsd-read-injection-scanner.js` through diff --git a/eslint-rules/no-unconfined-path-join.allowlist.json b/eslint-rules/no-unconfined-path-join.allowlist.json new file mode 100644 index 000000000..fe51488c7 --- /dev/null +++ b/eslint-rules/no-unconfined-path-join.allowlist.json @@ -0,0 +1 @@ +[] diff --git a/eslint-rules/no-unconfined-path-join.cjs b/eslint-rules/no-unconfined-path-join.cjs new file mode 100644 index 000000000..ec731f834 --- /dev/null +++ b/eslint-rules/no-unconfined-path-join.cjs @@ -0,0 +1,426 @@ +'use strict'; + +const path = require('path'); + +/** + * no-unconfined-path-join + * + * Epic #4636 consolidated every hand-rolled path-containment check in this + * repo onto a single implementation, `isContainedIn` in `src/security.cts`, + * exported as two deliberate families: + * + * - `assertWithinRoot` / `tryWithinRoot` — realpath-resolved containment. + * This is the DEFAULT for any boundary that takes external input (a user + * path, a config value, an argv path): it resolves symlinks before + * comparing, so a symlink planted inside the root that points outside it + * cannot escape the check. + * - `assertWithinRootLexical` / `tryWithinRootLexical` — lexical (string + * prefix) containment, no filesystem access. This family exists ONLY for + * the narrow case where a symlink must be PRESERVED rather than + * resolved, or where the candidate path does not exist on disk yet (so + * `realpath` would throw or silently resolve nothing). A caller must + * pick the family deliberately — collapsing a lexical call site onto the + * realpath form is not a safe "simplification": doing exactly that broke + * four tests in Phase 3 of this epic (#4653), because those sites + * depended on the symlink surviving unresolved. + * + * This rule has two arms, both aimed at call sites that never went through + * that consolidation and so get neither guarantee. + * + * ## Arm 1 — hand-rolled containment comparison (`handRolledContainment`) + * + * Flags `x.startsWith(y + path.sep)` and the string-literal equivalents + * (`y + '/'`, `y + '\\'`). This shape looks like a containment check but is + * not one: it is a plain string-prefix test with no symlink resolution, no + * normalization of `..` segments, and no protection against a `y` that is + * itself a prefix of a sibling directory name (`/root-evil` starts with + * `/root` + sep only if the separator is included, which this shape happens + * to get right — but every other edge case `isContainedIn` handles is still + * missing). Replace it with `assertWithinRoot`/`tryWithinRoot` (or the + * `*Lexical` variant only if a symlink genuinely must not be resolved, or + * the target does not exist yet) from `src/security.cts`. + * + * A one-line justified holdout can suppress a single occurrence with a + * trailing same-line comment: + * + * resolved.startsWith(root + path.sep); // allow-handrolled-containment: + * + * Reporting is deferred to `Program:exit` so a marker can be matched against + * the SPECIFIC violation it trails: among the violations that END on the + * marker's line and before the marker starts, only the one with the LARGEST + * end offset (i.e. the one immediately preceding the marker) is suppressed. + * An earlier violation sharing the same line is still reported, and a marker + * suppresses at most one violation. + * + * The marker covers two distinct justifications, and the mandatory reason + * text is what distinguishes them for review: + * + * (a) the comparison is not a containment decision at all — prefix + * filtering for selection, grouping or display, an ancestor-walk loop + * condition, or identity matching. + * (b) it IS a containment decision, but the canonical predicate in + * `src/security.cts` is unreachable from this file. Two real cases: + * `gsd-core/bin/lib/capability-validator.cjs` is a committed `.cjs` + * that must run before `npm run build:lib`, and + * `gsd-core/bin/lib/security.cjs` (the compiled predicate) is build + * output that is gitignored and untracked — requiring it would break + * a fresh clone. `scripts/lib/drift-scan.cjs` runs under `lint:ci` + * with the same exposure. + * + * The text after the colon must be non-empty after trimming — an empty or + * missing reason does not suppress, and neither does the marker text without + * a colon at all. + * + * ## Arm 2 — a discarded containment answer (`discardedContainmentResult`) + * + * Flags a bare statement-position call to one of the `src/security.cts` + * containment predicates (`assertWithinRoot`, `tryWithinRoot`, + * `requireSafePath`, `assertWithinRootLexical`, `tryWithinRootLexical`, + * `isPathConfined`, `assertDestWithinConfigHome`) whose return value is + * discarded. The return value IS the answer — for the `try*`/`is*` members + * of this family in particular, calling them and throwing away the result is + * indistinguishable from never having called them at all: no exception is + * thrown on containment failure, so the call is a no-op that merely looks + * like a check. This "validate one path, use another" defect recurred five + * separate times across epic #4636's call sites. `assertWithinRoot` and its + * lexical sibling throw on failure, so a bare statement call to those two + * IS meaningful as a guard — but this rule still flags it, because the + * common bug pattern is copy-pasting a `try*`/`is*` call as if it were an + * `assert*` one; the fix in every case is to use the returned/normalized + * path (or the boolean) rather than re-deriving it, or re-reading the + * original unchecked value, downstream. + * + * ## Allowlist + * + * `allowlist` (repo-relative POSIX paths) exempts pre-existing legacy + * violations pending migration, with mechanics identical to + * `no-adhoc-timeout-literal.cjs`: an allowlisted file's violations are + * counted internally but not reported; a listed file with zero violations + * reports `staleAllowlistEntry` so the dead entry gets deleted. The + * allowlist only ever ratchets down. + * + * ## Known gaps + * + * This rule raises the COST of the accidental hand-rolled copy — the failure + * mode this repo actually observed five separate times — it is not a proof + * that every unconfined path comparison is caught. Specifically: + * + * - `x.startsWith(root)` with NO separator — the genuinely unsafe variant, + * since it accepts a sibling like `/root-evil` — is NOT flagged. Flagging + * every bare `startsWith(identifier)` call in the codebase would swamp + * the rule with unrelated string-prefix checks, so arm 1 only fires once + * a separator is visibly appended. The canonical predicate + * (`isContainedIn` / `assertWithinRoot` / `tryWithinRoot`) is + * separator-aware internally, which is exactly why replacing either + * shape — the correct-looking `root + sep` form and the actually-unsafe + * bare form — with a call to it is the fix. + * - Other spellings of the same comparison are not recognized: + * `indexOf(root + path.sep) === 0` and + * `x.slice(root.length).startsWith(path.sep)`. + * - The suppression marker is anchored to the reported node's END line + * (`node.loc.end.line`), not its start line: a call whose closing paren + * lands on a later line than its first argument needs the marker after + * THAT (closing) line, not after the call's opening line. `end.line` was + * chosen over `start.line` specifically so a multi-line call CAN be + * suppressed — the marker just has to trail the line the call's closing + * paren is on, which is where a reader's eye (and a trailing `//` + * comment) naturally lands after a multi-line expression. There is still + * no "anywhere in this call" anchoring: a marker placed after the + * opening line of a multi-line call does not suppress it. + * - A separator reached through more than one level of `const` aliasing + * (e.g. `const s1 = path.sep; const sep = s1; x.startsWith(root + sep)`) + * is not resolved — only a single hop from a `+` right-operand Identifier + * to its unique `const` initializer is followed, and only when that + * initializer is itself a separator operand. `let`/reassigned/parameter + * bindings are deliberately left unresolved: this is targeted alias + * resolution for the one common shape, not general constant folding. + * - Four shipped installer-migration bodies — + * `src/installer-migrations/003-rename-get-shit-done-to-gsd-core.cts`, + * `004-prune-stale-pristine-snapshots.cts`, + * `009-pi-retire-reserved-hooks-dir.cts`, and + * `010-antigravity-retire-confighome-artifacts.cts` — are entirely + * un-ratcheted (excluded via `ignores` in `eslint.config.mjs`, listed by + * exact path, not a directory wildcard). Their `plan` bodies are hashed + * into `EXPECTED_CHECKSUMS` via `plan.toString()` + * (tests/installer-migrations.test.cjs, issue #670), and that hash + * includes comments — so neither a code fix NOR a suppression marker can + * land inside these four bodies without drifting the checksum and + * breaking upgrade state for anyone who already applied the migration. + * A justification-(c) marker was tried and measured to still drift the + * checksum, which is why (c) does not appear above: a marker cannot + * serve this case. The only remedy is a fix-forward migration; a NEW + * migration file is unaffected and still fully linted. + */ + +const DISCARDED_CALL_NAMES = new Set([ + 'assertWithinRoot', + 'tryWithinRoot', + 'requireSafePath', + 'assertWithinRootLexical', + 'tryWithinRootLexical', + 'isPathConfined', + 'assertDestWithinConfigHome', + // `isContainedIn` (src/security.cts) is a boolean predicate: calling it as a + // bare statement and discarding the boolean is a pure no-op — arm 2's + // failure mode, on the one function the whole consolidation funnels through. + 'isContainedIn', +]); + +const MARKER_RE = /allow-handrolled-containment:(.*)$/; + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Disallow hand-rolled path-containment comparisons and discarded containment-check results', + category: 'Security', + }, + schema: [ + { + type: 'object', + properties: { + allowlist: { type: 'array', items: { type: 'string' } }, + }, + additionalProperties: false, + }, + ], + messages: { + handRolledContainment: + 'Hand-rolled containment comparison `x.startsWith(y + sep)`: this repo consolidated every ' + + 'path-containment check onto one implementation (`isContainedIn`, src/security.cts), exported ' + + 'as `assertWithinRoot`/`tryWithinRoot` (realpath-resolved — use this at any boundary that takes ' + + 'external input) and `assertWithinRootLexical`/`tryWithinRootLexical` (lexical, no filesystem ' + + 'access — use ONLY where a symlink must be PRESERVED rather than resolved, or the target does ' + + 'not exist yet). A caller must pick the family deliberately: collapsing a lexical site onto the ' + + 'realpath form broke four tests in Phase 3 of epic #4636 (#4653). Replace this comparison with a ' + + 'call to the correct family member, or if this is a justified holdout add a trailing same-line ' + + 'comment `// allow-handrolled-containment: ` explaining either (a) this is not ' + + 'actually a containment decision (prefix filtering for selection/grouping/display, an ' + + 'ancestor-walk loop condition, or identity matching), or (b) it is a containment decision but the ' + + 'canonical predicate in src/security.cts is unreachable from this file.', + discardedContainmentResult: + 'Containment predicate `{{name}}(...)` called as a bare statement: the return value IS the ' + + 'answer, and discarding it is indistinguishable from never calling it — no exception is thrown ' + + 'on failure for the `try*`/`is*` members of this family, so the call becomes a no-op that only ' + + 'looks like a check. This "validate one path, use another" defect recurred five times across ' + + 'epic #4636. Use the returned/normalized path (or boolean) at the point where you assign, branch, ' + + 'or pass a value downstream instead of discarding it.', + staleAllowlistEntry: + '{{file}} no longer contains an unconfined path-join violation. Delete its line from ' + + 'eslint-rules/no-unconfined-path-join.allowlist.json — the allowlist only ratchets down.', + }, + }, + + create(context) { + const options = context.options[0] || {}; + const allowlist = Array.isArray(options.allowlist) ? options.allowlist : []; + + const filename = context.filename || context.getFilename(); + const cwd = context.cwd || (context.getCwd ? context.getCwd() : process.cwd()); + const rel = path.relative(cwd, filename).split(path.sep).join('/'); + const allowlisted = allowlist.includes(rel); + let violations = 0; + + const sourceCode = context.sourceCode || context.getSourceCode(); + + /** + * Returns the list of valid suppression-marker comments in the file: a + * Line comment carrying `allow-handrolled-containment: `. A BLOCK comment never qualifies — the convention is a + * trailing `//` marker, and accepting `/* *\/` would let an unrelated + * block comment silently suppress too. + */ + function findMarkerComments() { + const allComments = + typeof sourceCode.getAllComments === 'function' ? sourceCode.getAllComments() : []; + const markers = []; + for (const comment of allComments) { + if (comment.type !== 'Line') continue; + const match = MARKER_RE.exec(comment.value); + if (match && match[1] && match[1].trim().length > 0) { + markers.push(comment); + } + } + return markers; + } + + /** + * Right operand of `y + ` is a separator: either `path.sep`-shaped + * (a non-computed MemberExpression whose property is the identifier + * `sep`) or a single-character separator string literal. + */ + function isSeparatorOperand(node) { + if (!node) return false; + if ( + node.type === 'MemberExpression' && + !node.computed && + node.property.type === 'Identifier' && + node.property.name === 'sep' + ) { + return true; + } + if (node.type === 'Literal' && typeof node.value === 'string') { + return node.value === '/' || node.value === '\\'; + } + return false; + } + + /** + * A template-literal argument ends with a separator: either the trailing + * quasi text itself ends with `/` or `\` (`` `${root}/` ``), or the + * trailing quasi is empty and the LAST expression is a separator operand + * (`` `${root}${path.sep}` ``). Only the tail matters — a separator + * embedded mid-template followed by further literal text is not a + * containment-prefix shape. + */ + function isSeparatorEndingTemplate(node) { + if (!node || node.type !== 'TemplateLiteral') return false; + const quasis = node.quasis; + if (!quasis || quasis.length === 0) return false; + const lastQuasi = quasis[quasis.length - 1]; + const tail = + lastQuasi.value.cooked !== null && lastQuasi.value.cooked !== undefined + ? lastQuasi.value.cooked + : lastQuasi.value.raw; + if (tail && (tail.endsWith('/') || tail.endsWith('\\'))) { + return true; + } + if (tail === '' && node.expressions.length > 0) { + const lastExpr = node.expressions[node.expressions.length - 1]; + return isSeparatorOperand(lastExpr); + } + return false; + } + + /** + * Resolves a `+` right-operand Identifier to a separator via a single + * `const` alias hop: `const sep = path.sep; x.startsWith(root + sep)`. + * Uses scope analysis to find the unique binding for the identifier and + * checks whether ITS initializer is a separator operand. Deliberately + * narrow: only a `const` declarator with exactly one definition is + * resolved, and only one hop is followed — this is not general constant + * folding, and `let`/reassigned/parameter bindings are left unresolved. + */ + function resolveSeparatorAlias(identifierNode) { + const scope = sourceCode.getScope(identifierNode); + const ref = scope.references.find((r) => r.identifier === identifierNode); + if (!ref || !ref.resolved) return false; + const variable = ref.resolved; + if (variable.defs.length !== 1) return false; + const def = variable.defs[0]; + if (def.type !== 'Variable') return false; + if (!def.parent || def.parent.kind !== 'const') return false; + const declarator = def.node; + if (!declarator || !declarator.init) return false; + return isSeparatorOperand(declarator.init); + } + + /** + * Resolves the callee name of a discarded-containment candidate call: + * a bare Identifier callee, or a non-computed MemberExpression whose + * property is an Identifier. + */ + function calleeName(callee) { + if (callee.type === 'Identifier') return callee.name; + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.property.type === 'Identifier' + ) { + return callee.property.name; + } + return null; + } + + // Violations are not reported (or counted) during traversal. Each + // would-be violation is deferred so that, once every node in the file has + // been visited, a trailing marker can be matched against the SPECIFIC + // violation it trails rather than every violation that happens to share + // its line. See `Program:exit` for the resolution pass. + // + // Truth table (after resolution): + // marked -> not counted, not reported + // allowlisted + real violations -> counted (entry justified), not reported + // allowlisted + only marked violations -> counter 0 -> staleAllowlistEntry fires, entry removable + // neither -> counted and reported + const pending = []; + function reportViolation(node, messageId, data) { + pending.push({ node, messageId, data }); + } + + return { + CallExpression(node) { + // Arm 1: x.startsWith(y + sep) + const callee = node.callee; + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.property.type === 'Identifier' && + callee.property.name === 'startsWith' && + node.arguments.length === 1 + ) { + const arg = node.arguments[0]; + if ( + arg.type === 'BinaryExpression' && + arg.operator === '+' && + (isSeparatorOperand(arg.right) || + (arg.right.type === 'Identifier' && resolveSeparatorAlias(arg.right))) + ) { + reportViolation(node, 'handRolledContainment'); + return; + } + if (isSeparatorEndingTemplate(arg)) { + reportViolation(node, 'handRolledContainment'); + return; + } + } + + // Arm 2: a discarded containment-predicate result. + if (node.parent && node.parent.type === 'ExpressionStatement') { + const name = calleeName(callee); + if (name && DISCARDED_CALL_NAMES.has(name)) { + reportViolation(node, 'discardedContainmentResult', { name }); + } + } + }, + + 'Program:exit'(node) { + // Resolve marker suppression: for each valid marker comment, find + // among the still-unresolved pending violations the one whose node + // ends on the marker's line and before the marker starts, preferring + // the LARGEST such end offset — i.e. the violation the marker + // visibly trails most closely. A marker suppresses at most one + // violation; a violation is suppressed by at most one marker. + const markers = findMarkerComments(); + const suppressed = new Set(); + for (const comment of markers) { + let best = null; + for (const entry of pending) { + if (suppressed.has(entry)) continue; + if (entry.node.loc.end.line !== comment.loc.start.line) continue; + if (entry.node.range[1] > comment.range[0]) continue; + if (!best || entry.node.range[1] > best.node.range[1]) { + best = entry; + } + } + if (best) suppressed.add(best); + } + + for (const entry of pending) { + if (suppressed.has(entry)) continue; + violations += 1; + if (allowlisted) continue; + context.report({ node: entry.node, messageId: entry.messageId, data: entry.data }); + } + + if (allowlisted && violations === 0) { + context.report({ node, messageId: 'staleAllowlistEntry', data: { file: rel } }); + } + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index f63a69fcf..5e08a64cc 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -40,8 +40,10 @@ import noSwallowedPrecondition from './eslint-rules/no-swallowed-precondition.cj import noExactCaseEnvAccess from './eslint-rules/no-exact-case-env-access.cjs'; import noAdhocTimeoutLiteral from './eslint-rules/no-adhoc-timeout-literal.cjs'; import noRenderedTextLengthAssert from './eslint-rules/no-rendered-text-length-assert.cjs'; +import noUnconfinedPathJoin from './eslint-rules/no-unconfined-path-join.cjs'; const adhocTimeoutLiteralAllowlist = require('./eslint-rules/no-adhoc-timeout-literal.allowlist.json'); +const unconfinedPathJoinAllowlist = require('./eslint-rules/no-unconfined-path-join.allowlist.json'); const localPlugin = { rules: { @@ -74,6 +76,7 @@ const localPlugin = { 'no-exact-case-env-access': noExactCaseEnvAccess, 'no-adhoc-timeout-literal': noAdhocTimeoutLiteral, 'no-rendered-text-length-assert': noRenderedTextLengthAssert, + 'no-unconfined-path-join': noUnconfinedPathJoin, }, }; @@ -663,6 +666,51 @@ export default tseslint.config( }, }, + // ── first-party source — no-unconfined-path-join (#4636) ────────────────── + // No single existing block covers this exact shape (src/**/*.cts + src/**/*.ts + // + scripts/**/*.cjs + gsd-core/bin/**/*.cjs + hooks/**/*.js), so this is a + // NEW block rather than a reuse. tests/** is deliberately excluded — a test + // may legitimately construct a hand-rolled containment shape as a fixture. + { + files: ['src/**/*.cts', 'src/**/*.ts', 'scripts/**/*.cjs', 'gsd-core/bin/**/*.cjs', 'hooks/**/*.js'], + // These four shipped installer-migration bodies are checksum-locked + // (tests/installer-migrations.test.cjs, #670): `migrationChecksum` hashes + // `migration.plan.toString()`, which includes comments, so neither a code + // fix nor a suppression marker can be added to these bodies without + // drifting EXPECTED_CHECKSUMS. Listed by exact path (not a directory + // wildcard) so a NEW migration file still gets linted — only these four + // already-shipped bodies are exempt. This leaves the corresponding + // containment comparisons in these four files permanently un-ratcheted; + // the remedy is a fix-forward migration, never an edit to a shipped body. + ignores: [ + 'src/installer-migrations/003-rename-get-shit-done-to-gsd-core.cts', + 'src/installer-migrations/004-prune-stale-pristine-snapshots.cts', + 'src/installer-migrations/009-pi-retire-reserved-hooks-dir.cts', + 'src/installer-migrations/010-antigravity-retire-confighome-artifacts.cts', + ], + plugins: { + local: localPlugin, + }, + languageOptions: { + sourceType: 'commonjs', + globals: { + ...globals.node, + }, + }, + rules: { + // #4636: bans a hand-rolled containment comparison (`x.startsWith(y + sep)`, + // a plain string-prefix test with no symlink resolution or `..` normalization) + // and a discarded containment answer (a bare-statement call to one of the + // src/security.cts containment predicates whose return value is thrown away — + // "validate one path, use another" recurred five times across this epic). A + // justified holdout on the first arm can suppress a single occurrence with a + // trailing same-line `// allow-handrolled-containment: ` comment. Like + // `no-unbounded-spawn`, the allowlist is seeded empty and stays empty — this + // epic migrated every call site, so the rule runs with no exemption surface. + 'local/no-unconfined-path-join': ['error', { allowlist: unconfinedPathJoinAllowlist }], + }, + }, + // ── root *.mjs config files (#3059) ──────────────────────────────────────── { files: ['*.mjs'], diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 112f1a8f6..852d40e11 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -438,8 +438,9 @@ function dispatchCapabilityCommand({ command, args, cwd, raw, error, registry, r // Step 2: confinement check — belt-and-suspenders even after the basename // validation above. Resolved path must be inside libDir (not equal to it, // and must start with libDir + sep so "libDir-suffix" can't sneak through). - const resolved = path.resolve(libDir, m); - if (resolved === libDir || !resolved.startsWith(libDir + path.sep)) { + const { tryWithinRootLexical } = require('./lib/security.cjs'); + const resolved = tryWithinRootLexical(m, libDir); + if (resolved === null || resolved === path.resolve(libDir)) { throw new Error('capability module path escapes bin/lib/: ' + JSON.stringify(m)); } // Step 3: require the resolved absolute path — the SAME representation that @@ -515,18 +516,19 @@ function defaultRequireFromInstallRoot(installRoot, m) { if (typeof m !== 'string' || !/^[A-Za-z0-9._-]+\.cjs$/.test(m)) { throw new Error('capability module must be a bare .cjs basename: ' + JSON.stringify(m)); } - // Realpath the root so a symlinked ancestor can't widen confinement. - const realRoot = fs.realpathSync(installRoot); - const resolved = path.resolve(realRoot, m); - if (resolved === realRoot || !resolved.startsWith(realRoot + path.sep)) { + const { tryWithinRoot, tryWithinRootLexical, PathAcceptance } = require('./lib/security.cjs'); + // Lexical containment check: a symlinked ancestor can't widen confinement. + const lexical = tryWithinRootLexical(m, installRoot); + if (lexical === null || lexical === path.resolve(installRoot)) { throw new Error('capability module path escapes its install root: ' + JSON.stringify(m)); } - // The module file itself must not be a symlink pointing outside the root. - const realResolved = fs.realpathSync(resolved); - if (realResolved !== realRoot && !realResolved.startsWith(realRoot + path.sep)) { + // Realpath/symlink check: the module file itself must not be a symlink + // pointing outside the root. + const real = tryWithinRoot(m, installRoot, PathAcceptance.RelativeOnly); + if (real === null) { throw new Error('capability module resolves outside its install root (symlink): ' + JSON.stringify(m)); } - return require(realResolved); + return require(real); } /** diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 7f07974b2..cbc8fc617 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -2801,7 +2801,7 @@ function materializeHookFragments(cap, capDir) { const abs = path.resolve(capDir, fragment.path); const capRoot = path.resolve(capDir); - if (abs !== capRoot && !abs.startsWith(capRoot + path.sep)) { + if (abs !== capRoot && !abs.startsWith(capRoot + path.sep)) { // allow-handrolled-containment: committed pre-build .cjs; compiled security.cjs is untracked build output errors.push( cap.id + '/' + groupName + '[' + i + '].fragment.path escapes capability directory: ' + fragment.path, diff --git a/gsd-core/bin/verify-reapply-patches.cjs b/gsd-core/bin/verify-reapply-patches.cjs index a1a8e0046..d7db3e344 100755 --- a/gsd-core/bin/verify-reapply-patches.cjs +++ b/gsd-core/bin/verify-reapply-patches.cjs @@ -406,7 +406,7 @@ function resolveSkillsRedirect(configDir) { if (!kind) return null; const root = path.resolve(path.join(kind.home || configDir, kind.destSubpath)); const resolvedConfig = path.resolve(configDir); - if (root === resolvedConfig || root.startsWith(resolvedConfig + path.sep)) return null; + if (root === resolvedConfig || root.startsWith(resolvedConfig + path.sep)) return null; // allow-handrolled-containment: answers whether the skills root is a separate location, not a containment/admission decision // Same descriptor-driven manifest prefix writeManifest uses for skills // keys (hermes nests under 'skills/gsd/', everyone else 'skills/'), read // from the same shipped registry the installer's _resolveHostBehaviors diff --git a/scripts/affected-tests-lib.cjs b/scripts/affected-tests-lib.cjs index 88603f7d4..1b9f85737 100644 --- a/scripts/affected-tests-lib.cjs +++ b/scripts/affected-tests-lib.cjs @@ -339,7 +339,7 @@ function pickAffectedTests(changedFiles, allTests, reverseIndex, options = {}) { if (!isSourceFile) continue; // Check: is this file under a recognised source tree? const isUnderSourceTree = SOURCE_TREES.some( - tree => file === tree || file.startsWith(tree + '/'), + tree => file === tree || file.startsWith(tree + '/'), // allow-handrolled-containment: test-selection path filtering, not a safety decision ); if (!isUnderSourceTree) continue; // Does it have any test dependents? diff --git a/scripts/diff-touches-shipped-paths.cjs b/scripts/diff-touches-shipped-paths.cjs index ae27dc64c..d6ece99eb 100644 --- a/scripts/diff-touches-shipped-paths.cjs +++ b/scripts/diff-touches-shipped-paths.cjs @@ -72,7 +72,7 @@ function isShipped(diffPath, shipPrefixes) { // forward slashes, but a developer running this locally on a different // tool's output shouldn't get a false negative). const p = diffPath.replace(/\\/g, '/'); - return shipPrefixes.some((s) => p === s || p.startsWith(s + '/')); + return shipPrefixes.some((s) => p === s || p.startsWith(s + '/')); // allow-handrolled-containment: shipped-path filtering for a CI check, not a safety decision } // #2980: commits that touch `.github/workflows/*` cannot be cherry-picked diff --git a/scripts/gen-adr-index.cjs b/scripts/gen-adr-index.cjs index 3429c36f0..c724a759c 100644 --- a/scripts/gen-adr-index.cjs +++ b/scripts/gen-adr-index.cjs @@ -31,6 +31,7 @@ const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); +const { isContainedIn } = require('../gsd-core/bin/lib/security.cjs'); const ROOT = path.resolve(__dirname, '..'); const ADR_DIR = path.join(ROOT, 'docs', 'adr'); @@ -176,10 +177,15 @@ const ADR_FILENAME_RE = /^[0-9]+-[a-z0-9-]+\.md$/; * segment merely BEGINS with two dots (`..hidden.md`), and a false "escapes * the repository" on a valid path is a worse failure than a miss — hence the * whole-segment `rel === '..' || rel.startsWith('..' + sep)` form. + * + * Delegates to the shared `isContainedIn` (ADR-4650): every call site here + * already resolved both operands itself (either lexically, before any + * filesystem call, or via realpathSync after a symlink) and needs only this + * comparison step — the exact already-resolved-caller case `isContainedIn` + * documents. */ function escapesRoot(abs, root) { - const rel = path.relative(root, abs); - return rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel); + return !isContainedIn(abs, root); } /** diff --git a/scripts/lib/drift-scan.cjs b/scripts/lib/drift-scan.cjs index 1c30d0927..305ebd3ff 100644 --- a/scripts/lib/drift-scan.cjs +++ b/scripts/lib/drift-scan.cjs @@ -162,7 +162,7 @@ function readRegexLiteralAt(line, start) { // root + separator — a plain `startsWith(root)` would also accept a sibling // directory whose name merely starts with the root's name (`/repo-evil`). function isInsideRoot(realPath, realRoot) { - return realPath === realRoot || realPath.startsWith(realRoot + path.sep); + return realPath === realRoot || realPath.startsWith(realRoot + path.sep); // allow-handrolled-containment: lint:ci guard; runs before build:lib, compiled security.cjs may not exist } // True when `realPath` (already confirmed inside `realRoot` by `isInsideRoot`) diff --git a/scripts/lint-source-test-name-collision.cjs b/scripts/lint-source-test-name-collision.cjs index a0e0c040b..015803098 100644 --- a/scripts/lint-source-test-name-collision.cjs +++ b/scripts/lint-source-test-name-collision.cjs @@ -114,7 +114,7 @@ function toPosix(p) { function isGeneratedOutput(relPath) { const posixRel = toPosix(relPath); return GENERATED_OUTPUT_PREFIXES.some( - (prefix) => posixRel === prefix || posixRel.startsWith(`${prefix}/`) + (prefix) => posixRel === prefix || posixRel.startsWith(`${prefix}/`) // allow-handrolled-containment: scan-exclusion membership test against a fixed generated-output prefix list, not a filesystem root-confinement gate ); } diff --git a/src/capability-lifecycle.cts b/src/capability-lifecycle.cts index 4b6e915c3..78523b14b 100644 --- a/src/capability-lifecycle.cts +++ b/src/capability-lifecycle.cts @@ -22,6 +22,7 @@ import fs from 'node:fs'; import path from 'node:path'; import crypto from 'node:crypto'; +import { tryWithinRootLexical } from './security.cjs'; /* eslint-disable @typescript-eslint/no-require-imports */ const sourceMod = require('./capability-source.cjs') as { @@ -371,7 +372,10 @@ function safeRmUnder(runtimeDir: string, rel: string): boolean { const target = path.resolve(realRoot, rel); let realParent: string; try { realParent = fs.realpathSync(path.dirname(target)); } catch { return false; } - if (realParent !== realRoot && !realParent.startsWith(realRoot + path.sep)) return false; + // Containment decision is the canonical LEXICAL predicate (ADR-4650 decision 6); lexical because + // both operands are already realpath-resolved here and the final component is deliberately + // handled as a link (see above). + if (tryWithinRootLexical(realParent, realRoot) === null) return false; const realTarget = path.join(realParent, path.basename(target)); let st: fs.Stats; try { st = fs.lstatSync(realTarget); } catch { return true; /* already gone — idempotent */ } @@ -405,10 +409,10 @@ function confinedSharedFile(runtimeDir: string, relFile: unknown): string | null } catch { // Parent does not exist yet (created inside the scope on write): a non-existent path cannot be a // symlink escaping the root, so a lexical containment check is sufficient. - if (parentDir !== realRoot && !parentDir.startsWith(realRoot + path.sep)) return null; + if (tryWithinRootLexical(parentDir, realRoot) === null) return null; return target; } - if (realParent !== realRoot && !realParent.startsWith(realRoot + path.sep)) return null; + if (tryWithinRootLexical(realParent, realRoot) === null) return null; return path.join(realParent, path.basename(target)); } @@ -512,7 +516,7 @@ function confinedBundleScript(capDirPath: string, script: string): string | null // disk): a non-existent root cannot be a symlink escaping itself, so confine lexically. realCapRoot = path.resolve(capDirPath); const targetLex = path.resolve(realCapRoot, script); - if (targetLex !== realCapRoot && !targetLex.startsWith(realCapRoot + path.sep)) return null; + if (tryWithinRootLexical(targetLex, realCapRoot) === null) return null; return targetLex; } @@ -524,12 +528,12 @@ function confinedBundleScript(capDirPath: string, script: string): string | null } catch { // Parent does not exist yet (created inside the bundle): lexical containment is sufficient // because a non-existent path cannot be a symlink escaping the root. - if (parentDir !== realCapRoot && !parentDir.startsWith(realCapRoot + path.sep)) return null; + if (tryWithinRootLexical(parentDir, realCapRoot) === null) return null; return target; } // The realpath'd parent chain must remain inside the bundle — an ancestor symlink escaping the // bundle is refused here (the symlink is followed by realpathSync, so its real location is checked). - if (realParent !== realCapRoot && !realParent.startsWith(realCapRoot + path.sep)) return null; + if (tryWithinRootLexical(realParent, realCapRoot) === null) return null; return path.join(realParent, path.basename(target)); } diff --git a/src/check-command-router.cts b/src/check-command-router.cts index 1c1ddfcd9..566c144bb 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -30,7 +30,7 @@ import type { Decision } from './decisions.cjs'; import frontmatterMod = require('./frontmatter.cjs'); const { extractFrontmatter } = frontmatterMod; import { stripFencedCode, collectSections } from './markdown-sectionizer.cjs'; -import { tryWithinRoot, PathAcceptance } from './security.cjs'; +import { tryWithinRoot, tryWithinRootLexical, PathAcceptance } from './security.cjs'; import { checkUiPresence } from './ui-safety-gate.cjs'; import { hasStaticFrontendEvidence } from './ui-frontend-evidence.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -415,12 +415,6 @@ function recentCommitMessages(projectDir: string): string { } } -function isInsideRoot(candidatePath: string, rootDir: string): boolean { - const root = path.resolve(rootDir); - const target = path.resolve(root, candidatePath); - return target === root || target.startsWith(`${root}${path.sep}`); -} - function readModifiedFilesContent(projectDir: string, summaries: string[]): string { const out: string[] = []; let total = 0; @@ -431,8 +425,15 @@ function readModifiedFilesContent(projectDir: string, summaries: string[]): stri .map((match) => match[1].trim().replace(/^["']|["']$/g, '')); for (const file of files) { if (total >= 50) break; - if (!file || !isInsideRoot(file, projectDir)) continue; - const raw = readIfExists(resolvePath(file, projectDir)); + if (!file) continue; + // Migrated off the hand-rolled prefix check (ADR-4650): resolve+contain in one + // step via the canonical realpath predicate — the eventual read below follows + // symlinks, so containment must be decided on the resolved target, not a lexical + // prefix. Read the value the predicate RETURNED; do not re-derive the path. + const candidate = path.isAbsolute(file) ? file : path.join(projectDir, file); + const contained = tryWithinRoot(candidate, projectDir, PathAcceptance.AbsoluteInsideRoot); + if (contained === null) continue; + const raw = readIfExists(contained); out.push(raw.length > 256 * 1024 ? raw.slice(0, 256 * 1024) : raw); total++; } @@ -1494,7 +1495,14 @@ function cmdApiCoverageVerifyPre(projectDir: string, args: string[], raw: boolea // Defense-in-depth: the resolved dir must be inside the phases root (or a // milestone archive under .planning/milestones). const milestonesRoot = path.join(pDir, 'milestones'); - if (!isInsideRoot(resolvedDir, phasesRoot) && !isInsideRoot(resolvedDir, milestonesRoot)) { + // Lexical containment (ADR-4650): resolvedDir is a directory path, not read + // through here — mirrors the prior path.resolve(root, candidate)-based check + // without introducing a filesystem/realpath dependency this defense-in-depth + // recheck never had. + if ( + tryWithinRootLexical(resolvedDir, phasesRoot) === null && + tryWithinRootLexical(resolvedDir, milestonesRoot) === null + ) { output( { block: true, diff --git a/src/code-review-depth.cts b/src/code-review-depth.cts index 975c97360..73ba7ae68 100644 --- a/src/code-review-depth.cts +++ b/src/code-review-depth.cts @@ -131,7 +131,7 @@ export function normalizeRelPath(p: unknown, repoRoot?: string): string { if (typeof repoRoot === 'string' && repoRoot !== '') { const rootNormalized = repoRoot.trim().replace(/\\/g, '/').replace(/\/+$/, ''); - if (rootNormalized !== '' && value.startsWith(`${rootNormalized}/`)) { + if (rootNormalized !== '' && value.startsWith(`${rootNormalized}/`)) { // allow-handrolled-containment: display-path normalization — strips a caller-declared repoRoot prefix so a path renders repo-relative, not a security root-confinement decision value = value.slice(rootNormalized.length + 1); relativized = true; } @@ -160,7 +160,7 @@ export function normalizeRelPath(p: unknown, repoRoot?: string): string { * with `rulePath + '/'`. Both arguments must already be normalized. Case-sensitive. */ export function ruleMatchesFile(rulePath: string, filePath: string): boolean { - return filePath === rulePath || filePath.startsWith(`${rulePath}/`); + return filePath === rulePath || filePath.startsWith(`${rulePath}/`); // allow-handrolled-containment: rule-to-file segment match for selecting which review-depth rule applies — not a filesystem root-confinement gate } interface RulePathValidation { diff --git a/src/commands.cts b/src/commands.cts index c7413596f..90a00eb12 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, tryWithinRoot, PathAcceptance } from './security.cjs'; +import { requireSafePath, sanitizeForDisplay, tryWithinRoot, assertWithinRoot, PathAcceptance } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, ERROR_REASON } = ioMod; @@ -657,13 +657,11 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method const { getGlobalConfigDir } = require('./runtime-homes.cjs') as { getGlobalConfigDir(runtime: string, explicitDir?: string | null): string }; const agentsDirEff = path.join(getGlobalConfigDir(runtime), 'agents'); - const agentPath = path.join(agentsDirEff, `${agentType}.md`); // agentType is an unvalidated CLI positional: keep the read inside the // agents dir so `../../x` cannot point it elsewhere (defense in depth — - // the reflected surface is only a frontmatter effort line). - if (!path.resolve(agentPath).startsWith(path.resolve(agentsDirEff) + path.sep)) { - throw new Error('agent path escapes the agents directory'); - } + // the reflected surface is only a frontmatter effort line). Untrusted + // input feeding a real read → realpath family (ADR-4650 decision 6). + const agentPath = assertWithinRoot(`${agentType}.md`, agentsDirEff, 'agent file'); const agentContent = fs.readFileSync(agentPath, 'utf8'); // eslint-disable-next-line local/no-unbounded-quantifier -- same lazy `*?` bounded by the `^---$/m` closing anchor as the sibling frontmatter regexes in this file const fmMatchEff = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(agentContent); @@ -2707,7 +2705,7 @@ function groupFilesBySubrepo(files: string[], subRepos: string[]): GroupFilesByS let matchLen = -1; if (candidates) { for (const repo of candidates) { - if (file.startsWith(repo + '/')) { + if (file.startsWith(repo + '/')) { // allow-handrolled-containment: sub-repo file grouping, not a safety decision const repoLen = String(repo).length; if (repoLen > matchLen) { match = repo; diff --git a/src/health-diagnostic-rules/worktree-health.cts b/src/health-diagnostic-rules/worktree-health.cts index 8300d895d..421eea4d2 100644 --- a/src/health-diagnostic-rules/worktree-health.cts +++ b/src/health-diagnostic-rules/worktree-health.cts @@ -162,7 +162,7 @@ function isActiveWorktreePath( ): boolean { const active = shellCmdProjection.toComparablePathKey(activeCwd, platform); const worktree = shellCmdProjection.toComparablePathKey(worktreePath, platform); - return active === worktree || active.startsWith(worktree + '/'); + return active === worktree || active.startsWith(worktree + '/'); // allow-handrolled-containment: worktree identity matching, not a containment gate } function checkW027(snapshot: PlanningSnapshot): Diagnostic[] { diff --git a/src/install-engine.cts b/src/install-engine.cts index a108dab2e..2de0f1ef6 100644 --- a/src/install-engine.cts +++ b/src/install-engine.cts @@ -23,6 +23,7 @@ import os from 'node:os'; import path from 'node:path'; import runtimeArtifactConversion = require('./runtime-artifact-conversion.cjs'); +import { tryWithinRootLexical } from './security.cjs'; import runtimeArtifactLayout = require('./runtime-artifact-layout.cjs'); import runtimeArtifactInstallPlan = require('./runtime-artifact-install-plan.cjs'); import runtimeNamePolicy = require('./runtime-name-policy.cjs'); @@ -132,7 +133,7 @@ function previousOwnedCorpusFiles(configDir: string, prefix: string): string[] { function pruneEmptyCorpusParents(start: string, stop: string): void { let current = path.dirname(start); - while (current !== stop && current.startsWith(stop + path.sep)) { + while (current !== stop && current.startsWith(stop + path.sep)) { // allow-handrolled-containment: ancestor-walk loop condition, not a containment gate if (installFs().readdirSync(current).length > 0) return; installFs().rmdirSync(current); current = path.dirname(current); @@ -389,8 +390,11 @@ function hasExistingSymlinkBetween( const resolvedFullPath = path.resolve(fullPath); // (a) Path-traversal refusal — ALWAYS enforced, even with opt-in. An untrusted // destSubpath string that escapes the install root via '..' is rejected - // regardless of user opt-in state (ADR-1239 Phase B threat (a)). - if (resolvedFullPath !== resolvedRoot && !resolvedFullPath.startsWith(resolvedRoot + path.sep)) { + // regardless of user opt-in state (ADR-1239 Phase B threat (a)). Lexical + // (ADR-4650 decision 6): this function's whole purpose is to DETECT + // symlinks between root and target, so resolving them here would erase what + // it measures. + if (tryWithinRootLexical(resolvedFullPath, resolvedRoot) === null) { return true; } diff --git a/src/installer-migrations.cts b/src/installer-migrations.cts index f08923fde..56f8ebce6 100644 --- a/src/installer-migrations.cts +++ b/src/installer-migrations.cts @@ -124,8 +124,16 @@ function evaluateRemoveEmptyDir(configDir: string, fullPath: string): string { } catch { return 'left-in-place'; } - if (resolvedTarget === resolvedRoot || !resolvedTarget.startsWith(resolvedRoot + path.sep)) { - // Refuses both "target IS configDir" and "target escaped configDir". + // `resolvedTarget === resolvedRoot` is a DELIBERATE ADDITIONAL rejection, + // separate from the containment decision: `tryWithinRootLexical` treats + // target === root as CONTAINED, but removing the config root itself is + // never in scope for this action (see the doc comment above) — this arm + // prevents `rmdirSync` from ever being asked to remove `configDir` itself. + // Kept as its own check per ADR-4650 decision 6 (a wrapper may add its own + // conditions on top of the canonical predicate, never invert it). + if (resolvedTarget === resolvedRoot) return 'left-in-place'; + if (tryWithinRootLexical(resolvedTarget, resolvedRoot) === null) { + // Refuses "target escaped configDir". return 'left-in-place'; } diff --git a/src/milestone.cts b/src/milestone.cts index b29fe2d9e..bbb288343 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -21,7 +21,7 @@ import { transitionCore } from './state-transition.cjs'; import { writeSetComplete } from './write-set.cjs'; import type { WriteSet } from './write-set.cjs'; import { updateTableCell, resetQuickTaskRows, QUICK_TASKS_SECTION_ABSENT } from './markdown-table.cjs'; -import { requireSafePath, PathAcceptance } from './security.cjs'; +import { requireSafePath, PathAcceptance, type ContainedPath } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- audit.cjs is an export= CommonJS module import auditMod = require('./audit.cjs'); const { resolveQuickTaskSummaryFile } = auditMod; @@ -917,7 +917,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo // never disagree with what a real run actually archives. Absent // --archive-quick this stays `[]` and nothing on disk is touched either // way (dry-run always returns before any mutation below). - const quickDirsToArchive: string[] = options.archiveQuick ? listQuickTaskDirsForArchive(cwd) : []; + const quickDirsToArchive: string[] = options.archiveQuick ? listQuickTaskDirsForArchive(cwd).map((d) => d.name) : []; const dryRunResult = { dry_run: true, version, @@ -1625,8 +1625,18 @@ function writeQuickArchiveReadme(archiveQuickDir: string): void { * written three times, and only the real-run copy applied `requireSafePath`, * so a dry-run preview could list a directory the real run would silently * skip). + * + * Returns the proven `ContainedPath` alongside each entry's bare `name` + * (`no-unconfined-path-join`'s `discardedContainmentResult` arm — a bare + * statement call to `requireSafePath` throws away the exact answer it just + * computed). The two dry-run previews only need `name` for display; + * `archiveQuickTaskDirectories` deliberately does NOT reuse `abs` for its + * rename — it re-derives and re-validates independently as TOCTOU + * defense-in-depth (see its own comment), so `abs` exists here only to make + * this function's own discard explicit, not to be trusted downstream as a + * stale-safe proof. */ -function listQuickTaskDirsForArchive(cwd: string): string[] { +function listQuickTaskDirsForArchive(cwd: string): Array<{ name: string; abs: ContainedPath }> { const planningBase = planningPaths(cwd).planning; const quickDir = planningPaths(cwd).quick; let sourceEntries: fs.Dirent[]; @@ -1636,17 +1646,18 @@ function listQuickTaskDirsForArchive(cwd: string): string[] { // .planning/quick absent or unreadable — nothing to select. return []; } - const names: string[] = []; + const results: Array<{ name: string; abs: ContainedPath }> = []; for (const entry of sourceEntries) { if (!entry.isDirectory()) continue; // excludes symlinks too — see MAJOR 3 note above + let abs: ContainedPath; try { - requireSafePath(path.join(quickDir, entry.name), planningBase, 'quick task dir', PathAcceptance.AbsoluteInsideRoot); + abs = requireSafePath(path.join(quickDir, entry.name), planningBase, 'quick task dir', PathAcceptance.AbsoluteInsideRoot); } catch { continue; // symlink/escape attempt — never a candidate, in preview OR real run } - names.push(entry.name); + results.push({ name: entry.name, abs }); } - return names.sort(); + return results.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); } /** @@ -1697,7 +1708,7 @@ function archiveQuickTaskDirectories(cwd: string, version: string): { archiveDir // #2142 MAJOR 5 (review): dirNames is the SAME selection // `listQuickTaskDirsForArchive` hands to both dry-run previews — this is // the real run, so it cannot disagree with what a preview reported. - const dirNames = listQuickTaskDirsForArchive(cwd); + const dirNames = listQuickTaskDirsForArchive(cwd).map((d) => d.name); if (dirNames.length === 0) { // Boundary 0 (#2142): zero (safe) directory entries (empty dir, only // stray files, or every entry excluded by the selection rule) must not @@ -1820,7 +1831,7 @@ function cmdQuickArchive(cwd: string, version: string, options: QuickArchiveOpti // `cmdMilestoneComplete`'s own dry-run preview and the real // `archiveQuickTaskDirectories` both use, so all three can never disagree. if (options.dryRun) { - const quickDirsToArchive: string[] = listQuickTaskDirsForArchive(cwd); + const quickDirsToArchive: string[] = listQuickTaskDirsForArchive(cwd).map((d) => d.name); output( { dry_run: true, diff --git a/src/planning-inspect.cts b/src/planning-inspect.cts index a6d175e50..8935b077b 100644 --- a/src/planning-inspect.cts +++ b/src/planning-inspect.cts @@ -94,7 +94,7 @@ import coreUtilsMod = require('./core-utils.cjs'); const { normalizeLineEndings } = coreUtilsMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import securityMod = require('./security.cjs'); -const { tryWithinRoot, PathAcceptance } = securityMod; +const { tryWithinRoot, PathAcceptance, isContainedIn } = securityMod; /** * The wire schema version. A consumer MUST reject any value other than this @@ -217,15 +217,14 @@ function toPosix(value: string): string { * (and its own not-found/broken-symlink handling). * * NOT an independent containment implementation — it is the comparison step - * of one. `readDocument` below realpaths target and root itself (to keep its - * own exists-vs-escaped tri-state) and calls this directly; `isPathContained` - * gets its containment DECISION from the canonical `tryWithinRoot` predicate - * instead (ADR-4650 decision 6) and no longer uses this function. Every + * of one, and that comparison now comes from `security.cts`'s exported + * `isContainedIn` rather than being redeclared here. `readDocument` below + * realpaths target and root itself (to keep its own exists-vs-escaped + * tri-state) and calls `isContainedIn` directly; `isPathContained` gets its + * containment DECISION from the canonical `tryWithinRoot` predicate instead + * (ADR-4650 decision 6) and never called this comparison directly. Every * caller owns its own resolution. */ -function isWithinRoot(resolvedTarget: string, resolvedRoot: string): boolean { - return resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); -} /** * Containment check for a path (file OR directory), used where the caller @@ -234,7 +233,7 @@ function isWithinRoot(resolvedTarget: string, resolvedRoot: string): boolean { * call site that uses this (an escaped or unresolvable phase directory is * treated identically to an unreadable one). `readDocument` below needs * that distinction for its own exists/readable tri-state, so it keeps its - * own inline `realpathSync` calls and calls `isWithinRoot` directly instead + * own inline `realpathSync` calls and calls `isContainedIn` directly instead * of this wrapper. * * The containment DECISION comes from the canonical `tryWithinRoot` @@ -277,7 +276,7 @@ function readDocument(filePath: string, root: string): { text: string | null; ex // here — the same non-answer `readDocument` already gives "not exists". return { text: null, exists: false, readable: false }; } - if (!isWithinRoot(realTarget, realRoot)) { + if (!isContainedIn(realTarget, realRoot)) { return { text: null, exists: true, readable: false }; } diff --git a/src/refactor-trigger-command-router.cts b/src/refactor-trigger-command-router.cts index 32de4f0b1..ccb5bb208 100644 --- a/src/refactor-trigger-command-router.cts +++ b/src/refactor-trigger-command-router.cts @@ -33,6 +33,7 @@ import path from 'node:path'; import fs from 'node:fs'; import { retryRenameSync } from './shell-command-projection.cjs'; +import { tryWithinRootLexical } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import io = require('./io.cjs'); @@ -240,8 +241,11 @@ function resolvePhaseDirForArg(cwd: string, phaseArg: string): ResolvedPhase | n */ function resolveConfinedPath(cwd: string, relFile: string): string | null { const root = path.resolve(cwd); - const resolved = path.resolve(root, relFile); - if (resolved !== root && !resolved.startsWith(root + path.sep)) return null; + // ADR-4650 decision 6: lexical family — resolving the symlink here would + // undo the refuse-don't-resolve posture documented above; `lstatSync` below + // is the gate that actually refuses a symlink. + const resolved = tryWithinRootLexical(relFile, root); + if (resolved === null) return null; try { if (!fs.lstatSync(resolved).isFile()) return null; } catch { diff --git a/src/research-store.cts b/src/research-store.cts index ef1fd2388..19f1d9b60 100644 --- a/src/research-store.cts +++ b/src/research-store.cts @@ -12,6 +12,10 @@ import os from 'node:os'; import path from 'node:path'; import crypto from 'node:crypto'; import { platformWriteSync } from './shell-command-projection.cjs'; +// ADR-4650 decision 6: lexical family — the key is validated before +// `fs.mkdirSync` creates the store dir, so the candidate legitimately does +// not exist yet at check time. +import { tryWithinRootLexical } from './security.cjs'; // --------------------------------------------------------------------------- // Constants @@ -160,16 +164,13 @@ function putResearch( const entry: ResearchEntry = { content, source, provider, confidence, fetched_at, ttl, kind }; const dir = resolveStorePath(cwd, source, { homeDir }); - // Belt-and-suspenders: ensure the resolved file path stays inside the store dir. - const resolvedDir = path.resolve(dir); - const filePath = path.join(dir, `${key}.json`); - const resolvedFile = path.resolve(filePath); - if (!resolvedFile.startsWith(resolvedDir + path.sep)) { + const containedFile = tryWithinRootLexical(`${key}.json`, dir); + if (containedFile === null || containedFile === path.resolve(dir)) { throw new Error('invalid research key'); } fs.mkdirSync(dir, { recursive: true }); - platformWriteSync(filePath, JSON.stringify(entry)); + platformWriteSync(containedFile, JSON.stringify(entry)); return entry; } @@ -198,16 +199,14 @@ function getResearch(cwd: string, key: string, { clock = Date, homeDir = os.home const candidates: Candidate[] = []; for (const dir of tierDirs) { - const resolvedDir = path.resolve(dir); - const filePath = path.join(dir, `${key}.json`); - // Belt-and-suspenders: ensure path stays inside tier dir - if (!path.resolve(filePath).startsWith(resolvedDir + path.sep)) continue; + const containedFile = tryWithinRootLexical(`${key}.json`, dir); + if (containedFile === null || containedFile === path.resolve(dir)) continue; - if (!fs.existsSync(filePath)) continue; + if (!fs.existsSync(containedFile)) continue; let entry: ResearchEntry; try { - entry = JSON.parse(fs.readFileSync(filePath, 'utf8')) as ResearchEntry; + entry = JSON.parse(fs.readFileSync(containedFile, 'utf8')) as ResearchEntry; } catch { // Corrupt file in this tier — skip it continue; diff --git a/src/reviewer-step-dispatch.cts b/src/reviewer-step-dispatch.cts index 72b600647..cefbfa7f9 100644 --- a/src/reviewer-step-dispatch.cts +++ b/src/reviewer-step-dispatch.cts @@ -36,6 +36,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { estimateTokens } from './prompt-budget.cjs'; +import { tryWithinRootLexical } from './security.cjs'; import type { LanePlan, ResolveResult } from './review-lane-invocation.cjs'; import { resolveLaneBudget, artifactPaths } from './review-lane-invocation.cjs'; import type { ReviewerLane } from './review-lane-descriptor.cjs'; @@ -190,8 +191,13 @@ function validatePaths( if (typeof p !== 'string' || p.length === 0 || CONTROL_CHAR.test(p)) { return { ok: false, reason: DISPATCH_REASON.INVALID_PATHS }; } + // ADR-4650 decision 6: lexical family — this is the first of the two + // deliberate halves (#4209 WR-05); the ENOENT-tolerant realpath half + // below cannot be folded into a single `tryWithinRoot` call (its + // ancestor-walk would accept a deleted path via the nearest existing + // ancestor, not the explicit `continue` this code requires). const resolved = path.resolve(root, p); - if (resolved !== root && !resolved.startsWith(root + path.sep)) { + if (tryWithinRootLexical(p, root) === null) { return { ok: false, reason: DISPATCH_REASON.PATH_ESCAPES_REPO_ROOT }; } // #4209 WR-05: `path.resolve` is lexical only — a symlink whose OWN path sits inside @@ -205,7 +211,7 @@ function validatePaths( } catch { continue; } - if (real !== realRoot && !real.startsWith(realRoot + path.sep)) { + if (tryWithinRootLexical(real, realRoot) === null) { return { ok: false, reason: DISPATCH_REASON.PATH_ESCAPES_REPO_ROOT }; } } diff --git a/src/runtime-artifact-install-plan.cts b/src/runtime-artifact-install-plan.cts index 8ca66eeed..4991b55a2 100644 --- a/src/runtime-artifact-install-plan.cts +++ b/src/runtime-artifact-install-plan.cts @@ -11,6 +11,7 @@ // In .cts (CommonJS output) files, `require` is available as a global. const _require: NodeRequire = require; const path = _require('node:path') as typeof import('node:path'); +const { tryWithinRootLexical } = _require('./security.cjs') as typeof import('./security.cjs'); // #2870: InstallScope is owned by install-scope.cts, not re-declared here. // `isGlobalScope` centralizes the `scope === 'global'` boolean projection @@ -140,13 +141,21 @@ function assertDestWithinConfigHome(configDir: string, destSubpath: string): str ); } const root = path.resolve(configDir); - const resolved = path.resolve(configDir, destSubpath); - if (resolved === root || !resolved.startsWith(root + path.sep)) { + // `resolved === root` is a DELIBERATE ADDITIONAL rejection, separate from + // the containment decision: `tryWithinRootLexical` treats target === root + // as CONTAINED, but a destSubpath of "" (or one that resolves to configDir + // itself) must never be accepted here — this is the strict-subpath + // requirement Phase B of ADR-1239 imposes on third-party descriptors, and + // it prevents a descriptor from writing at configHome itself. Kept as its + // own check per ADR-4650 decision 6 (a wrapper may add its own conditions + // on top of the canonical predicate, never invert it). + const contained = tryWithinRootLexical(destSubpath, configDir); + if (contained === null || contained === root) { throw new Error( `destSubpath "${destSubpath}" must be a strict subpath of configHome "${configDir}" — not configHome itself or outside it (escapes configHome)`, ); } - return resolved; + return contained; } function errorMessage(err: unknown): string { diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index 9d8006e0d..06655f83c 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -23,6 +23,7 @@ import os from 'node:os'; // unless the top-level installRuntimeArtifacts call injected a `deps.fs`. // eslint-disable-next-line @typescript-eslint/no-require-imports import installFsAdapter = require('./install-fs-adapter.cjs'); +import { tryWithinRootLexical } from './security.cjs'; const { installFs, mkInstallTempDir } = installFsAdapter; // Reuse the install manifest's existing parser and streamed SHA-256 // classification instead of deriving a second integrity implementation here. @@ -196,10 +197,15 @@ function isReadableDirectory(candidate: string, routed: boolean): boolean { } function isPhysicallyConfinedTo(root: string, candidate: string): boolean { + // ADR-4650 decision 6: lexical family on already-realpath'd operands — the + // surrounding try/catch must survive verbatim, since a non-existent + // candidate throwing out of realpathSync (not `tryWithinRootLexical`, which + // would accept it) is exactly the "incomplete manifest" signal this + // function's callers depend on. try { const physicalRoot = installFs().realpathSync(root); const physicalCandidate = installFs().realpathSync(candidate); - return physicalCandidate === physicalRoot || physicalCandidate.startsWith(physicalRoot + path.sep); + return tryWithinRootLexical(physicalCandidate, physicalRoot) !== null; } catch { return false; } @@ -253,9 +259,11 @@ function installedManifestIsComplete( for (const key of expected) { const parts = key.split('/'); if (parts.some((part) => part === '' || part === '.' || part === '..')) return false; - const candidate = path.resolve(runtimeConfigDir, ...parts); - const root = path.resolve(runtimeConfigDir); - if (!candidate.startsWith(root + path.sep)) return false; + // ADR-4650 decision 6: lexical family — the object is lstat'd (never + // stat'd) and refused if it is a symlink just below, so this gate must + // refuse rather than resolve. + const candidate = tryWithinRootLexical(parts.join('/'), runtimeConfigDir); + if (candidate === null || candidate === path.resolve(runtimeConfigDir)) return false; const stat = io.lstatSync(candidate); if (!stat.isFile() || stat.isSymbolicLink()) return false; if (installerMigrations.classifyArtifact(runtimeConfigDir, key, manifest).classification !== 'managed-pristine') { @@ -308,7 +316,7 @@ function providersShareRequiredRoots( const overlap = (leftPath: string, rightPath: string): boolean => { const relative = path.relative(leftPath, rightPath); return relative === '' || - (relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); + (relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); // allow-handrolled-containment: bidirectional physical-root overlap/identity check between two providers for dedup detection — not a security confinement gate on untrusted input }; const physicalLeft = canonicalize(leftFs, leftRoot); const physicalRight = canonicalize(rightFs, rightRoot); diff --git a/src/security.cts b/src/security.cts index 950f42f46..a0e13305f 100644 --- a/src/security.cts +++ b/src/security.cts @@ -35,14 +35,22 @@ import path from 'node:path'; * * `pathImpl` lets a caller supply `path.win32` / `path.posix` instead of the * ambient module, so win32 separator semantics are testable off Windows. + * + * Exported for callers that have ALREADY resolved both operands themselves + * and need only this comparison step (e.g. a caller that owns its own + * `fs.realpathSync` calls to preserve an exists-vs-escaped tri-state). A + * caller that has NOT resolved its operands must NOT reach for this function + * directly — the comparison alone is not a containment check — and should use + * `assertWithinRoot` / `tryWithinRoot` (or the `assertWithinRootLexical` / + * `tryWithinRootLexical` pair) instead. */ -function isContainedIn( +export function isContainedIn( resolvedTarget: string, resolvedRoot: string, pathImpl: { sep: string } = path, ): boolean { if (resolvedTarget === resolvedRoot) return true; - return (resolvedTarget + pathImpl.sep).startsWith(resolvedRoot + pathImpl.sep); + return (resolvedTarget + pathImpl.sep).startsWith(resolvedRoot + pathImpl.sep); // allow-handrolled-containment: this IS the canonical comparison every other site routes through } /** diff --git a/src/task-command-router.cts b/src/task-command-router.cts index a06d02e08..8fdd7900d 100644 --- a/src/task-command-router.cts +++ b/src/task-command-router.cts @@ -8,6 +8,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { tryWithinRootLexical } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error, ERROR_REASON } = ioMod; @@ -164,12 +165,14 @@ function routeResolveContent( } const projectRoot = path.resolve(cwd || process.cwd()); - const resolvedPlanPath = path.resolve(projectRoot, plan); - const rel = path.relative(projectRoot, resolvedPlanPath); - if (rel === '..' || rel.startsWith(`..${path.sep}`)) { + // Lexical containment (ADR-4650): this path is validated before existence is + // checked below, so realpath resolution is neither available nor required. + const contained = tryWithinRootLexical(plan, projectRoot); + if (contained === null) { error(`Plan file is outside project scope: ${plan}`, ERROR_REASON.USAGE); return; } + const resolvedPlanPath = contained; if (!fs.existsSync(resolvedPlanPath)) { error(`Plan file not found: ${plan}`, ERROR_REASON.USAGE); return; @@ -238,11 +241,14 @@ function routeTaskCommand({ args, cwd, raw }: RouteTaskCommandOptions): void { } else if (args[2]) { const projectRoot = path.resolve(cwd || process.cwd()); const requestedPath = args[2]; - const resolvedTaskPath = path.resolve(projectRoot, requestedPath); - const rel = path.relative(projectRoot, resolvedTaskPath); - if (rel === '..' || rel.startsWith(`..${path.sep}`)) { + // Lexical containment (ADR-4650): validated before existence is checked below. + // `error()` here does not return/throw (preserved from before this migration), + // so resolvedTaskPath must still be computed identically on the rejected path. + const contained = tryWithinRootLexical(requestedPath, projectRoot); + if (contained === null) { error(`Task file is outside project scope: ${requestedPath}`, ERROR_REASON.USAGE); } + const resolvedTaskPath = contained ?? path.resolve(projectRoot, requestedPath); if (!fs.existsSync(resolvedTaskPath)) { error(`Task file not found: ${requestedPath}`, ERROR_REASON.USAGE); } diff --git a/src/verification.cts b/src/verification.cts index 6ed2acd72..43823a78a 100644 --- a/src/verification.cts +++ b/src/verification.cts @@ -45,6 +45,7 @@ import coreUtilsMod = require('./core-utils.cjs'); import planningScopeMod = require('./planning-scope.cjs'); import { execGit } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; +import { isContainedIn } from './security.cjs'; const { output, error } = io; const { extractPhaseToken, scopeToPhase } = phaseId; @@ -297,8 +298,11 @@ function computeCoveredDigest(projectRoot: string, coveredFiles: readonly string // confinement check above is not enough. realpathSync resolves the // actual target; re-confining against realRoot closes that gap. const real = fs.realpathSync(resolved); - const realRel = path.relative(realRoot, real); - if (realRel === '' || realRel === '..' || realRel.startsWith(`..${path.sep}`) || path.isAbsolute(realRel)) { + // Both operands are already realpath-resolved (this fn's own realpathSync calls + // above), so the shared containment comparison applies directly (ADR-4650) — + // no re-resolution through assertWithinRoot/tryWithinRoot, which would redo work + // this function already owns for its exists-vs-escaped tri-state. + if (!isContainedIn(real, realRoot)) { return null; } const st = fs.statSync(real); diff --git a/src/verify-command-grounding.cts b/src/verify-command-grounding.cts index 73ba7cb9a..392073431 100644 --- a/src/verify-command-grounding.cts +++ b/src/verify-command-grounding.cts @@ -606,7 +606,7 @@ function declaredPathCovers(declaredPaths: string[] | undefined, norm: string): return declaredPaths.some(p => { if (typeof p !== 'string') return false; const dp = stripLeadingDotSlash(toSlash(p)); - return dp === target || dp.startsWith(target + '/'); + return dp === target || dp.startsWith(target + '/'); // allow-handrolled-containment: declared-path coverage for pending-creation detection, not containment }); } diff --git a/src/verify.cts b/src/verify.cts index 91e6015d6..9d7b4a275 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -10,6 +10,7 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; import { textEncodingError } from './validate.cjs'; +import { tryWithinRootLexical } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module @@ -173,9 +174,12 @@ function verifySummaryCore( const firstSegment = candidate.split('/')[0] || ''; if (firstSegment.indexOf('.') > 0) return false; // Containment guard: a `../`-bearing reference must not turn this advisory - // into a filesystem existence probe outside the project. - const resolved = path.resolve(projectRoot, candidate); - if (resolved !== projectRoot && !resolved.startsWith(projectRoot + path.sep)) return false; + // into a filesystem existence probe outside the project. Lexical (ADR-4650 + // decision 6): the candidate is a string pulled from a SUMMARY document and + // by construction may not exist yet — existence is what gets probed + // downstream — and this is a pure string-heuristic filter with no other fs + // access, so a realpath call would also change its cost profile. + if (tryWithinRootLexical(candidate, projectRoot) === null) return false; return true; }; diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index d0b23ecb6..c42b5d864 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -11,6 +11,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { execGit as execGitSeam, posixNormalize, type SpawnResultOutput } from './shell-command-projection.cjs'; +import { isContainedIn } from './security.cjs'; // Default timeout for worktree-related git subprocess calls. // 10 s is generous enough for normal git operations on large repos while still @@ -743,7 +744,7 @@ function normalizeScopePath(raw: string): string { */ function isSummaryArtifactRelPath(relPath: string): boolean { const normalized = normalizeScopePath(relPath); - return normalized.startsWith(`${SUMMARY_ARTIFACT_DIR}/`) + return normalized.startsWith(`${SUMMARY_ARTIFACT_DIR}/`) // allow-handrolled-containment: artifact-type classification by a fixed known subdirectory name, not a filesystem root-confinement gate && normalized.endsWith(SUMMARY_ARTIFACT_SUFFIX); } @@ -947,7 +948,7 @@ function planWaveScopeConformance( seen.add(changed); if (isSummaryArtifactRelPath(changed)) continue; const covered = prefixes.some((prefix) => ( - prefix === null || changed === prefix || changed.startsWith(`${prefix}/`) + prefix === null || changed === prefix || changed.startsWith(`${prefix}/`) // allow-handrolled-containment: advisory scope-coverage match against a caller-declared prefix (documented above as deliberately distinct from a security gate) — not a filesystem root-confinement decision )); if (covered) continue; warnings.push({ code: WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, branch, path: changed }); @@ -1879,11 +1880,15 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm { const absRoot = path.resolve(cwd, rootFlag); const absWorktree = path.resolve(cwd, plan.entry.worktree_path); - const rel = path.relative(absRoot, absWorktree); - // rel === '' → the worktree IS the root (would clobber the checkout) - // rel === '..' / '../…' → escapes the root - // path.isAbsolute(rel) → a different Windows drive or UNC root - if (rel === '' || rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel)) { + // Lexical containment (ADR-4650): the worktree does not exist yet, so there is + // nothing to realpath. Both operands are already resolved above, so the shared + // `isContainedIn` comparison applies directly (mirrors the already-resolved-caller + // exception documented on `isContainedIn`) — a different Windows drive/UNC root + // fails the prefix comparison the same way an escaping relative path does. + // The canonical predicate treats target === root as CONTAINED; this call site + // explicitly REJECTS that case (absWorktree === absRoot below), because a worktree + // AT the root would clobber the checkout — the inversion this migration preserves. + if (absWorktree === absRoot || !isContainedIn(absWorktree, absRoot)) { const hint = `--path must resolve INSIDE --root (root="${absRoot}", path="${absWorktree}"). A worktree outside the declared root is unreachable by manifest-scoped cleanup and would let a spawned executor write outside the project.`; writeErr(`[gsd] worktree.create: path_outside_root — ${hint}\n`); write(`${JSON.stringify({ ok: false, reason: 'path_outside_root', hint }, null, 2)}\n`); diff --git a/tests/no-unconfined-path-join.rule.test.cjs b/tests/no-unconfined-path-join.rule.test.cjs new file mode 100644 index 000000000..6864bb350 --- /dev/null +++ b/tests/no-unconfined-path-join.rule.test.cjs @@ -0,0 +1,499 @@ +'use strict'; + +/** + * no-unconfined-path-join.rule.test.cjs + * + * RuleTester unit tests for the local/no-unconfined-path-join ESLint rule. + * + * Arm 1: hand-rolled containment comparison `x.startsWith(y + sep)`. + * Arm 2: a discarded containment-predicate result (bare statement call). + * Plus the `allow-handrolled-containment: ` marker escape and the + * allowlist ratchet mechanics. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const rule = require('../eslint-rules/no-unconfined-path-join.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('no-unconfined-path-join rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof rule.meta, 'object'); + assert.strictEqual(typeof rule.create, 'function'); + assert.strictEqual(rule.meta.type, 'problem'); + assert.ok(rule.meta.messages.handRolledContainment, 'handRolledContainment message must exist'); + assert.ok( + rule.meta.messages.discardedContainmentResult, + 'discardedContainmentResult message must exist', + ); + assert.ok(rule.meta.messages.staleAllowlistEntry, 'staleAllowlistEntry message must exist'); + }); +}); + +// ─── Arm 1: hand-rolled containment — fires ────────────────────────────────── + +describe('no-unconfined-path-join: arm 1 fires', () => { + test('invalid: resolved.startsWith(root + path.sep)', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: resolved !== root && !resolved.startsWith(root + path.sep) reports exactly one error', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `if (resolved !== root && !resolved.startsWith(root + path.sep)) { throw new Error('x'); }`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: x.startsWith(y + "/")', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `x.startsWith(y + '/');`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: x.startsWith(y + "\\\\")', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `x.startsWith(y + '\\\\');`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: x.startsWith(y + p.sep)', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `x.startsWith(y + p.sep);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: x.startsWith(`${root}${path.sep}`) — template literal ending in a `.sep` expression', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: 'x.startsWith(`${root}${path.sep}`);', + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: x.startsWith(`${root}/`) — template literal ending in a literal separator character', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: 'x.startsWith(`${root}/`);', + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: const sep = path.sep; x.startsWith(root + sep) — separator reached through a single const alias', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: 'const sep = path.sep; x.startsWith(root + sep);', + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); +}); + +// ─── Arm 1: hand-rolled containment — silent ───────────────────────────────── + +describe('no-unconfined-path-join: arm 1 silent', () => { + test('valid: x.startsWith(prefix) — bare identifier argument', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith(prefix);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: x.startsWith("gsd-") — plain literal argument', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith('gsd-');`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: x.startsWith(y + "-") — right side is not a separator', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith(y + '-');`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: x.startsWith(y + z) — right side is an unresolvable identifier', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith(y + z);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: let sep = path.sep; x.startsWith(root + sep) — reassignable `let` binding is not resolved', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `let sep = path.sep; x.startsWith(root + sep);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: function f(sep) { x.startsWith(root + sep); } — parameter binding is not resolved', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { code: `function f(sep) { x.startsWith(root + sep); }`, filename: 'src/foo.cts' }, + ], + invalid: [], + }); + }); + + test('valid: x.startsWith(a, b) — wrong arity', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith(a, b);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: x.startsWith(`${root}-suffix`) — template literal NOT ending in a separator', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: 'x.startsWith(`${root}-suffix`);', filename: 'src/foo.cts' }], + invalid: [], + }); + }); +}); + +// ─── Arm 2: discarded containment result — fires ───────────────────────────── + +describe('no-unconfined-path-join: arm 2 fires', () => { + test('invalid: assertWithinRoot(p, root); as a bare statement', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `assertWithinRoot(p, root);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'discardedContainmentResult' }], + }, + ], + }); + }); + + test('invalid: tryWithinRoot(p, root); as a bare statement', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `tryWithinRoot(p, root);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'discardedContainmentResult' }], + }, + ], + }); + }); + + test('invalid: isContainedIn(p, root); as a bare statement — boolean predicate discarded', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `isContainedIn(p, root);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'discardedContainmentResult' }], + }, + ], + }); + }); +}); + +// ─── Arm 2: discarded containment result — silent ──────────────────────────── + +describe('no-unconfined-path-join: arm 2 silent', () => { + test('valid: const x = assertWithinRoot(p, root); — VariableDeclarator init', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `const x = assertWithinRoot(p, root);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); + + test('valid: if (tryWithinRoot(p, root) === null) {} — inside a comparison', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { code: `if (tryWithinRoot(p, root) === null) {}`, filename: 'src/foo.cts' }, + ], + invalid: [], + }); + }); + + test('valid: return assertWithinRoot(p, root); — return value', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `function f() { return assertWithinRoot(p, root); }`, + filename: 'src/foo.cts', + }, + ], + invalid: [], + }); + }); + + test('valid: fs.readFileSync(assertWithinRoot(p, root)); — call argument', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { code: `fs.readFileSync(assertWithinRoot(p, root));`, filename: 'src/foo.cts' }, + ], + invalid: [], + }); + }); + + test('valid: someOtherFn(p, root); — unrelated callee name', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `someOtherFn(p, root);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); +}); + +// ─── marker escape ──────────────────────────────────────────────────────────── + +describe('no-unconfined-path-join: marker escape', () => { + test('valid: suppressed with a real reason', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-handrolled-containment: legacy call site pending migration`, + filename: 'src/foo.cts', + }, + ], + invalid: [], + }); + }); + + test('invalid: does NOT suppress with an empty reason', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-handrolled-containment:`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: does NOT suppress without a colon', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-handrolled-containment`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: does NOT suppress when the marker is on a different line', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `// allow-handrolled-containment: legacy call site pending migration\nresolved.startsWith(root + path.sep);`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('invalid: does NOT suppress with a BLOCK comment on the same line', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep); /* allow-handrolled-containment: legacy call site pending migration */`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('two violations on one line: trailing marker suppresses only the one it trails', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `a.startsWith(root + path.sep); b.startsWith(root + path.sep); // allow-handrolled-containment: legacy call site pending migration`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('valid: suppressed with a justification (b) reason — pre-build bootstrap file', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-handrolled-containment: this is a committed .cjs that must run before npm run build:lib, so src/security.cts is unreachable here`, + filename: 'gsd-core/bin/lib/capability-validator.cjs', + }, + ], + invalid: [], + }); + }); + + test('valid: marker suppresses a discarded arm-2 call — tryWithinRoot(p, root); // allow-handrolled-containment: ', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `tryWithinRoot(p, root); // allow-handrolled-containment: legacy call site pending migration`, + filename: 'src/foo.cts', + }, + ], + invalid: [], + }); + }); + + test('invalid: the old spelling allow-lexical-prefix-match no longer suppresses', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-lexical-prefix-match: legacy call site pending migration`, + filename: 'src/foo.cts', + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); +}); + +// ─── allowlist ratchet ──────────────────────────────────────────────────────── + +describe('no-unconfined-path-join: allowlist', () => { + test('valid: allowlisted file with violations reports nothing', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `resolved.startsWith(root + path.sep);`, + filename: 'src/legacy.cts', + options: [{ allowlist: ['src/legacy.cts'] }], + }, + ], + invalid: [], + }); + }); + + test('invalid: allowlisted file with ZERO violations reports staleAllowlistEntry', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `const x = 1;`, + filename: 'src/legacy.cts', + options: [{ allowlist: ['src/legacy.cts'] }], + errors: [{ messageId: 'staleAllowlistEntry' }], + }, + ], + }); + }); + + test('invalid: allowlisted file whose only occurrence is marker-suppressed reports staleAllowlistEntry', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep); // allow-handrolled-containment: legacy call site pending migration`, + filename: 'src/legacy.cts', + options: [{ allowlist: ['src/legacy.cts'] }], + errors: [{ messageId: 'staleAllowlistEntry' }], + }, + ], + }); + }); + + test('invalid: non-allowlisted file with violations reports', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [], + invalid: [ + { + code: `resolved.startsWith(root + path.sep);`, + filename: 'src/other.cts', + options: [{ allowlist: ['src/legacy.cts'] }], + errors: [{ messageId: 'handRolledContainment' }], + }, + ], + }); + }); + + test('valid: allowlisted file with one marked and one unmarked occurrence keeps the entry alive', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [ + { + code: `a.startsWith(root + path.sep); // allow-handrolled-containment: legacy call site pending migration\nb.startsWith(root + path.sep);`, + filename: 'src/legacy.cts', + options: [{ allowlist: ['src/legacy.cts'] }], + }, + ], + invalid: [], + }); + }); + + test('valid: rule configured with no options at all does not crash', () => { + ruleTester.run('no-unconfined-path-join', rule, { + valid: [{ code: `x.startsWith(prefix);`, filename: 'src/foo.cts' }], + invalid: [], + }); + }); +});