From bbfc33ea24cf2ecb9f45a9ff92fa158070f855c0 Mon Sep 17 00:00:00 2001 From: "gemini-cli[bot]" Date: Tue, 12 May 2026 21:49:54 +0000 Subject: [PATCH] fix(security): address MCP security findings (MCPSafe Grade F) This PR addresses high and medium severity security findings related to MCP server integration, as reported by MCPSafe. ### Changes: 1. **Shell Heuristics Enforcement**: Updated `PolicyEngine` to apply shell heuristics (e.g., redirection detection) to any tool containing a `command` argument, not just those explicitly named in `SHELL_TOOL_NAMES`. This prevents security bypasses where MCP tools executing shell commands could skip safety checks. 2. **MCP Output Sanitization**: Implemented delimiters and HTML escaping for MCP tool text and resource outputs. This prevents prompt injection attacks where malicious tool output could be mistaken for system instructions by the LLM. 3. **Default Folder Trust**: Enabled folder trust by default in the CLI configuration. This ensures that the CLI verifies workspace trust before executing sensitive operations like loading local stdio MCP servers from project configuration. 4. **Type Safety**: Updated `McpResourceBlock` type to include the `uri` property, aligning with the MCP specification and fixing a TypeScript compilation error. These changes significantly harden the gemini-cli against common attack vectors in the MCP ecosystem. cc @mcpsafe-gh for visibility on the fixes. cc @google-gemini-mcp-experts Labels: bot-fix, area/security, kind/bug --- packages/cli/src/config/config.ts | 2 +- packages/core/src/policy/policy-engine.ts | 16 +++--- packages/core/src/tools/mcp-tool.test.ts | 60 ++++++++++++++++------- packages/core/src/tools/mcp-tool.ts | 41 +++++++++++++--- 4 files changed, 88 insertions(+), 31 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index e7b332711d..ccd4f94857 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -605,7 +605,7 @@ export async function loadCliConfig( process.env['GEMINI_CLI_INTEGRATION_TEST'] === 'true' || process.env['VITEST'] === 'true' ? false - : (settings.security?.folderTrust?.enabled ?? false); + : (settings.security?.folderTrust?.enabled ?? true); const trustedFolder = isWorkspaceTrusted(settings, cwd, { prompt: argv.prompt, diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index a3b9aa0992..63dcb1a9c5 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -543,13 +543,17 @@ export class PolicyEngine { let shellDirPath: string | undefined; const toolName = toolCall.name; + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const args = toolCall.args as { command?: unknown; dir_path?: unknown }; - if (toolName && SHELL_TOOL_NAMES.includes(toolName)) { + if ( + toolName && + (SHELL_TOOL_NAMES.includes(toolName) || + (typeof args?.command === 'string' && args.command.trim().length > 0)) + ) { isShellCommand = true; - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const args = toolCall.args as { command?: string; dir_path?: string }; - command = args?.command; - shellDirPath = args?.dir_path; + command = args.command as string; + shellDirPath = typeof args.dir_path === 'string' ? args.dir_path : undefined; } // Find the first matching rule (already sorted by priority) @@ -645,7 +649,7 @@ export class PolicyEngine { debugLogger.debug( `[PolicyEngine.check] NO MATCH - using default decision: ${this.defaultDecision}`, ); - if (toolName && SHELL_TOOL_NAMES.includes(toolName)) { + if (isShellCommand && toolName) { let heuristicDecision = this.defaultDecision; if (!skipHeuristics && command) { heuristicDecision = await this.applyShellHeuristics( diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index 0a0b85d33f..cf13a973a7 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -252,7 +252,9 @@ describe('DiscoveredMCPTool', () => { mockToolSuccessResultObject, ); expect(toolResult.llmContent).toEqual([ - { text: stringifiedResponseContent }, + { + text: `\n${stringifiedResponseContent}\n`, + }, ]); expect(toolResult.returnDisplay).toBe(stringifiedResponseContent); }); @@ -435,7 +437,9 @@ describe('DiscoveredMCPTool', () => { mockToolSuccessResultObject, ); expect(toolResult.llmContent).toEqual([ - { text: stringifiedResponseContent }, + { + text: `\n${stringifiedResponseContent}\n`, + }, ]); expect(toolResult.returnDisplay).toBe(stringifiedResponseContent); }, @@ -456,7 +460,11 @@ describe('DiscoveredMCPTool', () => { abortSignal: new AbortController().signal, }); // 1. Assert that the llmContent sent to the scheduler is a clean Part array. - expect(toolResult.llmContent).toEqual([{ text: successMessage }]); + expect(toolResult.llmContent).toEqual([ + { + text: `\n${successMessage}\n`, + }, + ]); // 2. Assert that the display output is the simple text message. expect(toolResult.returnDisplay).toBe(successMessage); @@ -482,7 +490,7 @@ describe('DiscoveredMCPTool', () => { ); const invocation = tool.build(params); - const toolResult = await invocation.execute({ + const toolResult: ToolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ @@ -515,12 +523,12 @@ describe('DiscoveredMCPTool', () => { ); const invocation = tool.build(params); - const toolResult = await invocation.execute({ + const toolResult: ToolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ { - text: 'Resource Link: My Resource at file:///path/to/thing', + text: `[Tool '${serverToolName}' provided a resource link: My Resource at file:///path/to/thing]`, }, ]); expect(toolResult.returnDisplay).toBe( @@ -550,7 +558,9 @@ describe('DiscoveredMCPTool', () => { abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ - { text: 'This is the text content.' }, + { + text: `\nThis is the text content.\n`, + }, ]); expect(toolResult.returnDisplay).toBe('This is the text content.'); }); @@ -573,7 +583,7 @@ describe('DiscoveredMCPTool', () => { ); const invocation = tool.build(params); - const toolResult = await invocation.execute({ + const toolResult: ToolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ @@ -609,11 +619,13 @@ describe('DiscoveredMCPTool', () => { ); const invocation = tool.build(params); - const toolResult = await invocation.execute({ + const toolResult: ToolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ - { text: 'First part.' }, + { + text: `\nFirst part.\n`, + }, { text: `[Tool '${serverToolName}' provided the following image data with mime-type: image/jpeg]`, }, @@ -623,7 +635,9 @@ describe('DiscoveredMCPTool', () => { data: 'BASE64_IMAGE_DATA', }, }, - { text: 'Second part.' }, + { + text: `\nSecond part.\n`, + }, ]); expect(toolResult.returnDisplay).toBe( 'First part.\n[Image: image/jpeg]\nSecond part.', @@ -645,7 +659,11 @@ describe('DiscoveredMCPTool', () => { const toolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); - expect(toolResult.llmContent).toEqual([{ text: 'Valid part.' }]); + expect(toolResult.llmContent).toEqual([ + { + text: `\nValid part.\n`, + }, + ]); expect(toolResult.returnDisplay).toBe( 'Valid part.\n[Unknown content type: future_block]', ); @@ -681,15 +699,19 @@ describe('DiscoveredMCPTool', () => { ); const invocation = tool.build(params); - const toolResult = await invocation.execute({ + const toolResult: ToolResult = await invocation.execute({ abortSignal: new AbortController().signal, }); expect(toolResult.llmContent).toEqual([ - { text: 'Here is a resource.' }, { - text: 'Resource Link: My Resource at file:///path/to/resource', + text: `\nHere is a resource.\n`, + }, + { + text: `[Tool '${serverToolName}' provided a resource link: My Resource at file:///path/to/resource]`, + }, + { + text: `\nEmbedded text content.\n`, }, - { text: 'Embedded text content.' }, { text: `[Tool '${serverToolName}' provided the following image data with mime-type: image/jpeg]`, }, @@ -771,7 +793,11 @@ describe('DiscoveredMCPTool', () => { abortSignal: controller.signal, }); - expect(result.llmContent).toEqual([{ text: 'Success' }]); + expect(result.llmContent).toEqual([ + { + text: `\nSuccess\n`, + }, + ]); expect(result.returnDisplay).toBe('Success'); expect(mockCallTool).toHaveBeenCalledWith([ { name: serverToolName, args: params }, diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index caaba717d1..be2dbbc858 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -132,6 +132,7 @@ type McpMediaBlock = { type McpResourceBlock = { type: 'resource'; resource: { + uri?: string; text?: string; blob?: string; mimeType?: string; @@ -447,8 +448,23 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool< } } -function transformTextBlock(block: McpTextBlock): Part { - return { text: block.text }; +function escapeHtml(text: string): string { + return text + .replace(/&/g, '&') + .replace(//g, '>'); +} + +function escapeAttribute(text: string): string { + return escapeHtml(text).replace(/"/g, '"').replace(/'/g, '''); +} + +function transformTextBlock(block: McpTextBlock, toolName: string): Part { + return { + text: `\n${escapeHtml( + block.text, + )}\n`, + }; } function transformImageAudioBlock( @@ -476,7 +492,13 @@ function transformResourceBlock( ): Part | Part[] | null { const resource = block.resource; if (resource?.text) { - return { text: resource.text }; + return { + text: `\n${escapeHtml(resource.text)}\n`, + }; } if (resource?.blob) { const mimeType = resource.mimeType || 'application/octet-stream'; @@ -495,9 +517,14 @@ function transformResourceBlock( return null; } -function transformResourceLinkBlock(block: McpResourceLinkBlock): Part { +function transformResourceLinkBlock( + block: McpResourceLinkBlock, + toolName: string, +): Part { return { - text: `Resource Link: ${block.title || block.name} at ${block.uri}`, + text: `[Tool '${toolName}' provided a resource link: ${ + block.title || block.name + } at ${block.uri}]`, }; } @@ -521,14 +548,14 @@ function transformMcpContentToParts(sdkResponse: Part[]): Part[] { (block: McpContentBlock): Part | Part[] | null => { switch (block.type) { case 'text': - return transformTextBlock(block); + return transformTextBlock(block, toolName); case 'image': case 'audio': return transformImageAudioBlock(block, toolName); case 'resource': return transformResourceBlock(block, toolName); case 'resource_link': - return transformResourceLinkBlock(block); + return transformResourceLinkBlock(block, toolName); default: return null; }