Compare commits

...

3 Commits

Author SHA1 Message Date
Kashyap Murali
72a5802bde
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.
2026-03-17 12:24:27 -07:00
Ashwin Bhat
f4ac33b9a4
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
2026-02-15 12:44:24 -08:00
Ashwin Bhat
19a4f34b17
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
2026-02-15 12:39:01 -08:00
4 changed files with 103 additions and 15 deletions

View File

@ -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

View File

@ -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`);

View File

@ -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);

View File

@ -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,