Skip to content

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

Open
TheAmericanMaker wants to merge 1 commit into
mainfrom
fix/readspecarg-containment
Open

fix: make spec_path containment mandatory in readSpecArg (#76)#79
TheAmericanMaker wants to merge 1 commit into
mainfrom
fix/readspecarg-containment

Conversation

@TheAmericanMaker

Copy link
Copy Markdown
Member

Closes #76.

Problem

readSpecArg enforced containment only when allowedRoots was non-empty — and allowedRoots defaulted to []:

async function readSpecArg(args, allowedRoots: string[] = []) {
  ...
  if (allowedRoots.length > 0) {   // ← containment was opt-in
    /* containment check */
  }
  return readFile(args.spec_path, "utf8");
}

Any caller that omitted the argument would read whatever absolute path the client supplied — the same arbitrary-file-read class fixed in v0.12.11 (#75).

No released version is exploitable. The only live caller, handlePublish, always passes a non-empty root set (library path + cwd/.codecarto). This is defense-in-depth.

Fix

Containment is now the default posture and an empty root set fails closed:

  • allowedRoots is a required parameter — omitting it is a compile error, not a silent bypass. This is the part that actually prevents recurrence.
  • An empty root set throws when spec_path is used.
  • The guard runs before the existence check, so it can't be used to probe which files exist — the error is identical for present and absent paths.
  • Inline spec is unaffected — it returns before containment, as before.

One deviation from the issue: it describes readSpecArg as already exported. It wasn't — it was module-private with a single caller, so the exposure was narrower than stated. I've exported it now so the guarantee is directly testable, which is safe because the function is hardened rather than merely hidden.

Tests

Four added to tests/publish-path-containment.test.mjs, matching the issue's acceptance criteria:

Test Asserts
refuses spec_path when allowedRoots is empty fails closed
fails closed without touching the filesystem no existence oracle
still reads spec_path within an allowed root no regression
accepts inline spec regardless of roots inline path unaffected

I verified the two guard tests fail against the previous implementation by reverting the fix and re-running — they reported not ok, so they are not vacuous.

274/274 pass, build clean.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant