Disable auto-approval of project MCP servers when .mcp.json changes in PR

[ashwin-ant]

What and why

When a PR modifies .mcp.json, the checked-out working directory contains a version that may differ from the base branch. The current code unconditionally sets enableAllProjectMcpServers = true in setupClaudeCodeSettings, which auto-approves all project MCP servers — including those introduced in the PR's .mcp.json — without the user having reviewed them.

This PR makes auto-approval conditional: if .mcp.json changed in the triggering PR, we skip setting enableAllProjectMcpServers = true so those servers are not automatically trusted.

Changes

base-action/src/setup-claude-code-settings.ts

Added a mcpJsonChanged boolean parameter (default false). The enableAllProjectMcpServers = true assignment now only runs when mcpJsonChanged is false. When it's true, a log message is emitted instead. All existing behaviour is preserved for non-PR events and for PRs that don't touch .mcp.json.

src/entrypoints/run.ts

Before calling setupClaudeCodeSettings, use octokit.rest.pulls.listFiles to fetch the files changed in the PR (when the event is a PR-type event). If any changed file is .mcp.json or ends with /.mcp.json, pass mcpJsonChanged=true. Errors from the API call are caught and logged; on any failure the flag stays false (safe default).

base-action/test/setup-claude-code-settings.test.ts

• Updated all existing test calls to pass false for mcpJsonChanged (no behaviour change)
• Renamed the override test to clarify it applies when mcpJsonChanged=false
• Added two new tests for mcpJsonChanged=true: one verifying enableAllProjectMcpServers stays undefined when not set in input, one verifying it stays false when the input sets it to false

[claude]

Code Review Summary

The security motivation here is sound — auto-approving project MCP servers when .mcp.json is attacker-controlled via a PR is a real RCE risk, and this is a worthwhile mitigation. However, the current implementation has several gaps that could undermine the protection it's trying to provide.

Must fix

1. Broken call site in base-action/src/index.ts — The signature change inserted mcpJsonChanged as the second parameter, but base-action/src/index.ts was not updated. It still passes undefined with a // homeDir comment that now maps to mcpJsonChanged. This works by accident (undefined is falsy), but the intent is wrong and fragile. See inline comment.

2. Pagination bypass — pulls.listFiles with per_page: 100 and no pagination means PRs with >100 changed files can hide .mcp.json beyond the first page. Use octokit.paginate(). See inline comment.

3. Fail-open error handling — API errors default to mcpJsonChanged=false (auto-approve everything). A security check should fail closed. See inline comment.

Should fix

4. User settings can re-enable auto-approval — When mcpJsonChanged=true, the code skips setting enableAllProjectMcpServers=true, but if the user's INPUT_SETTINGS already included enableAllProjectMcpServers: true via the merge step, it persists unchecked. The else branch should explicitly delete or set it to false. See inline comment.

5. Missing test for enableAllProjectMcpServers: true + mcpJsonChanged: true — Tests cover the false and undefined cases but not the case where user input explicitly sets it to true while .mcp.json changed.

Consider

6. Positional boolean parameter — Inserting a boolean between two optional strings is error-prone (as demonstrated by issue Modify base action #1). An options object would be safer and more extensible, especially for a public API.

7. No test coverage for the detection logic in run.ts — The filename matching, pagination, error handling, and context gating are all untested. Extracting this into a standalone helper (e.g., checkMcpJsonChanged(octokit, owner, repo, pullNumber)) would make it straightforward to unit test.

[km-anthropic]

@claude review

[claude]

Claude finished @km-anthropic's task in 1m 39s —— View job

---

Code Review for PR #942

• Read changed files and understand current state
• Check if previous review issues were addressed
• Provide review feedback

---

Good security motivation — preventing auto-approval of MCP servers when .mcp.json is attacker-controlled is important. The second commit (f4ac33b) addressed the pagination bypass and fail-closed issues from the prior review. However, two issues from that review remain unresolved.

Must fix

1. Broken call site in base-action/src/index.ts (still unfixed from prior review)

base-action/src/index.ts:14-17 still calls:

await setupClaudeCodeSettings(
  process.env.INPUT_SETTINGS,
  undefined, // homeDir
);

After the signature change to (settingsInput?, mcpJsonChanged?, homeDir?), undefined now maps to mcpJsonChanged (happens to be falsy, so behavior is accidentally correct) and homeDir is never passed. The // homeDir comment is now misleading. This needs to be updated to:

await setupClaudeCodeSettings(
  process.env.INPUT_SETTINGS,
  false, // mcpJsonChanged — standalone base-action doesn't detect PR changes
  undefined, // homeDir
);

Fix this →

Should fix

2. User INPUT_SETTINGS can re-enable enableAllProjectMcpServers when .mcp.json changed (still unfixed)

In base-action/src/setup-claude-code-settings.ts:63-72, when mcpJsonChanged is true, the else branch only skips setting enableAllProjectMcpServers. But the merge on line 59 (settings = { ...settings, ...inputSettings }) already ran — so if the user's INPUT_SETTINGS includes enableAllProjectMcpServers: true, it persists unchecked in the settings object.

The else branch should explicitly remove it:

} else {
    delete settings.enableAllProjectMcpServers;
    console.log(
      `Removing enableAllProjectMcpServers because .mcp.json changed in this PR`,
    );
}

Fix this →

3. Missing test for enableAllProjectMcpServers: true + mcpJsonChanged: true

Tests cover the cases where input doesn't set enableAllProjectMcpServers or sets it to false when mcpJsonChanged=true, but not the case where input explicitly sets it to true. This is exactly the scenario that issue #2 above would protect against.

Status of prior review items

#	Issue	Status
1	Broken index.ts call site	Still open
2	Pagination bypass	Fixed (uses octokit.rest.paginate)
3	Fail-open error handling	Fixed (defaults to mcpJsonChanged=true)
4	User settings override	Still open
5	Missing test for true + true case	Still open
6	Positional boolean parameter	Not addressed (consider)
7	No test coverage for detection logic in run.ts	Not addressed (consider)

[claude]

The pagination and fail-closed fixes address two of my earlier findings — thanks for that. Two issues remain: the base-action/src/index.ts call site still has a stale // homeDir comment and skips the new mcpJsonChanged parameter (works by accident), and the else branch in setup-claude-code-settings.ts still doesn't delete settings.enableAllProjectMcpServers when mcpJsonChanged is true, so user input with enableAllProjectMcpServers: true can bypass the protection.

Extended reasoning...

Overview

This PR adds a security check to disable auto-approval of project MCP servers when .mcp.json is modified in a PR. It touches three files: setup-claude-code-settings.ts (new mcpJsonChanged parameter), run.ts (detection logic using GitHub API), and the test file.

Progress since last review

Two of my four critical findings from the initial review have been fixed:

• Pagination: Now uses octokit.rest.paginate instead of a single listFiles call
• Fail-closed: Error handling now defaults to mcpJsonChanged = true

Both fixes match the suggestions I provided.

Remaining issues

1. Call site in base-action/src/index.ts (not in PR diff): Line 16 still passes undefined with the comment // homeDir, but after the signature change undefined now maps to mcpJsonChanged. This works by accident (falsy = false = old behavior), and homeDir uses its default. The comment is incorrect and the behavior is fragile.

2. Security gap: When mcpJsonChanged is true, the else branch only logs a message. It does not delete settings.enableAllProjectMcpServers. If the user's INPUT_SETTINGS includes enableAllProjectMcpServers: true, the merge on line 59 ({ ...settings, ...inputSettings }) will have already set it, and the else branch won't remove it — effectively bypassing the protection this PR is meant to provide.

Security risks

This PR is explicitly a security mitigation against RCE via malicious MCP servers in PRs. The remaining security gap (#2 above) means the protection can be circumvented if a workflow happens to set enableAllProjectMcpServers: true in its settings input. While this requires a specific workflow configuration, it undermines the security invariant the PR is trying to establish.

Level of scrutiny

High — this is security-critical code that guards against RCE. The fixes so far are good, but the remaining gap in the else branch should be addressed before merging.

[claude]

All 5 issues from my prior reviews are now resolved (index.ts call site, pagination, fail-closed, settings override, missing test). Deferring to human review since this touches security-sensitive MCP server auto-approval logic.

Extended reasoning...

Overview

This PR adds a security guard: when a PR modifies .mcp.json, the action skips auto-approving project MCP servers to prevent RCE via attacker-injected MCP configurations. It touches 4 files: run.ts (detection logic), setup-claude-code-settings.ts (conditional approval), index.ts (call site update), and the test file.

Status of previously raised issues

All 5 must-fix/should-fix items from my two prior reviews are now resolved:

1. Broken index.ts call site — Now passes false explicitly with correct comment
2. Pagination bypass — Uses octokit.rest.paginate to fetch all changed files
3. Fail-open error handling — Defaults to mcpJsonChanged=true (fail-closed)
4. User settings override — else branch now does delete settings.enableAllProjectMcpServers
5. Missing test — Added test for enableAllProjectMcpServers: true + mcpJsonChanged: true

The two remaining "consider" items (positional boolean parameter, no unit test for run.ts detection logic) are style/coverage suggestions, not correctness issues.

Security risks

The PR is a security improvement. The implementation correctly handles the key attack vectors: large PRs (pagination), API failures (fail-closed), nested .mcp.json paths, and user settings bypass. One residual limitation is GitHub's hard cap of 3,000 files per PR via the API, but this is an API-level constraint.

Level of scrutiny

This modifies security-sensitive logic controlling which MCP servers are auto-trusted. While the implementation is now correct and well-tested, a human should verify the security design decisions (fail-closed behavior, scope of .mcp.json path matching, interaction with other settings).

Other factors

Test coverage is comprehensive for the settings logic. The detection logic in run.ts lacks direct unit tests but is straightforward. A human reviewer (ddworken) confirmed two of the original bugs. The PR description accurately reflects the changes.