Harden SF safe path validation

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<ProductAuditParams, "milestoneId"> = {
+	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 });
+	}
+});

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",
 	);

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<ReassessRoadmapResult | { error: string }> {
+): 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(

src/resources/extensions/sf/tools/validate-milestone.ts

@@ -98,7 +98,11 @@ export async function handleValidateMilestone(
 	params: ValidateMilestoneParams,
 	basePath: string,
 	opts?: ValidateMilestoneOptions,
-): Promise<ValidateMilestoneResult | { error: string }> {
+): Promise<
+	| ValidateMilestoneResult
+	| { error: string }
+	| { error: string; details: { error: string; field: string; reason: string } }
+> {
 	if (
 		!params.milestoneId ||
 		typeof params.milestoneId !== "string" ||