fix(#4652): use the validated path, collapse the duplication, correct two false claims
Seven findings from the two-axis review, all fixed in place. THE ONE THAT MATTERS: cmdTodoComplete validated sourcePath and targetPath and then ran every fs call against the RAW strings — existsSync, statSync, readFileSync, platformWriteSync, unlinkSync, and the dry-run path payload — never sourceCheck.resolved / targetCheck.resolved. That is the exact "validate one path, use another" shape ADR-4650 names as the defect this epic exists to prevent, and it is the same bug this phase had just fixed in check-command-router. Committed inside the fix for it. All I/O now uses the resolved paths; user-facing messages still echo the raw filename, never a resolved absolute path. A VACUOUS TEST, and the false doc claim it was propping up. The test "[RED #4327] an absolute path outside the project is rejected" would have passed with ZERO containment logic: path.join(pendingDir, '/abs/outside/x') yields <pendingDir>/abs/outside/x — Node does not let a later absolute segment escape — so the name is FOLDED under the root, passes containment, and simply 404s. The test only ever observed "Todo not found". It now asserts what is actually true and actually valuable: an absolute name is neutralized, and the real outside file is not read, not moved, and still present afterward. docs/CLI-TOOLS.md claimed such a path "is rejected as a usage error", which was false; it now describes the fold-under-root behavior. Traversal and embedded separators ARE rejected, and those claims stand. DUPLICATION THIS EPIC EXISTS TO REMOVE. resolvePath already did isAbsolute-or-join + validatePath + reject; cmdGapAnalysisPlanPost and cmdCheckPredicate each re-inlined the identical triplet in the same file. Both now call resolvePath. Cost, stated rather than hidden: its generic message replaces the two sites' distinct "phase-dir escapes…" wording. The message still names the offending input, and one predicate with one message is the point. SYMLINK COVERAGE was required by #4652's "Done when" and was missing. Added for both the todos root and --phase-dir, skipping cleanly on EPERM so the Windows lanes do not fail where unprivileged symlink creation is disallowed. Both fast-check properties were UNSEEDED. Seeded now. The changeset named "check decision-coverage-plan" as a boundary; that is a caller of the shared resolvePath, which the body never mentioned. Corrected. DISCLOSED, not hidden: ctx.phaseDir is now always the resolved ABSOLUTE path, so ${PHASE_DIR} interpolation and the "not found in <targetDir>" message show an absolute value where a relative --phase-dir previously produced a relative one. That is an observable output change. A test pins it and docs/reference/gate-predicates.md states it. Also regenerated scripts/lib/platform-conformance-tier.generated.cjs and its macos twin — the new tests changed check-predicate.test.cjs's tier classification. Caught by npm run lint:ci locally rather than by a bench run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -890,13 +890,38 @@ describe('todo complete — containment boundary (#4327)', () => {
|
||||
});
|
||||
}
|
||||
|
||||
test('[RED #4327] an absolute path outside the project is rejected', () => {
|
||||
test('[#4327] an absolute filename is folded under the pending dir, not rejected as containment violation — the outside file is untouched', () => {
|
||||
// MEASURED: path.join(pendingDir, '/abs/outside/evil.md') === `${pendingDir}/abs/outside/evil.md`
|
||||
// — Node's path.join does not let a later absolute segment escape a prior
|
||||
// one. So an absolute `filename` is folded INSIDE todosRoot, passes
|
||||
// containment, and simply 404s as "Todo not found" (unless a file of
|
||||
// that joined name happens to exist under pendingDir). It is NOT
|
||||
// rejected as a containment/escape violation.
|
||||
const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-'));
|
||||
try {
|
||||
const outsideFile = path.join(outsideDir, 'evil.md');
|
||||
fs.writeFileSync(outsideFile, '---\nstatus: pending\n---\nSENTINEL\n');
|
||||
const sentinel = '---\nstatus: pending\n---\nSENTINEL\n';
|
||||
fs.writeFileSync(outsideFile, sentinel);
|
||||
const result = runGsdTools(['todo', 'complete', outsideFile], tmpDir);
|
||||
assert.strictEqual(result.success, false, 'an absolute path outside the project must be rejected');
|
||||
|
||||
assert.strictEqual(result.success, false, 'the command must fail (the folded path does not exist under pending/)');
|
||||
assert.ok(
|
||||
(result.error || '').includes('not found'),
|
||||
`must fail as a plain "not found", not a containment rejection (got: ${result.error})`,
|
||||
);
|
||||
assert.ok(fs.existsSync(outsideFile), 'the real outside file must still exist');
|
||||
assert.strictEqual(
|
||||
fs.readFileSync(outsideFile, 'utf-8'),
|
||||
sentinel,
|
||||
'the real outside file must never be read/touched',
|
||||
);
|
||||
const completedDir = path.join(tmpDir, '.planning', 'todos', 'completed');
|
||||
if (fs.existsSync(completedDir)) {
|
||||
assert.ok(
|
||||
!fs.readdirSync(completedDir).includes('evil.md'),
|
||||
'the outside file must never land inside completed/',
|
||||
);
|
||||
}
|
||||
} finally {
|
||||
cleanup(outsideDir);
|
||||
}
|
||||
@@ -961,6 +986,38 @@ describe('todo complete — containment boundary (#4327)', () => {
|
||||
cleanup(outsideDir);
|
||||
}
|
||||
});
|
||||
|
||||
test('[#4652] a symlink inside pending/ whose target is a real file outside the todos root is rejected — the outside target is untouched', (t) => {
|
||||
const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-todo-outside-'));
|
||||
const linkPath = path.join(pendingDir, 'linked.md');
|
||||
try {
|
||||
const outsideFile = path.join(outsideDir, 'real-target.md');
|
||||
const sentinel = '---\nstatus: pending\n---\nSENTINEL-SYMLINK\n';
|
||||
fs.writeFileSync(outsideFile, sentinel);
|
||||
try {
|
||||
fs.symlinkSync(outsideFile, linkPath, 'file');
|
||||
} catch (e) {
|
||||
if (e.code === 'EPERM') {
|
||||
t.skip('symlink creation is not permitted on this platform (EPERM)');
|
||||
return;
|
||||
}
|
||||
throw e;
|
||||
}
|
||||
|
||||
const result = runGsdTools(['todo', 'complete', 'linked.md'], tmpDir);
|
||||
|
||||
assert.strictEqual(result.success, false, 'a symlink pointing outside the todos root must be rejected');
|
||||
assert.ok(fs.existsSync(outsideFile), 'the outside symlink target must still exist');
|
||||
assert.strictEqual(
|
||||
fs.readFileSync(outsideFile, 'utf-8'),
|
||||
sentinel,
|
||||
'the outside symlink target content must be byte-for-byte untouched',
|
||||
);
|
||||
} finally {
|
||||
cleanup(outsideDir);
|
||||
try { fs.unlinkSync(linkPath); } catch { /* not created, or already gone */ }
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user