mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-20 02:51:55 -07:00
fix(browser-agent): enable "Allow all server tools" session policy (#22343)
This commit is contained in:
5
integration-tests/browser-policy.responses
Normal file
5
integration-tests/browser-policy.responses
Normal file
@@ -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}}]}
|
||||
178
integration-tests/browser-policy.test.ts
Normal file
178
integration-tests/browser-policy.test.ts
Normal file
@@ -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');
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown> {
|
||||
return {
|
||||
_serverName: BROWSER_AGENT_NAME,
|
||||
};
|
||||
}
|
||||
|
||||
build(
|
||||
params: Record<string, unknown>,
|
||||
): ToolInvocation<Record<string, unknown>, ToolResult> {
|
||||
|
||||
@@ -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',
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)',
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user