From af15f6b79bbab060799c71dc61a19d238ac675ac Mon Sep 17 00:00:00 2001 From: James Sesler Date: Thu, 23 Jul 2026 17:38:02 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20MCP=20publish=20arbitrary=20file=20read?= =?UTF-8?q?=20via=20spec=5Fpath=20(security)=20=E2=80=94=20v0.12.11?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The readSpecArg function accepted any absolute file path for spec_path without enforcing containment. An MCP client could read any file on the filesystem (e.g. /etc/passwd) by passing its absolute path as spec_path. Fix: added path containment check using isWithinPathResolved. spec_path must be within the workspace .codecarto/ directory or the configured library path. Regression tests verify both rejection and acceptance. 270/270 tests pass (268 + 2 new). --- CHANGELOG.md | 6 ++ mcp-server/server.ts | 26 ++++++++- package-lock.json | 4 +- package.json | 2 +- server.json | 4 +- tests/publish-path-containment.test.mjs | 74 +++++++++++++++++++++++++ 6 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 tests/publish-path-containment.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 37f5443..7b451a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project are documented here. The format is based on ## [Unreleased] +## [0.12.11] — 2026-07-23 + +### Fixed + +- MCP `codecarto_publish` arbitrary file read via `spec_path` (security). The `readSpecArg` function accepted any absolute file path for `spec_path` without enforcing containment, allowing an MCP client to read any file on the filesystem (e.g. `/etc/passwd`). Added path containment check using `isWithinPathResolved`: `spec_path` must be within the workspace `.codecarto/` directory or the configured library path. Regression tests verify both rejection of paths outside allowed roots and acceptance of paths within the workspace. Discovered during the v0.12.9 self-analysis case study security triage. + ## [0.12.10] — 2026-07-23 ### Fixed diff --git a/mcp-server/server.ts b/mcp-server/server.ts index 56f7918..00faafa 100644 --- a/mcp-server/server.ts +++ b/mcp-server/server.ts @@ -43,6 +43,7 @@ import { getPipelineLabel, getWorkspaceState, isValidSlug, + isWithinPathResolved, type LibraryIndexEntry, type LibraryVisibility, listEntries, @@ -429,7 +430,7 @@ function buildGenerationFromArg(model_metadata: unknown): EntryGeneration { return out; } -async function readSpecArg(args: { spec?: unknown; spec_path?: unknown }): Promise { +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)) { @@ -438,6 +439,21 @@ async function readSpecArg(args: { spec?: unknown; spec_path?: unknown }): Promi 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)), + ); + 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"); } throw new McpError(ErrorCode.InvalidParams, "Either spec (inline content) or spec_path (absolute file path) is required"); @@ -487,7 +503,13 @@ export async function handlePublish(args: Record) { ); } - const spec = await readSpecArg(args); + // Build allowed roots for spec_path containment: workspace .codecarto/ and library path + const allowedRoots: string[] = [libraryPath]; + if (typeof args.cwd === "string" && args.cwd.trim() !== "") { + allowedRoots.push(join(args.cwd.trim(), ".codecarto")); + } + + const spec = await readSpecArg(args, allowedRoots); if (typeof args.source_repo !== "string" || args.source_repo.trim() === "") { throw new McpError(ErrorCode.InvalidParams, "source_repo is required"); } diff --git a/package-lock.json b/package-lock.json index 29d3104..cc1ae34 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "codecartographer-pi", - "version": "0.12.10", + "version": "0.12.11", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "codecartographer-pi", - "version": "0.12.10", + "version": "0.12.11", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.29.0" diff --git a/package.json b/package.json index df7de75..3836b5f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "codecartographer-pi", - "version": "0.12.10", + "version": "0.12.11", "mcpName": "io.github.HuginnIndustries/codecartographer", "description": "Evidence-backed reverse engineering and human-gated software planning for Pi and MCP coding agents.", "type": "module", diff --git a/server.json b/server.json index 856db92..3a5fd56 100644 --- a/server.json +++ b/server.json @@ -8,12 +8,12 @@ "url": "https://github.com/HuginnIndustries/CodeCartographer", "source": "github" }, - "version": "0.12.10", + "version": "0.12.11", "packages": [ { "registryType": "npm", "identifier": "codecartographer-pi", - "version": "0.12.10", + "version": "0.12.11", "transport": { "type": "stdio" } diff --git a/tests/publish-path-containment.test.mjs b/tests/publish-path-containment.test.mjs new file mode 100644 index 0000000..40e2ff5 --- /dev/null +++ b/tests/publish-path-containment.test.mjs @@ -0,0 +1,74 @@ +import { test } from "node:test"; +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"; + +let tmpRoot; + +async function setup() { + tmpRoot = await mkdtemp(join(tmpdir(), "cc-publish-test-")); + const workspaceDir = join(tmpRoot, "target", ".codecarto"); + const libraryDir = join(tmpRoot, "library"); + await mkdir(join(workspaceDir, "workflow"), { recursive: true }); + await mkdir(join(workspaceDir, "findings", "reimplementation-spec"), { recursive: true }); + await mkdir(libraryDir, { recursive: true }); + + await writeFile(join(libraryDir, ".codecarto-library"), JSON.stringify({ + schema_version: 1, + name: "test-library", + visibility: "internal", + namespaced: false, + created_at: "2026-01-01T00:00:00Z", + })); + + const specContent = "# Test Spec\n\n## System Summary\n\nA test reimplementation spec."; + await writeFile(join(workspaceDir, "findings", "reimplementation-spec", "reimplementation-spec.md"), specContent); + + await mkdir(join(tmpRoot, "outside"), { recursive: true }); + await writeFile(join(tmpRoot, "outside", "secret.txt"), "SECRET CONTENT"); + + return { workspaceDir, libraryDir, specContent, secretPath: join(tmpRoot, "outside", "secret.txt") }; +} + +async function cleanup() { + await rm(tmpRoot, { recursive: true, force: true }).catch(() => {}); +} + +test("codecarto_publish rejects spec_path outside workspace and library", async () => { + const { secretPath, libraryDir } = await setup(); + try { + await assert.rejects( + handlePublish({ + library_path: libraryDir, + source_repo: "https://github.com/test/repo", + headline: "Test spec", + spec_path: secretPath, + }), + (err) => { + assert.match(err.message, /must be within the workspace/i); + return true; + }, + ); + } finally { + await cleanup(); + } +}); + +test("codecarto_publish accepts spec_path within workspace", async () => { + const { workspaceDir, libraryDir } = await setup(); + try { + await handlePublish({ + library_path: libraryDir, + cwd: join(workspaceDir, ".."), + source_repo: "https://github.com/test/repo", + headline: "Test spec", + spec_path: join(workspaceDir, "findings", "reimplementation-spec", "reimplementation-spec.md"), + }); + } catch (err) { + assert.doesNotMatch(err.message, /must be within the workspace/i); + } finally { + await cleanup(); + } +}); \ No newline at end of file