From bbc3f131be8983c6f9d921d2da53ee2424a198d4 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 18:33:01 -0400 Subject: [PATCH] refactor(#4653): make containment ONE decision, resolved two ways MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Satisfies #4653 DW1 and DW9, which were the phase's outstanding acceptance criteria: every other implementation must be deleted or route its containment DECISION through the canonical predicate, and no surviving wrapper may decide WHETHER a path is contained. Three implementations were being retained with their own comparisons, on the argument that each needs LEXICAL resolution — a realpath-based predicate is the wrong tool wherever a symlink must be preserved rather than resolved. That argument is correct about RESOLUTION and was being used to justify owning the DECISION too. Those are separable, and separating them is what closes the criteria honestly rather than by reinterpretation. isContainedIn(resolvedTarget, resolvedRoot, pathImpl?) module-internal is now the single place this repo decides containment. It is separator-aware, so a sibling merely sharing a prefix (`-evil` against ``) is still rejected. Two exported families sit on it and differ ONLY in how a candidate is resolved before the decision: assertWithinRoot / tryWithinRoot realpath-resolving assertWithinRootLexical / tryWithinRootLexical path.resolve only, no I/O The lexical pair carries `opts.pathImpl`, so win32 separator semantics stay testable off Windows — that seam already existed in isPathConfined and would have been lost by a naive collapse. The three call sites now take their decision from the predicate and keep only what is genuinely theirs: external-descriptor-trust isPathConfined delegates outright; pathImpl forwarded installer-migrations ensureInsideConfig delegates; keeps its own message and its LEXICAL fullPath, which callers consume for existsSync and journal rows gsd-tools.cjs isInsideDir delegates; keeps its own `target !== root` condition, and the separate symlink refusal above it stands DW5 is not weakened by this. That criterion binds the symlink oracle and the ancestor canonicalization; both are untouched. The only change inside validatePath is three comparison lines becoming one call, and the rejection string `Path escapes allowed directory: is outside ` stays byte-identical because it is an observable CLI contract. What this does NOT do, stated plainly: the lexical family still cannot see a symlink. That is a property of lexical resolution, not a gap in the seam, and the three callers that need it are the three that must pair it with their own symlink refusal — which is exactly what the fix earlier in this phase added at the install sites. The doc comment says so at the definition, and CONTEXT.md and docs/explanation/security-model.md are corrected: they previously described these three as deliberately NOT routed through the predicate, which is no longer true. Co-Authored-By: Claude Opus 5 --- CONTEXT.md | 2 +- docs/explanation/security-model.md | 17 +++++-- gsd-core/bin/gsd-tools.cjs | 20 +++++--- src/external-descriptor-trust.cts | 12 +++-- src/installer-migrations.cts | 23 +++++---- src/security.cts | 78 ++++++++++++++++++++++++++++-- 6 files changed, 123 insertions(+), 29 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 66052e7d9..dd11b68aa 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. **Three implementations are deliberately NOT collapsed, and each is stricter or narrower rather than duplicative** — a realpath-based predicate is the wrong tool wherever a symlink must be preserved rather than resolved: `gsd-core/bin/gsd-tools.cjs`'s restore gate rejects symlinks OUTRIGHT (the canonical predicate accepts a link resolving inside the root, which for a restore still writes through the link) and treats `target === root` as NOT contained; `external-descriptor-trust.cts`'s `isPathConfined` is lexical by design because two install callers validate a `destSubpath` BEFORE the `mkdirSync` that creates it, where realpath cannot resolve; and `installer-migrations.cts`'s `ensureInsideConfig` is lexical because that module's contract is that a symlinked managed path is snapshotted, restored and backed up AS A LINK and never dereferenced — routing it through the canonical predicate dereferenced exactly those links and then rejected them for escaping `configDir`, which the remote matrix caught as four failing tests. `normalizeRelPath` is its real pre-gate, throwing on absolute paths and `..` segments before the containment check 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. 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 bbcc839d2..99a67d377 100644 --- a/docs/explanation/security-model.md +++ b/docs/explanation/security-model.md @@ -162,10 +162,19 @@ module is the central security utility. It provides: - Shell argument validation: arguments passed to subshell commands are validated before use -Three containment checks elsewhere in the tree are deliberately NOT routed through -this predicate, because each is narrower or stricter rather than a second opinion. -The common thread is that a realpath-based predicate is the wrong tool wherever a -symlink must be *preserved* rather than resolved. +Containment is decided in exactly one place, but resolved two ways. The +comparison itself — separator-aware, so a sibling merely sharing a prefix is +never accepted — is internal to `security.cjs` 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` resolve symlinks, +and `assertWithinRootLexical` / `tryWithinRootLexical` use string resolution +alone and never touch the filesystem. + +The lexical form exists because a realpath-based predicate is the wrong tool +wherever a symlink must be *preserved* rather than resolved, or where the target +legitimately does not exist yet. 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 use it, each for a stated reason. The backup-restore gate in `gsd-core/bin/gsd-tools.cjs` rejects symlinks outright: the canonical predicate accepts a link whose target resolves inside the root, but diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 7378564f0..9c6a23ff2 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -3359,16 +3359,22 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // at the backed-up path. These checks reject links outright, which is // strictly stricter than assertWithinRoot/tryWithinRoot, not a // reimplementation of them. Do not "simplify" this to assertWithinRoot or - // tryWithinRoot. Reviewed under epic #4636 Phase 3 and deliberately NOT - // collapsed. Note also: isInsideDir below treats target === root as NOT - // contained (it requires a non-empty relative path), unlike every other - // containment implementation in this repo, which treats target === root as - // contained. + // tryWithinRoot. Reviewed under epic #4636 Phase 3: the containment + // DECISION now routes through the canonical lexical predicate + // (`tryWithinRootLexical`, ADR-4650 decision 6); isInsideDir below still + // treats target === root as NOT contained via its own extra `!==` check + // (unlike every other containment implementation in this repo, which + // treats target === root as contained) — that condition is this gate's + // own and is layered on top of the shared predicate, not folded into it. /** True when `target` resolves strictly inside `root`. */ function isInsideDir(root, target) { - const rel = path.relative(path.resolve(root), path.resolve(target)); - return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); + // Containment decision: canonical lexical predicate (ADR-4650 decision 6). + // The extra `!==` condition is this gate's own: a restore must never + // target the config directory itself, only something strictly inside it. + if (path.resolve(target) === path.resolve(root)) return false; + const { tryWithinRootLexical } = require('./lib/security.cjs'); + return tryWithinRootLexical(target, root) !== null; } /** diff --git a/src/external-descriptor-trust.cts b/src/external-descriptor-trust.cts index ce3ee854f..e34c99378 100644 --- a/src/external-descriptor-trust.cts +++ b/src/external-descriptor-trust.cts @@ -20,6 +20,7 @@ 'use strict'; import path from 'node:path'; +import { tryWithinRootLexical } from './security.cjs'; /** * Pure LEXICAL path-containment check (cross-platform). `target` is confined @@ -53,6 +54,11 @@ import path from 'node:path'; * in this repo, e.g. src/shell-command-projection.cts's `opts.platform` * (#4641). All existing 2-arg callers are unaffected: the default resolves to * the ambient `path`, preserving byte-identical behaviour. + * + * The containment DECISION here now comes from the canonical predicate in + * src/security.cts (`tryWithinRootLexical`, ADR-4650 decision 6) — this + * function keeps only the lexical RESOLUTION policy (no realpath, no + * filesystem access) as its own choice; the comparison itself is shared. */ export function isPathConfined( target: string, @@ -62,11 +68,7 @@ export function isPathConfined( if (typeof target !== 'string' || typeof root !== 'string' || target.length === 0 || root.length === 0) { return false; } - const p = opts.pathImpl ?? path; - const rootResolved = p.resolve(root); - const targetResolved = p.resolve(root, target); - const prefix = rootResolved + p.sep; - return targetResolved === rootResolved || targetResolved.startsWith(prefix); + return tryWithinRootLexical(target, root, { pathImpl: opts.pathImpl }) !== null; } export interface DescriptorArtifactKind { diff --git a/src/installer-migrations.cts b/src/installer-migrations.cts index 69e3f1a61..f08923fde 100644 --- a/src/installer-migrations.cts +++ b/src/installer-migrations.cts @@ -19,6 +19,7 @@ import { import { platformWriteSync, retryRenameSync, posixNormalize } from './shell-command-projection.cjs'; import { realClock, type Clock } from './clock.cjs'; import { isInstallScopeId, type InstallScope } from './install-scope.cjs'; +import { tryWithinRootLexical } from './security.cjs'; // #2874 (ADR-58 cleanup phase): this file is the ~1200-line migration // plan/apply/rollback/lock/journal engine — almost none of it is on the // installRuntimeArtifacts call tree. Only `readInstallManifest` and @@ -664,26 +665,30 @@ interface EnsureInsideConfigResult { fullPath: string; } -// DELIBERATELY LEXICAL — do not route this through the canonical containment -// predicate (`assertWithinRoot` / `tryWithinRoot`, src/security.cts). +// DELIBERATELY LEXICAL — the RESOLUTION policy stays lexical, never realpath +// (`assertWithinRoot` / `tryWithinRoot`, src/security.cts). // // Reviewed under epic #4636 Phase 3 and reverted after the remote matrix proved -// the collapse wrong. This module's contract is that a symlinked managed path is -// treated AS A LINK and never dereferenced — it is snapshotted as a link, -// restored as a link, and backed up as a link. The canonical predicate -// realpath-resolves, so it dereferences exactly the symlinks this module exists -// to preserve and then rejects them for escaping configDir +// the realpath collapse wrong. This module's contract is that a symlinked +// managed path is treated AS A LINK and never dereferenced — it is snapshotted +// as a link, restored as a link, and backed up as a link. The realpath-based +// predicate dereferences exactly the symlinks this module exists to preserve +// and then rejects them for escaping configDir // ("migration path escapes configDir: extensions/gsd.cjs"). Four tests in // tests/installer-migrations.test.cjs pin that behavior. // +// The containment DECISION now routes through the canonical LEXICAL predicate +// (`tryWithinRootLexical`, ADR-4650 decision 6) — only the comparison moved; +// the lexical policy itself remains this module's own required choice, and +// the thrown message / returned `fullPath` are unchanged. +// // `normalizeRelPath` is the pre-gate: it throws on absolute paths and on any // '..' segment BEFORE this runs, so the check below is defense-in-depth over // already-traversal-free input rather than the primary boundary. function ensureInsideConfig(configDir: string, relPath: string): EnsureInsideConfigResult { const normalized = normalizeRelPath(relPath); const fullPath = path.resolve(configDir, normalized); - const root = path.resolve(configDir); - if (fullPath !== root && !fullPath.startsWith(root + path.sep)) { + if (tryWithinRootLexical(fullPath, configDir) === null) { throw new Error(`migration path escapes configDir: ${relPath}`); } return { normalized, fullPath }; diff --git a/src/security.cts b/src/security.cts index 260832f3c..950f42f46 100644 --- a/src/security.cts +++ b/src/security.cts @@ -24,6 +24,27 @@ import path from 'node:path'; // ─── Path Traversal Prevention ────────────────────────────────────────────── +/** + * THE containment comparison — the single place this repo decides whether an + * already-resolved path lies inside an already-resolved root (ADR-4650). + * + * Separator-aware on purpose: comparing the bare strings would accept a + * sibling that merely shares a prefix (`-evil` against ``), so both + * sides get a trailing separator before the prefix test. `target === root` is + * contained. + * + * `pathImpl` lets a caller supply `path.win32` / `path.posix` instead of the + * ambient module, so win32 separator semantics are testable off Windows. + */ +function isContainedIn( + resolvedTarget: string, + resolvedRoot: string, + pathImpl: { sep: string } = path, +): boolean { + if (resolvedTarget === resolvedRoot) return true; + return (resolvedTarget + pathImpl.sep).startsWith(resolvedRoot + pathImpl.sep); +} + /** * Validate that a file path resolves within an allowed base directory. * Prevents path traversal attacks via ../ sequences, symlinks, or absolute paths. @@ -102,9 +123,7 @@ function validatePath(filePath: unknown, baseDir: unknown, opts: { allowAbsolute } } } - const normalizedBase = resolvedBase + path.sep; - const normalizedPath = resolvedPath + path.sep; - if (resolvedPath !== resolvedBase && !normalizedPath.startsWith(normalizedBase)) { + if (!isContainedIn(resolvedPath, resolvedBase)) { return { safe: false, resolved: resolvedPath, @@ -261,6 +280,59 @@ export function requireSafePath(filePath: unknown, baseDir: unknown, label: stri return assertWithinRoot(filePath, baseDir, label, policy); } +/** + * LEXICAL containment — `path.resolve` only, never any filesystem access. + * + * Shares `isContainedIn` with the realpath-based predicate, so there is ONE + * containment decision in this repo; these differ only in how a path is + * RESOLVED before that decision, never in the decision itself (ADR-4650 + * decisions 1 and 6). + * + * Use this — and say why at the call site — only where a symlink must be + * PRESERVED rather than resolved, or where the target legitimately does not + * exist yet. Three such cases exist: a destination validated before the + * `mkdirSync` that creates it, a migration that snapshots and restores a + * symlinked path AS A LINK, and a restore gate that refuses links outright. + * Everywhere else the realpath-based `assertWithinRoot` / `tryWithinRoot` is + * the correct predicate, because a lexical check CANNOT SEE A SYMLINK: a + * caller relying on one for a write-confinement guarantee must pair it with + * its own symlink refusal. + * + * `candidate` is resolved RELATIVE TO `root` (so an absolute candidate is + * taken as-is, matching `path.resolve` semantics). `target === root` is + * contained. + * + * DELIBERATELY ABSENT: no NUL-byte rejection here. The existing lexical + * callers do not reject NUL at this layer (one of them checks NUL itself, + * separately), and adding it here would change their behavior. Callers that + * need it keep their own check. + */ +export function tryWithinRootLexical( + candidate: unknown, + root: unknown, + opts: { pathImpl?: { resolve(...segments: string[]): string; sep: string } } = {}, +): ContainedPath | null { + const p = opts.pathImpl || path; + if (typeof candidate !== 'string' || candidate === '') return null; + if (typeof root !== 'string' || root === '') return null; + const rootResolved = p.resolve(root); + const targetResolved = p.resolve(root, candidate); + return isContainedIn(targetResolved, rootResolved, p) ? (targetResolved as ContainedPath) : null; +} + +export function assertWithinRootLexical( + candidate: unknown, + root: unknown, + label?: string | null, + opts: { pathImpl?: { resolve(...segments: string[]): string; sep: string } } = {}, +): ContainedPath { + const contained = tryWithinRootLexical(candidate, root, opts); + if (contained === null) { + throw new Error(`${label || 'Path'} validation failed: lexical containment check failed`); + } + return contained; +} + // ─── Prompt Injection Detection ──────────────────────────────────────────────────── /**