refactor(#4653): make containment ONE decision, resolved two ways
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 (`<root>-evil` against `<root>`) 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: <resolved> is outside <base>` 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 (`<root>-evil` against `<root>`), 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 ────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user