fix: make spec_path containment mandatory in readSpecArg (#76)

mcp-server/server.ts

@@ -430,29 +430,42 @@ function buildGenerationFromArg(model_metadata: unknown): EntryGeneration {
 	return out;
 }
 
-async function readSpecArg(args: { spec?: unknown; spec_path?: unknown; cwd?: unknown }, allowedRoots: string[] = []): Promise<string> {
+// `allowedRoots` is deliberately required and must be non-empty whenever
+// spec_path is used. It previously defaulted to `[]`, which made containment
+// opt-in: a caller that omitted it would read any absolute path the client
+// asked for, silently reopening the arbitrary-file-read class of bug fixed in
+// v0.12.11. Containment is now the default posture and an empty root set is a
+// programming error rather than a bypass.
+export async function readSpecArg(
+	args: { spec?: unknown; spec_path?: unknown; cwd?: unknown },
+	allowedRoots: string[],
+): Promise<string> {
 	if (typeof args.spec === "string" && args.spec.length > 0) return args.spec;
 	if (typeof args.spec_path === "string" && args.spec_path.length > 0) {
 		if (!isAbsolute(args.spec_path)) {
 			throw new McpError(ErrorCode.InvalidParams, `spec_path must be absolute, got: ${args.spec_path}`);
 		}
+		if (!Array.isArray(allowedRoots) || allowedRoots.length === 0) {
+			throw new McpError(
+				ErrorCode.InternalError,
+				"refusing to read spec_path without a containment root — this is a caller bug, not a client error",
+			);
+		}
 		if (!(await pathExists(args.spec_path))) {
 			throw new McpError(ErrorCode.InvalidParams, `spec_path does not exist: ${args.spec_path}`);
 		}
 		// Enforce path containment: spec_path must be within an allowed root
 		// (cwd's .codecarto/ or the configured library path) to prevent
 		// arbitrary file reads.
-		if (allowedRoots.length > 0) {
-			const resolvedSpecPath = await canonicalPath(args.spec_path);
-			const withinAllowed = await Promise.all(
-				allowedRoots.map((root) => isWithinPathResolved(resolvedSpecPath, root)),
+		const resolvedSpecPath = await canonicalPath(args.spec_path);
+		const withinAllowed = await Promise.all(
+			allowedRoots.map((root) => isWithinPathResolved(resolvedSpecPath, root)),
+		);
+		if (!withinAllowed.some((result) => result)) {
+			throw new McpError(
+				ErrorCode.InvalidParams,
+				`spec_path must be within the workspace (.codecarto/) or the configured library path. Got: ${args.spec_path}`,
 			);
-			if (!withinAllowed.some((result) => result)) {
-				throw new McpError(
-					ErrorCode.InvalidParams,
-					`spec_path must be within the workspace (.codecarto/) or the configured library path. Got: ${args.spec_path}`,
-				);
-			}
 		}
 		return readFile(args.spec_path, "utf8");
 	}

tests/publish-path-containment.test.mjs

@@ -3,7 +3,7 @@ import assert from "node:assert/strict";
 import { mkdir, mkdtemp, writeFile, rm } from "node:fs/promises";
 import { join } from "node:path";
 import { tmpdir } from "node:os";
-import { handlePublish } from "../mcp-server/server.ts";
+import { handlePublish, readSpecArg } from "../mcp-server/server.ts";
 
 let tmpRoot;
 
@@ -71,4 +71,58 @@ test("codecarto_publish accepts spec_path within workspace", async () => {
 	} finally {
 		await cleanup();
 	}
-});
+});
+// ── readSpecArg defense-in-depth (issue #76) ────────────────────────────────
+// Containment used to be skipped entirely when allowedRoots was empty, making
+// it opt-in. These lock in that an empty root set fails closed.
+
+test("readSpecArg refuses spec_path when allowedRoots is empty", async () => {
+	await setup();
+	try {
+		await assert.rejects(
+			() => readSpecArg({ spec_path: join(tmpRoot, "outside", "secret.txt") }, []),
+			(err) => {
+				assert.match(err.message, /without a containment root/i);
+				return true;
+			},
+		);
+	} finally {
+		await cleanup();
+	}
+});
+
+test("readSpecArg fails closed on an empty root set without touching the filesystem", async () => {
+	await setup();
+	try {
+		// A path that does not exist must still be refused for the same reason,
+		// so the guard cannot be used to probe which files are present.
+		await assert.rejects(
+			() => readSpecArg({ spec_path: join(tmpRoot, "outside", "does-not-exist.txt") }, []),
+			(err) => {
+				assert.match(err.message, /without a containment root/i);
+				assert.doesNotMatch(err.message, /does not exist/i);
+				return true;
+			},
+		);
+	} finally {
+		await cleanup();
+	}
+});
+
+test("readSpecArg still reads spec_path within an allowed root", async () => {
+	const { workspaceDir, specContent } = await setup();
+	try {
+		const out = await readSpecArg(
+			{ spec_path: join(workspaceDir, "findings", "reimplementation-spec", "reimplementation-spec.md") },
+			[workspaceDir],
+		);
+		assert.equal(out, specContent);
+	} finally {
+		await cleanup();
+	}
+});
+
+test("readSpecArg accepts inline spec regardless of allowedRoots", async () => {
+	const out = await readSpecArg({ spec: "inline spec body" }, []);
+	assert.equal(out, "inline spec body");
+});