From 19a4f34b1746f3921c8c97b63b85d7dfe72818ff Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Fri, 13 Feb 2026 22:41:07 +0000 Subject: [PATCH 1/3] Disable auto-approval of project MCP servers when .mcp.json changes in PR When a PR modifies .mcp.json, the checked-out working directory contains a version that may differ from the base branch. Blindly setting enableAllProjectMcpServers=true in that context auto-approves those MCP servers without the user having reviewed them. Fix: before calling setupClaudeCodeSettings, check whether .mcp.json changed in the triggering PR using the GitHub REST API (pulls.listFiles). If it did, pass mcpJsonChanged=true to suppress the enableAllProjectMcpServers=true override. When .mcp.json is unmodified (the common case), behaviour is unchanged. - Add mcpJsonChanged parameter to setupClaudeCodeSettings - Add PR file-list check in src/entrypoints/run.ts - Update tests: update existing override test, add two new mcpJsonChanged=true tests --- base-action/src/setup-claude-code-settings.ts | 14 ++++-- .../test/setup-claude-code-settings.test.ts | 50 +++++++++++++++---- src/entrypoints/run.ts | 30 ++++++++++- 3 files changed, 80 insertions(+), 14 deletions(-) diff --git a/base-action/src/setup-claude-code-settings.ts b/base-action/src/setup-claude-code-settings.ts index 0fe68414f..a5109ae17 100644 --- a/base-action/src/setup-claude-code-settings.ts +++ b/base-action/src/setup-claude-code-settings.ts @@ -4,6 +4,7 @@ import { readFile } from "fs/promises"; export async function setupClaudeCodeSettings( settingsInput?: string, + mcpJsonChanged = false, homeDir?: string, ) { const home = homeDir ?? homedir(); @@ -59,9 +60,16 @@ export async function setupClaudeCodeSettings( console.log(`Merged settings with input settings`); } - // Always set enableAllProjectMcpServers to true - settings.enableAllProjectMcpServers = true; - console.log(`Updated settings with enableAllProjectMcpServers: true`); + 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`, + ); + } await $`echo ${JSON.stringify(settings, null, 2)} > ${settingsPath}`.quiet(); console.log(`Settings saved successfully`); diff --git a/base-action/test/setup-claude-code-settings.test.ts b/base-action/test/setup-claude-code-settings.test.ts index defe25149..1438931be 100644 --- a/base-action/test/setup-claude-code-settings.test.ts +++ b/base-action/test/setup-claude-code-settings.test.ts @@ -28,7 +28,7 @@ describe("setupClaudeCodeSettings", () => { }); test("should always set enableAllProjectMcpServers to true when no input", async () => { - await setupClaudeCodeSettings(undefined, testHomeDir); + await setupClaudeCodeSettings(undefined, false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -42,7 +42,7 @@ describe("setupClaudeCodeSettings", () => { env: { API_KEY: "test-key" }, }); - await setupClaudeCodeSettings(inputSettings, testHomeDir); + await setupClaudeCodeSettings(inputSettings, false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -69,7 +69,7 @@ describe("setupClaudeCodeSettings", () => { await writeFile(testSettingsPath, JSON.stringify(testSettings, null, 2)); - await setupClaudeCodeSettings(testSettingsPath, testHomeDir); + await setupClaudeCodeSettings(testSettingsPath, false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -79,13 +79,13 @@ describe("setupClaudeCodeSettings", () => { expect(settings.permissions).toEqual(testSettings.permissions); }); - test("should override enableAllProjectMcpServers even if false in input", async () => { + test("should override enableAllProjectMcpServers even if false in input when mcpJsonChanged is false", async () => { const inputSettings = JSON.stringify({ enableAllProjectMcpServers: false, model: "test-model", }); - await setupClaudeCodeSettings(inputSettings, testHomeDir); + await setupClaudeCodeSettings(inputSettings, false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -94,20 +94,49 @@ describe("setupClaudeCodeSettings", () => { expect(settings.model).toBe("test-model"); }); + test("should not set enableAllProjectMcpServers to true when mcpJsonChanged is true", async () => { + const inputSettings = JSON.stringify({ + model: "test-model", + }); + + await setupClaudeCodeSettings(inputSettings, true, testHomeDir); + + const settingsContent = await readFile(settingsPath, "utf-8"); + const settings = JSON.parse(settingsContent); + + expect(settings.enableAllProjectMcpServers).toBeUndefined(); + expect(settings.model).toBe("test-model"); + }); + + test("should not override enableAllProjectMcpServers when mcpJsonChanged is true and input sets it false", async () => { + const inputSettings = JSON.stringify({ + enableAllProjectMcpServers: false, + model: "test-model", + }); + + await setupClaudeCodeSettings(inputSettings, true, testHomeDir); + + const settingsContent = await readFile(settingsPath, "utf-8"); + const settings = JSON.parse(settingsContent); + + expect(settings.enableAllProjectMcpServers).toBe(false); + expect(settings.model).toBe("test-model"); + }); + test("should throw error for invalid JSON string", async () => { expect(() => - setupClaudeCodeSettings("{ invalid json", testHomeDir), + setupClaudeCodeSettings("{ invalid json", false, testHomeDir), ).toThrow(); }); test("should throw error for non-existent file path", async () => { expect(() => - setupClaudeCodeSettings("/non/existent/file.json", testHomeDir), + setupClaudeCodeSettings("/non/existent/file.json", false, testHomeDir), ).toThrow(); }); test("should handle empty string input", async () => { - await setupClaudeCodeSettings("", testHomeDir); + await setupClaudeCodeSettings("", false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -116,7 +145,7 @@ describe("setupClaudeCodeSettings", () => { }); test("should handle whitespace-only input", async () => { - await setupClaudeCodeSettings(" \n\t ", testHomeDir); + await setupClaudeCodeSettings(" \n\t ", false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); @@ -128,6 +157,7 @@ describe("setupClaudeCodeSettings", () => { // First, create some existing settings await setupClaudeCodeSettings( JSON.stringify({ existingKey: "existingValue" }), + false, testHomeDir, ); @@ -137,7 +167,7 @@ describe("setupClaudeCodeSettings", () => { model: "claude-opus-4-1-20250805", }); - await setupClaudeCodeSettings(newSettings, testHomeDir); + await setupClaudeCodeSettings(newSettings, false, testHomeDir); const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); diff --git a/src/entrypoints/run.ts b/src/entrypoints/run.ts index a5de5131b..c9a62e376 100644 --- a/src/entrypoints/run.ts +++ b/src/entrypoints/run.ts @@ -217,7 +217,35 @@ async function run() { validateEnvironmentVariables(); - await setupClaudeCodeSettings(process.env.INPUT_SETTINGS); + // Check if .mcp.json changed in this PR to guard against RCE via malicious MCP servers. + // If .mcp.json was modified, skip auto-approving project MCP servers so attacker-injected + // servers are not trusted automatically. + let mcpJsonChanged = false; + if (isEntityContext(context) && context.isPR) { + try { + const { data: changedFiles } = await octokit.rest.pulls.listFiles({ + owner: context.repository.owner, + repo: context.repository.repo, + pull_number: context.entityNumber, + per_page: 100, + }); + mcpJsonChanged = changedFiles.some( + (f) => + f.filename === ".mcp.json" || f.filename.endsWith("/.mcp.json"), + ); + if (mcpJsonChanged) { + console.log( + ".mcp.json changed in this PR — disabling auto-approval of project MCP servers", + ); + } + } catch (e) { + console.log( + `Could not check PR changed files: ${e}. Defaulting to mcpJsonChanged=false.`, + ); + } + } + + await setupClaudeCodeSettings(process.env.INPUT_SETTINGS, mcpJsonChanged); await installPlugins( process.env.INPUT_PLUGIN_MARKETPLACES, From f4ac33b9a4a7aa4e31e02a9786330244751860ef Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Sun, 15 Feb 2026 12:44:24 -0800 Subject: [PATCH 2/3] Fix pagination bypass and fail-open bugs in .mcp.json change detection - Use octokit.rest.paginate() to fetch all pages of PR changed files, preventing attackers from padding PRs with 100+ files to push .mcp.json off the first page - Change catch block to fail closed (mcpJsonChanged=true) so MCP servers are not auto-approved when the API call fails --- src/entrypoints/run.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/entrypoints/run.ts b/src/entrypoints/run.ts index c9a62e376..9291c28cc 100644 --- a/src/entrypoints/run.ts +++ b/src/entrypoints/run.ts @@ -223,12 +223,15 @@ async function run() { let mcpJsonChanged = false; if (isEntityContext(context) && context.isPR) { try { - 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.rest.paginate( + octokit.rest.pulls.listFiles, + { + owner: context.repository.owner, + repo: context.repository.repo, + pull_number: context.entityNumber, + per_page: 100, + }, + ); mcpJsonChanged = changedFiles.some( (f) => f.filename === ".mcp.json" || f.filename.endsWith("/.mcp.json"), @@ -240,8 +243,9 @@ async function run() { } } catch (e) { console.log( - `Could not check PR changed files: ${e}. Defaulting to mcpJsonChanged=false.`, + `Could not check PR changed files: ${e}. Defaulting to mcpJsonChanged=true (fail-closed).`, ); + mcpJsonChanged = true; } } From 72a5802bde9d93e9d1e0473029b5f3476e167ffe Mon Sep 17 00:00:00 2001 From: Kashyap Murali Date: Tue, 17 Mar 2026 12:24:27 -0700 Subject: [PATCH 3/3] Close remaining gaps in .mcp.json change protection When mcpJsonChanged is true, delete enableAllProjectMcpServers from the merged settings object so user-provided INPUT_SETTINGS cannot re-enable it and bypass the check. Also fix the stale positional arg in the standalone base-action call site and add test coverage for the bypass case. --- base-action/src/index.ts | 2 +- base-action/src/setup-claude-code-settings.ts | 5 ++++- .../test/setup-claude-code-settings.test.ts | 19 +++++++++++++++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/base-action/src/index.ts b/base-action/src/index.ts index 970e79d1c..66ac61530 100644 --- a/base-action/src/index.ts +++ b/base-action/src/index.ts @@ -13,7 +13,7 @@ async function run() { await setupClaudeCodeSettings( process.env.INPUT_SETTINGS, - undefined, // homeDir + false, // mcpJsonChanged — standalone base-action has no PR context to check ); // Install Claude Code plugins if specified diff --git a/base-action/src/setup-claude-code-settings.ts b/base-action/src/setup-claude-code-settings.ts index a5109ae17..cdcaa79ae 100644 --- a/base-action/src/setup-claude-code-settings.ts +++ b/base-action/src/setup-claude-code-settings.ts @@ -66,8 +66,11 @@ export async function setupClaudeCodeSettings( settings.enableAllProjectMcpServers = true; console.log(`Updated settings with enableAllProjectMcpServers: true`); } else { + // .mcp.json is untrusted in this PR — strip any enableAllProjectMcpServers + // that came from user input so the protection cannot be bypassed. + delete settings.enableAllProjectMcpServers; console.log( - `Skipping enableAllProjectMcpServers=true because .mcp.json changed in this PR`, + `Removing enableAllProjectMcpServers because .mcp.json changed in this PR`, ); } diff --git a/base-action/test/setup-claude-code-settings.test.ts b/base-action/test/setup-claude-code-settings.test.ts index 1438931be..eb4c5393b 100644 --- a/base-action/test/setup-claude-code-settings.test.ts +++ b/base-action/test/setup-claude-code-settings.test.ts @@ -108,7 +108,7 @@ describe("setupClaudeCodeSettings", () => { expect(settings.model).toBe("test-model"); }); - test("should not override enableAllProjectMcpServers when mcpJsonChanged is true and input sets it false", async () => { + test("should remove enableAllProjectMcpServers when mcpJsonChanged is true and input sets it false", async () => { const inputSettings = JSON.stringify({ enableAllProjectMcpServers: false, model: "test-model", @@ -119,7 +119,22 @@ describe("setupClaudeCodeSettings", () => { const settingsContent = await readFile(settingsPath, "utf-8"); const settings = JSON.parse(settingsContent); - expect(settings.enableAllProjectMcpServers).toBe(false); + expect(settings.enableAllProjectMcpServers).toBeUndefined(); + expect(settings.model).toBe("test-model"); + }); + + test("should remove enableAllProjectMcpServers when mcpJsonChanged is true even if input sets it true", async () => { + const inputSettings = JSON.stringify({ + enableAllProjectMcpServers: true, + model: "test-model", + }); + + await setupClaudeCodeSettings(inputSettings, true, testHomeDir); + + const settingsContent = await readFile(settingsPath, "utf-8"); + const settings = JSON.parse(settingsContent); + + expect(settings.enableAllProjectMcpServers).toBeUndefined(); expect(settings.model).toBe("test-model"); });