Skip to content

fix(security): set strictMcpConfig to block cloned-PR .mcp.json auto-load#210

Merged
chrisleekr merged 2 commits into
mainfrom
fix/issue-196
Jun 3, 2026
Merged

fix(security): set strictMcpConfig to block cloned-PR .mcp.json auto-load#210
chrisleekr merged 2 commits into
mainfrom
fix/issue-196

Conversation

@chrisleekr

@chrisleekr chrisleekr commented Jun 3, 2026

Copy link
Copy Markdown
Owner

What

Closes #196. Follow-up to #191 / PR #195. That PR pinned settingSources: [] to close the .claude/settings.json SessionStart-hook RCE in the cloned PR working tree. This closes the sibling vector: a hostile .mcp.json committed in the same tree.

Determination (verified, not assumed)

settingSources: [] does not gate project .mcp.json discovery:

  1. The agent-sdk arg builder (node_modules/@anthropic-ai/claude-agent-sdk/sdk.mjs) maps strictMcpConfig → the CLI --strict-mcp-config flag, mcpServers--mcp-config, and settingSources--setting-sources.
  2. The Claude Code CLI reference --bare flag ("skip auto-discovery of hooks, skills, plugins, MCP servers, auto memory, and CLAUDE.md") shows MCP servers are auto-discovered by default and are a distinct item from settings/memory (what settingSources controls).
  3. --strict-mcp-config restricts MCP loading to the --mcp-config set, ignoring all other configuration including project .mcp.json.

So with cwd = the cloned PR tree + permissionMode: bypassPermissions, a hostile .mcp.json was auto-discovered and loaded — registering a stdio MCP server whose command runs on connect, the same RCE primitive class as #191.

Fix

  • src/core/executor.ts — add strictMcpConfig: true to queryOptions, next to the settingSources: [] pin, with an inline comment recording the determination (the TS docstring only mentions validation, but the flag it emits also suppresses discovery).
  • test/core/executor.test.ts — regression test: options.strictMcpConfig === true, the injected mcpServers are still forwarded (no-loss), and settingSources stays [].

No-loss: the bot supplies all its MCP servers explicitly via mcpServers; it never relies on a project .mcp.json, so this keeps every legitimate server and blocks only the attacker-controlled file.

Acceptance criteria

  1. ✅ Determined: project .mcp.json at cwd IS auto-loaded independent of settingSources: [].
  2. strictMcpConfig: true added with a regression test; injected mcpServers preserved.

Out of scope (residual, operator-controlled, not cloned-tree)

~/.claude.json, plugin-provided, and MCPB-bundled MCP servers live in operator-controlled locations, not the attacker-controllable PR tree. Both cloned-tree attacker config classes (settings.json, .mcp.json) are now closed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved MCP server configuration controls to prevent unintended auto-registration from project files, ensuring only explicitly specified servers load.

…load

settingSources:[] (issue #191) gates settings.json/CLAUDE.md but not a
project .mcp.json, which the CLI auto-discovers from cwd by default. With
cwd = the cloned PR tree under bypassPermissions, a hostile .mcp.json could
register a stdio MCP server whose command runs on connect, the same RCE
primitive class as #191. strictMcpConfig:true emits --strict-mcp-config,
restricting MCP loading to the explicitly-injected mcpServers.

Closes #196

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 3, 2026 08:26
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 44f3c61a-5362-457c-aea9-2560bbbd412f

📥 Commits

Reviewing files that changed from the base of the PR and between 5407dcd and 3d625f6.

📒 Files selected for processing (2)
  • src/core/executor.ts
  • test/core/executor.test.ts

📝 Walkthrough

Walkthrough

This PR hardens MCP (Model Context Protocol) server discovery in executeAgent by setting strictMcpConfig: true, which blocks auto-registration of .mcp.json files committed to the project and restricts MCP loading to explicitly provided mcpServers. The implementation adds inline documentation clarifying the interaction with settings discovery, and corresponding test coverage verifies the configuration is set correctly.

Changes

MCP Configuration Hardening

Layer / File(s) Summary
Executor strictMcpConfig implementation
src/core/executor.ts
executeAgent sets strictMcpConfig: true in SDK query options with expanded documentation explaining that .mcp.json discovery under workDir is blocked and MCP loading is restricted to executor-provided mcpServers.
Test infrastructure and MCP hardening assertions
test/core/executor.test.ts
QueryCall interface is extended with strictMcpConfig, settingSources, and mcpServers fields, and a new test case verifies executeAgent sets strictMcpConfig to true, preserves injected mcpServers, and keeps settingSources empty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

  • chrisleekr/github-app-playground#196: This PR directly implements the MCP config hardening control by setting strictMcpConfig: true in executeAgent and adding corresponding test assertions for this setting and the related mcpServers/settingSources behavior.

Possibly related PRs

  • chrisleekr/github-app-playground#195: Both PRs modify src/core/executor.ts's executeAgent SDK query options around settingSources field and settings/MCP discovery interaction, with this PR building directly on the earlier filesystem-settings isolation change.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding strictMcpConfig to block .mcp.json auto-loading in cloned PRs, which is the primary security fix across both modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Claude Agent SDK invocation against hostile MCP auto-discovery from an attacker-controlled cloned PR working directory by forcing MCP loading to only the explicitly injected mcpServers set.

Changes:

  • Set strictMcpConfig: true in the Agent SDK queryOptions to suppress project .mcp.json auto-discovery.
  • Add a regression test asserting strictMcpConfig is enabled while preserving mcpServers forwarding and the existing settingSources: [] pin.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/core/executor.ts Pins strictMcpConfig: true alongside settingSources: [] to prevent cloned-tree .mcp.json auto-load.
test/core/executor.test.ts Adds a regression test ensuring strictMcpConfig is set and mcpServers/settingSources are preserved.

Comment thread test/core/executor.test.ts
Make the #196/#191 assertions type-visible instead of relying on the
captured object's runtime shape.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@chrisleekr chrisleekr merged commit 2c58ec1 into main Jun 3, 2026
22 checks passed
@chrisleekr chrisleekr deleted the fix/issue-196 branch June 3, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(agent-sdk): verify whether cloned-PR .mcp.json is auto-loaded despite settingSources []

2 participants