golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor(#4653): narrow the containment export and migrate all 15 validatePath sites

Browse Source 
Phase 3 of epic #4636, stages 1-2. Implements ADR-4650 decisions 1 and 2: one
containment predicate, and an exported shape that cannot hand a caller a usable
path when the answer is unsafe.

THE EXPORT IS NOW A PAIR, BOTH RETURNING A BRANDED TYPE:

  assertWithinRoot(candidate, root, label?, opts?) -> ContainedPath   (throws)
  tryWithinRoot(candidate, root, opts?)            -> ContainedPath | null

ADR-4650 names only the throwing form. That does not survive contact with the
call sites: findPhaseArtifact probes a direct path, then a .planning/ path, then
each readdir entry, and throwing on the first miss breaks it outright. Six of the
fifteen sites need a non-throwing check. Recorded here rather than papered over.

WHY BRANDED. validatePath returns { safe, resolved, error } and populates
resolved with the ESCAPING path on the traversal branch — so a caller who skips
the boolean gets an attacker-controlled value precisely in the dangerous case.
tryWithinRoot returns exactly null there; assertWithinRoot throws. A plain string
is not assignable to ContainedPath, so a migrated site that validates one path
and then passes a different one is now a type error rather than a silent bug.
That is the defect this epic exists to close, and I introduced it twice in
Phase 2.

THE ENGINE IS UNTOUCHED. validatePath's body is not re-derived — the diff shows
zero edits to the dangling-symlink existence-oracle closure, the ancestor
canonicalization (macOS /var vs /private/var), or the separator-aware boundary
test. Each was acquired as a bug fix and a re-derivation would silently lose one.
requireSafePath now delegates to assertWithinRoot, so there is one implementation
beneath both names; its return type is branded, which is why its 13 call sites
compile unchanged.

A TYPESCRIPT LIMITATION, FIXED AT THE ROOT RATHER THAN WORKED AROUND. TS applies
never-return control-flow narrowing only when the callee is a function
declaration or a const with an EXPLICIT type annotation. Both routers do
`const { error } = io` — destructured, unannotated — so `error(...)` did not
narrow ContainedPath | null and four sites wanted a dead
`throw new Error('unreachable')` after it. Annotating the const
(`const error: typeof io.error = io.error`) makes TS narrow properly and the dead
throws are gone.

That annotation has a large, deliberate consequence: with narrowing working,
`@typescript-eslint/no-unnecessary-type-assertion` fires at 37 sites in
commands.cts where `as string` / `!` existed ONLY to paper over the missing
narrowing. They are removed. The rule is type-aware and fires only where the
assertion changes nothing, and both forms erase at compile time, so the emitted
behavior is unchanged — but the module loses 37 unchecked casts over
string | undefined, which is the same class of "trust me" the containment work
is removing. Widening the diff here buys that.

TWO SITES LOSE DIAGNOSTIC TEXT, deliberately. tryWithinRoot has no error channel,
so init.cts's agent-skills warning and cmdPrSubrepo's rejection now name the
condition rather than echoing validatePath's message. verify.cts had already
stopped echoing it on purpose — the message embeds absolute host paths — so this
makes the three agree instead of two-of-three.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
sim
2026-09-12 12:16:24 -04:00
parent 889c7efba0
commit 4ebcec7750
7 changed files with 135 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
src/init.cts
View File

@@ -44,7 +44,7 @@ import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
import { resolveReportedRuntime } from './host-runtime-detection.cjs';
// eslint-disable-next-line @typescript-eslint/no-require-imports -- commands.cjs is an export= CommonJS module
import commandsMod = require('./commands.cjs');
import { validatePath, loadTrustedGlobalRoots } from './security.cjs';
import { tryWithinRoot, loadTrustedGlobalRoots } from './security.cjs';
import { getGlobalSkillDir, getGlobalSkillDisplayPath, getGlobalSkillsBase, getGlobalConfigDir } from './runtime-homes.cjs';
// eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module
import frontmatterMod = require('./frontmatter.cjs');
@@ -4015,11 +4015,10 @@ function buildAgentSkillsBlock(
);
continue;
}
const pathCheck = validatePath(globalSkillMd, globalSkillsBase, { allowAbsolute: true }) as unknown as Record<string, unknown>;
if (!pathCheck['safe']) {
const globalSkillMdContained = tryWithinRoot(globalSkillMd, globalSkillsBase, { allowAbsolute: true });
if (globalSkillMdContained === null) {
const acceptedViaTrustedRoot = trustedGlobalRoots.some((root) => {
const rootCheck = validatePath(globalSkillMd, root, { allowAbsolute: true }) as unknown as Record<string, unknown>;
return Boolean(rootCheck['safe']);
return tryWithinRoot(globalSkillMd, root, { allowAbsolute: true }) !== null;
});
if (!acceptedViaTrustedRoot) {
warn(
@@ -4035,10 +4034,10 @@ function buildAgentSkillsBlock(
continue;
}
const pathCheck = validatePath(skillPath, projectRoot) as unknown as Record<string, unknown>;
if (!pathCheck['safe']) {
const skillPathContained = tryWithinRoot(skillPath, projectRoot);
if (skillPathContained === null) {
warn(
`[agent-skills] WARNING: Skipping unsafe path "${skillPath}": ${pathCheck['error'] as string}\n`,
`[agent-skills] WARNING: Skipping unsafe path "${skillPath}": resolves outside the project directory\n`,
);
continue;
}