From 3fa7cf443a601c647e2736fa6cfdb7ea96732d78 Mon Sep 17 00:00:00 2001 From: James Sesler Date: Sun, 2 Aug 2026 12:38:12 -0400 Subject: [PATCH] fix: make spec_path containment mandatory in readSpecArg (#76) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readSpecArg enforced path containment only when allowedRoots was non-empty, and allowedRoots defaulted to []. Containment was therefore opt-in: any caller that omitted the argument would read whatever absolute path the client supplied, reopening the arbitrary-file-read class of bug fixed in v0.12.11 (#75). The only live caller (handlePublish) always passes a non-empty root set, so no released version is exploitable. This closes the gap by construction: - allowedRoots is now a required parameter, so omitting it is a compile error rather than a silent bypass. - An empty root set throws when spec_path is used, instead of skipping the check. - The guard runs before the existence check, so it cannot be used to probe which files exist. - The inline `spec` path is unaffected — it returns before containment. readSpecArg is now exported so the guarantee is directly testable; it is hardened rather than merely private. Four tests added, two of which fail against the previous implementation (verified by reverting). Closes #76 Co-Authored-By: Claude Opus 5 --- mcp-server/server.ts | 35 ++++++++++----- tests/publish-path-containment.test.mjs | 58 ++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/mcp-server/server.ts b/mcp-server/server.ts index 00faafa..b3acab7 100644 --- a/mcp-server/server.ts +++ b/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 { +// `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 { 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"); } diff --git a/tests/publish-path-containment.test.mjs b/tests/publish-path-containment.test.mjs index 40e2ff5..df114f9 100644 --- a/tests/publish-path-containment.test.mjs +++ b/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(); } -}); \ No newline at end of file +}); +// ── 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"); +});