From 7de06162292e0dbc0672eebf93e13f2dd01436a6 Mon Sep 17 00:00:00 2001 From: cynthialong0-0 <82900738+cynthialong0-0@users.noreply.github.com> Date: Thu, 19 Mar 2026 09:32:35 -0700 Subject: [PATCH] fix(browser-agent): enable "Allow all server tools" session policy (#22343) --- integration-tests/browser-policy.responses | 5 + integration-tests/browser-policy.test.ts | 178 ++++++++++++++++++ .../core/src/agents/browser/mcpToolWrapper.ts | 22 ++- .../mcpToolWrapperConfirmation.test.ts | 6 +- packages/core/src/policy/config.test.ts | 28 +++ packages/core/src/policy/config.ts | 2 + .../core/src/policy/policy-updater.test.ts | 62 ++++++ 7 files changed, 297 insertions(+), 6 deletions(-) create mode 100644 integration-tests/browser-policy.responses create mode 100644 integration-tests/browser-policy.test.ts diff --git a/integration-tests/browser-policy.responses b/integration-tests/browser-policy.responses new file mode 100644 index 0000000000..23d14e0cb3 --- /dev/null +++ b/integration-tests/browser-policy.responses @@ -0,0 +1,5 @@ +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"I'll help you with that."},{"functionCall":{"name":"browser_agent","args":{"task":"Open https://example.com and check if there is a heading"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":50,"totalTokenCount":150}}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"new_page","args":{"url":"https://example.com"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":50,"totalTokenCount":150}}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"take_snapshot","args":{}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":50,"totalTokenCount":150}}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"complete_task","args":{"success":true,"summary":"SUCCESS_POLICY_TEST_COMPLETED"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":50,"totalTokenCount":150}}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"Task completed successfully. The page has the heading \"Example Domain\"."}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":200,"candidatesTokenCount":50,"totalTokenCount":250}}]} diff --git a/integration-tests/browser-policy.test.ts b/integration-tests/browser-policy.test.ts new file mode 100644 index 0000000000..1bfdc27415 --- /dev/null +++ b/integration-tests/browser-policy.test.ts @@ -0,0 +1,178 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { TestRig, poll } from './test-helper.js'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { execSync } from 'node:child_process'; +import { existsSync, writeFileSync, readFileSync, mkdirSync } from 'node:fs'; +import stripAnsi from 'strip-ansi'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +const chromeAvailable = (() => { + try { + if (process.platform === 'darwin') { + execSync( + 'test -d "/Applications/Google Chrome.app" || test -d "/Applications/Chromium.app"', + { + stdio: 'ignore', + }, + ); + } else if (process.platform === 'linux') { + execSync( + 'which google-chrome || which chromium-browser || which chromium', + { stdio: 'ignore' }, + ); + } else if (process.platform === 'win32') { + const chromePaths = [ + 'C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe', + 'C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe', + `${process.env['LOCALAPPDATA'] ?? ''}\\Google\\Chrome\\Application\\chrome.exe`, + ]; + const found = chromePaths.some((p) => existsSync(p)); + if (!found) { + execSync('where chrome || where chromium', { stdio: 'ignore' }); + } + } else { + return false; + } + return true; + } catch { + return false; + } +})(); + +describe.skipIf(!chromeAvailable)('browser-policy', () => { + let rig: TestRig; + + beforeEach(() => { + rig = new TestRig(); + }); + + afterEach(async () => { + await rig.cleanup(); + }); + + it('should skip confirmation when "Allow all server tools for this session" is chosen', async () => { + rig.setup('browser-policy-skip-confirmation', { + fakeResponsesPath: join(__dirname, 'browser-policy.responses'), + settings: { + agents: { + overrides: { + browser_agent: { + enabled: true, + }, + }, + browser: { + headless: true, + sessionMode: 'isolated', + allowedDomains: ['example.com'], + }, + }, + }, + }); + + // Manually trust the folder to avoid the dialog and enable option 3 + const geminiDir = join(rig.homeDir!, '.gemini'); + mkdirSync(geminiDir, { recursive: true }); + + // Write to trustedFolders.json + const trustedFoldersPath = join(geminiDir, 'trustedFolders.json'); + const trustedFolders = { + [rig.testDir!]: 'TRUST_FOLDER', + }; + writeFileSync(trustedFoldersPath, JSON.stringify(trustedFolders, null, 2)); + + // Force confirmation for browser agent. + // NOTE: We don't force confirm browser tools here because "Allow all server tools" + // adds a rule with ALWAYS_ALLOW_PRIORITY (3.9x) which would be overshadowed by + // a rule in the user tier (4.x) like the one from this TOML. + // By removing the explicit mcp rule, the first MCP tool will still prompt + // due to default approvalMode = 'default', and then "Allow all" will correctly + // bypass subsequent tools. + const policyFile = join(rig.testDir!, 'force-confirm.toml'); + writeFileSync( + policyFile, + ` +[[rule]] +name = "Force confirm browser_agent" +toolName = "browser_agent" +decision = "ask_user" +priority = 200 +`, + ); + + // Update settings.json in both project and home directories to point to the policy file + for (const baseDir of [rig.testDir!, rig.homeDir!]) { + const settingsPath = join(baseDir, '.gemini', 'settings.json'); + if (existsSync(settingsPath)) { + const settings = JSON.parse(readFileSync(settingsPath, 'utf-8')); + settings.policyPaths = [policyFile]; + // Ensure folder trust is enabled + settings.security = settings.security || {}; + settings.security.folderTrust = settings.security.folderTrust || {}; + settings.security.folderTrust.enabled = true; + writeFileSync(settingsPath, JSON.stringify(settings, null, 2)); + } + } + + const run = await rig.runInteractive({ + approvalMode: 'default', + env: { + GEMINI_CLI_INTEGRATION_TEST: 'true', + }, + }); + + await run.sendKeys( + 'Open https://example.com and check if there is a heading\r', + ); + await run.sendKeys('\r'); + + // Handle confirmations. + // 1. Initial browser_agent delegation (likely only 3 options, so use option 1: Allow once) + await poll( + () => stripAnsi(run.output).toLowerCase().includes('action required'), + 60000, + 1000, + ); + await run.sendKeys('1\r'); + await new Promise((r) => setTimeout(r, 2000)); + + // Handle privacy notice + await poll( + () => stripAnsi(run.output).toLowerCase().includes('privacy notice'), + 5000, + 100, + ); + await run.sendKeys('1\r'); + await new Promise((r) => setTimeout(r, 5000)); + + // new_page (MCP tool, should have 4 options, use option 3: Allow all server tools) + await poll( + () => { + const stripped = stripAnsi(run.output).toLowerCase(); + return ( + stripped.includes('new_page') && + stripped.includes('allow all server tools for this session') + ); + }, + 60000, + 1000, + ); + + // Select "Allow all server tools for this session" (option 3) + await run.sendKeys('3\r'); + await new Promise((r) => setTimeout(r, 30000)); + + const output = stripAnsi(run.output).toLowerCase(); + + expect(output).toContain('browser_agent'); + expect(output).toContain('completed successfully'); + }); +}); diff --git a/packages/core/src/agents/browser/mcpToolWrapper.ts b/packages/core/src/agents/browser/mcpToolWrapper.ts index 3af3f307da..7a352e975c 100644 --- a/packages/core/src/agents/browser/mcpToolWrapper.ts +++ b/packages/core/src/agents/browser/mcpToolWrapper.ts @@ -31,6 +31,8 @@ import type { MessageBus } from '../../confirmation-bus/message-bus.js'; import type { BrowserManager, McpToolCallResult } from './browserManager.js'; import { debugLogger } from '../../utils/debugLogger.js'; import { suspendInputBlocker, resumeInputBlocker } from './inputBlocker.js'; +import { MCP_TOOL_PREFIX } from '../../tools/mcp-tool.js'; +import { BROWSER_AGENT_NAME } from './browserAgentDefinition.js'; /** * Tools that interact with page elements and require the input blocker @@ -62,7 +64,13 @@ class McpToolInvocation extends BaseToolInvocation< messageBus: MessageBus, private readonly shouldDisableInput: boolean, ) { - super(params, messageBus, toolName, toolName); + super( + params, + messageBus, + `${MCP_TOOL_PREFIX}${BROWSER_AGENT_NAME}_${toolName}`, + toolName, + BROWSER_AGENT_NAME, + ); } getDescription(): string { @@ -79,7 +87,7 @@ class McpToolInvocation extends BaseToolInvocation< return { type: 'mcp', title: `Confirm MCP Tool: ${this.toolName}`, - serverName: 'browser-agent', + serverName: BROWSER_AGENT_NAME, toolName: this.toolName, toolDisplayName: this.toolName, onConfirm: async (outcome: ToolConfirmationOutcome) => { @@ -92,7 +100,7 @@ class McpToolInvocation extends BaseToolInvocation< _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { return { - mcpName: 'browser-agent', + mcpName: BROWSER_AGENT_NAME, }; } @@ -202,6 +210,14 @@ class McpDeclarativeTool extends DeclarativeTool< ); } + // Used for determining tool identity in the policy engine to check if a tool + // call is allowed based on policy. + override get toolAnnotations(): Record { + return { + _serverName: BROWSER_AGENT_NAME, + }; + } + build( params: Record, ): ToolInvocation, ToolResult> { diff --git a/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts b/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts index 25c65f612f..2dcbc21538 100644 --- a/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts +++ b/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts @@ -61,7 +61,7 @@ describe('mcpToolWrapper Confirmation', () => { expect(details).toEqual( expect.objectContaining({ type: 'mcp', - serverName: 'browser-agent', + serverName: 'browser_agent', toolName: 'test_tool', }), ); @@ -76,7 +76,7 @@ describe('mcpToolWrapper Confirmation', () => { expect(mockMessageBus.publish).toHaveBeenCalledWith( expect.objectContaining({ type: MessageBusType.UPDATE_POLICY, - mcpName: 'browser-agent', + mcpName: 'browser_agent', persist: false, }), ); @@ -94,7 +94,7 @@ describe('mcpToolWrapper Confirmation', () => { ); expect(options).toEqual({ - mcpName: 'browser-agent', + mcpName: 'browser_agent', }); }); }); diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index 0e2301c1c8..c4204e3c6c 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -630,6 +630,34 @@ name = "invalid-name" ).toBeUndefined(); }); + it('should support mcpName in policy rules from TOML', async () => { + mockPolicyFile( + nodePath.join(MOCK_DEFAULT_DIR, 'mcp.toml'), + ` + [[rule]] + toolName = "my-tool" + mcpName = "my-server" + decision = "allow" + priority = 150 + `, + ); + + const config = await createPolicyEngineConfig( + {}, + ApprovalMode.DEFAULT, + MOCK_DEFAULT_DIR, + ); + + const rule = config.rules?.find( + (r) => + r.toolName === 'mcp_my-server_my-tool' && + r.mcpName === 'my-server' && + r.decision === PolicyDecision.ALLOW, + ); + expect(rule).toBeDefined(); + expect(rule?.priority).toBeCloseTo(1.15, 5); + }); + it('should have default ASK_USER rule for discovered tools', async () => { const config = await createPolicyEngineConfig({}, ApprovalMode.DEFAULT); const discoveredRule = config.rules?.find( diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index 392ab15c0c..eb53196c92 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -576,6 +576,7 @@ export function createPolicyUpdater( decision: PolicyDecision.ALLOW, priority, argsPattern: new RegExp(pattern), + mcpName: message.mcpName, source: 'Dynamic (Confirmed)', }); } @@ -611,6 +612,7 @@ export function createPolicyUpdater( decision: PolicyDecision.ALLOW, priority, argsPattern, + mcpName: message.mcpName, source: 'Dynamic (Confirmed)', }); } diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 7aafcd5153..3bf3579bbc 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -30,6 +30,8 @@ vi.mock('../utils/shell-utils.js', () => ({ interface ParsedPolicy { rule?: Array<{ commandPrefix?: string | string[]; + mcpName?: string; + toolName?: string; }>; } @@ -67,6 +69,7 @@ describe('createPolicyUpdater', () => { type: MessageBusType.UPDATE_POLICY, toolName: 'run_shell_command', commandPrefix: ['echo', 'ls'], + mcpName: 'test-mcp', persist: false, }); @@ -76,6 +79,7 @@ describe('createPolicyUpdater', () => { expect.objectContaining({ toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, + mcpName: 'test-mcp', argsPattern: new RegExp( escapeRegex('"command":"echo') + '(?:[\\s"]|\\\\")', ), @@ -86,6 +90,7 @@ describe('createPolicyUpdater', () => { expect.objectContaining({ toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, + mcpName: 'test-mcp', argsPattern: new RegExp( escapeRegex('"command":"ls') + '(?:[\\s"]|\\\\")', ), @@ -93,6 +98,63 @@ describe('createPolicyUpdater', () => { ); }); + it('should pass mcpName to policyEngine.addRule for argsPattern updates', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + argsPattern: '"foo":"bar"', + mcpName: 'test-mcp', + persist: false, + }); + + expect(policyEngine.addRule).toHaveBeenCalledWith( + expect.objectContaining({ + toolName: 'test_tool', + mcpName: 'test-mcp', + argsPattern: /"foo":"bar"/, + }), + ); + }); + + it('should persist mcpName to TOML', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' }); + vi.mocked(fs.mkdir).mockResolvedValue(undefined); + + const mockFileHandle = { + writeFile: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + vi.mocked(fs.open).mockResolvedValue( + mockFileHandle as unknown as fs.FileHandle, + ); + vi.mocked(fs.rename).mockResolvedValue(undefined); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'mcp_test-mcp_tool', + mcpName: 'test-mcp', + commandPrefix: 'ls', + persist: true, + }); + + // Wait for the async listener to complete + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(fs.open).toHaveBeenCalled(); + const [content] = mockFileHandle.writeFile.mock.calls[0] as [ + string, + string, + ]; + const parsed = toml.parse(content) as unknown as ParsedPolicy; + + expect(parsed.rule).toHaveLength(1); + expect(parsed.rule![0].mcpName).toBe('test-mcp'); + expect(parsed.rule![0].toolName).toBe('tool'); // toolName should be stripped of MCP prefix + }); + it('should add a single rule when commandPrefix is a string', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage);