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 0fe68414f..cdcaa79ae 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,19 @@ 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 { + // .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( + `Removing enableAllProjectMcpServers 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..eb4c5393b 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,64 @@ 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 remove 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).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"); + }); + 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 +160,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 +172,7 @@ describe("setupClaudeCodeSettings", () => { // First, create some existing settings await setupClaudeCodeSettings( JSON.stringify({ existingKey: "existingValue" }), + false, testHomeDir, ); @@ -137,7 +182,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..9291c28cc 100644 --- a/src/entrypoints/run.ts +++ b/src/entrypoints/run.ts @@ -217,7 +217,39 @@ 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 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"), + ); + 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=true (fail-closed).`, + ); + mcpJsonChanged = true; + } + } + + await setupClaudeCodeSettings(process.env.INPUT_SETTINGS, mcpJsonChanged); await installPlugins( process.env.INPUT_PLUGIN_MARKETPLACES,