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
This commit is contained in:
parent
f5088835af
commit
19a4f34b17
@ -4,6 +4,7 @@ import { readFile } from "fs/promises";
|
|||||||
|
|
||||||
export async function setupClaudeCodeSettings(
|
export async function setupClaudeCodeSettings(
|
||||||
settingsInput?: string,
|
settingsInput?: string,
|
||||||
|
mcpJsonChanged = false,
|
||||||
homeDir?: string,
|
homeDir?: string,
|
||||||
) {
|
) {
|
||||||
const home = homeDir ?? homedir();
|
const home = homeDir ?? homedir();
|
||||||
@ -59,9 +60,16 @@ export async function setupClaudeCodeSettings(
|
|||||||
console.log(`Merged settings with input settings`);
|
console.log(`Merged settings with input settings`);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Always set enableAllProjectMcpServers to 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;
|
settings.enableAllProjectMcpServers = true;
|
||||||
console.log(`Updated settings with 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();
|
await $`echo ${JSON.stringify(settings, null, 2)} > ${settingsPath}`.quiet();
|
||||||
console.log(`Settings saved successfully`);
|
console.log(`Settings saved successfully`);
|
||||||
|
|||||||
@ -28,7 +28,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test("should always set enableAllProjectMcpServers to true when no input", async () => {
|
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 settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -42,7 +42,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
env: { API_KEY: "test-key" },
|
env: { API_KEY: "test-key" },
|
||||||
});
|
});
|
||||||
|
|
||||||
await setupClaudeCodeSettings(inputSettings, testHomeDir);
|
await setupClaudeCodeSettings(inputSettings, false, testHomeDir);
|
||||||
|
|
||||||
const settingsContent = await readFile(settingsPath, "utf-8");
|
const settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -69,7 +69,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
|
|
||||||
await writeFile(testSettingsPath, JSON.stringify(testSettings, null, 2));
|
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 settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -79,13 +79,13 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
expect(settings.permissions).toEqual(testSettings.permissions);
|
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({
|
const inputSettings = JSON.stringify({
|
||||||
enableAllProjectMcpServers: false,
|
enableAllProjectMcpServers: false,
|
||||||
model: "test-model",
|
model: "test-model",
|
||||||
});
|
});
|
||||||
|
|
||||||
await setupClaudeCodeSettings(inputSettings, testHomeDir);
|
await setupClaudeCodeSettings(inputSettings, false, testHomeDir);
|
||||||
|
|
||||||
const settingsContent = await readFile(settingsPath, "utf-8");
|
const settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -94,20 +94,49 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
expect(settings.model).toBe("test-model");
|
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 () => {
|
test("should throw error for invalid JSON string", async () => {
|
||||||
expect(() =>
|
expect(() =>
|
||||||
setupClaudeCodeSettings("{ invalid json", testHomeDir),
|
setupClaudeCodeSettings("{ invalid json", false, testHomeDir),
|
||||||
).toThrow();
|
).toThrow();
|
||||||
});
|
});
|
||||||
|
|
||||||
test("should throw error for non-existent file path", async () => {
|
test("should throw error for non-existent file path", async () => {
|
||||||
expect(() =>
|
expect(() =>
|
||||||
setupClaudeCodeSettings("/non/existent/file.json", testHomeDir),
|
setupClaudeCodeSettings("/non/existent/file.json", false, testHomeDir),
|
||||||
).toThrow();
|
).toThrow();
|
||||||
});
|
});
|
||||||
|
|
||||||
test("should handle empty string input", async () => {
|
test("should handle empty string input", async () => {
|
||||||
await setupClaudeCodeSettings("", testHomeDir);
|
await setupClaudeCodeSettings("", false, testHomeDir);
|
||||||
|
|
||||||
const settingsContent = await readFile(settingsPath, "utf-8");
|
const settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -116,7 +145,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test("should handle whitespace-only input", async () => {
|
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 settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
@ -128,6 +157,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
// First, create some existing settings
|
// First, create some existing settings
|
||||||
await setupClaudeCodeSettings(
|
await setupClaudeCodeSettings(
|
||||||
JSON.stringify({ existingKey: "existingValue" }),
|
JSON.stringify({ existingKey: "existingValue" }),
|
||||||
|
false,
|
||||||
testHomeDir,
|
testHomeDir,
|
||||||
);
|
);
|
||||||
|
|
||||||
@ -137,7 +167,7 @@ describe("setupClaudeCodeSettings", () => {
|
|||||||
model: "claude-opus-4-1-20250805",
|
model: "claude-opus-4-1-20250805",
|
||||||
});
|
});
|
||||||
|
|
||||||
await setupClaudeCodeSettings(newSettings, testHomeDir);
|
await setupClaudeCodeSettings(newSettings, false, testHomeDir);
|
||||||
|
|
||||||
const settingsContent = await readFile(settingsPath, "utf-8");
|
const settingsContent = await readFile(settingsPath, "utf-8");
|
||||||
const settings = JSON.parse(settingsContent);
|
const settings = JSON.parse(settingsContent);
|
||||||
|
|||||||
@ -217,7 +217,35 @@ async function run() {
|
|||||||
|
|
||||||
validateEnvironmentVariables();
|
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(
|
await installPlugins(
|
||||||
process.env.INPUT_PLUGIN_MARKETPLACES,
|
process.env.INPUT_PLUGIN_MARKETPLACES,
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user