Skip to content

MCP readSpecArg skips containment check when allowedRoots is empty (defense-in-depth) #76

Description

@TheAmericanMaker

Description

During a security audit of the v0.12.11 publish-path-containment fix, I found a defense-in-depth gap in mcp-server/server.ts:readSpecArg.

The function enforces path containment on spec_path only when allowedRoots.length > 0:

// mcp-server/server.ts:445-456
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(/* ... spec_path must be within ... */);
  }
}
return readFile(args.spec_path, "utf8");

The live caller handlePublish always passes a non-empty allowedRoots (library path + cwd/.codecarto), so the current runtime is safe. However:

  • readSpecArg is exported and reusable. A future caller that forgets to pass allowedRoots silently reopens the arbitrary-file-read class of bug fixed in v0.12.11 (fix: MCP publish arbitrary file read via spec_path (security) — v0.12.11 #75) — spec_path would be read with no containment check.
  • The empty-default makes the containment opt-in rather than the default, which is the wrong posture for a function that reads attacker-influenced file paths.

Proposed fix

readSpecArg should throw when allowedRoots is empty rather than silently skipping the check, making containment the default behavior. The spec (inline content) path is unaffected — it returns before the containment check.

Suggested acceptance test

  • readSpecArg({ spec_path: "/etc/passwd" }, []) throws (or returns a McpError) rather than reading the file.
  • readSpecArg({ spec_path: "<within-workspace>" }, [workspaceRoot]) succeeds (existing behavior preserved).
  • readSpecArg({ spec: "inline spec" }, []) succeeds (inline path unaffected).

Context

Discovered during an end-to-end security audit of all three delivery surfaces (Pi extension, MCP server, library). The audit found the v0.12.10 symlink fix and v0.12.11 publish-path-containment fix both correct with regression coverage; this is the only residual defense-in-depth gap.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions