From 7e7a239a652d6b29f4c5773c443447b24d189910 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 12:31:01 -0400 Subject: [PATCH] refactor(#4653): replace the allowAbsolute flag with a named acceptance policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of epic #4636, stage 3b. Satisfies #4653's criterion that `opts.allowAbsolute` become "a named acceptance policy on the predicate, not a per-call-site boolean". The flag was actively misleading at the call site. `{ allowAbsolute: true }` reads as "containment is relaxed here". It never was: an absolute path that resolves outside the root is rejected exactly as a traversal is. The flag only ever controlled whether an absolute candidate was CONSIDERED. On a security predicate that is the wrong thing for a reviewer to have to infer, and 31 call sites were asking them to infer it. PathAcceptance.RelativeOnly relative candidates only PathAcceptance.AbsoluteInsideRoot absolute accepted, containment unchanged The three exported wrappers take the policy and translate it inward. validatePath keeps its internal `{ allowAbsolute }` opts and its body untouched — the engine is not re-derived here either, only the exported surface is renamed. MEASURED, NOT ESTIMATED. 31 call sites across 10 files, counted by walking the AST with the repo's own @typescript-eslint/parser rather than grepping: a text match would have folded in the options-type declaration, default parameter values and comments. All 31 pass the literal `true`; none passes `false` or a dynamic value, so the migration is uniform and `RelativeOnly` is purely the existing default made nameable. audit.cts alone holds 18 of them. This migration is compiler-verified in a way the containment-value migration in the previous commit was not: the parameter type changed from an object to a string union, so any missed site is a build error rather than a silent behavioral difference. That is why a 31-site mechanical edit is acceptable in the phase whose stated risk is the width of mechanical change. Co-Authored-By: Claude Opus 5 --- src/adr-parser.cts | 4 ++-- src/audit.cts | 38 ++++++++++++++++++------------------ src/check-command-router.cts | 4 ++-- src/commands.cts | 8 ++++---- src/coverage.cts | 4 ++-- src/init.cts | 6 +++--- src/milestone.cts | 6 +++--- src/quick-batch.cts | 4 ++-- src/security.cts | 37 +++++++++++++++++++++++++++++------ src/state.cts | 4 ++-- src/uat.cts | 4 ++-- tests/agent-skills.test.cjs | 12 ++++++------ tests/security.test.cjs | 13 ++++++------ 13 files changed, 85 insertions(+), 59 deletions(-) diff --git a/src/adr-parser.cts b/src/adr-parser.cts index 0337d609f..fe6110373 100644 --- a/src/adr-parser.cts +++ b/src/adr-parser.cts @@ -9,7 +9,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { requireSafePath } from './security.cjs'; +import { requireSafePath, PathAcceptance } from './security.cjs'; import { collectSections } from './markdown-sectionizer.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import cliExitModule = require('./cli-exit.cjs'); @@ -458,7 +458,7 @@ function parseCliArgs(argv: string[]): CliOpts { function main(argv: string[]): void { const opts = parseCliArgs(argv); - const safePath = requireSafePath(opts.input, path.resolve(opts.projectDir), 'ADR input path', { allowAbsolute: true }); + const safePath = requireSafePath(opts.input, path.resolve(opts.projectDir), 'ADR input path', PathAcceptance.AbsoluteInsideRoot); const content = fs.readFileSync(safePath, 'utf8'); const parsed = parseAdrMarkdown(content, { sourcePath: opts.input ?? undefined, format: opts.format }); process.stdout.write(JSON.stringify(parsed, null, 2)); diff --git a/src/audit.cts b/src/audit.cts index 50da84a5c..f983b4c4b 100644 --- a/src/audit.cts +++ b/src/audit.cts @@ -32,7 +32,7 @@ const { PHASE_NUMBER_TOKEN_SOURCE, scopeToPhase } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocator = require('./phase-locator.cjs'); const { getAllArchivedPhaseDirs } = phaseLocator; -import { requireSafePath, sanitizeForDisplay, sanitizeLabel } from './security.cjs'; +import { requireSafePath, sanitizeForDisplay, sanitizeLabel, PathAcceptance } from './security.cjs'; import { platformWriteSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import io = require('./io.cjs'); @@ -447,7 +447,7 @@ function scanDebugSessions(planDir: string): ScanOutcome { let safeFilePath: string; try { - safeFilePath = requireSafePath(filePath, planDir, 'debug session file', { allowAbsolute: true }); + safeFilePath = requireSafePath(filePath, planDir, 'debug session file', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -563,7 +563,7 @@ function scanQuickTasks(planDir: string): ScanOutcome { let safeTaskDir: string; try { - safeTaskDir = requireSafePath(taskDir, planDir, 'quick task dir', { allowAbsolute: true }); + safeTaskDir = requireSafePath(taskDir, planDir, 'quick task dir', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -577,7 +577,7 @@ function scanQuickTasks(planDir: string): ScanOutcome { if (summaryPath && fs.existsSync(summaryPath)) { let safeSum: string; try { - safeSum = requireSafePath(summaryPath, planDir, 'quick task summary', { allowAbsolute: true }); + safeSum = requireSafePath(summaryPath, planDir, 'quick task summary', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -658,7 +658,7 @@ function scanThreads(planDir: string): ScanOutcome { let safeFilePath: string; try { - safeFilePath = requireSafePath(filePath, planDir, 'thread file', { allowAbsolute: true }); + safeFilePath = requireSafePath(filePath, planDir, 'thread file', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -748,7 +748,7 @@ function scanTodos(todosBase: string): ScanOutcome { let safeFilePath: string; try { - safeFilePath = requireSafePath(filePath, todosBase, 'todo file', { allowAbsolute: true }); + safeFilePath = requireSafePath(filePath, todosBase, 'todo file', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -829,7 +829,7 @@ function scanSeeds(planDir: string): ScanOutcome { let safeFilePath: string; try { - safeFilePath = requireSafePath(filePath, planDir, 'seed file', { allowAbsolute: true }); + safeFilePath = requireSafePath(filePath, planDir, 'seed file', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -1014,7 +1014,7 @@ function scanUatGaps(planDir: string, cwd: string): ScanOutcome { let safeFilePath: string; try { - safeFilePath = requireSafePath(filePath, planDir, 'UAT file', { allowAbsolute: true }); + safeFilePath = requireSafePath(filePath, planDir, 'UAT file', PathAcceptance.AbsoluteInsideRoot); } catch { continue; } @@ -1097,7 +1097,7 @@ function scanVerificationGaps(planDir: string, cwd: string): ScanOutcome { - return tryWithinRoot(globalSkillMd, root, { allowAbsolute: true }) !== null; + return tryWithinRoot(globalSkillMd, root, PathAcceptance.AbsoluteInsideRoot) !== null; }); if (!acceptedViaTrustedRoot) { warn( diff --git a/src/milestone.cts b/src/milestone.cts index ce78418a4..b29fe2d9e 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -21,7 +21,7 @@ import { transitionCore } from './state-transition.cjs'; import { writeSetComplete } from './write-set.cjs'; import type { WriteSet } from './write-set.cjs'; import { updateTableCell, resetQuickTaskRows, QUICK_TASKS_SECTION_ABSENT } from './markdown-table.cjs'; -import { requireSafePath } from './security.cjs'; +import { requireSafePath, PathAcceptance } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- audit.cjs is an export= CommonJS module import auditMod = require('./audit.cjs'); const { resolveQuickTaskSummaryFile } = auditMod; @@ -1640,7 +1640,7 @@ function listQuickTaskDirsForArchive(cwd: string): string[] { for (const entry of sourceEntries) { if (!entry.isDirectory()) continue; // excludes symlinks too — see MAJOR 3 note above try { - requireSafePath(path.join(quickDir, entry.name), planningBase, 'quick task dir', { allowAbsolute: true }); + requireSafePath(path.join(quickDir, entry.name), planningBase, 'quick task dir', PathAcceptance.AbsoluteInsideRoot); } catch { continue; // symlink/escape attempt — never a candidate, in preview OR real run } @@ -1719,7 +1719,7 @@ function archiveQuickTaskDirectories(cwd: string, version: string): { archiveDir // rename are two separate filesystem observations, and an entry // that was a safe real directory at selection time could in theory // be swapped for a symlink before this loop reaches it. - safeSrc = requireSafePath(src, planningBase, 'quick task dir', { allowAbsolute: true }); + safeSrc = requireSafePath(src, planningBase, 'quick task dir', PathAcceptance.AbsoluteInsideRoot); } catch { continue; // symlink/escape attempt — skip, not archived } diff --git a/src/quick-batch.cts b/src/quick-batch.cts index 4b48f0e3f..75dae3ab8 100644 --- a/src/quick-batch.cts +++ b/src/quick-batch.cts @@ -34,7 +34,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { requireSafePath, safeJsonParse } from './security.cjs'; +import { requireSafePath, safeJsonParse, PathAcceptance } from './security.cjs'; import { appendQuickTaskRow, parseMarkdownTable, @@ -305,7 +305,7 @@ function parseTaskListFromFile(cwd: string, filePath: string): Result'); } - const resolvedPath = requireSafePath(filePath, cwd, 'UAT file', { allowAbsolute: true }); + const resolvedPath = requireSafePath(filePath, cwd, 'UAT file', PathAcceptance.AbsoluteInsideRoot); if (!fs.existsSync(resolvedPath)) { error(`UAT file not found: ${filePath}`); } diff --git a/tests/agent-skills.test.cjs b/tests/agent-skills.test.cjs index f1f935c02..dfe718cb6 100644 --- a/tests/agent-skills.test.cjs +++ b/tests/agent-skills.test.cjs @@ -35,7 +35,7 @@ function runGsdToolsWithStderr(args, cwd, env) { }; } -const { loadTrustedGlobalRoots, tryWithinRoot } = require('../gsd-core/bin/lib/security.cjs'); +const { loadTrustedGlobalRoots, tryWithinRoot, PathAcceptance } = require('../gsd-core/bin/lib/security.cjs'); // ─── helpers ────────────────────────────────────────────────────────────────── @@ -915,21 +915,21 @@ describe('trusted_global_roots guard logic', () => { test('tryWithinRoot rejects skill outside globalSkillsBase (baseline — no trusted roots)', () => { const skillMd = path.join(externalDir, 'SKILL.md'); - const result = tryWithinRoot(skillMd, tmpDir, { allowAbsolute: true }); + const result = tryWithinRoot(skillMd, tmpDir, PathAcceptance.AbsoluteInsideRoot); assert.equal(result, null, 'skill outside base must be rejected by tryWithinRoot'); }); test('with trusted root matching real target dir — tryWithinRoot accepts', () => { // Simulate the trusted-root fallback: skill is outside base but inside trusted root const skillMd = path.join(externalDir, 'SKILL.md'); - const baseCheck = tryWithinRoot(skillMd, tmpDir, { allowAbsolute: true }); + const baseCheck = tryWithinRoot(skillMd, tmpDir, PathAcceptance.AbsoluteInsideRoot); assert.equal(baseCheck, null, 'base check must fail (prerequisite)'); // Trusted root fallback: check against externalDir const config = { agent_skills_security: { trusted_global_roots: [externalDir] } }; const trustedRoots = loadTrustedGlobalRoots(config); const acceptedViaTrustedRoot = trustedRoots.some((root) => { - const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true }); + const rootCheck = tryWithinRoot(skillMd, root, PathAcceptance.AbsoluteInsideRoot); return rootCheck !== null; }); assert.ok(acceptedViaTrustedRoot, 'skill must be accepted when within a trusted root'); @@ -942,7 +942,7 @@ describe('trusted_global_roots guard logic', () => { const config = { agent_skills_security: { trusted_global_roots: [unrelatedDir] } }; const trustedRoots = loadTrustedGlobalRoots(config); const acceptedViaTrustedRoot = trustedRoots.some((root) => { - const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true }); + const rootCheck = tryWithinRoot(skillMd, root, PathAcceptance.AbsoluteInsideRoot); return rootCheck !== null; }); assert.ok(!acceptedViaTrustedRoot, 'skill must still be rejected when trusted root is unrelated'); @@ -957,7 +957,7 @@ describe('trusted_global_roots guard logic', () => { const trustedRoots = loadTrustedGlobalRoots(config); assert.strictEqual(trustedRoots.length, 0, 'no roots loaded'); const acceptedViaTrustedRoot = trustedRoots.some((root) => { - const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true }); + const rootCheck = tryWithinRoot(skillMd, root, PathAcceptance.AbsoluteInsideRoot); return rootCheck !== null; }); assert.ok(!acceptedViaTrustedRoot, 'skill must be rejected when trusted roots is empty'); diff --git a/tests/security.test.cjs b/tests/security.test.cjs index be8cfa297..5c65a84db 100644 --- a/tests/security.test.cjs +++ b/tests/security.test.cjs @@ -25,6 +25,7 @@ const { validatePromptStructure, assertWithinRoot, tryWithinRoot, + PathAcceptance, } = require('../gsd-core/bin/lib/security.cjs'); // ─── Path Traversal Prevention ────────────────────────────────────────────── @@ -58,12 +59,12 @@ describe('assertWithinRoot / tryWithinRoot — engine invariance', () => { }); test('allows absolute paths within base when opted in', () => { - const r = tryWithinRoot(path.join(base, 'src/file.js'), base, { allowAbsolute: true }); + const r = tryWithinRoot(path.join(base, 'src/file.js'), base, PathAcceptance.AbsoluteInsideRoot); assert.ok(r !== null); }); test('rejects absolute paths outside base even when opted in', () => { - assert.equal(tryWithinRoot('/etc/passwd', base, { allowAbsolute: true }), null); + assert.equal(tryWithinRoot('/etc/passwd', base, PathAcceptance.AbsoluteInsideRoot), null); }); test('rejects null bytes', () => { @@ -1367,7 +1368,7 @@ describe('assertWithinRoot / tryWithinRoot — narrowed export (#4653)', () => { }); test('returns the resolved path for an absolute input INSIDE the root when {allowAbsolute:true}', () => { - const resolved = assertWithinRoot(path.join(base, 'src/file.js'), base, null, { allowAbsolute: true }); + const resolved = assertWithinRoot(path.join(base, 'src/file.js'), base, null, PathAcceptance.AbsoluteInsideRoot); assert.equal(resolved, path.resolve(base, 'src/file.js')); }); @@ -1376,7 +1377,7 @@ describe('assertWithinRoot / tryWithinRoot — narrowed export (#4653)', () => { }); test('throws on an absolute path outside the root even with {allowAbsolute:true}', () => { - assert.throws(() => assertWithinRoot('/etc/passwd', base, null, { allowAbsolute: true })); + assert.throws(() => assertWithinRoot('/etc/passwd', base, null, PathAcceptance.AbsoluteInsideRoot)); }); test('throws on a null byte', () => { @@ -1406,7 +1407,7 @@ describe('assertWithinRoot / tryWithinRoot — narrowed export (#4653)', () => { }); test('returns the resolved path for an absolute input INSIDE the root when {allowAbsolute:true}', () => { - const resolved = tryWithinRoot(path.join(base, 'src/file.js'), base, { allowAbsolute: true }); + const resolved = tryWithinRoot(path.join(base, 'src/file.js'), base, PathAcceptance.AbsoluteInsideRoot); assert.equal(resolved, path.resolve(base, 'src/file.js')); }); @@ -1416,7 +1417,7 @@ describe('assertWithinRoot / tryWithinRoot — narrowed export (#4653)', () => { }); test('returns exactly null for an absolute path outside the root even with {allowAbsolute:true}', () => { - const result = tryWithinRoot('/etc/passwd', base, { allowAbsolute: true }); + const result = tryWithinRoot('/etc/passwd', base, PathAcceptance.AbsoluteInsideRoot); assert.strictEqual(result, null); });