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]

Bug: Pagination bypass allows .mcp.json to be hidden in large PRs

pulls.listFiles returns at most per_page results (100 here) and requires pagination for PRs with more files. An attacker could pad a PR with 100+ changed files to push .mcp.json off the first page, silently bypassing this security check.

GitHub orders files alphabetically, so root .mcp.json sorts early (. < most chars), but a nested path like zzz/.mcp.json would sort late and be trivially exploitable.

Consider using octokit.paginate to iterate all pages:

Suggested change

	const { data: changedFiles } = await octokit.rest.pulls.listFiles({
	owner: context.repository.owner,
	repo: context.repository.repo,
	pull_number: context.entityNumber,
	per_page: 100,
	});
	const changedFiles = await octokit.paginate(
	octokit.rest.pulls.listFiles,
	{
	owner: context.repository.owner,
	repo: context.repository.repo,
	pull_number: context.entityNumber,
	per_page: 100,
	},
	);

Note: the GitHub API has a hard cap of 3,000 files per PR regardless of pagination, but this covers the vast majority of cases.

[ddworken]

+1, this seems like a legitimate bug.

[claude]

Security: Fail-open on error is the wrong default for a security check

If the API call fails (rate limiting, network error, token permission issue), this defaults to mcpJsonChanged = false, meaning all project MCP servers are auto-approved. For a check explicitly designed to prevent RCE via malicious MCP servers, this should fail closed:

Suggested change

	} catch (e) {
	console.log(
	`Could not check PR changed files: ${e}. Defaulting to mcpJsonChanged=false.`,
	);
	} catch (e) {
	console.log(
	`Could not check PR changed files: ${e}. Defaulting to mcpJsonChanged=true (fail-closed).`,
	);
	mcpJsonChanged = true;

[ddworken]

+1.

[claude]

Bug: This signature change breaks the call site in base-action/src/index.ts

The standalone base-action/src/index.ts calls this function as:

await setupClaudeCodeSettings(
  process.env.INPUT_SETTINGS,
  undefined, // homeDir   <-- now maps to mcpJsonChanged, not homeDir
);

After inserting mcpJsonChanged as the second parameter, that call now passes undefined for mcpJsonChanged (happens to be falsy, so it works by accident) and never passes homeDir. The // homeDir comment is now incorrect.

This needs to be fixed in index.ts — at minimum update it to (process.env.INPUT_SETTINGS, false, undefined).

More broadly, consider using an options object instead of positional parameters to avoid this class of bug:

setupClaudeCodeSettings(options: {
  settingsInput?: string;
  mcpJsonChanged?: boolean;
  homeDir?: string;
})

This is especially important since base-action is published as a standalone package (@anthropic-ai/claude-code-base-action) and the CLAUDE.md warns "Don't break its public API."

[claude]

Security gap: User settings input can re-enable enableAllProjectMcpServers when .mcp.json changed

When mcpJsonChanged is true, this code skips setting enableAllProjectMcpServers = true. However, the merge on line 59 (settings = { ...settings, ...inputSettings }) already ran — so if the user's INPUT_SETTINGS includes enableAllProjectMcpServers: true, it's already in the settings object and the else branch doesn't remove it.

This means a workflow configured with enableAllProjectMcpServers: true in its settings input would override the protection. Consider explicitly deleting or setting it to false in the else branch:

Suggested change

	if (!mcpJsonChanged) {
	// Only auto-approve project MCP servers if .mcp.json hasn't changed in this PR.
	// If .mcp.json changed, the checked-out version may differ from the base branch.
	settings.enableAllProjectMcpServers = true;
	console.log(`Updated settings with enableAllProjectMcpServers: true`);
	} else {
	console.log(
	`Skipping enableAllProjectMcpServers=true because .mcp.json changed in this PR`,
	);
	}
	if (!mcpJsonChanged) {
	// Only auto-approve project MCP servers if .mcp.json hasn't changed in this PR.
	// If .mcp.json changed, the checked-out version may differ from the base branch.
	settings.enableAllProjectMcpServers = true;
	console.log(`Updated settings with enableAllProjectMcpServers: true`);
	} else {
	// Force-disable even if user input set it to true, since .mcp.json is untrusted.
	delete settings.enableAllProjectMcpServers;
	console.log(
	`Removing enableAllProjectMcpServers because .mcp.json changed in this PR`,
	);
	}

The test suite also doesn't cover the case where input sets enableAllProjectMcpServers: true with mcpJsonChanged: true.

[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.

[ddworken]

+1, this seems like a legitimate bug.

[ddworken]

+1.