From cd58aaabf4683bee87d77bd0b22f4e1f97d79dba Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 13:29:54 -0400 Subject: [PATCH] refactor(#4653): drain the containment duplicates and record the two rulings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of epic #4636, stage 3c. ADR-4650 decision 6: a wrapper may decide HOW to degrade, never WHETHER a path is contained. Four implementations are drained on that rule; two are retained, with the reasons recorded rather than assumed. DRAINED — the containment decision now comes from the canonical predicate: scripts/check-glossary-refs.cjs local isWithinRoot deleted outright. src/installer-migrations.cts ensureInsideConfig keeps its throw and its lexical fullPath; only the decision moves. src/planning-inspect.cts isPathContained keeps must-exist as its own condition; only the decision moves. Two of those are wrappers rather than deletions, and each is a wrapper for a reason that would have been a silent behavior change if collapsed naively: - `isPathContained` returns FALSE for a path that does not exist, because fs.realpathSync throws ENOENT and its catch swallows it. The canonical predicate does the opposite: for a missing target it walks up to the nearest existing ancestor and ACCEPTS a not-yet-created path under the root. Its callers at planning-inspect.cts:747 and :839 guard a phaseDir immediately before readdirSync, so under a naive swap a missing phaseDir would stop reporting scope UNREADABLE and start throwing ENOENT out of readdirSync. Existence is therefore kept as an explicit local requirement. - `ensureInsideConfig` returns a LEXICAL fullPath that both callers consume for existsSync and for journal entries. The canonical predicate realpath-resolves, so if configDir is itself a symlink the two differ. The decision is canonical; the returned value stays lexical. Its message is likewise preserved verbatim, which is why this uses tryWithinRoot plus an explicit throw rather than assertWithinRoot. `isWithinRoot` in planning-inspect is left in place and documented: it is a pure comparison over paths the CALLER has already resolved, which readDocument does inline specifically to keep a third degradation shape (exists-but-unreadable vs absent) that neither isPathContained nor the canonical predicate expresses. It is the comparison step of one implementation, not a second implementation. RETAINED, DELIBERATELY — gsd-core/bin/gsd-tools.cjs. My own design document said "collapse" and that was wrong. The file carries an explicit comment forbidding it, and the comment is correct: its three checks reject symlinks OUTRIGHT, which is strictly stricter than the canonical predicate, not a reimplementation of it. The canonical predicate accepts a link whose target lands inside the root — for a restore that is still wrong, because writing through the link overwrites whatever it points at instead of materializing a regular file. Collapsing would have reintroduced that hole. The comment is updated to name the current exported predicate, to record that this was reviewed under this phase and deliberately not collapsed, and to note that isInsideDir treats target === root as NOT contained — the one implementation in the repo that does. THE configHome RULING — retained lexical, and a false safety claim corrected. isPathConfined stays lexical because two of its callers must validate a destSubpath BEFORE the mkdirSync that creates it (install-engine.cts:1608, install-profiles.cts:880), where realpath cannot resolve and a realpath-based predicate would reject every legitimate install. Its docstring's justification, however, did not survive being checked. It cited capability-source.cts:491,577,675 as the upstream symlink rejection that made the lexical form safe. Read directly: :491 is a blank line before assertSafeId's JSDoc and :577 is an entry-count budget check. Neither is a symlink check. The real guards are :585-586 and :671-674. Worse than stale line numbers, the claim that this "keeps every caller of this function's callers symlink-safe" is false: that rejection lives in capability-source's staging path and covers only the capability-loader route to assertDescriptorConfined. Three other callers do not reach it, and only retired-artifact-cleanup.cts:69 carries its own defense (its lstatSync check at :77). The docstring now states what is actually true and cites the lines that actually exist. Co-Authored-By: Claude Opus 5 --- gsd-core/bin/gsd-tools.cjs | 20 +++++++++----- scripts/check-glossary-refs.cjs | 18 +++---------- src/external-descriptor-trust.cts | 16 +++++++---- src/installer-migrations.cts | 12 +++++++-- src/planning-inspect.cts | 44 +++++++++++++++++++++---------- 5 files changed, 67 insertions(+), 43 deletions(-) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index d575db4e3..7378564f0 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -3351,13 +3351,19 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load return out; } - // Why these three checks rather than security.cjs's `validatePath`: that seam - // resolves symlinks with realpathSync and then tests containment, so a link - // whose target sits inside the config dir passes. For a restore that is still - // wrong — writing through any link overwrites whatever it points at instead - // of materializing a regular file at the backed-up path. These checks reject - // links outright, which is strictly stricter than validatePath, not a - // reimplementation of it. Do not "simplify" this to validatePath. + // Why these three checks rather than security.cjs's `assertWithinRoot` / + // `tryWithinRoot`: that seam resolves symlinks with realpathSync and then + // tests containment, so a link whose target sits inside the config dir + // passes. For a restore that is still wrong — writing through any link + // overwrites whatever it points at instead of materializing a regular file + // 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. /** True when `target` resolves strictly inside `root`. */ function isInsideDir(root, target) { diff --git a/scripts/check-glossary-refs.cjs b/scripts/check-glossary-refs.cjs index d0bcd4d95..4a87a671b 100644 --- a/scripts/check-glossary-refs.cjs +++ b/scripts/check-glossary-refs.cjs @@ -37,6 +37,7 @@ const fs = require('node:fs'); const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { tryWithinRoot, PathAcceptance } = require('../gsd-core/bin/lib/security.cjs'); const ROOT = path.resolve(__dirname, '..'); const CONTEXT_PATH = path.join(ROOT, 'CONTEXT.md'); @@ -131,20 +132,6 @@ function isTracked(token) { return TRACKED_EXACT.has(token) || TRACKED_PREFIXES.some((prefix) => token.startsWith(prefix)); } -/** - * True if joining `token` to ROOT stays inside ROOT. `PATH_TOKEN_RE` admits `.` - * inside a segment, so a token like `src/../../../etc/passwd` matches and (via - * the `src/` prefix) reads as "tracked" — `path.join(ROOT, token)` would then - * normalize to an out-of-tree absolute path and `fs.existsSync` would probe it, - * turning a doc lint into a filesystem-existence oracle on the CI host. A - * CONTEXT.md reference is always a plain in-repo path, so a `..` escape is never - * legitimate: confine to ROOT and drop anything that climbs out. - */ -function isWithinRoot(token) { - const resolved = path.resolve(ROOT, token); - return resolved === ROOT || resolved.startsWith(ROOT + path.sep); -} - /** * Every distinct, trackable file-path token referenced in `text`, with any * trailing `:` suffix stripped. @@ -177,7 +164,8 @@ function extractTrackedRefs(text) { if (!/[A-Za-z0-9_]$/.test(token)) return; if (token.includes('NNNN')) return; if (!isTracked(token)) return; - if (!isWithinRoot(token)) return; + // Containment decision is the canonical predicate's, per ADR-4650. + if (tryWithinRoot(token, ROOT, PathAcceptance.AbsoluteInsideRoot) === null) return; tokens.add(token); }; const subTokenRe = /[\w.-]+(?:\/[\w.-]+)*/g; diff --git a/src/external-descriptor-trust.cts b/src/external-descriptor-trust.cts index 4bd346361..ce3ee854f 100644 --- a/src/external-descriptor-trust.cts +++ b/src/external-descriptor-trust.cts @@ -32,11 +32,17 @@ import path from 'node:path'; * `root`) that would redirect the LEXICALLY-confined path to a physically * different, unconfined location on disk. A caller relying on this for a * write-confinement guarantee against a symlink-planting attacker must pair - * it with a symlink check (or refuse to follow symlinks at write time) — see - * capability-source.cts's install adapters (:491,577,675), which is what - * currently keeps every caller of this function's callers symlink-safe: they - * reject symlinks upstream, before a target ever reaches a lexical-only check - * like this one. + * it with a symlink check (or refuse to follow symlinks at write time). Only + * the capability-loader.cts route into assertDescriptorConfined gets this for + * free today: capability-source.cts's staging path rejects symlinks upstream, + * before a target ever reaches a lexical-only check like this one — see + * copyDirRecursive's `entry.isSymbolicLink()` throw (capability-source.cts:585-586) + * and the post-copy budget-walk re-check (capability-source.cts:671-674). This + * does NOT extend to isPathConfined's other callers: install-engine.cts:1608 + * and install-profiles.cts:755,880 do not go through capability-source.cts's + * adapters at all and have no symlink guard of their own here. Of the + * remaining callers, only retired-artifact-cleanup.cts:69 carries its own + * defense, via a local `lstatSync(destDir).isSymbolicLink()` check at line 77. * * `opts.pathImpl` (default: the ambient `path` module) lets a caller inject * `path.win32` or `path.posix`. This is security-relevant: the win32 branch diff --git a/src/installer-migrations.cts b/src/installer-migrations.cts index 4a2eb0a9e..775877acd 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 { tryWithinRoot, PathAcceptance } 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 @@ -666,9 +667,16 @@ interface EnsureInsideConfigResult { function ensureInsideConfig(configDir: string, relPath: string): EnsureInsideConfigResult { const normalized = normalizeRelPath(relPath); + // fullPath stays the LEXICAL path.resolve result (not the canonical + // predicate's realpath-resolved value): both callers (readJson's + // ensureInsideConfig call and the migration-apply loop) use fullPath for + // fs.existsSync checks and journal entries, and those must not shift if + // configDir happens to be a symlink. Per ADR-4650 decision 6, the + // containment DECISION (whether fullPath is inside configDir) is owned by + // the canonical predicate — this wrapper only decides how to degrade + // (throw with this file's existing message), never whether contained. const fullPath = path.resolve(configDir, normalized); - const root = path.resolve(configDir); - if (fullPath !== root && !fullPath.startsWith(root + path.sep)) { + if (tryWithinRoot(fullPath, configDir, PathAcceptance.AbsoluteInsideRoot) === null) { throw new Error(`migration path escapes configDir: ${relPath}`); } return { normalized, fullPath }; diff --git a/src/planning-inspect.cts b/src/planning-inspect.cts index fe60b61e8..a6d175e50 100644 --- a/src/planning-inspect.cts +++ b/src/planning-inspect.cts @@ -92,6 +92,9 @@ const { parseMarkdownTable, matchTableSchema } = markdownTable; // eslint-disable-next-line @typescript-eslint/no-require-imports 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; /** * The wire schema version. A consumer MUST reject any value other than this @@ -212,32 +215,45 @@ function toPosix(value: string): string { * directory like `.planning-evil/` that merely shares a string prefix. Pure * string comparison, no I/O — callers own their own `fs.realpathSync` call * (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 + * 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) that resolves its own - * `fs.realpathSync`, then delegates the actual boundary comparison to - * `isWithinRoot`. Used where the caller does not need to distinguish "target - * vanished / broken symlink" from "target resolved but escapes root" — both - * degrade the same way at every 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 of this wrapper. + * Containment check for a path (file OR directory), used where the caller + * does not need to distinguish "target vanished / broken symlink" from + * "target resolved but escapes root" — both degrade the same way at every + * 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 + * of this wrapper. + * + * The containment DECISION comes from the canonical `tryWithinRoot` + * predicate (ADR-4650 decision 6: a wrapper may decide HOW to degrade, + * never WHETHER a path is contained). Must-exist stays this module's OWN + * degradation condition, applied after: `tryWithinRoot` deliberately accepts + * a not-yet-created path under the root (ancestor-walk realpath), but every + * caller of `isPathContained` guards an `fs` read that is about to happen + * against an already-existing directory, so a vanished/unresolvable path + * must still degrade the same as an escaped one. */ function isPathContained(target: string, root: string): boolean { - let realTarget: string; - let realRoot: string; + if (tryWithinRoot(target, root, PathAcceptance.AbsoluteInsideRoot) === null) return false; try { - realTarget = fs.realpathSync(target); - realRoot = fs.realpathSync(root); + fs.realpathSync(target); } catch { return false; } - return isWithinRoot(realTarget, realRoot); + return true; } function readDocument(filePath: string, root: string): { text: string | null; exists: boolean; readable: boolean } {