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: [], + }); + }); +});