fix(#4652): confine every boundary that joins argv to a managed root
Phase 2 of epic #4636, absorbing #4327 and #4354. Implements ADR-4650 decision 3: containment is a boundary concern — the predicate runs where external input enters, not at whichever interior call site remembered. Four boundaries now validate against their managed root and reject with a USAGE-shaped error before touching the filesystem: todo complete <name> -> todosDir(cwd) check predicate --phase-dir <dir> -> projectDir check decision-coverage-plan <dir> -> projectDir (via resolvePath) check gap-analysis.plan-post <dir> -> projectDir #4327 understated its own severity. It reports that a traversal name "resolves outside the todos root", which reads as an information leak. Measured, it was destructive: the command exited 0, MOVED the outside file into completed/, and unlinked the original. cmdTodoComplete ends in fs.unlinkSync(sourcePath), so an unconfined name consumed across the boundary rather than merely reading across it. Validation now precedes every fs call — existsSync, readFileSync, ensureDir, writeSync, unlinkSync — and both halves of the move are confined, so neither source nor destination can land outside the root. --dry-run is rejected on the same terms; a preview must not leak a resolved outside path either. #4354 reproduces exactly: a BLOCKING gate returned block:false sourced entirely from a SECURITY.md in a caller-chosen directory outside the project. THE HARDER HALF, found by the isolated adversarial review of the first attempt: validating a path and then using a DIFFERENT one closes nothing. The first fix validated `--phase-dir` joined against `--cwd`, then passed the RAW unjoined value into the predicate context. gate-predicate-evaluator uses it as-is and findPhaseArtifact resolves a relative path against the REAL process cwd — so validation and the read used two different roots whenever process.cwd() differed from --cwd. Reproduced: running from a directory holding a plan with `secret_field: LEAKED_VALUE`, a predicate declared against an empty --cwd project exited 0 and returned "actual":"LEAKED_VALUE". The rule now applied at all three router sites: **use the validated resolved path, never the raw input.** Independently re-verified after the fix — the lookup resolves in the --cwd project and no value leaks. gate-predicate-evaluator.cts is untouched and still imports no fs. Confining in the router is what keeps that pure-leaf contract intact AND covers ${PHASE_DIR} interpolation into command-exit-zero, which an evaluator-local fix would have missed entirely. Also fixed, same review: `todo complete .` and `..` passed containment (they resolve to the pending dir, which IS inside the root) and then threw an uncaught EISDIR with an absolute-path stack trace. Now a clean USAGE rejection naming the real reason — "todo name is not a file" — rather than borrowing the escape message, which would have stated something false. Ripples discharged BEFORE the verification checkpoint rather than after, per the Phase 1 retrospective: docs/reference/gate-predicates.md and docs/CLI-TOOLS.md document the new constraints, CONTEXT.md records why containment lives at the router rather than the evaluator, the changeset is written, and the install-tree goldens were regenerated to confirm unchanged (no new shipped file) rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -90,7 +90,12 @@ function readIfExists(filePath: string): string {
|
||||
}
|
||||
|
||||
function resolvePath(inputPath: string, projectDir: string): string {
|
||||
return path.isAbsolute(inputPath) ? inputPath : path.join(projectDir, inputPath);
|
||||
const candidate = path.isAbsolute(inputPath) ? inputPath : path.join(projectDir, inputPath);
|
||||
const check = validatePath(candidate, projectDir, { allowAbsolute: true });
|
||||
if (!check.safe) {
|
||||
error(`path escapes its allowed directory: ${inputPath}`, ERROR_REASON.USAGE);
|
||||
}
|
||||
return check.resolved;
|
||||
}
|
||||
|
||||
interface WorkflowConfig {
|
||||
@@ -1194,8 +1199,17 @@ function cmdGapAnalysisPlanPost(projectDir: string, args: string[], raw: boolean
|
||||
error('gap-analysis.plan-post requires a phase-dir argument: check gap-analysis.plan-post <phase-dir> [phase-req-ids]', ERROR_REASON.SDK_MISSING_ARG);
|
||||
return;
|
||||
}
|
||||
const phaseDirCheck = validatePath(
|
||||
path.isAbsolute(phaseDir) ? phaseDir : path.join(projectDir, phaseDir),
|
||||
projectDir,
|
||||
{ allowAbsolute: true },
|
||||
);
|
||||
if (!phaseDirCheck.safe) {
|
||||
error(`phase-dir escapes its allowed directory: ${phaseDir}`, ERROR_REASON.USAGE);
|
||||
return;
|
||||
}
|
||||
const phaseReqIds = args[3] ?? undefined;
|
||||
const result = runGapAnalysis(projectDir, phaseDir, { phaseReqIds });
|
||||
const result = runGapAnalysis(projectDir, phaseDirCheck.resolved, { phaseReqIds });
|
||||
// Uniform gate contract: block = false (gap-analysis is always advisory, never blocks).
|
||||
// `message` carries the human-readable gap analysis report so the dispatch's
|
||||
// advisory branch can surface it. --raw emits JSON (rawValue=undefined), not
|
||||
@@ -1362,10 +1376,24 @@ function cmdCheckPredicate(projectDir: string, args: string[], raw: boolean): vo
|
||||
error('predicate --predicate value must be valid JSON', ERROR_REASON.USAGE);
|
||||
return;
|
||||
}
|
||||
const rawPhaseDir = flags['phase-dir'];
|
||||
let resolvedPhaseDir: string | undefined = rawPhaseDir;
|
||||
if (typeof rawPhaseDir === 'string' && rawPhaseDir !== '') {
|
||||
const phaseDirCheck = validatePath(
|
||||
path.isAbsolute(rawPhaseDir) ? rawPhaseDir : path.join(projectDir, rawPhaseDir),
|
||||
projectDir,
|
||||
{ allowAbsolute: true },
|
||||
);
|
||||
if (!phaseDirCheck.safe) {
|
||||
error(`phase-dir escapes its allowed directory: ${rawPhaseDir}`, ERROR_REASON.USAGE);
|
||||
return;
|
||||
}
|
||||
resolvedPhaseDir = phaseDirCheck.resolved;
|
||||
}
|
||||
const ctx = {
|
||||
cwd: projectDir,
|
||||
phaseNumber: flags['phase-number'],
|
||||
phaseDir: flags['phase-dir'],
|
||||
phaseDir: resolvedPhaseDir,
|
||||
phaseReqIds: flags['phase-req-ids'],
|
||||
};
|
||||
let result;
|
||||
|
||||
@@ -11,7 +11,7 @@ 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 } from './security.cjs';
|
||||
import { requireSafePath, sanitizeForDisplay, validatePath } from './security.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import ioMod = require('./io.cjs');
|
||||
const { output, error, ERROR_REASON } = ioMod;
|
||||
@@ -3491,11 +3491,29 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod
|
||||
const pendingDir = path.join(todosRoot, 'pending');
|
||||
const completedDir = path.join(todosRoot, 'completed');
|
||||
const sourcePath = path.join(pendingDir, filename as string);
|
||||
const targetPath = path.join(completedDir, filename as string);
|
||||
|
||||
const sourceCheck = validatePath(sourcePath, todosRoot, { allowAbsolute: true });
|
||||
if (!sourceCheck.safe) {
|
||||
error(`todo file escapes its allowed directory: ${filename as string}`, 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);
|
||||
}
|
||||
|
||||
if (!fs.existsSync(sourcePath)) {
|
||||
error(`Todo not found: ${filename as string}`);
|
||||
}
|
||||
|
||||
// #4652: `.` and `..` resolve to the pending dir itself (which IS inside
|
||||
// todosRoot, so containment passes) but are not a todo file — reject them
|
||||
// the same way as any other invalid name instead of letting
|
||||
// fs.readFileSync throw an uncaught EISDIR with an absolute-path stack trace.
|
||||
if (!fs.statSync(sourcePath).isFile()) {
|
||||
error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE);
|
||||
}
|
||||
|
||||
const content = fs.readFileSync(sourcePath, 'utf-8');
|
||||
const today = realClock.localToday();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user