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
This commit is contained in:
gemini-cli[bot]
2026-05-12 21:49:54 +00:00
parent c37b9113d7
commit bbfc33ea24
4 changed files with 88 additions and 31 deletions

View File

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

View File

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

View File

@@ -252,7 +252,9 @@ describe('DiscoveredMCPTool', () => {
mockToolSuccessResultObject,
);
expect(toolResult.llmContent).toEqual([
{ text: stringifiedResponseContent },
{
text: `<mcp_output tool="${serverToolName}">\n${stringifiedResponseContent}\n</mcp_output>`,
},
]);
expect(toolResult.returnDisplay).toBe(stringifiedResponseContent);
});
@@ -435,7 +437,9 @@ describe('DiscoveredMCPTool', () => {
mockToolSuccessResultObject,
);
expect(toolResult.llmContent).toEqual([
{ text: stringifiedResponseContent },
{
text: `<mcp_output tool="${serverToolName}">\n${stringifiedResponseContent}\n</mcp_output>`,
},
]);
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: `<mcp_output tool="${serverToolName}">\n${successMessage}\n</mcp_output>`,
},
]);
// 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: `<mcp_resource_output tool="${serverToolName}" uri="file:///path/to/text.txt">\nThis is the text content.\n</mcp_resource_output>`,
},
]);
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: `<mcp_output tool="${serverToolName}">\nFirst part.\n</mcp_output>`,
},
{
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: `<mcp_output tool="${serverToolName}">\nSecond part.\n</mcp_output>`,
},
]);
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: `<mcp_output tool="${serverToolName}">\nValid part.\n</mcp_output>`,
},
]);
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: `<mcp_output tool="${serverToolName}">\nHere is a resource.\n</mcp_output>`,
},
{
text: `[Tool '${serverToolName}' provided a resource link: My Resource at file:///path/to/resource]`,
},
{
text: `<mcp_resource_output tool="${serverToolName}" uri="file:///path/to/text.txt">\nEmbedded text content.\n</mcp_resource_output>`,
},
{ 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: `<mcp_output tool="${serverToolName}">\nSuccess\n</mcp_output>`,
},
]);
expect(result.returnDisplay).toBe('Success');
expect(mockCallTool).toHaveBeenCalledWith([
{ name: serverToolName, args: params },

View File

@@ -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, '&amp;')
.replace(/</g, '&lt;')
.replace(/>/g, '&gt;');
}
function escapeAttribute(text: string): string {
return escapeHtml(text).replace(/"/g, '&quot;').replace(/'/g, '&#39;');
}
function transformTextBlock(block: McpTextBlock, toolName: string): Part {
return {
text: `<mcp_output tool="${escapeAttribute(toolName)}">\n${escapeHtml(
block.text,
)}\n</mcp_output>`,
};
}
function transformImageAudioBlock(
@@ -476,7 +492,13 @@ function transformResourceBlock(
): Part | Part[] | null {
const resource = block.resource;
if (resource?.text) {
return { text: resource.text };
return {
text: `<mcp_resource_output tool="${escapeAttribute(
toolName,
)}" uri="${escapeAttribute(
resource.uri || 'unknown',
)}">\n${escapeHtml(resource.text)}\n</mcp_resource_output>`,
};
}
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;
}