From ed19fa186458e28cfe36e9fccd0307e787de2367 Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Thu, 30 Apr 2026 08:08:10 +0200 Subject: [PATCH] Complete SF safe ID remediation sweep --- src/resources/extensions/sf/auto-dispatch.ts | 30 ++- .../sf/tests/reassess-roadmap-safe-id.test.ts | 198 ++++++++++++++++++ .../remediation-completion-guard.test.ts | 69 ++++++ .../tests/validate-milestone-safe-id.test.ts | 180 ++++++++++++++++ .../extensions/sf/tools/complete-slice.ts | 27 ++- .../extensions/sf/tools/complete-task.ts | 32 +-- .../extensions/sf/tools/product-audit-tool.ts | 6 +- 7 files changed, 510 insertions(+), 32 deletions(-) create mode 100644 src/resources/extensions/sf/tests/reassess-roadmap-safe-id.test.ts create mode 100644 src/resources/extensions/sf/tests/validate-milestone-safe-id.test.ts diff --git a/src/resources/extensions/sf/auto-dispatch.ts b/src/resources/extensions/sf/auto-dispatch.ts index 715ef4ba5..ffca3f311 100644 --- a/src/resources/extensions/sf/auto-dispatch.ts +++ b/src/resources/extensions/sf/auto-dispatch.ts @@ -278,6 +278,29 @@ function validationAttentionMarkerPath(basePath: string, mid: string): string { ); } +function validationAttentionRuntimePath(basePath: string, mid: string): string { + return join( + sfRoot(basePath), + "runtime", + "units", + `rewrite-docs-${mid}-validation-attention.json`, + ); +} + +function hasActiveValidationAttentionMarker( + basePath: string, + mid: string, +): boolean { + const markerPath = validationAttentionMarkerPath(basePath, mid); + if (!existsSync(markerPath)) return false; + if (existsSync(validationAttentionRuntimePath(basePath, mid))) return true; + logWarning( + "dispatch", + `ignoring stale validation attention marker for ${mid}: remediation unit was never recorded`, + ); + return false; +} + function buildValidationAttentionRemediationPrompt( mid: string, midTitle: string, @@ -1129,8 +1152,11 @@ export const DISPATCH_RULES: DispatchRule[] = [ if (verdict === "needs-attention") { const attentionPlan = extractValidationAttentionPlan(validationContent); - const markerPath = validationAttentionMarkerPath(basePath, mid); - if (attentionPlan && !existsSync(markerPath)) { + if ( + attentionPlan && + !hasActiveValidationAttentionMarker(basePath, mid) + ) { + const markerPath = validationAttentionMarkerPath(basePath, mid); try { mkdirSync( join(sfRoot(basePath), "runtime", "validation-attention"), diff --git a/src/resources/extensions/sf/tests/reassess-roadmap-safe-id.test.ts b/src/resources/extensions/sf/tests/reassess-roadmap-safe-id.test.ts new file mode 100644 index 000000000..0dcbbc20b --- /dev/null +++ b/src/resources/extensions/sf/tests/reassess-roadmap-safe-id.test.ts @@ -0,0 +1,198 @@ +/** + * Path-traversal ID validation tests for handleReassessRoadmap. + * Part of the validation-safe-id-path-segments self-feedback fix. + */ +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; + +import { handleReassessRoadmap } from "../tools/reassess-roadmap.ts"; + +function makeTmpBase(): string { + return mkdtempSync(join(tmpdir(), "sf-reassess-safe-id-")); +} + +function cleanup(base: string): void { + try { + rmSync(base, { recursive: true, force: true }); + } catch { + /* noop */ + } +} + +/** Minimal valid params — unsafe_id checks fire before any DB access. */ +function baseParams() { + return { + milestoneId: "M001", + completedSliceId: "S01", + verdict: "confirmed", + assessment: "All good.", + sliceChanges: { + modified: [], + added: [], + removed: [], + }, + }; +} + +// ─── milestoneId traversal ──────────────────────────────────────────────── + +test("handleReassessRoadmap rejects path traversal in milestoneId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { ...baseParams(), milestoneId: "../outside" }, + base, + ); + assert.ok("error" in result, "should return error"); + assert.equal(result.error, "unsafe_id"); + assert.ok("field" in result); + assert.ok("reason" in result); + } finally { + cleanup(base); + } +}); + +test("handleReassessRoadmap rejects path separator in milestoneId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { ...baseParams(), milestoneId: "M001/../../etc/passwd" }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + } finally { + cleanup(base); + } +}); + +// ─── completedSliceId traversal ─────────────────────────────────────────── + +test("handleReassessRoadmap rejects path traversal in completedSliceId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { ...baseParams(), completedSliceId: "../evil" }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + assert.ok("field" in result); + assert.ok("reason" in result); + } finally { + cleanup(base); + } +}); + +test("handleReassessRoadmap rejects newline in completedSliceId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { ...baseParams(), completedSliceId: "S01\nbad: true" }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + } finally { + cleanup(base); + } +}); + +// ─── sliceChanges.modified[].sliceId traversal ─────────────────────────── + +test("handleReassessRoadmap rejects path traversal in sliceChanges.modified sliceId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { + ...baseParams(), + sliceChanges: { + modified: [{ sliceId: "../bad", title: "title" }], + added: [], + removed: [], + }, + }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + assert.ok("field" in result); + assert.ok("reason" in result); + } finally { + cleanup(base); + } +}); + +// ─── sliceChanges.added[].sliceId traversal ────────────────────────────── + +test("handleReassessRoadmap rejects path traversal in sliceChanges.added sliceId", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { + ...baseParams(), + sliceChanges: { + modified: [], + added: [{ sliceId: "S01/../../x", title: "Injected" }], + removed: [], + }, + }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + assert.ok("field" in result); + assert.ok("reason" in result); + } finally { + cleanup(base); + } +}); + +// ─── sliceChanges.removed[] traversal ──────────────────────────────────── + +test("handleReassessRoadmap rejects path traversal in sliceChanges.removed entry", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { + ...baseParams(), + sliceChanges: { + modified: [], + added: [], + removed: ["../outside"], + }, + }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + assert.ok("field" in result); + assert.ok("reason" in result); + } finally { + cleanup(base); + } +}); + +test("handleReassessRoadmap rejects backslash separator in sliceChanges.removed entry", async () => { + const base = makeTmpBase(); + try { + const result = await handleReassessRoadmap( + { + ...baseParams(), + sliceChanges: { + modified: [], + added: [], + removed: ["S01\\..\\evil"], + }, + }, + base, + ); + assert.ok("error" in result); + assert.equal(result.error, "unsafe_id"); + } finally { + cleanup(base); + } +}); diff --git a/src/resources/extensions/sf/tests/remediation-completion-guard.test.ts b/src/resources/extensions/sf/tests/remediation-completion-guard.test.ts index 2bac5973d..eab916f54 100644 --- a/src/resources/extensions/sf/tests/remediation-completion-guard.test.ts +++ b/src/resources/extensions/sf/tests/remediation-completion-guard.test.ts @@ -144,6 +144,23 @@ test("completing-milestone dispatches validation-attention remediation once when "dispatch writes a loop-prevention marker", ); + mkdirSync(join(base, ".sf", "runtime", "units"), { recursive: true }); + writeFileSync( + join( + base, + ".sf", + "runtime", + "units", + "rewrite-docs-M001-validation-attention.json", + ), + JSON.stringify({ + version: 1, + unitType: "rewrite-docs", + unitId: "M001/validation-attention", + phase: "dispatched", + }), + ); + const second = await completingRule!.match(ctx); assert.ok(second !== null, "second rule should match"); assert.equal(second!.action, "stop", "second pass should not loop"); @@ -152,6 +169,58 @@ test("completing-milestone dispatches validation-attention remediation once when } }); +test("completing-milestone redispatches validation-attention when marker is stale", async () => { + const base = mkdtempSync(join(tmpdir(), "sf-remediation-")); + mkdirSync(join(base, ".sf", "milestones", "M001"), { recursive: true }); + mkdirSync(join(base, ".sf", "runtime", "validation-attention"), { + recursive: true, + }); + + try { + writeFileSync( + join(base, ".sf", "milestones", "M001", "M001-VALIDATION.md"), + [ + "---", + "verdict: needs-attention", + "remediation_round: 0", + "---", + "", + "# Validation Report", + "", + "## Remediation Plan", + "- Fix roadmap tracking.", + ].join("\n"), + ); + writeFileSync( + join(base, ".sf", "runtime", "validation-attention", "M001.json"), + JSON.stringify({ + milestoneId: "M001", + createdAt: new Date().toISOString(), + }), + ); + + const ctx = { + mid: "M001", + midTitle: "Test Milestone", + basePath: base, + state: { phase: "completing-milestone" } as any, + prefs: {} as any, + session: undefined, + }; + + const result = await completingRule!.match(ctx); + + assert.ok(result !== null, "rule should match"); + assert.equal(result!.action, "dispatch"); + if (result!.action === "dispatch") { + assert.equal(result!.unitType, "rewrite-docs"); + assert.equal(result!.unitId, "M001/validation-attention"); + } + } finally { + rmSync(base, { recursive: true, force: true }); + } +}); + test("completing-milestone proceeds normally when VALIDATION verdict is pass (#2675 guard)", async () => { const base = mkdtempSync(join(tmpdir(), "sf-remediation-")); mkdirSync(join(base, ".sf", "milestones", "M001"), { recursive: true }); diff --git a/src/resources/extensions/sf/tests/validate-milestone-safe-id.test.ts b/src/resources/extensions/sf/tests/validate-milestone-safe-id.test.ts new file mode 100644 index 000000000..dd39a61d8 --- /dev/null +++ b/src/resources/extensions/sf/tests/validate-milestone-safe-id.test.ts @@ -0,0 +1,180 @@ +/** + * Path-traversal ID validation tests for handleValidateMilestone. + * + * Verifies that unsafe milestoneId values are rejected at the entry point + * before any path construction or DB write occurs. + * See: validation-safe-id-path-segments self-feedback entry. + */ + +import assert from "node:assert/strict"; +import { randomUUID } from "node:crypto"; +import { mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { clearParseCache } from "../files.ts"; +import { clearPathCache } from "../paths.ts"; +import { closeDatabase, openDatabase } from "../sf-db.ts"; +import { handleValidateMilestone } from "../tools/validate-milestone.ts"; +import type { ValidateMilestoneParams } from "../tools/validate-milestone.ts"; + +function makeTmpBase(): string { + const base = join(tmpdir(), `sf-val-safeid-${randomUUID()}`); + mkdirSync(join(base, ".sf", "milestones"), { recursive: true }); + return base; +} + +function cleanup(base: string): void { + clearPathCache(); + clearParseCache(); + closeDatabase(); + try { + rmSync(base, { recursive: true, force: true }); + } catch { + /* swallow */ + } +} + +function openTestDb(base: string): void { + const dbPath = join(base, ".sf", "sf.db"); + assert.equal(openDatabase(dbPath), true, "test DB should open"); +} + +const baseParams: Omit = { + verdict: "pass", + remediationRound: 1, + successCriteriaChecklist: "- [x] done", + sliceDeliveryAudit: "all slices delivered", + crossSliceIntegration: "integrated", + requirementCoverage: "100%", + verdictRationale: "all criteria met", +}; + +test("handleValidateMilestone rejects path traversal milestoneId: ../outside", async () => { + const base = makeTmpBase(); + openTestDb(base); + try { + const result = await handleValidateMilestone( + { ...baseParams, milestoneId: "../outside" }, + base, + ); + assert.ok("error" in result, "should return an error for traversal ID"); + assert.ok( + typeof (result as { error: string }).error === "string", + "error should be a string", + ); + if ("details" in result) { + const details = ( + result as { error: string; details: { error: string; field: string; reason: string } } + ).details; + assert.equal(details.error, "unsafe_id"); + assert.equal(details.field, "milestone_id"); + } + } finally { + cleanup(base); + } +}); + +test("handleValidateMilestone rejects path traversal milestoneId: M001/../etc/passwd", async () => { + const base = makeTmpBase(); + openTestDb(base); + try { + const result = await handleValidateMilestone( + { ...baseParams, milestoneId: "M001/../etc/passwd" }, + base, + ); + assert.ok("error" in result, "should return an error for traversal ID"); + if ("details" in result) { + const details = ( + result as { error: string; details: { error: string; field: string; reason: string } } + ).details; + assert.equal(details.error, "unsafe_id"); + } + } finally { + cleanup(base); + } +}); + +test("handleValidateMilestone rejects milestoneId with path separator: a/b", async () => { + const base = makeTmpBase(); + openTestDb(base); + try { + const result = await handleValidateMilestone( + { ...baseParams, milestoneId: "a/b" }, + base, + ); + assert.ok("error" in result, "should return an error for ID with separator"); + if ("details" in result) { + const details = ( + result as { error: string; details: { error: string; field: string; reason: string } } + ).details; + assert.equal(details.error, "unsafe_id"); + assert.equal(details.field, "milestone_id"); + } + } finally { + cleanup(base); + } +}); + +test("handleValidateMilestone rejects milestoneId with embedded newline", async () => { + const base = makeTmpBase(); + openTestDb(base); + try { + const result = await handleValidateMilestone( + { ...baseParams, milestoneId: "M001\nbad: true" }, + base, + ); + assert.ok( + "error" in result, + "should return an error for ID with control char", + ); + if ("details" in result) { + const details = ( + result as { error: string; details: { error: string; field: string; reason: string } } + ).details; + assert.equal(details.error, "unsafe_id"); + assert.equal(details.field, "milestone_id"); + assert.ok( + details.reason.includes("control char") || details.reason.includes("newline"), + "reason should mention control char or newline", + ); + } + } finally { + cleanup(base); + } +}); + +test("handleValidateMilestone does not reject a safe milestoneId as unsafe_id (smoke)", async () => { + const base = makeTmpBase(); + openTestDb(base); + try { + // A safe ID should pass the safe-id check; later DB/disk errors are expected + // in this minimal test setup and do not indicate a safe-id rejection. + let result: + | { error: string } + | { error: string; details: { error: string; field: string; reason: string } } + | { milestoneId: string; verdict: string; validationPath: string } + | undefined; + try { + result = await handleValidateMilestone( + { ...baseParams, milestoneId: "M001" }, + base, + ); + } catch { + // DB/disk errors from the minimal test setup are acceptable here + return; + } + if (result !== undefined && "details" in result) { + const details = ( + result as { error: string; details: { error: string } } + ).details; + assert.notEqual( + details.error, + "unsafe_id", + "safe ID should not produce unsafe_id error", + ); + } + } finally { + cleanup(base); + } +}); diff --git a/src/resources/extensions/sf/tools/complete-slice.ts b/src/resources/extensions/sf/tools/complete-slice.ts index 7b77b5bcf..88022fccc 100644 --- a/src/resources/extensions/sf/tools/complete-slice.ts +++ b/src/resources/extensions/sf/tools/complete-slice.ts @@ -419,6 +419,18 @@ export async function handleCompleteSlice( paramsInput: CompleteSliceParams, basePath: string, ): Promise { + // ── Path-traversal guard (validation-safe-id-path-segments) ─────────── + // Checked on raw input before normalizeCompleteSliceParams so the + // structured error shape is preserved. Rejects any ID that could escape + // .sf/ when interpolated into a path or used in a DB write. + const idCheck = checkSafeIds({ + milestoneId: paramsInput.milestoneId, + sliceId: paramsInput.sliceId, + }); + if (idCheck) { + return { error: "unsafe_id", field: idCheck.fieldName, reason: idCheck.reason } as unknown as { error: string }; + } + let params: CompleteSliceParams; try { params = normalizeCompleteSliceParams(paramsInput); @@ -442,21 +454,6 @@ export async function handleCompleteSlice( return { error: "milestoneId is required and must be a non-empty string" }; } - // ── Path-traversal guard (validation-safe-id-path-segments) ─────────── - // Reject any ID that could escape .sf/ when interpolated into a path. - // Must run before sliceUnitKey, resolveSlicePath, and all DB writes. - const idCheck = checkSafeIds({ - milestoneId: params.milestoneId, - sliceId: params.sliceId, - }); - if (idCheck) { - return { - error: "unsafe_id", - field: idCheck.fieldName, - reason: idCheck.reason, - }; - } - // ── Ownership check (opt-in: only enforced when claim file exists) ────── const ownershipErr = checkOwnership( basePath, diff --git a/src/resources/extensions/sf/tools/complete-task.ts b/src/resources/extensions/sf/tools/complete-task.ts index db1decebf..5789c7dd2 100644 --- a/src/resources/extensions/sf/tools/complete-task.ts +++ b/src/resources/extensions/sf/tools/complete-task.ts @@ -256,6 +256,24 @@ export async function handleCompleteTask( paramsInput: CompleteTaskParams, basePath: string, ): Promise { + // ── Path-traversal safety check (runs on raw input before normalization) ─ + // Reject any ID that could escape .sf/ when interpolated into a path. + // Must run before normalizeCompleteTaskParams, which also validates IDs + // but returns a generic error string rather than the structured unsafe_id + // shape expected downstream. + const idCheck = checkSafeIds({ + milestoneId: paramsInput.milestoneId, + sliceId: paramsInput.sliceId, + taskId: paramsInput.taskId, + }); + if (idCheck) { + return { + error: "unsafe_id", + field: idCheck.fieldName, + reason: idCheck.reason, + }; + } + let params: CompleteTaskParams; try { params = normalizeCompleteTaskParams(paramsInput); @@ -286,20 +304,6 @@ export async function handleCompleteTask( return { error: "milestoneId is required and must be a non-empty string" }; } - // ── Path-traversal safety check ──────────────────────────────────────── - const idCheck = checkSafeIds({ - milestoneId: params.milestoneId, - sliceId: params.sliceId, - taskId: params.taskId, - }); - if (idCheck) { - return { - error: "unsafe_id", - field: idCheck.fieldName, - reason: idCheck.reason, - }; - } - // ── Ownership check (opt-in: only enforced when claim file exists) ────── const ownershipErr = checkOwnership( basePath, diff --git a/src/resources/extensions/sf/tools/product-audit-tool.ts b/src/resources/extensions/sf/tools/product-audit-tool.ts index 2f140bae5..11a387021 100644 --- a/src/resources/extensions/sf/tools/product-audit-tool.ts +++ b/src/resources/extensions/sf/tools/product-audit-tool.ts @@ -265,7 +265,11 @@ function renderAuditMarkdown( export async function handleProductAudit( rawParams: ProductAuditParams, basePath: string, -): Promise { +): Promise< + | ProductAuditResult + | { error: string } + | { error: string; field: string; reason: string } +> { let params: ProductAuditParams; try { params = validateProductAuditParams(rawParams);