diff --git a/src/resources/extensions/sf/tests/product-audit-safe-id.test.ts b/src/resources/extensions/sf/tests/product-audit-safe-id.test.ts new file mode 100644 index 000000000..b6d76a462 --- /dev/null +++ b/src/resources/extensions/sf/tests/product-audit-safe-id.test.ts @@ -0,0 +1,110 @@ +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 { + handleProductAudit, + type ProductAuditParams, +} from "../tools/product-audit-tool.ts"; + +const BASE_PARAMS: Omit = { + verdict: "no-gaps", + summary: "All product requirements are satisfied.", + gaps: [], +}; + +function makeTmpDir(): string { + return mkdtempSync(join(tmpdir(), "sf-product-audit-safe-id-")); +} + +test("handleProductAudit rejects path traversal in milestoneId", async () => { + const basePath = makeTmpDir(); + try { + const result = await handleProductAudit( + { milestoneId: "../outside", ...BASE_PARAMS }, + basePath, + ); + assert.ok("error" in result, "expected an error response"); + const err = result as { error: string; field?: string; reason?: string }; + assert.equal(err.error, "unsafe_id"); + assert.equal(err.field, "milestone_id"); + assert.ok( + typeof err.reason === "string" && err.reason.length > 0, + "reason should be a non-empty string", + ); + } finally { + rmSync(basePath, { recursive: true, force: true }); + } +}); + +test("handleProductAudit rejects slash in milestoneId", async () => { + const basePath = makeTmpDir(); + try { + const result = await handleProductAudit( + { milestoneId: "M001/../../etc/passwd", ...BASE_PARAMS }, + basePath, + ); + assert.ok("error" in result, "expected an error response"); + const err = result as { error: string; field?: string; reason?: string }; + assert.equal(err.error, "unsafe_id"); + assert.equal(err.field, "milestone_id"); + } finally { + rmSync(basePath, { recursive: true, force: true }); + } +}); + +test("handleProductAudit rejects newline in milestoneId", async () => { + const basePath = makeTmpDir(); + try { + const result = await handleProductAudit( + { milestoneId: "M001\nbad: true", ...BASE_PARAMS }, + basePath, + ); + assert.ok("error" in result, "expected an error response"); + const err = result as { error: string; field?: string; reason?: string }; + assert.equal(err.error, "unsafe_id"); + assert.equal(err.field, "milestone_id"); + } finally { + rmSync(basePath, { recursive: true, force: true }); + } +}); + +test("handleProductAudit rejects dot-dot milestoneId", async () => { + const basePath = makeTmpDir(); + try { + const result = await handleProductAudit( + { milestoneId: "..", ...BASE_PARAMS }, + basePath, + ); + assert.ok("error" in result, "expected an error response"); + const err = result as { error: string; field?: string; reason?: string }; + assert.equal(err.error, "unsafe_id"); + assert.equal(err.field, "milestone_id"); + } finally { + rmSync(basePath, { recursive: true, force: true }); + } +}); + +test("handleProductAudit accepts a safe milestoneId and writes files", async () => { + const basePath = makeTmpDir(); + try { + const result = await handleProductAudit( + { milestoneId: "M001", ...BASE_PARAMS }, + basePath, + ); + // Should NOT be an unsafe_id error — either success or a different error + if ("error" in result) { + const err = result as { error: string }; + assert.notEqual( + err.error, + "unsafe_id", + "a safe milestoneId should not produce unsafe_id error", + ); + } else { + assert.equal(result.milestoneId, "M001"); + } + } finally { + rmSync(basePath, { recursive: true, force: true }); + } +}); diff --git a/src/resources/extensions/sf/tools/product-audit-tool.ts b/src/resources/extensions/sf/tools/product-audit-tool.ts index 8599aba53..2f140bae5 100644 --- a/src/resources/extensions/sf/tools/product-audit-tool.ts +++ b/src/resources/extensions/sf/tools/product-audit-tool.ts @@ -12,6 +12,7 @@ import { join } from "node:path"; import { atomicWriteAsync } from "../atomic-write.js"; +import { checkSafeIds } from "../safety/safe-id.js"; import { isNonEmptyString } from "../validation.js"; export const PRODUCT_GAP_SEVERITIES = [ @@ -272,6 +273,18 @@ export async function handleProductAudit( return { error: `validation failed: ${(err as Error).message}` }; } + // Reject path-traversal in any ID before it reaches string interpolation + // below — this is the chokepoint. See safety/safe-id.ts for rationale and + // the validation-safe-id-path-segments self-feedback entry. + const idCheck = checkSafeIds({ milestoneId: params.milestoneId }); + if (idCheck) { + return { + error: "unsafe_id", + field: idCheck.fieldName, + reason: idCheck.reason, + }; + } + const actionableGaps = params.gaps.filter( (gap) => gap.severity === "critical" || gap.severity === "high", ); diff --git a/src/resources/extensions/sf/tools/reassess-roadmap.ts b/src/resources/extensions/sf/tools/reassess-roadmap.ts index 29469e3f6..f470fec7a 100644 --- a/src/resources/extensions/sf/tools/reassess-roadmap.ts +++ b/src/resources/extensions/sf/tools/reassess-roadmap.ts @@ -5,6 +5,7 @@ import { renderAssessmentFromDb, renderRoadmapFromDb, } from "../markdown-renderer.js"; +import { checkSafeIds, validateSafePathSegment } from "../safety/safe-id.js"; import { deleteAssessmentByScope, deleteSlice, @@ -109,7 +110,11 @@ function validateParams(params: ReassessRoadmapParams): ReassessRoadmapParams { export async function handleReassessRoadmap( rawParams: ReassessRoadmapParams, basePath: string, -): Promise { +): Promise< + | ReassessRoadmapResult + | { error: string } + | { error: "unsafe_id"; field: string; reason: string } +> { // ── Validate ────────────────────────────────────────────────────── let params: ReassessRoadmapParams; try { @@ -118,6 +123,55 @@ export async function handleReassessRoadmap( return { error: `validation failed: ${(err as Error).message}` }; } + // ── Path-traversal ID safety check ──────────────────────────────── + // Reject any ID that could escape .sf/ via path traversal before it + // reaches string interpolation or DB writes. See safety/safe-id.ts. + const idCheck = checkSafeIds({ + milestoneId: params.milestoneId, + sliceId: params.completedSliceId, + }); + if (idCheck) { + return { + error: "unsafe_id", + field: idCheck.fieldName, + reason: idCheck.reason, + }; + } + // Validate slice IDs inside the arrays (modified, added, removed) + for (let i = 0; i < params.sliceChanges.modified.length; i++) { + try { + validateSafePathSegment( + params.sliceChanges.modified[i]!.sliceId, + `sliceChanges.modified[${i}].sliceId`, + ); + } catch (e) { + const err = e as import("../safety/safe-id.js").UnsafeIdError; + return { error: "unsafe_id", field: err.fieldName, reason: err.reason }; + } + } + for (let i = 0; i < params.sliceChanges.added.length; i++) { + try { + validateSafePathSegment( + params.sliceChanges.added[i]!.sliceId, + `sliceChanges.added[${i}].sliceId`, + ); + } catch (e) { + const err = e as import("../safety/safe-id.js").UnsafeIdError; + return { error: "unsafe_id", field: err.fieldName, reason: err.reason }; + } + } + for (let i = 0; i < params.sliceChanges.removed.length; i++) { + try { + validateSafePathSegment( + params.sliceChanges.removed[i]!, + `sliceChanges.removed[${i}]`, + ); + } catch (e) { + const err = e as import("../safety/safe-id.js").UnsafeIdError; + return { error: "unsafe_id", field: err.fieldName, reason: err.reason }; + } + } + // ── Compute assessment artifact path ────────────────────────────── // Assessment lives in the completed slice's directory const assessmentRelPath = join( diff --git a/src/resources/extensions/sf/tools/validate-milestone.ts b/src/resources/extensions/sf/tools/validate-milestone.ts index bfbace745..1e2b331df 100644 --- a/src/resources/extensions/sf/tools/validate-milestone.ts +++ b/src/resources/extensions/sf/tools/validate-milestone.ts @@ -98,7 +98,11 @@ export async function handleValidateMilestone( params: ValidateMilestoneParams, basePath: string, opts?: ValidateMilestoneOptions, -): Promise { +): Promise< + | ValidateMilestoneResult + | { error: string } + | { error: string; details: { error: string; field: string; reason: string } } +> { if ( !params.milestoneId || typeof params.milestoneId !== "string" ||