diff --git a/docs/reference/policy-engine.md b/docs/reference/policy-engine.md index 65ab541749..bcf892262a 100644 --- a/docs/reference/policy-engine.md +++ b/docs/reference/policy-engine.md @@ -439,8 +439,7 @@ The Gemini CLI ships with a set of default policies to provide a safe out-of-the-box experience. - **Read-only tools** (like `read_file`, `glob`) are generally **allowed**. -- **MCP Read-only tools**: MCP tools that explicitly declare themselves as - read-only via the `readOnlyHint` annotation are automatically allowed. +- **MCP Read-only tools**: MCP tools that explicitly declare themselves as read-only via the `readOnlyHint` annotation are automatically allowed, but **only if tool sandboxing is enabled**. If sandboxing is disabled, they default to `ask_user`. - **Agent delegation** defaults to **`ask_user`** to ensure remote agents can prompt for confirmation, but local sub-agent actions are executed silently and checked individually. diff --git a/docs/tools/mcp-server.md b/docs/tools/mcp-server.md index 4f29f2c6d7..92f46f12a5 100644 --- a/docs/tools/mcp-server.md +++ b/docs/tools/mcp-server.md @@ -653,10 +653,7 @@ When confirmation is required, users can choose: #### Read-only bypass -If an MCP tool provides a `readOnlyHint` in its metadata (annotations), the -Gemini CLI will automatically execute it without prompting for confirmation, -regardless of the configured approval mode. This ensures a seamless experience -for safe, data-fetching operations. +If an MCP tool provides a `readOnlyHint` in its metadata (annotations) AND tool sandboxing is enabled, the Gemini CLI will automatically execute it without prompting for confirmation, regardless of the configured approval mode. This ensures a seamless experience for safe, data-fetching operations while maintaining system security. ### 3. Execution diff --git a/packages/cli/src/commands/mcp/add.test.ts b/packages/cli/src/commands/mcp/add.test.ts index e6f955b680..a69978af3c 100644 --- a/packages/cli/src/commands/mcp/add.test.ts +++ b/packages/cli/src/commands/mcp/add.test.ts @@ -27,6 +27,33 @@ vi.mock('fs/promises', () => ({ writeFile: vi.fn(), })); +vi.mock('@modelcontextprotocol/sdk/client/index.js', () => { + class MockClient { + async connect() {} + async listTools() { + return { + tools: [ + { name: 'read_only_tool', annotations: { readOnlyHint: true } }, + { name: 'write_tool', annotations: { readOnlyHint: false } }, + ], + }; + } + async close() {} + } + return { + Client: MockClient, + }; +}); + +vi.mock('@google/gemini-cli-core', async () => { + const actual = await vi.importActual('@google/gemini-cli-core'); + return { + ...actual, + createTransport: vi.fn().mockResolvedValue({}), + createSandboxManager: vi.fn().mockReturnValue({}), + }; +}); + vi.mock('os', () => { const homedir = vi.fn(() => '/home/user'); return { @@ -68,6 +95,33 @@ describe('mcp add command', () => { setValue: mockSetValue, workspace: { path: '/path/to/project' }, user: { path: '/home/user' }, + merged: {}, + }); + }); + + describe('sandboxing warnings', () => { + it('should discover tools and emit warning when sandboxing is enabled', async () => { + mockedLoadSettings.mockReturnValue({ + forScope: () => ({ settings: {} }), + setValue: mockSetValue, + workspace: { path: '/path/to/project' }, + user: { path: '/home/user' }, + merged: { + tools: { + sandbox: { enabled: true }, + }, + }, + }); + + const debugLoggerWarnSpy = vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); + + await parser.parseAsync('add sandbox-server /path/to/server'); + + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Warning: With sandboxing enabled, the following tools are marked as read-only and will AUTO-EXECUTE without confirmation:\n * read_only_tool', + ), + ); }); }); @@ -216,6 +270,7 @@ describe('mcp add command', () => { setValue: mockSetValue, workspace: { path: workspacePath }, user: { path: '/home/user' }, + merged: {}, }); }; @@ -383,6 +438,7 @@ describe('mcp add command', () => { setValue: mockSetValue, workspace: { path: '/path/to/project' }, user: { path: '/home/user' }, + merged: {}, }); }); diff --git a/packages/cli/src/commands/mcp/add.ts b/packages/cli/src/commands/mcp/add.ts index 98e6a70879..73894294f4 100644 --- a/packages/cli/src/commands/mcp/add.ts +++ b/packages/cli/src/commands/mcp/add.ts @@ -7,7 +7,14 @@ // File for 'gemini mcp add' command import type { CommandModule } from 'yargs'; import { loadSettings, SettingScope } from '../../config/settings.js'; -import { debugLogger, type MCPServerConfig } from '@google/gemini-cli-core'; +import { + debugLogger, + type MCPServerConfig, + type McpContext, + createTransport, + createSandboxManager, +} from '@google/gemini-cli-core'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { exitCli } from '../utils.js'; async function addMcpServer( @@ -135,6 +142,62 @@ async function addMcpServer( `MCP server "${name}" added to ${scope} settings. (${transport})`, ); } + + const isSandboxEnabled = + typeof settings.merged.tools?.sandbox === 'object' + ? settings.merged.tools.sandbox.enabled + : !!settings.merged.tools?.sandbox; + + if (isSandboxEnabled) { + const mcpContext: McpContext = { + sanitizationConfig: { + enableEnvironmentVariableRedaction: true, + allowedEnvironmentVariables: [], + blockedEnvironmentVariables: settings.merged.advanced?.excludedEnvVars ?? [], + }, + emitMcpDiagnostic: () => {}, + isTrustedFolder: () => true, + isSandboxEnabled: () => true, + sandboxManager: createSandboxManager( + { enabled: true }, + { workspace: process.cwd(), modeConfig: { readonly: true } }, + ), + }; + + try { + const serverTransport = await createTransport( + name, + newServer as MCPServerConfig, + false, + mcpContext, + ); + const client = new Client( + { name: 'gemini-cli-discovery', version: '1.0.0' }, + { capabilities: {} }, + ); + await client.connect(serverTransport); + const result = await client.listTools(); + const readOnlyTools = result.tools + .filter( + (t: { name: string; annotations?: { readOnlyHint?: boolean } }) => + t.annotations?.readOnlyHint === true, + ) + .map((t) => t.name); + + if (readOnlyTools.length > 0) { + debugLogger.warn( + `Warning: With sandboxing enabled, the following tools are marked as read-only and will AUTO-EXECUTE without confirmation:\n${readOnlyTools + .map((t) => ` * ${t}`) + .join('\n')}`, + ); + } + await client.close(); + } catch (e) { + debugLogger.warn( + 'Warning: With sandboxing enabled, any read-only tools provided by this server will AUTO-EXECUTE without confirmation.', + ); + } + } } export const addCommand: CommandModule = { diff --git a/packages/cli/src/commands/mcp/list.ts b/packages/cli/src/commands/mcp/list.ts index 8154e3b7bf..0324afb3ac 100644 --- a/packages/cli/src/commands/mcp/list.ts +++ b/packages/cli/src/commands/mcp/list.ts @@ -17,6 +17,7 @@ import { debugLogger, applyAdminAllowlist, getAdminBlockedMcpServersMessage, + NoopSandboxManager, } from '@google/gemini-cli-core'; import type { MCPServerConfig } from '@google/gemini-cli-core'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; @@ -115,6 +116,8 @@ async function testMCPConnection( } }, isTrustedFolder: () => isTrusted, + isSandboxEnabled: () => false, + sandboxManager: new NoopSandboxManager(), }; let transport; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 075c5439ad..1c86713c5b 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2831,6 +2831,10 @@ export class Config implements McpContext, AgentLoopContext { return this.folderTrust ? (this.trustedFolder ?? false) : true; } + isSandboxEnabled(): boolean { + return this.sandbox?.enabled ?? false; + } + setIdeMode(value: boolean): void { this.ideMode = value; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4633b5f4c3..4a4646a65a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -157,6 +157,7 @@ export * from './ide/types.js'; // Export Shell Execution Service export * from './services/shellExecutionService.js'; export * from './services/sandboxManager.js'; +export { createSandboxManager } from './services/sandboxManagerFactory.js'; // Export Execution Lifecycle Service export * from './services/executionLifecycleService.js'; diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index a96f3f7d29..a33dcae6b9 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -61,6 +61,7 @@ describe('McpClientManager', () => { isInitialized: vi.fn(), }), refreshMcpContext: vi.fn(), + isSandboxEnabled: vi.fn().mockReturnValue(false), } as unknown as Config); toolRegistry = vi.mockObject({ registerTool: vi.fn(), @@ -821,4 +822,70 @@ describe('McpClientManager', () => { expect(coreEventsMock.emitFeedback).toHaveBeenCalledTimes(2); // Now the actual error }); }); + + describe('auto-executing tools warning', () => { + let coreEventsMock: typeof import('../utils/events.js').coreEvents; + + beforeEach(async () => { + const eventsModule = await import('../utils/events.js'); + coreEventsMock = eventsModule.coreEvents; + vi.spyOn(coreEventsMock, 'emitFeedback').mockImplementation(() => {}); + + mockedMcpClient = vi.mockObject({ + connect: vi.fn(), + discoverInto: vi.fn(), + disconnect: vi.fn(), + getStatus: vi.fn().mockReturnValue(MCPServerStatus.DISCONNECTED), + getServerConfig: vi.fn(), + getServerName: vi.fn().mockReturnValue('test-server'), + getAutoExecutingTools: vi.fn().mockReturnValue([]), + } as unknown as McpClient); + vi.mocked(McpClient).mockReturnValue(mockedMcpClient); + }); + + it('should warn about auto-executing tools if sandboxing is enabled', async () => { + mockConfig.isSandboxEnabled.mockReturnValue(true); + mockConfig.getMcpServers.mockReturnValue({ + 'test-server': { command: 'node' }, + }); + mockedMcpClient.getAutoExecutingTools.mockReturnValue([ + '[test-server] tool1', + '[test-server] tool2', + ]); + + const manager = setupManager(new McpClientManager('0.0.1', mockConfig)); + await manager.startConfiguredMcpServers(); + + expect(coreEventsMock.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining( + 'With sandboxing enabled, the following MCP tools will AUTO-EXECUTE', + ), + ); + expect(coreEventsMock.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('* [test-server] tool1'), + ); + expect(coreEventsMock.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('* [test-server] tool2'), + ); + }); + + it('should not warn if sandboxing is disabled', async () => { + mockConfig.isSandboxEnabled.mockReturnValue(false); + mockConfig.getMcpServers.mockReturnValue({ + 'test-server': { command: 'node' }, + }); + mockedMcpClient.getAutoExecutingTools.mockReturnValue(['tool1']); + + const manager = setupManager(new McpClientManager('0.0.1', mockConfig)); + await manager.startConfiguredMcpServers(); + + expect(coreEventsMock.emitFeedback).not.toHaveBeenCalledWith( + 'warning', + expect.stringContaining('AUTO-EXECUTE'), + ); + }); + }); }); diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index 3e7ef75d4c..2069758c41 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -63,6 +63,8 @@ export class McpClientManager { */ private shownDiagnostics: Map = new Map(); + private warnedAutoExecutingTools = new Set(); + /** * Track whether the MCP "hint" has been shown. */ @@ -706,6 +708,7 @@ export class McpClientManager { this.pendingMcpContextRefresh = false; debugLogger.log('Executing MCP context refresh...'); await this.cliConfig.refreshMcpContext(); + this.warnAboutAutoExecutingTools(); debugLogger.log('MCP context refresh complete.'); // If more refresh requests came in during the execution, wait a bit @@ -730,6 +733,29 @@ export class McpClientManager { return this.pendingRefreshPromise; } + private warnAboutAutoExecutingTools() { + if (!this.cliConfig.isSandboxEnabled()) { + return; + } + const toolsToWarn: string[] = []; + for (const client of this.clients.values()) { + for (const toolName of client.getAutoExecutingTools()) { + if (!this.warnedAutoExecutingTools.has(toolName)) { + toolsToWarn.push(toolName); + this.warnedAutoExecutingTools.add(toolName); + } + } + } + if (toolsToWarn.length > 0) { + coreEvents.emitFeedback( + 'warning', + `With sandboxing enabled, the following MCP tools will AUTO-EXECUTE without confirmation:\n${toolsToWarn + .map((t) => ` * ${t}`) + .join('\n')}`, + ); + } + } + getMcpServerCount(): number { return this.clients.size; } diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 4a14b671a0..59820a7f19 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -28,6 +28,7 @@ import { import type { DiscoveredMCPTool } from './mcp-tool.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; +import { NoopSandboxManager } from '../services/sandboxManager.js'; import { connectToMcpServer, createTransport, @@ -63,6 +64,8 @@ const MOCK_CONTEXT_DEFAULT = { emitMcpDiagnostic: vi.fn(), setUserInteractedWithMcp: vi.fn(), isTrustedFolder: vi.fn().mockReturnValue(true), + isSandboxEnabled: vi.fn().mockReturnValue(false), + sandboxManager: new NoopSandboxManager(), }; let MOCK_CONTEXT: McpContext = MOCK_CONTEXT_DEFAULT; @@ -93,6 +96,8 @@ describe('mcp-client', () => { emitMcpDiagnostic: vi.fn(), setUserInteractedWithMcp: vi.fn(), isTrustedFolder: vi.fn().mockReturnValue(true), + isSandboxEnabled: vi.fn().mockReturnValue(false), + sandboxManager: new NoopSandboxManager(), }; // create a tmp dir for this test // Create a unique temporary directory for the workspace to avoid conflicts diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index fdd8bb7008..b0a33a2c4e 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -80,6 +80,7 @@ import { type EnvironmentSanitizationConfig, } from '../services/environmentSanitization.js'; import { expandEnvVars } from '../utils/envExpansion.js'; +import type { SandboxManager } from '../services/sandboxManager.js'; import { GEMINI_CLI_IDENTIFICATION_ENV_VAR, GEMINI_CLI_IDENTIFICATION_ENV_VAR_VALUE, @@ -161,6 +162,8 @@ export class McpClient implements McpProgressReporter { */ private readonly progressTokenToCallId = new Map(); + private autoExecutingTools: string[] = []; + constructor( private readonly serverName: string, private readonly serverConfig: MCPServerConfig, @@ -175,6 +178,10 @@ export class McpClient implements McpProgressReporter { return this.serverName; } + getAutoExecutingTools(): string[] { + return this.autoExecutingTools; + } + /** * Connects to the MCP server. */ @@ -236,6 +243,10 @@ export class McpClient implements McpProgressReporter { const resources = await this.discoverResources(); this.updateResourceRegistry(resources, registries.resourceRegistry); + this.autoExecutingTools = tools + .filter((t) => t.isReadOnly) + .map((t) => `[${t.serverName}] ${t.serverToolName}`); + if (prompts.length === 0 && tools.length === 0 && resources.length === 0) { throw new Error('No prompts, tools, or resources found on the server.'); } @@ -1754,6 +1765,8 @@ export interface McpContext { ): void; setUserInteractedWithMcp?(): void; isTrustedFolder(): boolean; + isSandboxEnabled(): boolean; + sandboxManager?: SandboxManager; getPolicyEngine?(): { getRules(): ReadonlyArray<{ toolName: string; @@ -2275,11 +2288,29 @@ export async function createTransport( } } + let preparedProgram = mcpServerConfig.command; + let preparedArgs = mcpServerConfig.args || []; + let preparedEnv = finalEnv; + let preparedCwd = mcpServerConfig.cwd || process.cwd(); + + if (cliConfig.isSandboxEnabled() && cliConfig.sandboxManager) { + const prepared = await cliConfig.sandboxManager.prepareCommand({ + command: preparedProgram, + args: preparedArgs, + cwd: preparedCwd, + env: preparedEnv, + }); + preparedProgram = prepared.program; + preparedArgs = prepared.args; + preparedCwd = prepared.cwd || preparedCwd; + preparedEnv = prepared.env as Record; + } + let transport: Transport = new StdioClientTransport({ - command: mcpServerConfig.command, - args: mcpServerConfig.args || [], - env: finalEnv, - cwd: mcpServerConfig.cwd, + command: preparedProgram, + args: preparedArgs, + env: preparedEnv, + cwd: preparedCwd, stderr: 'pipe', }); @@ -2369,3 +2400,4 @@ export function isEnabled( ) ); } + diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index 1d5b9d262c..1386d5f1d7 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -1022,7 +1022,7 @@ describe('DiscoveredMCPTool', () => { }); describe('shouldConfirmExecute with read-only hint', () => { - it('should return false if isReadOnly is true', async () => { + it('should return false if isReadOnly and isSandboxEnabled are true', async () => { const bus = createMockMessageBus(); getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; const readOnlyTool = new DiscoveredMCPTool( @@ -1034,6 +1034,8 @@ describe('DiscoveredMCPTool', () => { bus, false, // trust true, // isReadOnly + undefined, // nameOverride + { isSandboxEnabled: () => true, isTrustedFolder: () => true } as any, // cliConfig ); const invocation = readOnlyTool.build({ param: 'mock' }); expect( @@ -1041,6 +1043,29 @@ describe('DiscoveredMCPTool', () => { ).toBe(false); }); + it('should return confirmation details if isReadOnly is true but isSandboxEnabled is false', async () => { + const bus = createMockMessageBus(); + getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; + const readOnlyTool = new DiscoveredMCPTool( + mockCallableToolInstance, + serverName, + serverToolName, + baseDescription, + inputSchema, + bus, + false, // trust + true, // isReadOnly + undefined, // nameOverride + { isSandboxEnabled: () => false, isTrustedFolder: () => true } as any, // cliConfig + ); + const invocation = readOnlyTool.build({ param: 'mock' }); + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).not.toBe(false); + expect(confirmation).toHaveProperty('type', 'mcp'); + }); + it('should return confirmation details if isReadOnly is false', async () => { const bus = createMockMessageBus(); getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; @@ -1053,6 +1078,8 @@ describe('DiscoveredMCPTool', () => { bus, false, // trust false, // isReadOnly + undefined, // nameOverride + { isSandboxEnabled: () => true, isTrustedFolder: () => true } as any, // cliConfig ); const invocation = readWriteTool.build({ param: 'mock' }); const confirmation = await invocation.shouldConfirmExecute( diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index fa14f198b0..80fd28f2cc 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -206,8 +206,8 @@ export class DiscoveredMCPToolInvocation extends BaseToolInvocation< return false; // server is trusted, no confirmation needed } - if (this.isReadOnly) { - return false; // read-only tools do not require confirmation + if (this.isReadOnly && this.cliConfig?.isSandboxEnabled()) { + return false; // read-only tools do not require confirmation if sandboxing is enabled } if (