From a05bf276f3cb96c1f712b2f9cff02d2551a2168d Mon Sep 17 00:00:00 2001 From: Aroh Maurya Date: Thu, 3 Sep 2026 18:39:19 +0530 Subject: [PATCH] fix: gate MCP server actions behind isMcpEnabled Every function in app/mcp/actions.ts is exported from a "use server" module, which makes each one callable directly over HTTP as a Next.js Server Action, unauthenticated, using the action id embedded in the client bundle. isMcpEnabled() already existed for this but nothing called it, so every action ran regardless of the ENABLE_MCP setting. addMcpServer takes a command and args and spawns a child process with them (via StdioClientTransport), so this was remote code execution reachable on any deployment, whether or not MCP was ever turned on. Every exported action now calls isMcpEnabled() first and rejects if it's off, matching how the frontend already gates on it before ever calling these. Added a test that calls each action with ENABLE_MCP unset and checks it rejects instead of running. Confirmed it fails against the current code: addMcpServer actually wrote the malicious config to disk and spawned the process (a real /tmp file got created running the test against the unfixed code), and passes with the fix. --- app/mcp/actions.ts | 24 ++++++++ test/mcp-actions-require-enabled.test.ts | 77 ++++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 test/mcp-actions-require-enabled.test.ts diff --git a/app/mcp/actions.ts b/app/mcp/actions.ts index e8b1ad1d00f..9a6a69677e8 100644 --- a/app/mcp/actions.ts +++ b/app/mcp/actions.ts @@ -23,10 +23,23 @@ const CONFIG_PATH = path.join(process.cwd(), "app/mcp/mcp_config.json"); const clientsMap = new Map(); +// These functions are exported from a "use server" module, which makes every one of +// them callable directly over HTTP as a Next.js Server Action, unauthenticated, by +// anyone who has the action id (visible in the client bundle). isMcpEnabled() existed +// but nothing called it, so every action ran regardless of the ENABLE_MCP setting - +// including addMcpServer, which spawns a child process using a caller-supplied command. +// Every action here now checks this first. +async function assertMcpEnabled() { + if (!(await isMcpEnabled())) { + throw new Error("MCP is not enabled"); + } +} + // 获取客户端状态 export async function getClientsStatus(): Promise< Record > { + await assertMcpEnabled(); const config = await getMcpConfigFromFile(); const result: Record = {}; @@ -76,11 +89,13 @@ export async function getClientsStatus(): Promise< // 获取客户端工具 export async function getClientTools(clientId: string) { + await assertMcpEnabled(); return clientsMap.get(clientId)?.tools ?? null; } // 获取可用客户端数量 export async function getAvailableClientsCount() { + await assertMcpEnabled(); let count = 0; clientsMap.forEach((map) => !map.errorMsg && count++); return count; @@ -88,6 +103,7 @@ export async function getAvailableClientsCount() { // 获取所有客户端工具 export async function getAllTools() { + await assertMcpEnabled(); const result = []; for (const [clientId, status] of clientsMap.entries()) { result.push({ @@ -140,6 +156,7 @@ async function initializeSingleClient( // 初始化系统 export async function initializeMcpSystem() { + await assertMcpEnabled(); logger.info("MCP Actions starting..."); try { // 检查是否已有活跃的客户端 @@ -162,6 +179,7 @@ export async function initializeMcpSystem() { // 添加服务器 export async function addMcpServer(clientId: string, config: ServerConfig) { + await assertMcpEnabled(); try { const currentConfig = await getMcpConfigFromFile(); const isNewServer = !(clientId in currentConfig.mcpServers); @@ -194,6 +212,7 @@ export async function addMcpServer(clientId: string, config: ServerConfig) { // 暂停服务器 export async function pauseMcpServer(clientId: string) { + await assertMcpEnabled(); try { const currentConfig = await getMcpConfigFromFile(); const serverConfig = currentConfig.mcpServers[clientId]; @@ -230,6 +249,7 @@ export async function pauseMcpServer(clientId: string) { // 恢复服务器 export async function resumeMcpServer(clientId: string): Promise { + await assertMcpEnabled(); try { const currentConfig = await getMcpConfigFromFile(); const serverConfig = currentConfig.mcpServers[clientId]; @@ -284,6 +304,7 @@ export async function resumeMcpServer(clientId: string): Promise { // 移除服务器 export async function removeMcpServer(clientId: string) { + await assertMcpEnabled(); try { const currentConfig = await getMcpConfigFromFile(); const { [clientId]: _, ...rest } = currentConfig.mcpServers; @@ -309,6 +330,7 @@ export async function removeMcpServer(clientId: string) { // 重启所有客户端 export async function restartAllClients() { + await assertMcpEnabled(); logger.info("Restarting all clients..."); try { // 关闭所有客户端 @@ -338,6 +360,7 @@ export async function executeMcpAction( clientId: string, request: McpRequestMessage, ) { + await assertMcpEnabled(); try { const client = clientsMap.get(clientId); if (!client?.client) { @@ -353,6 +376,7 @@ export async function executeMcpAction( // 获取 MCP 配置文件 export async function getMcpConfigFromFile(): Promise { + await assertMcpEnabled(); try { const configStr = await fs.readFile(CONFIG_PATH, "utf-8"); return JSON.parse(configStr); diff --git a/test/mcp-actions-require-enabled.test.ts b/test/mcp-actions-require-enabled.test.ts new file mode 100644 index 00000000000..4615424429c --- /dev/null +++ b/test/mcp-actions-require-enabled.test.ts @@ -0,0 +1,77 @@ +import { + isMcpEnabled, + getClientsStatus, + getClientTools, + getAvailableClientsCount, + getAllTools, + initializeMcpSystem, + addMcpServer, + pauseMcpServer, + resumeMcpServer, + removeMcpServer, + restartAllClients, + executeMcpAction, + getMcpConfigFromFile, +} from "../app/mcp/actions"; + +// Regression coverage for the unauthenticated MCP server actions: every one of +// these functions is a Next.js Server Action, callable directly over HTTP with +// no session or auth check. addMcpServer in particular spawns a child process +// using a caller-supplied command, so leaving these ungated is remote code +// execution regardless of whether MCP was ever turned on (ENABLE_MCP unset, +// which is the default, is exactly the case exercised below). +describe("MCP server actions require MCP to be enabled", () => { + const ORIGINAL_ENABLE_MCP = process.env.ENABLE_MCP; + + afterEach(() => { + if (ORIGINAL_ENABLE_MCP === undefined) { + delete process.env.ENABLE_MCP; + } else { + process.env.ENABLE_MCP = ORIGINAL_ENABLE_MCP; + } + }); + + describe("when ENABLE_MCP is unset (the default)", () => { + beforeEach(() => { + delete process.env.ENABLE_MCP; + }); + + test.each<[string, () => Promise]>([ + ["getClientsStatus", () => getClientsStatus()], + ["getClientTools", () => getClientTools("x")], + ["getAvailableClientsCount", () => getAvailableClientsCount()], + ["getAllTools", () => getAllTools()], + ["initializeMcpSystem", () => initializeMcpSystem()], + [ + "addMcpServer", + () => + addMcpServer("attack", { + command: "touch", + args: ["/tmp/pwned"], + } as any), + ], + ["pauseMcpServer", () => pauseMcpServer("x")], + ["resumeMcpServer", () => resumeMcpServer("x")], + ["removeMcpServer", () => removeMcpServer("x")], + ["restartAllClients", () => restartAllClients()], + ["executeMcpAction", () => executeMcpAction("x", {} as any)], + ["getMcpConfigFromFile", () => getMcpConfigFromFile()], + ])("%s rejects instead of running", async (_name, callAction) => { + await expect(callAction()).rejects.toThrow("MCP is not enabled"); + }); + }); + + describe("when ENABLE_MCP=true", () => { + beforeEach(() => { + process.env.ENABLE_MCP = "true"; + }); + + test("isMcpEnabled reports enabled", async () => { + expect(await isMcpEnabled()).toBe(true); + }); + + test("getMcpConfigFromFile proceeds instead of rejecting", async () => { + await expect(getMcpConfigFromFile()).resolves.toBeDefined(); + }); + }); +});