test(#4652): failing-first coverage for the four unconfined boundaries
Phase 2 of epic #4636, absorbing #4327 and #4354. Tests only; no fix. These MUST fail. Four CLI boundaries join externally-supplied input to a managed root with no containment validation. Each was driven through the real CLI and confirmed unconfined before the assertions were written: todo complete <name> src/commands.cts cmdTodoComplete check predicate --phase-dir <dir> check-command-router cmdCheckPredicate check decision-coverage-plan <dir> check-command-router resolvePath check gap-analysis.plan-post <dir> check-command-router Boundary 1 is worse than the issue describes. #4327 reports that a traversal name "resolves outside the todos root", which reads as an information leak. Measured, it is destructive: `todo complete ../../../../b1out/leak.md` exited 0, MOVED the outside file into completed/, and unlinked the original. The file was gone. cmdTodoComplete ends in fs.unlinkSync(sourcePath), so an unconfined name does not merely read across the boundary, it consumes across it. Boundary 2 reproduces #4354 exactly: a BLOCKING gate returned {"block":false,"details":{"match":true}} sourced entirely from a SECURITY.md in a directory the caller chose, outside the project. Boundaries 3 and 4 are not named in the epic. Both accepted an outside phase dir and exited 0. Rows that exist because they are the ones nobody enumerates: - ORDERING. A real file is created outside the todos root, then the traversal name targeting it is asserted rejected AND the outside file asserted still present and unmoved. #4327 notes the existence check and the move target BOTH follow the unvalidated join, so a rejection that lands after the read has already leaked — and, per the finding above, after the unlink has already destroyed. - `a/../../b.md` — looks balanced, resolves outside. - --dry-run must reject too; a preview must not leak a resolved outside path. - ${PHASE_DIR} interpolation into a command-exit-zero predicate is the SECOND predicate kind, which a fix inside gate-predicate-evaluator.cts would miss. - An absolute path INSIDE the project must still be accepted at every boundary — absolute is not a synonym for escaping. Cross-boundary rows loop over one shared list of escaping inputs and assert all four reject with the same shape, so four sites adopting one predicate cannot drift into four rejection contracts. Property tests cover BOTH directions — outside is always rejected, inside is always accepted. A property asserting only rejection is satisfied by a predicate that rejects everything, which is the degenerate-implementation trap found in Phase 1's review. Both are seeded. Regressions fold into the owning module suites rather than a new tests/fix-NNNN-*.test.cjs, per scripts/lint-regression-test-names.cjs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
sim
@@ -20,6 +20,7 @@ const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const { spawnSync } = require('child_process');
|
||||
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
@@ -717,3 +718,136 @@ describe('resolveLoopHooks plan:post — pure function against real registry', (
|
||||
assert.strictEqual(entry.gates[0].capId, 'gap-analysis');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #4652: containment boundaries — resolvePath (check-command-router.cts:92)
|
||||
// and `check gap-analysis.plan-post <phase-dir>` ───────────────────────────────
|
||||
//
|
||||
// Boundary 3: `check decision-coverage-plan <phase-dir>` resolves the phase-dir
|
||||
// positional via `resolvePath()`, which just does
|
||||
// `path.isAbsolute(p) ? p : path.join(projectDir, p)` — no containment check.
|
||||
// Boundary 4: `check gap-analysis.plan-post <phase-dir>` takes `args[2]`
|
||||
// unconfined and joins it directly in `runGapAnalysis` (gap-checker.cts).
|
||||
|
||||
function runDecisionCoveragePlan(extraFlags, phaseDir, contextPath, cwd) {
|
||||
return runGsdTools(['query', 'check.decision-coverage-plan', ...extraFlags, phaseDir, contextPath], cwd);
|
||||
}
|
||||
|
||||
describe('resolvePath / check decision-coverage-plan — containment boundary (#4652)', () => {
|
||||
let tmpDir;
|
||||
let phaseDir;
|
||||
let outsideDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-decision-outside-'));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
cleanup(outsideDir);
|
||||
});
|
||||
|
||||
test('[RED #4652] an outside phase-dir is rejected (currently resolves and proceeds unconfined)', () => {
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
fs.writeFileSync(
|
||||
contextPath,
|
||||
'# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n</decisions>\n',
|
||||
);
|
||||
fs.writeFileSync(path.join(outsideDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n');
|
||||
const relOutside = path.relative(tmpDir, outsideDir);
|
||||
|
||||
const result = runGsdTools(
|
||||
['--json-errors', 'query', 'check.decision-coverage-plan', relOutside, contextPath],
|
||||
tmpDir,
|
||||
);
|
||||
|
||||
assert.strictEqual(
|
||||
result.success,
|
||||
false,
|
||||
`an outside phase-dir must be rejected before evaluating plan coverage ` +
|
||||
`(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`,
|
||||
);
|
||||
});
|
||||
|
||||
test('[regression] a valid in-project relative phase-dir still proceeds', () => {
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
fs.writeFileSync(
|
||||
contextPath,
|
||||
'# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n</decisions>\n',
|
||||
);
|
||||
fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n');
|
||||
const relPhaseDir = path.relative(tmpDir, phaseDir);
|
||||
|
||||
const result = runDecisionCoveragePlan([], relPhaseDir, contextPath, tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const out = JSON.parse(result.output);
|
||||
assert.strictEqual(out.passed, true, 'in-project phase-dir with covered decision must pass');
|
||||
});
|
||||
|
||||
test('[regression] an absolute path INSIDE the project is accepted', () => {
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
fs.writeFileSync(
|
||||
contextPath,
|
||||
'# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n- **D-01:** Use pattern X\n</decisions>\n',
|
||||
);
|
||||
fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan\n\nImplements D-01.\n');
|
||||
|
||||
const result = runDecisionCoveragePlan([], phaseDir, contextPath, tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const out = JSON.parse(result.output);
|
||||
assert.strictEqual(out.passed, true, 'absolute in-project phase-dir must be accepted');
|
||||
});
|
||||
});
|
||||
|
||||
describe('check gap-analysis.plan-post — containment boundary (#4652)', () => {
|
||||
let tmpDir;
|
||||
let phaseDir;
|
||||
let outsideDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
const init = runGsdTools('config-ensure-section', tmpDir);
|
||||
assert.ok(init.success, `config-ensure-section failed: ${init.error}`);
|
||||
outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-gap-outside-'));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
cleanup(outsideDir);
|
||||
});
|
||||
|
||||
test('[RED #4652] an outside phase-dir is rejected (currently resolves and proceeds unconfined)', () => {
|
||||
fs.writeFileSync(path.join(outsideDir, '01-PLAN.md'), '# Plan\n\nSome content.\n');
|
||||
const relOutside = path.relative(tmpDir, outsideDir);
|
||||
|
||||
const result = runGsdTools(['--json-errors', 'check', 'gap-analysis.plan-post', relOutside, '--raw'], tmpDir);
|
||||
|
||||
assert.strictEqual(
|
||||
result.success,
|
||||
false,
|
||||
`an outside phase-dir must be rejected ` +
|
||||
`(currently: ${result.success ? `SUCCEEDED with output ${result.output}` : 'failed for an unrelated reason'})`,
|
||||
);
|
||||
});
|
||||
|
||||
test('[regression] a valid phase-dir still proceeds (advisory, block:false)', () => {
|
||||
writeRequirements(path.join(tmpDir, '.planning'), ['REQ-01']);
|
||||
writePlan(phaseDir, '01', '# Plan\n\nImplements REQ-01.\n');
|
||||
|
||||
const r = runGapCheck([phaseDir], tmpDir);
|
||||
assert.ok(r.success, `check failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.strictEqual(out.block, false, 'gap-analysis is always advisory');
|
||||
});
|
||||
|
||||
test('[regression] a missing phase-dir argument still gives the existing SDK_MISSING_ARG error', () => {
|
||||
const result = runGsdTools(['--json-errors', 'check', 'gap-analysis.plan-post', '--raw'], tmpDir);
|
||||
assert.strictEqual(result.success, false, 'must fail when phaseDir omitted');
|
||||
const parsed = JSON.parse(result.error);
|
||||
assert.strictEqual(parsed.reason, 'sdk_missing_arg', 'must keep the existing SDK_MISSING_ARG reason');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user