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; }