From 4ebcec7750e70fcd1eb81e692d50787b1acc4128 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 12:16:24 -0400 Subject: [PATCH] refactor(#4653): narrow the containment export and migrate all 15 validatePath sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/check-command-router.cts | 32 +++++++----- src/commands.cts | 98 ++++++++++++++++++------------------ src/init.cts | 15 +++--- src/mcp-catalog.cts | 6 +-- src/security.cts | 48 ++++++++++++++++-- src/state.cts | 9 ++-- src/verify.cts | 18 +++---- 7 files changed, 135 insertions(+), 91 deletions(-) diff --git a/src/check-command-router.cts b/src/check-command-router.cts index 576116a57..80e0e7974 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -11,7 +11,13 @@ import path from 'node:path'; import { execFileSync } from 'node:child_process'; // eslint-disable-next-line @typescript-eslint/no-require-imports import io = require('./io.cjs'); -const { output, error, ERROR_REASON } = io; +const { output, ERROR_REASON } = io; +// Explicitly annotated so TypeScript applies never-return control-flow narrowing. +// A destructured `const { error } = io` is a const WITHOUT a type annotation, and TS +// only narrows after a never-returning call when the callee is a function declaration +// or an annotated const. Without the annotation every `error(...)` guard below would +// need a dead `throw` after it to convince the checker that the value is non-null. +const error: typeof io.error = io.error; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspaceMod = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspaceMod; @@ -24,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 { validatePath } from './security.cjs'; +import { tryWithinRoot } 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 @@ -91,11 +97,11 @@ function readIfExists(filePath: string): string { function resolvePath(inputPath: string, projectDir: string): string { const candidate = path.isAbsolute(inputPath) ? inputPath : path.join(projectDir, inputPath); - const check = validatePath(candidate, projectDir, { allowAbsolute: true }); - if (!check.safe) { + const contained = tryWithinRoot(candidate, projectDir, { allowAbsolute: true }); + if (contained === null) { error(`path escapes its allowed directory: ${inputPath}`, ERROR_REASON.USAGE); } - return check.resolved; + return contained; } interface WorkflowConfig { @@ -1271,20 +1277,20 @@ function buildPredicateDeps() { ) { return null; } - const directPath = validatePath(artifactSuffix, phaseDir); - if (directPath.safe && fs.existsSync(directPath.resolved) && fs.statSync(directPath.resolved).isFile()) { - return directPath.resolved; + const directContained = tryWithinRoot(artifactSuffix, phaseDir); + if (directContained !== null && fs.existsSync(directContained) && fs.statSync(directContained).isFile()) { + return directContained; } - const planningPath = validatePath(path.join('.planning', artifactSuffix), phaseDir); - if (planningPath.safe && fs.existsSync(planningPath.resolved) && fs.statSync(planningPath.resolved).isFile()) { - return planningPath.resolved; + const planningContained = tryWithinRoot(path.join('.planning', artifactSuffix), phaseDir); + if (planningContained !== null && fs.existsSync(planningContained) && fs.statSync(planningContained).isFile()) { + return planningContained; } try { const files = fs.readdirSync(phaseDir); for (const f of files) { if (f.endsWith('-' + artifactSuffix) || f === artifactSuffix) { - const candidate = validatePath(f, phaseDir); - if (candidate.safe && fs.statSync(candidate.resolved).isFile()) return candidate.resolved; + const candidateContained = tryWithinRoot(f, phaseDir); + if (candidateContained !== null && fs.statSync(candidateContained).isFile()) return candidateContained; } } } catch { /* ignore */ } diff --git a/src/commands.cts b/src/commands.cts index 663e02b39..95ba378dc 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -11,10 +11,14 @@ 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, validatePath } from './security.cjs'; +import { requireSafePath, sanitizeForDisplay, tryWithinRoot } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); -const { output, error, ERROR_REASON } = ioMod; +const { output, ERROR_REASON } = ioMod; +// Explicitly annotated so TypeScript applies never-return control-flow narrowing. +// See the identical note in check-command-router.cts: a destructured const carries no +// type annotation, so TS will not narrow after `error(...)` without this. +const error: typeof ioMod.error = ioMod.error; // eslint-disable-next-line @typescript-eslint/no-require-imports import configLoaderMod = require('./config-loader.cjs'); const { loadConfig, isGitIgnored } = configLoaderMod; @@ -401,11 +405,11 @@ function cmdVerifyPathExists(cwd: string, targetPath: string | undefined, raw: b } // Reject null bytes and validate path does not contain traversal attempts - if ((targetPath as string).includes('\0')) { + if (targetPath.includes('\0')) { error('path contains null bytes'); } - const fullPath = path.isAbsolute(targetPath as string) ? targetPath as string : path.join(cwd, targetPath as string); + const fullPath = path.isAbsolute(targetPath) ? targetPath : path.join(cwd, targetPath); try { const stats = fs.statSync(fullPath); @@ -541,20 +545,20 @@ function cmdResolveModel(cwd: string, agentType: string | undefined, raw: boolea const config = loadConfig(cwd); const profile = (config['model_profile'] as string) || 'balanced'; - const model = resolveModelInternal(cwd, agentType!); - const effort = resolveEffortInternal(cwd, agentType!); + const model = resolveModelInternal(cwd, agentType); + const effort = resolveEffortInternal(cwd, agentType); // Own-property guard: agentType is an unvalidated CLI positional, so a // prototype-chain value ("toString", "constructor") would otherwise return // an inherited truthy member from this plain object and misreport a // genuinely unknown agent as known (unknown_agent dropped from the result). const agentModelsMap = MODEL_PROFILES as Record; - const agentModels = Object.hasOwn(agentModelsMap, agentType!) ? agentModelsMap[agentType!] : undefined; + const agentModels = Object.hasOwn(agentModelsMap, agentType) ? agentModelsMap[agentType] : undefined; // #2229: `tier` is additive — existing keys and their values are untouched, so // every `--pick model` / `--pick profile` / `--raw` consumer is unaffected. It // exists because the model id is deliberately blank under resolve_model_ids:"omit", // which leaves a tier-sensitive guard with nothing to read. - const tier = resolveTierInternal(cwd, agentType!); + const tier = resolveTierInternal(cwd, agentType); const result = agentModels ? { model, profile, effort, tier } : { model, profile, effort, tier, unknown_agent: true }; @@ -567,7 +571,7 @@ function cmdResolveGranularity(cwd: string, phaseType: string | undefined, raw: } assertValidGranularityOverride(override, error); const granularity = resolveGranularityInternal(cwd, phaseType, override); - const result = (VALID_PHASE_TYPES).has(phaseType!) + const result = (VALID_PHASE_TYPES).has(phaseType) ? { granularity, phase_type: phaseType } : { granularity, phase_type: phaseType, unknown_phase_type: true }; output(result, raw, granularity); @@ -599,8 +603,8 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo // explicit attempt routes through the tier ladder. resolveModelForTier itself // still falls back to resolveModelInternal when dynamic_routing is off. let model = (opts.attempt !== undefined && opts.attempt !== null) - ? resolveModelForTier(cwd, agentType!, opts.attempt) - : resolveModelInternal(cwd, agentType!); + ? resolveModelForTier(cwd, agentType, opts.attempt) + : resolveModelInternal(cwd, agentType); // #2296: when the caller reports WHY the previous attempt failed, consult the // provider-escalation ladder. Only a quota/rate-limit class warrants it — a @@ -610,7 +614,7 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo let escalation: Record | undefined; if (opts.failureClass !== undefined) { const applicable = opts.failureClass === AGENT_FAILURE_CLASSES.QUOTA_EXCEEDED; - const resolved = resolveProviderEscalation(cwd, agentType!, opts.attempt, applicable); + const resolved = resolveProviderEscalation(cwd, agentType, opts.attempt, applicable); if (resolved.escalated) model = resolved.to; escalation = { class: opts.failureClass, ...resolved }; } @@ -622,10 +626,10 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo if (typeof opts.fastModeOverride === 'boolean') fastModeOpts['override'] = opts.fastModeOverride; const effort = (opts.attempt !== undefined && opts.attempt !== null) - ? resolveEffortForTier(cwd, agentType!, opts.attempt) - : resolveEffortInternal(cwd, agentType!, effortOpts); + ? resolveEffortForTier(cwd, agentType, opts.attempt) + : resolveEffortInternal(cwd, agentType, effortOpts); - const fastMode = resolveFastModeInternal(cwd, agentType!, fastModeOpts); + const fastMode = resolveFastModeInternal(cwd, agentType, fastModeOpts); const runtime = (config['runtime'] as string) || 'claude'; // #3007: pass the resolved model so the per-model advertised-effort ceiling @@ -681,7 +685,7 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo // an inherited truthy member from this plain object and misreport a // genuinely unknown agent as known (unknown_agent dropped from the result). const agentModelsMap = MODEL_PROFILES as Record; - const agentModels = Object.hasOwn(agentModelsMap, agentType!) ? agentModelsMap[agentType!] : undefined; + const agentModels = Object.hasOwn(agentModelsMap, agentType) ? agentModelsMap[agentType] : undefined; const result: Record = { model, profile, @@ -2738,7 +2742,7 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str } // Group files by sub-repo prefix - const { grouped, unmatched } = groupFilesBySubrepo(files as string[], subRepos as string[]); + const { grouped, unmatched } = groupFilesBySubrepo(files, subRepos); if (unmatched.length > 0) { process.stderr.write(`Warning: ${unmatched.length} file(s) did not match any sub-repo prefix: ${unmatched.join(', ')}\n`); @@ -2793,8 +2797,8 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str const isMergeInProgressSub = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd: repoCwd }).exitCode === 0; const canScopeSub = stagedRelPaths.length > 0 && !isMergeInProgressSub; const commitArgs = canScopeSub - ? ['commit', '-m', message as string, '--', ...stagedRelPaths] - : ['commit', '-m', message as string]; + ? ['commit', '-m', message, '--', ...stagedRelPaths] + : ['commit', '-m', message]; // #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs // this override for pathspec-scoped AND whole-index commits alike. const commitEnvSub: Record = { @@ -2866,22 +2870,18 @@ function cmdPrSubrepo( if (!commitMessage || commitMessage.startsWith('--')) { error('commit message required'); } - if ((branch as string).startsWith('-')) { + if (branch.startsWith('-')) { error(`Branch name must not start with '-': ${branch}`); } // 0. Security: validate repo path is contained within the workspace root. - // Uses security.cjs validatePath (symlink-safe realpathSync + startsWith guard) + // Uses security.cjs tryWithinRoot (symlink-safe realpathSync + startsWith guard) // to reject ../escape, absolute paths, and symlink traversal. - // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method - const { validatePath } = require('./security.cjs') as { - validatePath(filePath: string, baseDir: string): { safe: boolean; resolved: string; error?: string }; - }; - const pathCheck = validatePath(repo as string, cwd); - if (!pathCheck.safe) { - error(`Sub-repo path is unsafe: ${pathCheck.error}`); + const repoContained = tryWithinRoot(repo, cwd); + if (repoContained === null) { + error(`Sub-repo path is unsafe: resolves outside the workspace root`); } - const repoCwd = pathCheck.resolved; + const repoCwd = repoContained; if (!fs.existsSync(repoCwd)) { error(`Sub-repo not found: ${repoCwd}`); } @@ -2938,7 +2938,7 @@ function cmdPrSubrepo( } // 2. Guard: refuse if branch already exists — checkout -b is non-idempotent - const branchCheck = execGit(['rev-parse', '--verify', branch as string], { cwd: repoCwd }); + const branchCheck = execGit(['rev-parse', '--verify', branch], { cwd: repoCwd }); if (branchCheck.exitCode === 0) { error(`Branch already exists in ${repo}: ${branch}. Delete it first or choose a unique name.`); } @@ -2949,7 +2949,7 @@ function cmdPrSubrepo( const prevBranchName = prevBranchResult.exitCode === 0 ? prevBranchResult.stdout.trim() : null; // 3. Create branch - const checkoutResult = execGit(['checkout', '-b', branch as string], { cwd: repoCwd }); + const checkoutResult = execGit(['checkout', '-b', branch], { cwd: repoCwd }); if (checkoutResult.exitCode !== 0) { error(`Failed to create branch ${branch} in ${repo}: ${checkoutResult.stderr}`); } @@ -2959,7 +2959,7 @@ function cmdPrSubrepo( if (prevBranchName) { execGit(['checkout', prevBranchName], { cwd: repoCwd }); } - execGit(['branch', '-D', branch as string], { cwd: repoCwd }); + execGit(['branch', '-D', branch], { cwd: repoCwd }); }; // 4. Stage explicit files (never git add -A per universal-anti-patterns.md:44) @@ -2978,8 +2978,8 @@ function cmdPrSubrepo( const isMergeInProgressPr = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd: repoCwd }).exitCode === 0; const canScopePr = changedFiles.length > 0 && !isMergeInProgressPr; const commitArgs = canScopePr - ? ['commit', '-m', commitMessage as string, '--', ...changedFiles] - : ['commit', '-m', commitMessage as string]; + ? ['commit', '-m', commitMessage, '--', ...changedFiles] + : ['commit', '-m', commitMessage]; // #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs // this override for pathspec-scoped AND whole-index commits alike. const commitEnvPr: Record = { @@ -3017,7 +3017,7 @@ function cmdPrSubrepo( // Do NOT rollback on push failure — the commit already exists on the local branch. // Deleting the branch here would destroy the only ref holding the user's work. // Leave the branch in place so the user can retry the push. - const pushResult = execGit(['push', '--set-upstream', 'origin', branch as string], { cwd: repoCwd, timeout: 60_000 }); + const pushResult = execGit(['push', '--set-upstream', 'origin', branch], { cwd: repoCwd, timeout: 60_000 }); if (pushResult.exitCode !== 0) { error(`Failed to push ${branch} in ${repo}: ${pushResult.stderr}\nBranch ${branch} was created locally — retry with: git -C ${repo} push --set-upstream origin ${branch}`); } @@ -3040,7 +3040,7 @@ function cmdSummaryExtract(cwd: string, summaryPath: string | undefined, fields: error('summary-path required for summary-extract'); } - const fullPath = path.join(cwd, summaryPath as string); + const fullPath = path.join(cwd, summaryPath); if (!fs.existsSync(fullPath)) { output({ error: 'File not found', path: summaryPath }, raw, undefined); @@ -3501,7 +3501,7 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod // `\` explicitly (not just path.basename) matters on POSIX, where a // literal backslash is just an ordinary filename character to // path.basename but not to path.win32.basename or to the user's intent. - const rawFilename = filename as string; + const rawFilename = filename; if ( rawFilename === '.' || rawFilename === '..' || @@ -3514,23 +3514,23 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod error(`todo name must be a plain filename inside the pending directory, not a path: ${rawFilename}`, ERROR_REASON.USAGE); } - const sourcePath = path.join(pendingDir, filename as string); - const targetPath = path.join(completedDir, filename as string); + const sourcePath = path.join(pendingDir, filename); + const targetPath = path.join(completedDir, filename); - const sourceCheck = validatePath(sourcePath, todosRoot, { allowAbsolute: true }); - if (!sourceCheck.safe) { - error(`todo file escapes its allowed directory: ${filename as string}`, ERROR_REASON.USAGE); + const sourceContained = tryWithinRoot(sourcePath, todosRoot, { allowAbsolute: true }); + if (sourceContained === null) { + error(`todo file escapes its allowed directory: ${filename}`, ERROR_REASON.USAGE); } - const targetCheck = validatePath(targetPath, todosRoot, { allowAbsolute: true }); - if (!targetCheck.safe) { - error(`todo file escapes its allowed directory: ${filename as string}`, ERROR_REASON.USAGE); + const targetContained = tryWithinRoot(targetPath, todosRoot, { allowAbsolute: true }); + if (targetContained === null) { + error(`todo file escapes its allowed directory: ${filename}`, ERROR_REASON.USAGE); } - const resolvedSource = sourceCheck.resolved; - const resolvedTarget = targetCheck.resolved; + const resolvedSource = sourceContained; + const resolvedTarget = targetContained; if (!fs.existsSync(resolvedSource)) { - error(`Todo not found: ${filename as string}`); + error(`Todo not found: ${filename}`); } // #4652: a name that IS a bare basename can still resolve to something that @@ -3541,7 +3541,7 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod // with an absolute-path stack trace where every sibling case gives a clean // USAGE rejection. if (!fs.statSync(resolvedSource).isFile()) { - error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE); + error(`todo name is not a file: ${filename}`, ERROR_REASON.USAGE); } const content = fs.readFileSync(resolvedSource, 'utf-8'); diff --git a/src/init.cts b/src/init.cts index 93bfc37fe..9c9206948 100644 --- a/src/init.cts +++ b/src/init.cts @@ -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; - 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; - 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; - 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; } diff --git a/src/mcp-catalog.cts b/src/mcp-catalog.cts index a4aec05ae..c74d11375 100644 --- a/src/mcp-catalog.cts +++ b/src/mcp-catalog.cts @@ -93,7 +93,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { validatePath } from './security.cjs'; +import { tryWithinRoot } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- workflow-fragments.cjs is a CommonJS module compiled from a sibling .cts source; `import x = require()` reads its module.exports namespace directly. import workflowFragments = require('./workflow-fragments.cjs'); const { composeWorkflow } = workflowFragments; @@ -506,8 +506,8 @@ function classifyUnindexedUri(uri: string): string { /** Second, independent gate (design "Hostile inputs" gate 2): re-validate an INDEXED entry's relPath against the catalog root, catching a symlink planted after the index was built. */ function readIndexedResource(catalog: Catalog, entry: CatalogResourceEntry): ReadResourceResult { - const check = validatePath(entry.relPath, catalog.root); - if (!check.safe) { + const contained = tryWithinRoot(entry.relPath, catalog.root); + if (contained === null) { fail(REASON.TRAVERSAL_REFUSED, `indexed resource escapes catalog root: ${entry.relPath}`); } let raw: string; diff --git a/src/security.cts b/src/security.cts index 2821bdb97..0c696a2e2 100644 --- a/src/security.cts +++ b/src/security.cts @@ -182,16 +182,58 @@ export function loadTrustedGlobalRoots(config: unknown): string[] { return result; } +/** + * A path proven to resolve inside a declared root. + * + * A plain `string` is NOT assignable to `ContainedPath` — that asymmetry is + * the entire point. The shape being replaced (`validatePath`'s + * `{ resolved: string }`) returns a usable-looking path even when the answer + * is unsafe (the traversal branch still populates `resolved` with the + * escaping path), so a plain string in hand proves nothing. A + * `ContainedPath` can only be produced by `assertWithinRoot` / + * `tryWithinRoot` on their success paths, so possessing one is proof the + * containment check already passed. + */ +export type ContainedPath = string & { readonly __containedIn: unique symbol }; + /** * Validate a file path and throw on traversal attempt. * Convenience wrapper around validatePath for use in CLI commands. */ -export function requireSafePath(filePath: unknown, baseDir: unknown, label: string | null | undefined, opts: { allowAbsolute?: boolean } = {}): string { - const result = validatePath(filePath, baseDir, opts); +export function assertWithinRoot(candidate: unknown, root: unknown, label?: string | null, opts: { allowAbsolute?: boolean } = {}): ContainedPath { + const result = validatePath(candidate, root, opts); if (!result.safe) { throw new Error(`${label || 'Path'} validation failed: ${result.error}`); } - return result.resolved; + return result.resolved as ContainedPath; +} + +/** + * Validate a file path and return null on traversal attempt (no throw). + * + * Returns exactly `null` when unsafe — never `''`, never `result.resolved`. + * `validatePath` populates `resolved` with the ESCAPING path on the + * traversal branch, so returning it here would reproduce the defect this + * narrowing exists to remove. + */ +export function tryWithinRoot(candidate: unknown, root: unknown, opts: { allowAbsolute?: boolean } = {}): ContainedPath | null { + const result = validatePath(candidate, root, opts); + if (!result.safe) { + return null; + } + return result.resolved as ContainedPath; +} + +/** + * Validate a file path and throw on traversal attempt. + * Convenience wrapper around validatePath for use in CLI commands. + * + * Delegates to assertWithinRoot so there is one implementation beneath both + * names; its declared return type is ContainedPath (a branded string, still + * assignable to string) so existing callers keep compiling untouched. + */ +export function requireSafePath(filePath: unknown, baseDir: unknown, label: string | null | undefined, opts: { allowAbsolute?: boolean } = {}): ContainedPath { + return assertWithinRoot(filePath, baseDir, label, opts); } // ─── Prompt Injection Detection ──────────────────────────────────────────────────── diff --git a/src/state.cts b/src/state.cts index c294aee79..9bad609d5 100644 --- a/src/state.cts +++ b/src/state.cts @@ -620,14 +620,11 @@ function readTextArgOrFile(cwd: string, value: string | undefined, filePath: str // Path traversal guard: ensure file resolves within project directory // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method - const { validatePath } = require('./security.cjs') as { validatePath(filePath: unknown, baseDir: unknown, opts?: { allowAbsolute?: boolean }): { safe: boolean; resolved: string; error?: string } }; - const pathCheck = validatePath(filePath, cwd, { allowAbsolute: true }); - if (!pathCheck.safe) { - throw new Error(`${label} path rejected: ${pathCheck.error as string}`); - } + const { assertWithinRoot } = require('./security.cjs') as { assertWithinRoot(filePath: unknown, baseDir: unknown, label?: string | null, opts?: { allowAbsolute?: boolean }): string }; + const contained = assertWithinRoot(filePath, cwd, `${label} path`, { allowAbsolute: true }); try { - return fs.readFileSync(pathCheck.resolved, 'utf-8').trimEnd(); + return fs.readFileSync(contained, 'utf-8').trimEnd(); } catch { throw new Error(`${label} file not found: ${filePath}`); } diff --git a/src/verify.cts b/src/verify.cts index d8e1ef3e5..0a3c4e8db 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -34,7 +34,7 @@ import worktreeSafetyMod = require('./worktree-safety.cjs'); // codebase-drift --name-status parse loop). const { decodeGitQuotedPath } = worktreeSafetyMod; import { execGit, platformReadSync as safeReadFile } from './shell-command-projection.cjs'; -import { validatePath } from './security.cjs'; +import { tryWithinRoot } from './security.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { detectSchemaFiles, checkSchemaDrift } from './schema-detect.cjs'; import { extractTaggedBlocks } from './markdown-sectionizer.cjs'; @@ -1552,8 +1552,8 @@ function cmdVerifyKeyLinks(cwd: string, planFilePath: string, raw: boolean): voi // still reaches validatePath and safeReadFile below, and DOES read the // cwd directory (yielding "Source read failed: EISDIR") — this branch // only short-circuits the true empty-string case. - const fromCheck = validatePath(fromPath, cwd); - if (!fromCheck.safe) { + const fromContained = tryWithinRoot(fromPath, cwd); + if (fromContained === null) { // Do not echo result.error — it embeds absolute host paths. check['path_rejected'] = 'from'; check['detail'] = 'Source path rejected — resolves outside the project directory'; @@ -1561,7 +1561,7 @@ function cmdVerifyKeyLinks(cwd: string, planFilePath: string, raw: boolean): voi continue; } try { - sourceContent = safeReadFile(fromCheck.resolved); + sourceContent = safeReadFile(fromContained); } catch (err) { // Report the errno only — never the message or path (untrusted `from:` // can trigger EISDIR/EACCES, which platformReadSync re-throws for any @@ -1624,15 +1624,15 @@ function cmdVerifyKeyLinks(cwd: string, planFilePath: string, raw: boolean): voi // An empty/missing `to:` is a malformed plan, not a // path-confinement violation — only a non-empty path that // actually resolves outside the project is path_rejected. - const toCheck = validatePath(toPath, cwd); - if (!toCheck.safe) { + const toContained = tryWithinRoot(toPath, cwd); + if (toContained === null) { // Do not read a rejected `to:` — treat as no target content // and do not echo result.error, which embeds absolute host // paths. check['path_rejected'] = 'to'; check['detail'] = `Pattern "${link['pattern'] as string}" not found in source; target path rejected — resolves outside the project directory`; } else { - targetContent = safeReadFile(toCheck.resolved); + targetContent = safeReadFile(toContained); } } if (targetContent && pat.test(targetContent)) { @@ -2022,8 +2022,8 @@ function resolvePhaseDirByToken(phasesDir: string, phaseArg: string): string | n const dirNames = dirEntries.filter((e) => e.isDirectory()).map((e) => e.name); const matched = matchPhaseDirs(dirNames, normalizedPhase).matches[0]; if (matched) return path.join(phasesDir, matched); - const check = validatePath(phaseArg, phasesDir); - if (check.safe && fs.existsSync(check.resolved)) return check.resolved; + const contained = tryWithinRoot(phaseArg, phasesDir); + if (contained !== null && fs.existsSync(contained)) return contained; return null; }