From 3d5af8b3508ee8d94e50d307eefd1dae91d9c8d2 Mon Sep 17 00:00:00 2001 From: galz10 Date: Fri, 6 Mar 2026 14:57:42 -0800 Subject: [PATCH] WIP: macOS tool sandbox --- package-lock.json | 26 +- packages/cli/src/config/sandboxConfig.ts | 16 +- packages/cli/src/gemini.test.tsx | 9 +- packages/cli/src/gemini.tsx | 66 +---- .../messages/ToolConfirmationMessage.test.tsx | 101 ++++++- .../messages/ToolConfirmationMessage.tsx | 24 +- ...syntax-highlighting-SVG-snapshot-.snap.svg | 32 ++- .../ToolConfirmationMessage.test.tsx.snap | 9 + .../ui/hooks/shellCommandProcessor.test.tsx | 4 + .../ui/hooks/useTurnActivityMonitor.test.ts | 2 +- .../src/ui/hooks/useTurnActivityMonitor.ts | 19 +- packages/cli/src/utils/sandbox.test.ts | 26 +- packages/cli/src/utils/sandbox.ts | 15 +- .../src/agents/browser/browserManager.test.ts | 47 ++++ .../core/src/agents/browser/browserManager.ts | 26 +- packages/core/src/config/config.test.ts | 6 + packages/core/src/config/config.ts | 24 +- packages/core/src/confirmation-bus/types.ts | 1 + packages/core/src/ide/ide-client.test.ts | 62 ++++- packages/core/src/ide/ide-client.ts | 19 +- packages/core/src/index.ts | 1 + packages/core/src/policy/policy-engine.ts | 10 +- packages/core/src/policy/shell-safety.test.ts | 1 + .../core/src/services/sandboxManager.test.ts | 149 +++++++++++ packages/core/src/services/sandboxManager.ts | 248 ++++++++++++++++++ .../shellExecutionService.sandbox.test.ts | 172 ++++++++++++ .../src/services/shellExecutionService.ts | 82 ++++-- packages/core/src/tools/grep.ts | 3 + packages/core/src/tools/mcp-client.test.ts | 66 +++++ packages/core/src/tools/mcp-client.ts | 39 ++- packages/core/src/tools/ripGrep.ts | 2 + .../src/tools/sandboxed-transport.test.ts | 73 ++++++ .../core/src/tools/sandboxed-transport.ts | 45 ++++ packages/core/src/tools/shell.test.ts | 5 + packages/core/src/tools/shell.ts | 20 +- packages/core/src/tools/tools.ts | 8 +- packages/core/src/utils/editor.ts | 30 +-- packages/core/src/utils/shell-utils.ts | 239 +++++++++++++---- 38 files changed, 1475 insertions(+), 252 deletions(-) create mode 100644 packages/core/src/services/sandboxManager.test.ts create mode 100644 packages/core/src/services/sandboxManager.ts create mode 100644 packages/core/src/services/shellExecutionService.sandbox.test.ts create mode 100644 packages/core/src/tools/sandboxed-transport.test.ts create mode 100644 packages/core/src/tools/sandboxed-transport.ts diff --git a/package-lock.json b/package-lock.json index 85448711c7..a5437ac5c5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2292,6 +2292,7 @@ "integrity": "sha512-t54CUOsFMappY1Jbzb7fetWeO0n6K0k/4+/ZpkS+3Joz8I4VcvY9OiEBFRYISqaI2fq5sCiPtAjRDOzVYG8m+Q==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@octokit/auth-token": "^6.0.0", "@octokit/graphql": "^9.0.2", @@ -2472,6 +2473,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/api/-/api-1.9.0.tgz", "integrity": "sha512-3giAOQvZiH5F9bMlMiv8+GSPMeqg0dbaeo58/0SlA9sxSqZhnUtxzX9/2FzyhS9sWQf5S0GJE0AKBrFqjpeYcg==", "license": "Apache-2.0", + "peer": true, "engines": { "node": ">=8.0.0" } @@ -2521,6 +2523,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/core/-/core-2.5.0.tgz", "integrity": "sha512-ka4H8OM6+DlUhSAZpONu0cPBtPPTQKxbxVzC4CzVx5+K4JnroJVBtDzLAMx4/3CDTJXRvVFhpFjtl4SaiTNoyQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/semantic-conventions": "^1.29.0" }, @@ -2895,6 +2898,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/resources/-/resources-2.5.0.tgz", "integrity": "sha512-F8W52ApePshpoSrfsSk1H2yJn9aKjCrbpQF1M9Qii0GHzbfVeFUB+rc3X4aggyZD8x9Gu3Slua+s6krmq6Dt8g==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/semantic-conventions": "^1.29.0" @@ -2928,6 +2932,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/sdk-metrics/-/sdk-metrics-2.5.0.tgz", "integrity": "sha512-BeJLtU+f5Gf905cJX9vXFQorAr6TAfK3SPvTFqP+scfIpDQEJfRaGJWta7sJgP+m4dNtBf9y3yvBKVAZZtJQVA==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/resources": "2.5.0" @@ -2982,6 +2987,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/sdk-trace-base/-/sdk-trace-base-2.5.0.tgz", "integrity": "sha512-VzRf8LzotASEyNDUxTdaJ9IRJ1/h692WyArDBInf5puLCjxbICD6XkHgpuudis56EndyS7LYFmtTMny6UABNdQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/resources": "2.5.0", @@ -4178,6 +4184,7 @@ "integrity": "sha512-6mDvHUFSjyT2B2yeNx2nUgMxh9LtOWvkhIU3uePn2I2oyNymUAX1NIsdgviM4CH+JSrp2D2hsMvJOkxY+0wNRA==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "csstype": "^3.0.2" } @@ -4451,6 +4458,7 @@ "integrity": "sha512-klQbnPAAiGYFyI02+znpBRLyjL4/BrBd0nyWkdC0s/6xFLkXYQ8OoRrSkqacS1ddVxf/LDyODIKbQ5TgKAf/Fg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.56.1", "@typescript-eslint/types": "8.56.1", @@ -5298,6 +5306,7 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.15.0.tgz", "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -7901,6 +7910,7 @@ "integrity": "sha512-VmQ+sifHUbI/IcSopBCF/HO3YiHQx/AVd3UVyYL6weuwW+HvON9VYn5l6Zl1WZzPWXPNZrSQpxwkkZ/VuvJZzg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.1", @@ -8533,6 +8543,7 @@ "resolved": "https://registry.npmjs.org/express/-/express-5.2.1.tgz", "integrity": "sha512-hIS4idWWai69NezIdRt2xFVofaF4j+6INOpJlVOLDO8zXGpUVEVzIYk12UUi2JzjEzWL3IOAxcTubgz9Po0yXw==", "license": "MIT", + "peer": true, "dependencies": { "accepts": "^2.0.0", "body-parser": "^2.2.1", @@ -9847,6 +9858,7 @@ "resolved": "https://registry.npmjs.org/hono/-/hono-4.12.2.tgz", "integrity": "sha512-gJnaDHXKDayjt8ue0n8Gs0A007yKXj4Xzb8+cNjZeYsSzzwKc0Lr+OZgYwVfB0pHfUs17EPoLvrOsEaJ9mj+Tg==", "license": "MIT", + "peer": true, "engines": { "node": ">=16.9.0" } @@ -10126,6 +10138,7 @@ "resolved": "https://registry.npmjs.org/@jrichman/ink/-/ink-6.4.11.tgz", "integrity": "sha512-93LQlzT7vvZ1XJcmOMwN4s+6W334QegendeHOMnEJBlhnpIzr8bws6/aOEHG8ZCuVD/vNeeea5m1msHIdAY6ig==", "license": "MIT", + "peer": true, "dependencies": { "@alcalzone/ansi-tokenize": "^0.2.1", "ansi-escapes": "^7.0.0", @@ -13808,6 +13821,7 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.4.tgz", "integrity": "sha512-9nfp2hYpCwOjAN+8TZFGhtWEwgvWHXqESH8qT89AT/lWklpLON22Lc8pEtnpsZz7VmawabSU0gCjnj8aC0euHQ==", "license": "MIT", + "peer": true, "engines": { "node": ">=0.10.0" } @@ -13818,6 +13832,7 @@ "integrity": "sha512-ePrwPfxAnB+7hgnEr8vpKxL9cmnp7F322t8oqcPshbIQQhDKgFDW4tjhF2wjVbdXF9O/nyuy3sQWd9JGpiLPvA==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "shell-quote": "^1.6.1", "ws": "^7" @@ -15906,6 +15921,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -16129,7 +16145,8 @@ "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.8.1.tgz", "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", "dev": true, - "license": "0BSD" + "license": "0BSD", + "peer": true }, "node_modules/tsx": { "version": "4.20.3", @@ -16137,6 +16154,7 @@ "integrity": "sha512-qjbnuR9Tr+FJOMBqJCW5ehvIo/buZq7vH7qD7JziU98h6l3qGy0a/yPFjwO+y0/T7GFpNgNAvEcPPVfyT8rrPQ==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -16296,6 +16314,7 @@ "integrity": "sha512-p1diW6TqL9L07nNxvRMM7hMMw4c5XOo/1ibL4aAIGmSAt9slTE1Xgw5KWuof2uTOvCg9BY7ZRi+GaF+7sfgPeQ==", "devOptional": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -16519,6 +16538,7 @@ "resolved": "https://registry.npmjs.org/vite/-/vite-7.2.2.tgz", "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -16632,6 +16652,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -16644,6 +16665,7 @@ "resolved": "https://registry.npmjs.org/vitest/-/vitest-3.2.4.tgz", "integrity": "sha512-LUCP5ev3GURDysTWiP47wRRUpLKMOfPh+yKTx3kVIEiu5KOMeqzpnYNsKyOoVrULivR8tLcks4+lga33Whn90A==", "license": "MIT", + "peer": true, "dependencies": { "@types/chai": "^5.2.2", "@vitest/expect": "3.2.4", @@ -17288,6 +17310,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } @@ -17687,6 +17710,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, diff --git a/packages/cli/src/config/sandboxConfig.ts b/packages/cli/src/config/sandboxConfig.ts index 968d3e427a..1d53908eda 100644 --- a/packages/cli/src/config/sandboxConfig.ts +++ b/packages/cli/src/config/sandboxConfig.ts @@ -31,18 +31,13 @@ const VALID_SANDBOX_COMMANDS: ReadonlyArray = [ 'lxc', ]; -function isSandboxCommand(value: string): value is SandboxConfig['command'] { - return (VALID_SANDBOX_COMMANDS as readonly string[]).includes(value); +function isSandboxCommand(value: string | undefined): value is SandboxConfig['command'] { + return (VALID_SANDBOX_COMMANDS as readonly (string | undefined)[]).includes(value); } function getSandboxCommand( sandbox?: boolean | string | null, ): SandboxConfig['command'] | '' { - // If the SANDBOX env var is set, we're already inside the sandbox. - if (process.env['SANDBOX']) { - return ''; - } - // note environment variable takes precedence over argument (from command line or settings) const environmentConfiguredSandbox = process.env['GEMINI_SANDBOX']?.toLowerCase().trim() ?? ''; @@ -124,5 +119,10 @@ export async function loadSandboxConfig( process.env['GEMINI_SANDBOX_IMAGE_DEFAULT'] ?? packageJson?.config?.sandboxImageUri; - return command && image ? { command, image } : undefined; + const enabled = !!(command === 'sandbox-exec' || (command && image)); + if (enabled) { + console.error(`[DEBUG] Sandbox Enabled: ${command}${image ? ` (image: ${image})` : ''}`); + } + + return enabled ? { command: command as SandboxConfig['command'], image, enabled: true } : undefined; } diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 2784c5694a..ed38ddbe1c 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -234,6 +234,7 @@ vi.mock('./config/sandboxConfig.js', () => ({ loadSandboxConfig: vi.fn().mockResolvedValue({ command: 'docker', image: 'test-image', + enabled: true, }), })); @@ -618,13 +619,18 @@ describe('gemini.tsx main function kitty protocol', () => { const mockConfig = createMockConfig({ isInteractive: () => false, getQuestion: () => '', - getSandbox: () => ({ command: 'docker', image: 'test-image' }), + getSandbox: () => ({ + command: 'docker', + image: 'test-image', + enabled: true, + }), }); vi.mocked(loadCliConfig).mockResolvedValue(mockConfig); vi.mocked(loadSandboxConfig).mockResolvedValue({ command: 'docker', image: 'test-image', + enabled: true, }); process.env['GEMINI_API_KEY'] = 'test-key'; @@ -914,6 +920,7 @@ describe('gemini.tsx main function exit codes', () => { it('should exit with 41 for auth failure during sandbox setup', async () => { vi.stubEnv('SANDBOX', ''); vi.mocked(loadSandboxConfig).mockResolvedValue({ + enabled: true, command: 'docker', image: 'test-image', }); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 88f9f404cd..9dbdf385f7 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -15,7 +15,6 @@ import { createHash } from 'node:crypto'; import v8 from 'node:v8'; import os from 'node:os'; import dns from 'node:dns'; -import { start_sandbox } from './utils/sandbox.js'; import type { DnsResolutionOrder, LoadedSettings } from './config/settings.js'; import { loadTrustedFolders, @@ -94,11 +93,6 @@ import { SessionStatsProvider } from './ui/contexts/SessionContext.js'; import { VimModeProvider } from './ui/contexts/VimModeContext.js'; import { KeypressProvider } from './ui/contexts/KeypressContext.js'; import { useKittyKeyboardProtocol } from './ui/hooks/useKittyKeyboardProtocol.js'; -import { - relaunchAppInChildProcess, - relaunchOnExitCode, -} from './utils/relaunch.js'; -import { loadSandboxConfig } from './config/sandboxConfig.js'; import { deleteSession, listSessions } from './utils/sessions.js'; import { createPolicyUpdater } from './config/policy.js'; import { ScrollProvider } from './ui/contexts/ScrollProvider.js'; @@ -503,63 +497,9 @@ export async function main() { // Run deferred command now that we have admin settings. await runDeferredCommand(settings.merged); - // hop into sandbox if we are outside and sandboxing is enabled - if (!process.env['SANDBOX']) { - const memoryArgs = settings.merged.advanced.autoConfigureMemory - ? getNodeMemoryArgs(isDebugMode) - : []; - const sandboxConfig = await loadSandboxConfig(settings.merged, argv); - // We intentionally omit the list of extensions here because extensions - // should not impact auth or setting up the sandbox. - // TODO(jacobr): refactor loadCliConfig so there is a minimal version - // that only initializes enough config to enable refreshAuth or find - // another way to decouple refreshAuth from requiring a config. - - if (sandboxConfig) { - if (initialAuthFailed) { - await runExitCleanup(); - process.exit(ExitCodes.FATAL_AUTHENTICATION_ERROR); - } - let stdinData = ''; - if (!process.stdin.isTTY) { - stdinData = await readStdin(); - } - - // This function is a copy of the one from sandbox.ts - // It is moved here to decouple sandbox.ts from the CLI's argument structure. - const injectStdinIntoArgs = ( - args: string[], - stdinData?: string, - ): string[] => { - const finalArgs = [...args]; - if (stdinData) { - const promptIndex = finalArgs.findIndex( - (arg) => arg === '--prompt' || arg === '-p', - ); - if (promptIndex > -1 && finalArgs.length > promptIndex + 1) { - // If there's a prompt argument, prepend stdin to it - finalArgs[promptIndex + 1] = - `${stdinData}\n\n${finalArgs[promptIndex + 1]}`; - } else { - // If there's no prompt argument, add stdin as the prompt - finalArgs.push('--prompt', stdinData); - } - } - return finalArgs; - }; - - const sandboxArgs = injectStdinIntoArgs(process.argv, stdinData); - - await relaunchOnExitCode(() => - start_sandbox(sandboxConfig, memoryArgs, partialConfig, sandboxArgs), - ); - await runExitCleanup(); - process.exit(ExitCodes.SUCCESS); - } else { - // Relaunch app so we always have a child process that can be internally - // restarted if needed. - await relaunchAppInChildProcess(memoryArgs, [], remoteAdminSettings); - } + if (initialAuthFailed) { + await runExitCleanup(); + process.exit(ExitCodes.FATAL_AUTHENTICATION_ERROR); } // We are now past the logic handling potentially launching a child process diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx index fec1228c63..9a9e930ab3 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx @@ -6,15 +6,24 @@ import { describe, it, expect, vi } from 'vitest'; import { ToolConfirmationMessage } from './ToolConfirmationMessage.js'; -import type { - SerializableConfirmationDetails, - ToolCallConfirmationDetails, - Config, +import { + type SerializableConfirmationDetails, + type ToolCallConfirmationDetails, + type Config, + hasRedirection, } from '@google/gemini-cli-core'; import { renderWithProviders } from '../../../test-utils/render.js'; import { createMockSettings } from '../../../test-utils/settings.js'; import { useToolActions } from '../../contexts/ToolActionsContext.js'; +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + hasRedirection: vi.fn(), + }; +}); + vi.mock('../../contexts/ToolActionsContext.js', async (importOriginal) => { const actual = await importOriginal< @@ -240,6 +249,90 @@ describe('ToolConfirmationMessage', () => { unmount(); }); + it('should display redirection warning when hasRedirection is true in confirmationDetails', async () => { + const confirmationDetails: SerializableConfirmationDetails = { + type: 'exec', + title: 'Confirm Redirection', + command: 'ls > out.txt', + rootCommand: 'ls', + rootCommands: ['ls'], + hasRedirection: true, + }; + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + ); + await waitUntilReady(); + + const output = lastFrame(); + expect(output).toContain('Note: Command contains redirection which can be undesirable.'); + unmount(); + }); + + it('should display redirection warning via async fallback when hasRedirection is missing', async () => { + vi.mocked(hasRedirection).mockReturnValue(true); + + const confirmationDetails: SerializableConfirmationDetails = { + type: 'exec', + title: 'Confirm Redirection Fallback', + command: 'ls > out.txt', + rootCommand: 'ls', + rootCommands: ['ls'], + // hasRedirection is missing + }; + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + ); + await waitUntilReady(); + + // The warning should be present as it is now calculated synchronously + const output = lastFrame(); + expect(output).toContain('Note: Command contains redirection which can be undesirable.'); + unmount(); + }); + + it('should NOT display redirection warning when hasRedirection is false', async () => { + const confirmationDetails: SerializableConfirmationDetails = { + type: 'exec', + title: 'Confirm No Redirection', + command: 'ls -la', + rootCommand: 'ls', + rootCommands: ['ls'], + hasRedirection: false, + }; + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + ); + await waitUntilReady(); + + const output = lastFrame(); + expect(output).not.toContain('Note: This command uses shell redirection'); + unmount(); + }); + it('should render multiline shell scripts with correct newlines and syntax highlighting (SVG snapshot)', async () => { const confirmationDetails: SerializableConfirmationDetails = { type: 'exec', diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index b60dd4dc8b..d01416171f 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -90,6 +90,21 @@ export const ToolConfirmationMessage: React.FC< confirmationDetails.type === 'exit_plan_mode'; const isTrustedFolder = config.isTrustedFolder(); + const commandsToDisplay = + confirmationDetails.type === 'exec' && + confirmationDetails.commands && + confirmationDetails.commands.length > 1 + ? confirmationDetails.commands + : confirmationDetails.type === 'exec' + ? [confirmationDetails.command] + : []; + + const hasRedirectionCheck = + confirmationDetails.type === 'exec' + ? confirmationDetails.hasRedirection ?? + commandsToDisplay.some((cmd) => hasRedirection(cmd)) + : false; + const handleConfirm = useCallback( (outcome: ToolConfirmationOutcome, payload?: ToolConfirmationPayload) => { void confirm(callId, outcome, payload).catch((error: unknown) => { @@ -484,17 +499,14 @@ export const ToolConfirmationMessage: React.FC< } } else if (confirmationDetails.type === 'exec') { const executionProps = confirmationDetails; + const containsRedirection = hasRedirectionCheck; + let bodyContentHeight = availableBodyContentHeight(); + let warnings: React.ReactNode | null = null; const commandsToDisplay = executionProps.commands && executionProps.commands.length > 1 ? executionProps.commands : [executionProps.command]; - const containsRedirection = commandsToDisplay.some((cmd) => - hasRedirection(cmd), - ); - - let bodyContentHeight = availableBodyContentHeight(); - let warnings: React.ReactNode = null; if (bodyContentHeight !== undefined) { bodyContentHeight -= 2; // Account for padding; diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage-ToolConfirmationMessage-should-render-multiline-shell-scripts-with-correct-newlines-and-syntax-highlighting-SVG-snapshot-.snap.svg b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage-ToolConfirmationMessage-should-render-multiline-shell-scripts-with-correct-newlines-and-syntax-highlighting-SVG-snapshot-.snap.svg index f6500b1d1d..66afef8eca 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage-ToolConfirmationMessage-should-render-multiline-shell-scripts-with-correct-newlines-and-syntax-highlighting-SVG-snapshot-.snap.svg +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage-ToolConfirmationMessage-should-render-multiline-shell-scripts-with-correct-newlines-and-syntax-highlighting-SVG-snapshot-.snap.svg @@ -1,8 +1,8 @@ - + - + echo "hello" @@ -14,17 +14,21 @@ echo $i done - Allow execution of: 'echo'? - - ● - - - 1. - - - Allow once - - 2. Allow for this session - 3. No, suggest changes (esc) + Note: + Command contains redirection which can be undesirable. + Tip: + Toggle auto-edit (Shift+Tab) to allow redirection in the future. + Allow execution of: 'echo'? + + ● + + + 1. + + + Allow once + + 2. Allow for this session + 3. No, suggest changes (esc) \ No newline at end of file diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap index 3f207df881..ab33d2ab19 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap @@ -42,6 +42,9 @@ exports[`ToolConfirmationMessage > should render multiline shell scripts with co for i in 1 2 3; do echo $i done + +Note: Command contains redirection which can be undesirable. +Tip: Toggle auto-edit (Shift+Tab) to allow redirection in the future. Allow execution of: 'echo'? ● 1. Allow once @@ -93,6 +96,9 @@ Apply this change? exports[`ToolConfirmationMessage > with folder trust > 'for exec confirmations' > should NOT show "allow always" when folder is untrusted 1`] = ` "echo "hello" + +Note: Command contains redirection which can be undesirable. +Tip: Toggle auto-edit (Shift+Tab) to allow redirection in the future. Allow execution of: 'echo'? ● 1. Allow once @@ -102,6 +108,9 @@ Allow execution of: 'echo'? exports[`ToolConfirmationMessage > with folder trust > 'for exec confirmations' > should show "allow always" when folder is trusted 1`] = ` "echo "hello" + +Note: Command contains redirection which can be undesirable. +Tip: Toggle auto-edit (Shift+Tab) to allow redirection in the future. Allow execution of: 'echo'? ● 1. Allow once diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.test.tsx b/packages/cli/src/ui/hooks/shellCommandProcessor.test.tsx index 377cac9b7c..648b908352 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.test.tsx +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.test.tsx @@ -76,6 +76,7 @@ import { type ShellExecutionResult, type ShellOutputEvent, CoreToolCallStatus, + SandboxProfile, } from '@google/gemini-cli-core'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -214,6 +215,7 @@ describe('useShellCommandProcessor', () => { expect.any(Object), false, expect.any(Object), + SandboxProfile.WORKSPACE_WRITE, ); expect(onExecMock).toHaveBeenCalledWith(expect.any(Promise)); }); @@ -306,6 +308,7 @@ describe('useShellCommandProcessor', () => { expect.any(Object), false, // enableInteractiveShell expect.any(Object), + SandboxProfile.WORKSPACE_WRITE, ); // Wait for the async PID update to happen. @@ -430,6 +433,7 @@ describe('useShellCommandProcessor', () => { expect.any(Object), false, expect.any(Object), + SandboxProfile.WORKSPACE_WRITE, ); await act(async () => { diff --git a/packages/cli/src/ui/hooks/useTurnActivityMonitor.test.ts b/packages/cli/src/ui/hooks/useTurnActivityMonitor.test.ts index f3791d1b32..f02116aaf6 100644 --- a/packages/cli/src/ui/hooks/useTurnActivityMonitor.test.ts +++ b/packages/cli/src/ui/hooks/useTurnActivityMonitor.test.ts @@ -65,7 +65,7 @@ describe('useTurnActivityMonitor', () => { expect(result.current.operationStartTime).toBe(2000); }); - it('should detect redirection from tool calls', () => { + it('should detect redirection from tool calls', async () => { // Force mock implementation to ensure it's active vi.mocked(hasRedirection).mockImplementation((q: string) => q.includes('>'), diff --git a/packages/cli/src/ui/hooks/useTurnActivityMonitor.ts b/packages/cli/src/ui/hooks/useTurnActivityMonitor.ts index 8cd7883007..6e30590760 100644 --- a/packages/cli/src/ui/hooks/useTurnActivityMonitor.ts +++ b/packages/cli/src/ui/hooks/useTurnActivityMonitor.ts @@ -48,19 +48,14 @@ export const useTurnActivityMonitor = ( }, [streamingState, activePtyId]); // Detect redirection in the current query or tool calls. - // We derive this directly during render to ensure it's accurate from the first frame. - const isRedirectionActive = useMemo( - () => - // Check active tool calls for run_shell_command - pendingToolCalls.some((tc) => { - if (tc.request.name !== 'run_shell_command') return false; + const isRedirectionActive = useMemo(() => { + return pendingToolCalls.some((tc) => { + if (tc.request.name !== 'run_shell_command') return false; - const command = - (tc.request.args as { command?: string })?.command || ''; - return hasRedirection(command); - }), - [pendingToolCalls], - ); + const command = (tc.request.args as { command?: string })?.command || ''; + return hasRedirection(command); + }); + }, [pendingToolCalls]); return { operationStartTime, diff --git a/packages/cli/src/utils/sandbox.test.ts b/packages/cli/src/utils/sandbox.test.ts index fa562f7ad6..789a4f690b 100644 --- a/packages/cli/src/utils/sandbox.test.ts +++ b/packages/cli/src/utils/sandbox.test.ts @@ -137,7 +137,7 @@ describe('sandbox', () => { describe('start_sandbox', () => { it('should handle macOS seatbelt (sandbox-exec)', async () => { vi.mocked(os.platform).mockReturnValue('darwin'); - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'sandbox-exec', image: 'some-image', }; @@ -173,7 +173,7 @@ describe('sandbox', () => { it('should throw FatalSandboxError if seatbelt profile is missing', async () => { vi.mocked(os.platform).mockReturnValue('darwin'); vi.mocked(fs.existsSync).mockReturnValue(false); - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'sandbox-exec', image: 'some-image', }; @@ -182,7 +182,7 @@ describe('sandbox', () => { }); it('should handle Docker execution', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -231,7 +231,7 @@ describe('sandbox', () => { }); it('should pull image if missing', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'missing-image', }; @@ -300,7 +300,7 @@ describe('sandbox', () => { }); it('should throw if image pull fails', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'missing-image', }; @@ -338,7 +338,7 @@ describe('sandbox', () => { }); it('should mount volumes correctly', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -395,7 +395,7 @@ describe('sandbox', () => { }); it('should pass through GOOGLE_GEMINI_BASE_URL and GOOGLE_VERTEX_BASE_URL', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -442,7 +442,7 @@ describe('sandbox', () => { }); it('should handle user creation on Linux if needed', async () => { - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -508,7 +508,7 @@ describe('sandbox', () => { it('should run lxc exec with correct args for a running container', async () => { process.env['TEST_LXC_LIST_OUTPUT'] = LXC_RUNNING; - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'lxc', image: 'gemini-sandbox', }; @@ -542,7 +542,7 @@ describe('sandbox', () => { it('should throw FatalSandboxError if lxc list fails', async () => { process.env['TEST_LXC_LIST_OUTPUT'] = 'throw'; - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'lxc', image: 'gemini-sandbox', }; @@ -554,7 +554,7 @@ describe('sandbox', () => { it('should throw FatalSandboxError if container is not running', async () => { process.env['TEST_LXC_LIST_OUTPUT'] = LXC_STOPPED; - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'lxc', image: 'gemini-sandbox', }; @@ -564,7 +564,7 @@ describe('sandbox', () => { it('should throw FatalSandboxError if container is not found in list', async () => { process.env['TEST_LXC_LIST_OUTPUT'] = '[]'; - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'lxc', image: 'gemini-sandbox', }; @@ -577,7 +577,7 @@ describe('sandbox', () => { describe('gVisor (runsc)', () => { it('should use docker with --runtime=runsc on Linux', async () => { vi.mocked(os.platform).mockReturnValue('linux'); - const config: SandboxConfig = { + const config: SandboxConfig = { enabled: true, command: 'runsc', image: 'gemini-cli-sandbox', }; diff --git a/packages/cli/src/utils/sandbox.ts b/packages/cli/src/utils/sandbox.ts index df9a88cc4c..c4e54a42ff 100644 --- a/packages/cli/src/utils/sandbox.ts +++ b/packages/cli/src/utils/sandbox.ts @@ -268,6 +268,13 @@ export async function start_sandbox( } } + // stop if command or image is missing + if (!command || !image) { + throw new FatalSandboxError( + 'Sandbox configuration is missing required "command" or "image" field.', + ); + } + // stop if image is missing if (!(await ensureSandboxImageIsPresent(command, image, cliConfig))) { const remedy = @@ -714,10 +721,10 @@ export async function start_sandbox( // proxyProcess.stdout?.on('data', (data) => { // console.info(data.toString()); // }); - proxyProcess.stderr?.on('data', (data) => { + proxyProcess?.stderr?.on('data', (data) => { debugLogger.debug(`[PROXY STDERR]: ${data.toString().trim()}`); }); - proxyProcess.on('close', (code, signal) => { + proxyProcess?.on('close', (code, signal) => { if (sandboxProcess?.pid) { process.kill(-sandboxProcess.pid, 'SIGTERM'); } @@ -743,6 +750,10 @@ export async function start_sandbox( }); return await new Promise((resolve, reject) => { + if (!sandboxProcess) { + reject(new Error('Failed to spawn sandbox process.')); + return; + } sandboxProcess.on('error', (err) => { coreEvents.emitFeedback('error', 'Sandbox process error', err); reject(err); diff --git a/packages/core/src/agents/browser/browserManager.test.ts b/packages/core/src/agents/browser/browserManager.test.ts index 81d85bb505..5a7e62be0c 100644 --- a/packages/core/src/agents/browser/browserManager.test.ts +++ b/packages/core/src/agents/browser/browserManager.test.ts @@ -42,6 +42,21 @@ vi.mock('../../utils/debugLogger.js', () => ({ }, })); +vi.mock('../../utils/shell-utils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + prepareSandboxedCommand: vi.fn().mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ), + }; +}); +import { prepareSandboxedCommand } from '../../utils/shell-utils.js'; + import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; @@ -65,6 +80,14 @@ describe('BrowserManager', () => { }, }); + vi.mocked(prepareSandboxedCommand).mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ); + // Re-setup Client mock after reset vi.mocked(Client).mockImplementation( () => @@ -262,6 +285,30 @@ describe('BrowserManager', () => { expect(args).not.toContain('--isolated'); }); + it('should apply sandboxing to stdio transport', async () => { + const mockCleanup = vi.fn(); + vi.mocked(prepareSandboxedCommand).mockResolvedValue({ + program: 'sandboxed-npx', + args: ['sandboxed-arg'], + cleanup: mockCleanup, + }); + + const manager = new BrowserManager(mockConfig); + await manager.ensureConnection(); + + expect(prepareSandboxedCommand).toHaveBeenCalledWith( + process.platform === 'win32' ? 'npx.cmd' : 'npx', + expect.anything(), + ); + + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: 'sandboxed-npx', + args: ['sandboxed-arg'], + }), + ); + }); + it('should throw actionable error when existing mode connection fails', async () => { // Make the Client mock's connect method reject vi.mocked(Client).mockImplementation( diff --git a/packages/core/src/agents/browser/browserManager.ts b/packages/core/src/agents/browser/browserManager.ts index 67626c63e9..76ec8de2f2 100644 --- a/packages/core/src/agents/browser/browserManager.ts +++ b/packages/core/src/agents/browser/browserManager.ts @@ -19,6 +19,9 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; +import { SandboxedTransport } from '../../tools/sandboxed-transport.js'; +import { prepareSandboxedCommand } from '../../utils/shell-utils.js'; import type { Tool as McpTool } from '@modelcontextprotocol/sdk/types.js'; import { debugLogger } from '../../utils/debugLogger.js'; import type { Config } from '../../config/config.js'; @@ -67,7 +70,7 @@ export interface McpToolCallResult { export class BrowserManager { // Raw MCP SDK Client - NOT the wrapper McpClient private rawMcpClient: Client | undefined; - private mcpTransport: StdioClientTransport | undefined; + private mcpTransport: Transport | undefined; private discoveredTools: McpTool[] = []; constructor(private config: Config) {} @@ -282,14 +285,21 @@ export class BrowserManager { // Create stdio transport to npx chrome-devtools-mcp. // stderr is piped (not inherited) to prevent MCP server banners and // warnings from corrupting the UI in alternate buffer mode. - this.mcpTransport = new StdioClientTransport({ - command: process.platform === 'win32' ? 'npx.cmd' : 'npx', - args: mcpArgs, + const command = process.platform === 'win32' ? 'npx.cmd' : 'npx'; + const { + program: sandboxedCommand, + args: sandboxedArgs, + cleanup, + } = await prepareSandboxedCommand(command, mcpArgs); + + const stdioTransport = new StdioClientTransport({ + command: sandboxedCommand, + args: sandboxedArgs, stderr: 'pipe', }); // Forward piped stderr to debugLogger so it's visible with --debug. - const stderrStream = this.mcpTransport.stderr; + const stderrStream = stdioTransport.stderr; if (stderrStream) { stderrStream.on('data', (chunk: Buffer) => { debugLogger.log( @@ -298,6 +308,12 @@ export class BrowserManager { }); } + if (cleanup) { + this.mcpTransport = new SandboxedTransport(stdioTransport, cleanup); + } else { + this.mcpTransport = stdioTransport; + } + this.mcpTransport.onclose = () => { debugLogger.error( 'chrome-devtools-mcp transport closed unexpectedly. ' + diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 33a04b52ab..ed9b40d648 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -248,6 +248,7 @@ vi.mock('../code_assist/experiments/experiments.js'); describe('Server Config (config.ts)', () => { const MODEL = DEFAULT_GEMINI_MODEL; const SANDBOX: SandboxConfig = { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -1480,6 +1481,7 @@ describe('Server Config (config.ts)', () => { describe('GemmaModelRouterSettings', () => { const MODEL = DEFAULT_GEMINI_MODEL; const SANDBOX: SandboxConfig = { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -1860,6 +1862,7 @@ describe('isYoloModeDisabled', () => { describe('BaseLlmClient Lifecycle', () => { const MODEL = 'gemini-pro'; const SANDBOX: SandboxConfig = { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -1915,6 +1918,7 @@ describe('BaseLlmClient Lifecycle', () => { describe('Generation Config Merging (HACK)', () => { const MODEL = 'gemini-pro'; const SANDBOX: SandboxConfig = { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -2221,6 +2225,7 @@ describe('Config getHooks', () => { describe('LocalLiteRtLmClient Lifecycle', () => { const MODEL = 'gemini-pro'; const SANDBOX: SandboxConfig = { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }; @@ -2479,6 +2484,7 @@ describe('Config Quota & Preview Model Access', () => { usageStatisticsEnabled: false, embeddingModel: 'gemini-embedding', sandbox: { + enabled: true, command: 'docker', image: 'gemini-cli-sandbox', }, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 26f199f9c0..8f1f59f54c 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -105,14 +105,22 @@ import { import { DEFAULT_MODEL_CONFIGS } from './defaultModelConfigs.js'; import { ContextManager } from '../services/contextManager.js'; import { TrackerService } from '../services/trackerService.js'; +import { + type SandboxManager, + StandardSandboxManager, +} from '../services/sandboxManager.js'; import type { GenerateContentParameters } from '@google/genai'; // Re-export OAuth config type export type { MCPOAuthConfig, AnyToolInvocation, AnyDeclarativeTool }; import type { AnyToolInvocation, AnyDeclarativeTool } from '../tools/tools.js'; +import { setSandboxManager as setCentralSandboxManager } from '../utils/shell-utils.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { Storage } from './storage.js'; -import type { ShellExecutionConfig } from '../services/shellExecutionService.js'; +import { + ShellExecutionService, + type ShellExecutionConfig, +} from '../services/shellExecutionService.js'; import { FileExclusions } from '../utils/ignorePatterns.js'; import { MessageBus } from '../confirmation-bus/message-bus.js'; import type { EventEmitter } from 'node:events'; @@ -446,8 +454,10 @@ export enum AuthProviderType { } export interface SandboxConfig { - command: 'docker' | 'podman' | 'sandbox-exec' | 'runsc' | 'lxc'; - image: string; + enabled: boolean; + command?: 'docker' | 'podman' | 'sandbox-exec' | 'runsc' | 'lxc'; + image?: string; + allowedPaths?: string[]; } /** @@ -798,6 +808,7 @@ export class Config implements McpContext { private readonly planModeRoutingEnabled: boolean; private readonly modelSteering: boolean; private contextManager?: ContextManager; + private sandboxManager!: SandboxManager; private terminalBackground: string | undefined = undefined; private remoteAdminSettings: AdminControlsSettings | undefined; private latestApiRequest: GenerateContentParameters | undefined; @@ -813,6 +824,9 @@ export class Config implements McpContext { params.embeddingModel ?? DEFAULT_GEMINI_EMBEDDING_MODEL; this.fileSystemService = new StandardFileSystemService(); this.sandbox = params.sandbox; + this.sandboxManager = new StandardSandboxManager(this); + ShellExecutionService.setSandboxManager(this.sandboxManager); + setCentralSandboxManager(this.sandboxManager); this.targetDir = path.resolve(params.targetDir); this.folderTrust = params.folderTrust ?? false; this.workspaceContext = new WorkspaceContext(this.targetDir, []); @@ -1561,6 +1575,10 @@ export class Config implements McpContext { return this.sandbox; } + getSandboxManager(): SandboxManager { + return this.sandboxManager; + } + isRestrictiveSandbox(): boolean { const sandboxConfig = this.getSandbox(); const seatbeltProfile = process.env['SEATBELT_PROFILE']; diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index aefafe0fa0..598d7372b7 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -92,6 +92,7 @@ export type SerializableConfirmationDetails = rootCommand: string; rootCommands: string[]; commands?: string[]; + hasRedirection?: boolean; } | { type: 'mcp'; diff --git a/packages/core/src/ide/ide-client.test.ts b/packages/core/src/ide/ide-client.test.ts index 7cf2f101e6..78f4b8ab08 100644 --- a/packages/core/src/ide/ide-client.test.ts +++ b/packages/core/src/ide/ide-client.test.ts @@ -14,6 +14,7 @@ import { type Mocked, } from 'vitest'; import { IdeClient, IDEConnectionStatus } from './ide-client.js'; +import { SandboxedTransport } from '../tools/sandboxed-transport.js'; import type * as fs from 'node:fs'; import { getIdeProcessInfo } from './process-utils.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; @@ -50,6 +51,20 @@ vi.mock('@modelcontextprotocol/sdk/client/stdio.js'); vi.mock('./detect-ide.js'); vi.mock('node:os'); vi.mock('./ide-connection-utils.js'); +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + prepareSandboxedCommand: vi.fn().mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ), + }; +}); +import { prepareSandboxedCommand } from '../utils/shell-utils.js'; describe('IdeClient', () => { let mockClient: Mocked; @@ -97,6 +112,14 @@ describe('IdeClient', () => { vi.mocked(StreamableHTTPClientTransport).mockReturnValue(mockHttpTransport); vi.mocked(StdioClientTransport).mockReturnValue(mockStdioTransport); + vi.mocked(prepareSandboxedCommand).mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ); + await IdeClient.getInstance(); }); @@ -137,7 +160,9 @@ describe('IdeClient', () => { command: 'test-cmd', args: ['--foo'], }); - expect(mockClient.connect).toHaveBeenCalledWith(mockStdioTransport); + expect(mockClient.connect).toHaveBeenCalledWith( + expect.any(SandboxedTransport), + ); expect(ideClient.getConnectionStatus().status).toBe( IDEConnectionStatus.Connected, ); @@ -194,12 +219,45 @@ describe('IdeClient', () => { command: 'env-cmd', args: ['--bar'], }); - expect(mockClient.connect).toHaveBeenCalledWith(mockStdioTransport); + expect(mockClient.connect).toHaveBeenCalledWith( + expect.any(SandboxedTransport), + ); expect(ideClient.getConnectionStatus().status).toBe( IDEConnectionStatus.Connected, ); }); + it('should apply sandboxing to stdio transport', async () => { + vi.mocked(getConnectionConfigFromFile).mockResolvedValue(undefined); + vi.mocked(validateWorkspacePath).mockReturnValue({ isValid: true }); + vi.mocked(getStdioConfigFromEnv).mockReturnValue({ + command: 'original-command', + args: ['--original-arg'], + }); + + const mockCleanup = vi.fn(); + vi.mocked(prepareSandboxedCommand).mockResolvedValue({ + program: 'sandboxed-command', + args: ['--sandboxed-arg'], + cleanup: mockCleanup, + }); + + const ideClient = await IdeClient.getInstance(); + await ideClient.connect(); + + expect(prepareSandboxedCommand).toHaveBeenCalledWith( + 'original-command', + ['--original-arg'], + ); + + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: 'sandboxed-command', + args: ['--sandboxed-arg'], + }), + ); + }); + it('should prioritize file config over environment variables', async () => { const config = { port: '8080' }; vi.mocked(getConnectionConfigFromFile).mockResolvedValue(config); diff --git a/packages/core/src/ide/ide-client.ts b/packages/core/src/ide/ide-client.ts index 373df31f5f..68cea7a1ea 100644 --- a/packages/core/src/ide/ide-client.ts +++ b/packages/core/src/ide/ide-client.ts @@ -16,6 +16,9 @@ import { getIdeProcessInfo } from './process-utils.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; +import { SandboxedTransport } from '../tools/sandboxed-transport.js'; +import { prepareSandboxedCommand } from '../utils/shell-utils.js'; import { CallToolResultSchema, ListToolsResultSchema, @@ -618,7 +621,7 @@ export class IdeClient { command, args, }: StdioConfig): Promise { - let transport: StdioClientTransport | undefined; + let transport: Transport | undefined; try { logger.debug('Attempting to connect to IDE via stdio'); this.client = new Client({ @@ -627,10 +630,20 @@ export class IdeClient { version: '1.0.0', }); + const { + program: sandboxedCommand, + args: sandboxedArgs, + cleanup, + } = await prepareSandboxedCommand(command, args); + transport = new StdioClientTransport({ - command, - args, + command: sandboxedCommand, + args: sandboxedArgs, }); + + if (cleanup) { + transport = new SandboxedTransport(transport, cleanup); + } await this.client.connect(transport); this.registerClientHandlers(); await this.discoverTools(); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c4a9965e41..12b32cf65a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -123,6 +123,7 @@ export * from './services/sessionSummaryUtils.js'; export * from './services/contextManager.js'; export * from './services/trackerService.js'; export * from './services/trackerTypes.js'; +export * from './services/sandboxManager.js'; export * from './skills/skillManager.js'; export * from './skills/skillLoader.js'; diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 03087716ff..8526c8308f 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -197,10 +197,10 @@ export class PolicyEngine { return this.approvalMode; } - private shouldDowngradeForRedirection( + private async shouldDowngradeForRedirection( command: string, allowRedirection?: boolean, - ): boolean { + ): Promise { return ( !allowRedirection && hasRedirection(command) && @@ -274,7 +274,7 @@ export class PolicyEngine { let responsibleRule: PolicyRule | undefined; // Check for redirection on the full command string - if (this.shouldDowngradeForRedirection(command, allowRedirection)) { + if (await this.shouldDowngradeForRedirection(command, allowRedirection)) { debugLogger.debug( `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${command}`, ); @@ -286,7 +286,7 @@ export class PolicyEngine { const subCmd = rawSubCmd.trim(); // Prevent infinite recursion for the root command if (subCmd === command) { - if (this.shouldDowngradeForRedirection(subCmd, allowRedirection)) { + if (await this.shouldDowngradeForRedirection(subCmd, allowRedirection)) { debugLogger.debug( `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`, ); @@ -336,7 +336,7 @@ export class PolicyEngine { // Check for redirection in allowed sub-commands if ( subDecision === PolicyDecision.ALLOW && - this.shouldDowngradeForRedirection(subCmd, allowRedirection) + (await this.shouldDowngradeForRedirection(subCmd, allowRedirection)) ) { debugLogger.debug( `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`, diff --git a/packages/core/src/policy/shell-safety.test.ts b/packages/core/src/policy/shell-safety.test.ts index 340264485e..9e28b86501 100644 --- a/packages/core/src/policy/shell-safety.test.ts +++ b/packages/core/src/policy/shell-safety.test.ts @@ -71,6 +71,7 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => { hasRedirection: (command: string) => // Simple regex check sufficient for testing the policy engine's handling of the *result* of hasRedirection /[><]/.test(command), + }; }); diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts new file mode 100644 index 0000000000..89942b9bb8 --- /dev/null +++ b/packages/core/src/services/sandboxManager.test.ts @@ -0,0 +1,149 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { StandardSandboxManager, SandboxProfile } from './sandboxManager.js'; +import type { Config } from '../config/config.js'; +import * as fs from 'node:fs'; + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs'); + return { + ...actual, + mkdtempSync: vi.fn().mockReturnValue('/tmp/gemini-sandbox-123'), + writeFileSync: vi.fn(), + }; +}); + +describe('StandardSandboxManager', () => { + const mockConfig = { + getSandbox: vi.fn(), + } as unknown as Config; + + const manager = new StandardSandboxManager(mockConfig); + const originalPlatform = process.platform; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.stubGlobal('process', { ...process, platform: originalPlatform }); + }); + + it('should return original command when sandbox is disabled', async () => { + vi.mocked(mockConfig.getSandbox).mockReturnValue({ enabled: false }); + + const result = await manager.prepareCommand({ + command: 'ls', + args: ['-la'], + cwd: '/tmp', + }); + + expect(result).toEqual({ + program: 'ls', + args: ['-la'], + }); + }); + + it('should return original command when sandbox is undefined', async () => { + vi.mocked(mockConfig.getSandbox).mockReturnValue(undefined); + + const result = await manager.prepareCommand({ + command: 'ls', + args: ['-la'], + cwd: '/tmp', + }); + + expect(result).toEqual({ + program: 'ls', + args: ['-la'], + }); + }); + + it('should return sandboxed command on macOS when enabled', async () => { + vi.stubGlobal('process', { ...process, platform: 'darwin' }); + vi.mocked(mockConfig.getSandbox).mockReturnValue({ enabled: true, allowedPaths: ['/extra/path'] }); + + const result = await manager.prepareCommand({ + command: 'ls', + args: ['-la'], + cwd: '/my/workspace', + }); + + expect(result.program).toBe('/usr/bin/sandbox-exec'); + expect(result.args[0]).toBe('-f'); + expect(result.args[1]).toContain('sandbox.sb'); + expect(result.args[2]).toBe('ls'); + expect(result.args[3]).toBe('-la'); + + // Verify profile content + const writeCall = vi.mocked(fs.writeFileSync).mock.calls[0]; + const profileContent = writeCall[1] as string; + + expect(profileContent).toContain('(version 1)'); + expect(profileContent).toContain('(deny default)'); + expect(profileContent).toContain('(allow pseudo-tty)'); + expect(profileContent).toContain('(subpath "/my/workspace")'); + expect(profileContent).toContain('(subpath "/extra/path")'); + expect(profileContent).toContain('(subpath "/usr/lib")'); + expect(profileContent).toContain('(allow file-read* file-write* (subpath "/my/workspace"))'); + }); + + it('should use READ_ONLY profile when specified', async () => { + vi.stubGlobal('process', { ...process, platform: 'darwin' }); + vi.mocked(mockConfig.getSandbox).mockReturnValue({ enabled: true }); + + await manager.prepareCommand({ + command: 'grep', + args: ['pattern'], + cwd: '/my/workspace', + profile: SandboxProfile.READ_ONLY, + }); + + const writeCall = vi.mocked(fs.writeFileSync).mock.calls[0]; + const profileContent = writeCall[1] as string; + + expect(profileContent).toContain('(allow file-read* (subpath "/my/workspace"))'); + expect(profileContent).not.toContain('(allow file-read* file-write* (subpath "/my/workspace"))'); + }); + + it('should use WORKSPACE_WRITE profile when explicitly specified', async () => { + vi.stubGlobal('process', { ...process, platform: 'darwin' }); + vi.mocked(mockConfig.getSandbox).mockReturnValue({ enabled: true }); + + await manager.prepareCommand({ + command: 'ls', + args: ['-la'], + cwd: '/my/workspace', + profile: SandboxProfile.WORKSPACE_WRITE, + }); + + const writeCall = vi.mocked(fs.writeFileSync).mock.calls[0]; + const profileContent = writeCall[1] as string; + + expect(profileContent).toContain('(allow file-read* file-write* (subpath "/my/workspace"))'); + }); + + it('should fallback to original command if sandbox setup fails', async () => { + vi.stubGlobal('process', { ...process, platform: 'darwin' }); + vi.mocked(mockConfig.getSandbox).mockReturnValue({ enabled: true }); + vi.mocked(fs.writeFileSync).mockImplementation(() => { + throw new Error('Disk full'); + }); + + const result = await manager.prepareCommand({ + command: 'ls', + args: ['-la'], + cwd: '/tmp', + }); + + expect(result).toEqual({ + program: 'ls', + args: ['-la'], + }); + }); +}); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts new file mode 100644 index 0000000000..bd4452c65c --- /dev/null +++ b/packages/core/src/services/sandboxManager.ts @@ -0,0 +1,248 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import type { Config } from '../config/config.js'; +import { debugLogger } from '../utils/debugLogger.js'; + +/** + * Defines different levels of access for the sandbox. + */ +export enum SandboxProfile { + /** + * Only allows read access to the workspace and specified paths. + */ + READ_ONLY = 'READ_ONLY', + /** + * Allows both read and write access to the workspace and specified paths. + */ + WORKSPACE_WRITE = 'WORKSPACE_WRITE', +} + +/** + * Represents a command that has been modified to run in a sandbox. + */ +export interface SandboxedCommand { + /** + * The program to execute (may be a sandbox utility like 'sandbox-exec'). + */ + program: string; + /** + * The arguments to pass to the program. + */ + args: string[]; + /** + * Optional cleanup function to be called after the command finishes. + */ + cleanup?: () => void; +} + +/** + * Options for preparing a sandboxed command. + */ +export interface SandboxOptions { + command: string; + args: string[]; + cwd: string; + env?: NodeJS.ProcessEnv; + /** + * The profile to use for sandboxing (e.g., READ_ONLY, WORKSPACE_WRITE). + * Defaults to WORKSPACE_WRITE if not specified. + */ + profile?: SandboxProfile; +} + +/** + * Manages the preparation and execution of sandboxed commands. + */ +export interface SandboxManager { + /** + * Prepares a command for sandboxed execution. + * + * @param options The options for the command. + * @returns A promise that resolves to a SandboxedCommand. + */ + prepareCommand(options: SandboxOptions): Promise; + + /** + * Prepares a command for sandboxed execution synchronously. + * + * @param options The options for the command. + * @returns A SandboxedCommand. + */ + prepareCommandSync(options: SandboxOptions): SandboxedCommand; +} + +/** + * Standard implementation of the SandboxManager. + */ +export class StandardSandboxManager implements SandboxManager { + constructor(private readonly config: Config) {} + + async prepareCommand(options: SandboxOptions): Promise { + return this.prepareCommandSync(options); + } + + prepareCommandSync(options: SandboxOptions): SandboxedCommand { + const sandboxConfig = this.config.getSandbox(); + + // If sandbox is not enabled or not configured, return the original command. + if (!sandboxConfig?.enabled) { + return { + program: options.command, + args: options.args, + }; + } + + // Default to WORKSPACE_WRITE if not specified. + const optionsWithProfile = { + ...options, + profile: options.profile ?? SandboxProfile.WORKSPACE_WRITE, + }; + + // Handle macOS Seatbelt sandboxing. + if (process.platform === 'darwin') { + try { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-sandbox-')); + const profilePath = path.join(tempDir, 'sandbox.sb'); + + // Start with explicitly allowed paths from config + const allowedPaths = [...(sandboxConfig.allowedPaths || [])]; + + // Auto-detect Git Worktree metadata paths + try { + const gitDotPath = path.join(options.cwd, '.git'); + if (fs.existsSync(gitDotPath)) { + const stat = fs.lstatSync(gitDotPath); + if (stat.isFile()) { + // It's a worktree link (contains "gitdir: /path/to/repo/.git/worktrees/name") + const content = fs.readFileSync(gitDotPath, 'utf8').trim(); + const match = content.match(/^gitdir:\s*(.+)$/); + if (match?.[1]) { + const gitDir = path.resolve(options.cwd, match[1]); + allowedPaths.push(gitDir); + // Also allow the parent .git dir which often contains shared config/hooks + allowedPaths.push(path.dirname(gitDir)); + // And the main repo root if possible + allowedPaths.push(path.dirname(path.dirname(gitDir))); + } + } else if (stat.isDirectory()) { + allowedPaths.push(gitDotPath); + } + } + } catch (e) { + debugLogger.debug('Failed to auto-detect git metadata paths:', e); + } + + fs.writeFileSync( + profilePath, + this.generateSeatbeltProfile(optionsWithProfile, allowedPaths), + ); + + return { + program: '/usr/bin/sandbox-exec', + args: ['-f', profilePath, options.command, ...options.args], + cleanup: () => { + // Temporarily disabled cleanup for debugging + console.error(`[DEBUG] Sandbox profile kept at: ${profilePath}`); + /* + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch (error) { + debugLogger.error('macOS Sandbox cleanup failed:', error); + } + */ + }, + }; + } catch (error) { + // Log failure but fallback to original command to ensure execution. + debugLogger.error('macOS Sandbox setup failed:', error); + } + } + + // TODO: Support bwrap (Linux) and Restricted Tokens (Windows). + return { + program: options.command, + args: options.args, + }; + } + + /** + * Generates a macOS Seatbelt (.sb) profile content. + */ + private generateSeatbeltProfile( + options: SandboxOptions, + allowedPaths: string[], + ): string { + const isReadOnly = options.profile === SandboxProfile.READ_ONLY; + const workspacePermission = isReadOnly ? 'file-read*' : 'file-read* file-write*'; + + const rules = [ + '(version 1)', + '(deny default)', + '(allow process-fork)', + '(allow process-exec)', + '(allow signal (target same-sandbox))', + '(allow signal (target self))', + '(allow sysctl-read)', + '(allow pseudo-tty)', + '(allow mach-lookup)', + '(allow network-outbound)', + '(allow network-inbound (local ip "localhost:9229"))', + + // Essential system paths (Read-only) + '(allow file-read*', + ' (literal "/")', + ' (subpath "/usr")', + ' (subpath "/bin")', + ' (subpath "/sbin")', + ' (subpath "/Library")', + ' (subpath "/System")', + ' (subpath "/private")', + ' (subpath "/dev")', + ' (subpath "/etc")', + ' (subpath "/opt")', + ' (subpath "/Applications")', + ')', + + // Executable mapping + '(allow file-map-executable (subpath "/usr"))', + '(allow file-map-executable (subpath "/System"))', + '(allow file-map-executable (subpath "/bin"))', + '(allow file-map-executable (subpath "/sbin"))', + '(allow file-map-executable (subpath "/Library"))', + + // Standard special files and IOCTL + '(allow file-read* file-write-data (literal "/dev/null"))', + '(allow file-read* file-write-data (literal "/dev/zero"))', + '(allow file-read* file-write* (literal "/dev/stdout"))', + '(allow file-read* file-write* (literal "/dev/stderr"))', + '(allow file-read* file-write* (literal "/dev/tty"))', + '(allow file-ioctl (regex #"^/dev/tty.*"))', + '(allow file-ioctl (literal "/dev/ptmx"))', + + // Git and User Config (Read-only) + `(allow file-read* (literal "${path.join(os.homedir(), '.gitconfig')}"))`, + `(allow file-read* (subpath "${path.join(os.homedir(), '.config/git')}"))`, + ...(process.env['SSH_AUTH_SOCK'] ? [`(allow file-read* file-write* (literal "${process.env['SSH_AUTH_SOCK']}"))`] : []), + + // Project Workspace and Temp + `(allow ${workspacePermission} (subpath "${path.resolve(options.cwd)}"))`, + ...allowedPaths.map(p => `(allow ${workspacePermission} (subpath "${path.resolve(p)}"))`), + '(allow file-read* file-write* (subpath "/private/tmp"))', + '(allow file-read* file-write* (subpath "/tmp"))', + '(allow file-read* file-write* (subpath "/var/tmp"))', + '(allow file-read* file-write* (subpath "/private/var/folders"))', + + // Metadata access + '(allow file-read-metadata)', + ]; + + return rules.join('\n'); + } +} diff --git a/packages/core/src/services/shellExecutionService.sandbox.test.ts b/packages/core/src/services/shellExecutionService.sandbox.test.ts new file mode 100644 index 0000000000..51ae276618 --- /dev/null +++ b/packages/core/src/services/shellExecutionService.sandbox.test.ts @@ -0,0 +1,172 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + vi, + describe, + it, + expect, + beforeEach, + afterEach, +} from 'vitest'; +import { ShellExecutionService } from './shellExecutionService.js'; +import type { SandboxManager } from './sandboxManager.js'; +import * as cp from 'node:child_process'; +import EventEmitter from 'node:events'; + +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...(actual as object), + spawn: vi.fn(), + }; +}); + +vi.mock('../utils/getPty.js', () => ({ + getPty: vi.fn(), +})); + +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + resolveExecutable: vi.fn(), + }; +}); + +describe('ShellExecutionService Sandbox Wrapping', () => { + const mockSandboxManager: SandboxManager = { + prepareCommand: vi.fn(), + prepareCommandSync: vi.fn(), + }; + + beforeEach(() => { + vi.clearAllMocks(); + ShellExecutionService.setSandboxManager(mockSandboxManager); + }); + + afterEach(() => { + // @ts-expect-error accessing private + ShellExecutionService.sandboxManager = undefined; + }); + + it('should wrap the command using sandboxManager and call cleanup', async () => { + const mockCleanup = vi.fn(); + vi.mocked(mockSandboxManager.prepareCommand).mockResolvedValue({ + program: 'sandbox-exec', + args: ['-f', 'profile.sb', '/bin/zsh', '-c', 'echo hi'], + cleanup: mockCleanup, + }); + + const mockChildEmitter = new EventEmitter(); + // @ts-expect-error mock child + const mockChild: cp.ChildProcess = Object.assign(mockChildEmitter, { + pid: 123, + stdout: new EventEmitter(), + stderr: new EventEmitter(), + }); + vi.mocked(cp.spawn).mockReturnValue(mockChild); + + const abortController = new AbortController(); + const handle = await ShellExecutionService.execute( + 'echo hi', + '/tmp', + () => {}, + abortController.signal, + false, // child_process + { + sanitizationConfig: { + allowedEnvironmentVariables: [], + blockedEnvironmentVariables: [], + enableEnvironmentVariableRedaction: false, + }, + }, + ); + + // Verify prepareCommand was called + expect(mockSandboxManager.prepareCommand).toHaveBeenCalled(); + + // Verify spawn was called with wrapped arguments + expect(cp.spawn).toHaveBeenCalledWith( + 'sandbox-exec', + ['-f', 'profile.sb', '/bin/zsh', '-c', 'echo hi'], + expect.any(Object), + ); + + // Simulate exit + mockChild.emit('exit', 0, null); + + await handle.result; + + // Verify cleanup was called + expect(mockCleanup).toHaveBeenCalled(); + }); + + it('should wrap the command using sandboxManager for PTY', async () => { + const mockPtyEmitter = new EventEmitter(); + // @ts-expect-error mock pty process + const mockPtyProcess: import('@lydell/node-pty').IPty = Object.assign( + mockPtyEmitter, + { + pid: 456, + onData: vi.fn(), + onExit: vi.fn(), + }, + ); + + const mockPtyModule = { + spawn: vi.fn().mockReturnValue(mockPtyProcess), + }; + + const { getPty } = await import('../utils/getPty.js'); + vi.mocked(getPty).mockResolvedValue({ + name: 'node-pty', + module: mockPtyModule, + }); + + const { resolveExecutable } = await import('../utils/shell-utils.js'); + vi.mocked(resolveExecutable).mockResolvedValue('/bin/zsh'); + + const mockCleanup = vi.fn(); + vi.mocked(mockSandboxManager.prepareCommand).mockResolvedValue({ + program: 'sandbox-exec', + args: ['-f', 'profile.sb', '/bin/zsh', '-c', 'echo pty'], + cleanup: mockCleanup, + }); + + const abortController = new AbortController(); + const handle = await ShellExecutionService.execute( + 'echo pty', + '/tmp', + () => {}, + abortController.signal, + true, // shouldUseNodePty + { + sanitizationConfig: { + allowedEnvironmentVariables: [], + blockedEnvironmentVariables: [], + enableEnvironmentVariableRedaction: false, + }, + }, + ); + + expect(mockSandboxManager.prepareCommand).toHaveBeenCalled(); + expect(mockPtyModule.spawn).toHaveBeenCalledWith( + 'sandbox-exec', + ['-f', 'profile.sb', '/bin/zsh', '-c', 'echo pty'], + expect.any(Object), + ); + + // Simulate exit + const exitCallback = (mockPtyProcess.onExit as any).mock.calls[0][0]; + exitCallback({ exitCode: 0 }); + + await handle.result; + + expect(mockCleanup).toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index fdb2ca79b5..4df1154b17 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -12,8 +12,10 @@ import os from 'node:os'; import type { IPty } from '@lydell/node-pty'; import { getCachedEncodingForBuffer } from '../utils/systemEncoding.js'; import { + getSandboxManager, getShellConfiguration, resolveExecutable, + setSandboxManager as setCentralSandboxManager, type ShellType, } from '../utils/shell-utils.js'; import { isBinary } from '../utils/textUtils.js'; @@ -22,10 +24,8 @@ import { serializeTerminalToObject, type AnsiOutput, } from '../utils/terminalSerializer.js'; -import { - sanitizeEnvironment, - type EnvironmentSanitizationConfig, -} from './environmentSanitization.js'; +import { sanitizeEnvironment, type EnvironmentSanitizationConfig } from './environmentSanitization.js'; +import { type SandboxManager, SandboxProfile } from './sandboxManager.js'; import { killProcessGroup } from '../utils/process-utils.js'; const { Terminal } = pkg; @@ -196,6 +196,10 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { */ export class ShellExecutionService { + static setSandboxManager(manager: SandboxManager): void { + setCentralSandboxManager(manager); + } + private static activePtys = new Map(); private static activeChildProcesses = new Map(); private static exitedPtyInfo = new Map< @@ -227,18 +231,48 @@ export class ShellExecutionService { abortSignal: AbortSignal, shouldUseNodePty: boolean, shellExecutionConfig: ShellExecutionConfig, + profile?: SandboxProfile, ): Promise { + const { executable, argsPrefix, shell } = getShellConfiguration(); + const resolvedExecutable = (await resolveExecutable(executable)) ?? executable; + const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); + const originalArgs = [...argsPrefix, guardedCommand]; + + let finalExecutable = resolvedExecutable; + let finalArgs = originalArgs; + let cleanup: (() => void) | undefined; + + const manager = getSandboxManager(); + if (manager) { + const sandboxed = await manager.prepareCommand({ + command: resolvedExecutable, + args: originalArgs, + cwd, + env: sanitizeEnvironment( + process.env, + shellExecutionConfig.sanitizationConfig, + ), + profile, + }); + finalExecutable = sandboxed.program; + finalArgs = sandboxed.args; + cleanup = sandboxed.cleanup; + console.error(`[DEBUG] Executing: ${finalExecutable} ${finalArgs.join(' ')}`); + } + if (shouldUseNodePty) { const ptyInfo = await getPty(); if (ptyInfo) { try { return await this.executeWithPty( - commandToExecute, + finalExecutable, + finalArgs, cwd, onOutputEvent, abortSignal, shellExecutionConfig, ptyInfo, + cleanup, ); } catch (_e) { // Fallback to child_process @@ -247,11 +281,13 @@ export class ShellExecutionService { } return this.childProcessFallback( - commandToExecute, + finalExecutable, + finalArgs, cwd, onOutputEvent, abortSignal, shellExecutionConfig.sanitizationConfig, + cleanup, ); } @@ -293,17 +329,16 @@ export class ShellExecutionService { } private static childProcessFallback( - commandToExecute: string, + executable: string, + spawnArgs: string[], cwd: string, onOutputEvent: (event: ShellOutputEvent) => void, abortSignal: AbortSignal, sanitizationConfig: EnvironmentSanitizationConfig, + cleanup?: () => void, ): ShellExecutionHandle { try { const isWindows = os.platform() === 'win32'; - const { executable, argsPrefix, shell } = getShellConfiguration(); - const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); - const spawnArgs = [...argsPrefix, guardedCommand]; const child = cpSpawn(executable, spawnArgs, { cwd, @@ -414,7 +449,7 @@ export class ShellExecutionService { code: number | null, signal: NodeJS.Signals | null, ) => { - const { finalBuffer } = cleanup(); + const { finalBuffer } = cleanupProcess(); let combinedOutput = state.output; @@ -443,6 +478,10 @@ export class ShellExecutionService { this.activeListeners.delete(child.pid); } + if (cleanup) { + cleanup(); + } + resolve({ rawOutput: finalBuffer, output: finalStrippedOutput, @@ -478,7 +517,7 @@ export class ShellExecutionService { handleExit(code, signal); }); - function cleanup() { + function cleanupProcess() { exited = true; abortSignal.removeEventListener('abort', abortHandler); if (stdoutDecoder) { @@ -541,12 +580,14 @@ export class ShellExecutionService { } private static async executeWithPty( - commandToExecute: string, + executable: string, + args: string[], cwd: string, onOutputEvent: (event: ShellOutputEvent) => void, abortSignal: AbortSignal, shellExecutionConfig: ShellExecutionConfig, ptyInfo: PtyImplementation, + cleanup?: () => void, ): Promise { if (!ptyInfo) { // This should not happen, but as a safeguard... @@ -555,17 +596,6 @@ export class ShellExecutionService { try { const cols = shellExecutionConfig.terminalWidth ?? 80; const rows = shellExecutionConfig.terminalHeight ?? 30; - const { executable, argsPrefix, shell } = getShellConfiguration(); - - const resolvedExecutable = await resolveExecutable(executable); - if (!resolvedExecutable) { - throw new Error( - `Shell executable "${executable}" not found in PATH or at absolute location. Please ensure the shell is installed and available in your environment.`, - ); - } - - const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); - const args = [...argsPrefix, guardedCommand]; // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const ptyProcess = ptyInfo.module.spawn(executable, args, { @@ -825,6 +855,10 @@ export class ShellExecutionService { const finalBuffer = Buffer.concat(outputChunks); + if (cleanup) { + cleanup(); + } + resolve({ rawOutput: finalBuffer, output: getFullBufferText(headlessTerminal), diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index c7e676951a..dea582fbf6 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -11,6 +11,7 @@ import path from 'node:path'; import { spawn } from 'node:child_process'; import { globStream } from 'glob'; import { execStreaming } from '../utils/shell-utils.js'; +import { SandboxProfile } from '../services/sandboxManager.js'; import { DEFAULT_TOTAL_MAX_MATCHES, DEFAULT_SEARCH_TIMEOUT_MS, @@ -370,6 +371,7 @@ class GrepToolInvocation extends BaseToolInvocation< cwd: absolutePath, signal: options.signal, allowedExitCodes: [0, 1], + profile: SandboxProfile.READ_ONLY, }); const results: GrepMatch[] = []; @@ -441,6 +443,7 @@ class GrepToolInvocation extends BaseToolInvocation< cwd: absolutePath, signal: options.signal, allowedExitCodes: [0, 1], + profile: SandboxProfile.READ_ONLY, }); for await (const line of generator) { diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 0f7b58c39a..177ea84fe0 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -36,6 +36,7 @@ import { populateMcpServerCommand, type McpContext, } from './mcp-client.js'; +import { SandboxedTransport } from './sandboxed-transport.js'; import type { ToolRegistry } from './tool-registry.js'; import type { ResourceRegistry } from '../resources/resource-registry.js'; import * as fs from 'node:fs'; @@ -74,6 +75,20 @@ vi.mock('../mcp/oauth-token-storage.js'); vi.mock('../mcp/oauth-utils.js'); vi.mock('google-auth-library'); import { GoogleAuth } from 'google-auth-library'; +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + prepareSandboxedCommand: vi.fn().mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ), + }; +}); +import { prepareSandboxedCommand } from '../utils/shell-utils.js'; vi.mock('../utils/events.js', () => ({ coreEvents: { @@ -99,6 +114,13 @@ describe('mcp-client', () => { path.join(os.tmpdir(), 'gemini-agent-test-'), ); workspaceContext = new WorkspaceContext(testWorkspace); + vi.mocked(prepareSandboxedCommand).mockImplementation((cmd: string, args: string[]) => + Promise.resolve({ + program: cmd, + args, + cleanup: vi.fn(), + }), + ); }); afterEach(() => { @@ -1945,6 +1967,50 @@ describe('mcp-client', () => { expect(callArgs.env!['GEMINI_CLI']).toBe('1'); }); + it('should apply sandboxing to stdio transport', async () => { + const mockedTransport = vi + .spyOn(SdkClientStdioLib, 'StdioClientTransport') + .mockReturnValue({} as SdkClientStdioLib.StdioClientTransport); + + const mockCleanup = vi.fn(); + vi.mocked(prepareSandboxedCommand).mockResolvedValue({ + program: 'sandboxed-command', + args: ['--sandboxed-arg'], + cleanup: mockCleanup, + }); + + const transport = await createTransport( + 'test-server', + { + command: 'original-command', + args: ['--original-arg'], + cwd: 'test/cwd', + }, + false, + MOCK_CONTEXT, + ); + + expect(prepareSandboxedCommand).toHaveBeenCalledWith( + 'original-command', + ['--original-arg'], + expect.objectContaining({ + cwd: 'test/cwd', + env: expect.anything(), + }), + ); + + expect(mockedTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: 'sandboxed-command', + args: ['--sandboxed-arg'], + cwd: 'test/cwd', + stderr: 'pipe', + }), + ); + + expect(transport).toBeInstanceOf(SandboxedTransport); + }); + it('should exclude extension settings with undefined values from environment', async () => { const mockedTransport = vi .spyOn(SdkClientStdioLib, 'StdioClientTransport') diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index af55facaa3..bc8944c01e 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -46,6 +46,7 @@ import { GoogleCredentialProvider } from '../mcp/google-auth-provider.js'; import { ServiceAccountImpersonationProvider } from '../mcp/sa-impersonation-provider.js'; import { DiscoveredMCPTool } from './mcp-tool.js'; import { XcodeMcpBridgeFixTransport } from './xcode-mcp-fix-transport.js'; +import { SandboxedTransport } from './sandboxed-transport.js'; import type { CallableTool, FunctionCall, Part, Tool } from '@google/genai'; import { basename } from 'node:path'; @@ -84,6 +85,7 @@ import { GEMINI_CLI_IDENTIFICATION_ENV_VAR, GEMINI_CLI_IDENTIFICATION_ENV_VAR_VALUE, } from '../services/shellExecutionService.js'; +import { prepareSandboxedCommand } from '../utils/shell-utils.js'; export const MCP_DEFAULT_TIMEOUT_MSEC = 10 * 60 * 1000; // default to 10 minutes @@ -2218,14 +2220,31 @@ export async function createTransport( } } + const { + program: sandboxedCommand, + args: sandboxedArgs, + cleanup, + } = await prepareSandboxedCommand( + mcpServerConfig.command, + mcpServerConfig.args || [], + { + cwd: mcpServerConfig.cwd, + env: finalEnv, + }, + ); + let transport: Transport = new StdioClientTransport({ - command: mcpServerConfig.command, - args: mcpServerConfig.args || [], + command: sandboxedCommand, + args: sandboxedArgs, env: finalEnv, cwd: mcpServerConfig.cwd, stderr: 'pipe', }); + if (cleanup) { + transport = new SandboxedTransport(transport, cleanup); + } + // Fix for Xcode 26.3 mcpbridge non-compliant responses // It returns JSON in `content` instead of `structuredContent` if ( @@ -2236,15 +2255,17 @@ export async function createTransport( } if (debugMode) { - // The `XcodeMcpBridgeFixTransport` wrapper hides the underlying `StdioClientTransport`, + // Wrapper transports hide the underlying `StdioClientTransport`, // which exposes `stderr` for debug logging. We need to unwrap it to attach the listener. - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const underlyingTransport = - transport instanceof XcodeMcpBridgeFixTransport - ? // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion - (transport as any).transport - : transport; + let underlyingTransport: Transport = transport; + while ( + underlyingTransport instanceof XcodeMcpBridgeFixTransport || + underlyingTransport instanceof SandboxedTransport + ) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-type-assertion + underlyingTransport = (underlyingTransport as any).transport; + } if ( underlyingTransport instanceof StdioClientTransport && diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 000b4f0071..cc945d196d 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -30,6 +30,7 @@ import { } from '../utils/ignorePatterns.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { execStreaming } from '../utils/shell-utils.js'; +import { SandboxProfile } from '../services/sandboxManager.js'; import { DEFAULT_TOTAL_MAX_MATCHES, DEFAULT_SEARCH_TIMEOUT_MS, @@ -460,6 +461,7 @@ class GrepToolInvocation extends BaseToolInvocation< const generator = execStreaming(rgPath, rgArgs, { signal: options.signal, allowedExitCodes: [0, 1], + profile: SandboxProfile.READ_ONLY, }); let matchesFound = 0; diff --git a/packages/core/src/tools/sandboxed-transport.test.ts b/packages/core/src/tools/sandboxed-transport.test.ts new file mode 100644 index 0000000000..897522b467 --- /dev/null +++ b/packages/core/src/tools/sandboxed-transport.test.ts @@ -0,0 +1,73 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SandboxedTransport } from './sandboxed-transport.js'; +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; + +describe('SandboxedTransport', () => { + it('should call cleanup when the transport is closed via close()', async () => { + const mockTransport: Transport = { + close: vi.fn().mockResolvedValue(undefined), + start: vi.fn().mockResolvedValue(undefined), + send: vi.fn().mockResolvedValue(undefined), + }; + + const cleanup = vi.fn(); + const sandboxedTransport = new SandboxedTransport(mockTransport, cleanup); + + await sandboxedTransport.close(); + + expect(mockTransport.close).toHaveBeenCalled(); + expect(cleanup).toHaveBeenCalled(); + }); + + it('should call cleanup when the underlying transport calls onclose', () => { + const mockTransport: Transport = { + close: vi.fn().mockResolvedValue(undefined), + start: vi.fn().mockResolvedValue(undefined), + send: vi.fn().mockResolvedValue(undefined), + }; + const cleanup = vi.fn(); + const sandboxedTransport = new SandboxedTransport(mockTransport, cleanup); + + let oncloseCalled = false; + sandboxedTransport.onclose = () => { + oncloseCalled = true; + }; + + // Simulate underlying transport closing + mockTransport.onclose?.(); + + expect(cleanup).toHaveBeenCalled(); + expect(oncloseCalled).toBe(true); + }); + + it('should forward messages and errors', () => { + const mockTransport: Transport = { + close: vi.fn().mockResolvedValue(undefined), + start: vi.fn().mockResolvedValue(undefined), + send: vi.fn().mockResolvedValue(undefined), + }; + const cleanup = vi.fn(); + const sandboxedTransport = new SandboxedTransport(mockTransport, cleanup); + + const receivedMessages: any[] = []; + sandboxedTransport.onmessage = (msg) => receivedMessages.push(msg); + + const receivedErrors: Error[] = []; + sandboxedTransport.onerror = (err) => receivedErrors.push(err); + + const testMessage = { jsonrpc: '2.0', method: 'test' } as any; + mockTransport.onmessage?.(testMessage); + + const testError = new Error('test error'); + mockTransport.onerror?.(testError); + + expect(receivedMessages).toEqual([testMessage]); + expect(receivedErrors).toEqual([testError]); + }); +}); diff --git a/packages/core/src/tools/sandboxed-transport.ts b/packages/core/src/tools/sandboxed-transport.ts new file mode 100644 index 0000000000..b545f3415f --- /dev/null +++ b/packages/core/src/tools/sandboxed-transport.ts @@ -0,0 +1,45 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; +import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js'; + +/** + * A wrapper transport that handles sandbox environment cleanup. + */ +export class SandboxedTransport implements Transport { + constructor( + private readonly transport: Transport, + private readonly cleanup: () => void, + ) { + this.transport.onmessage = (message) => this.onmessage?.(message); + this.transport.onerror = (error) => this.onerror?.(error); + this.transport.onclose = () => { + this.cleanup(); + this.onclose?.(); + }; + } + + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: JSONRPCMessage) => void; + + async start(): Promise { + await this.transport.start(); + } + + async close(): Promise { + try { + await this.transport.close(); + } finally { + this.cleanup(); + } + } + + async send(message: JSONRPCMessage): Promise { + await this.transport.send(message); + } +} diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index d3e47de17f..5079e88e2c 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -43,6 +43,7 @@ vi.mock('../utils/summarizer.js'); import { initializeShellParsers } from '../utils/shell-utils.js'; import { ShellTool, OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; +import { SandboxProfile } from '../services/sandboxManager.js'; import { debugLogger } from '../index.js'; import { type Config } from '../config/config.js'; import { @@ -275,6 +276,7 @@ describe('ShellTool', () => { expect.any(AbortSignal), false, { pager: 'cat', sanitizationConfig: {} }, + SandboxProfile.WORKSPACE_WRITE, ); expect(result.llmContent).toContain('Background PIDs: 54322'); // The file should be deleted by the tool @@ -300,6 +302,7 @@ describe('ShellTool', () => { expect.any(AbortSignal), false, { pager: 'cat', sanitizationConfig: {} }, + SandboxProfile.WORKSPACE_WRITE, ); }); @@ -321,6 +324,7 @@ describe('ShellTool', () => { expect.any(AbortSignal), false, { pager: 'cat', sanitizationConfig: {} }, + SandboxProfile.WORKSPACE_WRITE, ); }); @@ -367,6 +371,7 @@ describe('ShellTool', () => { expect.any(AbortSignal), false, { pager: 'cat', sanitizationConfig: {} }, + SandboxProfile.WORKSPACE_WRITE, ); }, 20000, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 4ea83b0af4..ccd7a6f64f 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -31,6 +31,7 @@ import { type ShellExecutionConfig, type ShellOutputEvent, } from '../services/shellExecutionService.js'; +import { SandboxProfile } from '../services/sandboxManager.js'; import { formatBytes } from '../utils/formatters.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { @@ -98,9 +99,10 @@ export class ShellToolInvocation extends BaseToolInvocation< outcome === ToolConfirmationOutcome.ProceedAlways ) { const command = stripShellWrapper(this.params.command); - const rootCommands = [...new Set(getCommandRoots(command))]; - if (rootCommands.length > 0) { - return { commandPrefix: rootCommands }; + const rootCommands = getCommandRoots(command); + const uniqueRootCommands = [...new Set(rootCommands)]; + if (uniqueRootCommands.length > 0) { + return { commandPrefix: uniqueRootCommands }; } return { commandPrefix: this.params.command }; } @@ -112,14 +114,14 @@ export class ShellToolInvocation extends BaseToolInvocation< ): Promise { const command = stripShellWrapper(this.params.command); - const parsed = parseCommandDetails(command); + const parsed = await parseCommandDetails(command); let rootCommandDisplay = ''; if (!parsed || parsed.hasError || parsed.details.length === 0) { // Fallback if parser fails const fallback = command.trim().split(/\s+/)[0]; rootCommandDisplay = fallback || 'shell command'; - if (hasRedirection(command)) { + if (await hasRedirection(command)) { rootCommandDisplay += ', redirection'; } } else { @@ -128,7 +130,9 @@ export class ShellToolInvocation extends BaseToolInvocation< .join(', '); } - const rootCommands = [...new Set(getCommandRoots(command))]; + const rootCommands = await getCommandRoots(command); + const uniqueRootCommands = [...new Set(rootCommands)]; + const redirection = await hasRedirection(command); // Rely entirely on PolicyEngine for interactive confirmation. // If we are here, it means PolicyEngine returned ASK_USER (or no message bus), @@ -138,7 +142,8 @@ export class ShellToolInvocation extends BaseToolInvocation< title: 'Confirm Shell Command', command: this.params.command, rootCommand: rootCommandDisplay, - rootCommands, + rootCommands: uniqueRootCommands, + hasRedirection: redirection, onConfirm: async (_outcome: ToolConfirmationOutcome) => { // Policy updates are now handled centrally by the scheduler }, @@ -278,6 +283,7 @@ export class ShellToolInvocation extends BaseToolInvocation< shellExecutionConfig?.sanitizationConfig ?? this.config.sanitizationConfig, }, + SandboxProfile.WORKSPACE_WRITE, ); if (pid) { diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 0a82cc1510..5a7c2494ce 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -132,7 +132,10 @@ export abstract class BaseToolInvocation< */ protected getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, - ): PolicyUpdateOptions | undefined { + ): + | PolicyUpdateOptions + | undefined + | Promise { return undefined; } @@ -148,7 +151,7 @@ export abstract class BaseToolInvocation< outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave ) { if (this._toolName) { - const options = this.getPolicyUpdateOptions(outcome); + const options = await this.getPolicyUpdateOptions(outcome); void this.messageBus.publish({ type: MessageBusType.UPDATE_POLICY, toolName: this._toolName, @@ -772,6 +775,7 @@ export interface ToolExecuteConfirmationDetails { rootCommand: string; rootCommands: string[]; commands?: string[]; + hasRedirection?: boolean; } export interface ToolMcpConfirmationDetails { diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index cdc1e1d4a5..ebbf2f1ea8 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -4,11 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { exec, execSync, spawn, spawnSync } from 'node:child_process'; +import { exec, execSync } from 'node:child_process'; import { promisify } from 'node:util'; import { once } from 'node:events'; import { debugLogger } from './debugLogger.js'; import { coreEvents, CoreEvent, type EditorSelectedPayload } from './events.js'; +import { spawnAsync, spawnSyncAsync } from './shell-utils.js'; const GUI_EDITORS = [ 'vscode', @@ -302,7 +303,7 @@ export async function openDiff( if (isTerminalEditor(editor)) { try { - const result = spawnSync(diffCommand.command, diffCommand.args, { + const result = await spawnSyncAsync(diffCommand.command, diffCommand.args, { stdio: 'inherit', }); if (result.error) { @@ -317,22 +318,17 @@ export async function openDiff( return; } - return new Promise((resolve, reject) => { - const childProcess = spawn(diffCommand.command, diffCommand.args, { + try { + await spawnAsync(diffCommand.command, diffCommand.args, { stdio: 'inherit', shell: process.platform === 'win32', }); - - childProcess.on('close', (code) => { - if (code === 0) { - resolve(); - } else { - reject(new Error(`${editor} exited with code ${code}`)); - } - }); - - childProcess.on('error', (error) => { - reject(error); - }); - }); + } catch (error) { + if (error instanceof Error && error.message.includes('Command failed with exit code')) { + const match = error.message.match(/exit code (\d+)/); + const code = match ? match[1] : 'unknown'; + throw new Error(`${editor} exited with code ${code}`); + } + throw error; + } } diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 00b3533400..df0fe27027 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -11,15 +11,88 @@ import { quote } from 'shell-quote'; import { spawn, spawnSync, + type SpawnOptions, type SpawnOptionsWithoutStdio, + type SpawnSyncOptions, } from 'node:child_process'; import * as readline from 'node:readline'; import { Language, Parser, Query, type Node, type Tree } from 'web-tree-sitter'; import { loadWasmBinary } from './fileUtils.js'; import { debugLogger } from './debugLogger.js'; +import { + type SandboxManager, + type SandboxedCommand, + SandboxProfile, +} from '../services/sandboxManager.js'; export const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; +let sandboxManager: SandboxManager | undefined; + +/** + * Sets the global SandboxManager to be used for all shell spawning operations. + */ +export function setSandboxManager(manager: SandboxManager): void { + sandboxManager = manager; +} + +/** + * Gets the global SandboxManager. + */ +export function getSandboxManager(): SandboxManager | undefined { + return sandboxManager; +} + +function prepareSandboxedCommandSync( + command: string, + args: string[], + options?: { + cwd?: string | URL; + env?: NodeJS.ProcessEnv; + profile?: SandboxProfile; + }, +): SandboxedCommand { + if (!sandboxManager) return { program: command, args }; + + const cwd = + options?.cwd instanceof URL + ? options.cwd.pathname + : options?.cwd ?? process.cwd(); + + return sandboxManager.prepareCommandSync({ + command, + args, + cwd, + env: options?.env ?? process.env, + profile: options?.profile, + }); +} + +export async function prepareSandboxedCommand( + command: string, + args: string[], + options?: { + cwd?: string | URL; + env?: NodeJS.ProcessEnv; + profile?: SandboxProfile; + }, +): Promise { + if (!sandboxManager) return { program: command, args }; + + const cwd = + options?.cwd instanceof URL + ? options.cwd.pathname + : options?.cwd ?? process.cwd(); + + return sandboxManager.prepareCommand({ + command, + args, + cwd, + env: options?.env ?? process.env, + profile: options?.profile, + }); +} + /** * An identifier for the shell type. */ @@ -445,7 +518,11 @@ function parsePowerShellCommandDetails( } try { - const result = spawnSync( + const { + program: finalCommand, + args: finalArgs, + cleanup, + } = prepareSandboxedCommandSync( executable, [ '-NoLogo', @@ -459,10 +536,19 @@ function parsePowerShellCommandDetails( ...process.env, [POWERSHELL_COMMAND_ENV]: command, }, - encoding: 'utf-8', }, ); + const result = spawnSync(finalCommand, finalArgs, { + env: { + ...process.env, + [POWERSHELL_COMMAND_ENV]: command, + }, + encoding: 'utf-8', + }); + + if (cleanup) cleanup(); + if (result.error || result.status !== 0) { return null; } @@ -737,25 +823,39 @@ export function stripShellWrapper(command: string): string { * @param config The application configuration. * @returns An object with 'allowed' boolean and optional 'reason' string if not allowed. */ -export const spawnAsync = ( +export const spawnAsync = async ( command: string, args: string[], - options?: SpawnOptionsWithoutStdio, -): Promise<{ stdout: string; stderr: string }> => - new Promise((resolve, reject) => { - const child = spawn(command, args, options); + options?: SpawnOptions & { profile?: SandboxProfile }, +): Promise<{ stdout: string; stderr: string }> => { + const { + program: finalCommand, + args: finalArgs, + cleanup, + } = await prepareSandboxedCommand(command, args, options); + + return new Promise((resolve, reject) => { + const child = options + ? spawn(finalCommand, finalArgs, options) + : spawn(finalCommand, finalArgs); + let stdout = ''; let stderr = ''; - child.stdout.on('data', (data) => { - stdout += data.toString(); - }); + if (child.stdout) { + child.stdout.on('data', (data: Buffer | string) => { + stdout += data.toString(); + }); + } - child.stderr.on('data', (data) => { - stderr += data.toString(); - }); + if (child.stderr) { + child.stderr.on('data', (data: Buffer | string) => { + stderr += data.toString(); + }); + } - child.on('close', (code) => { + child.on('close', (code: number | null) => { + if (cleanup) cleanup(); if (code === 0) { resolve({ stdout, stderr }); } else { @@ -763,10 +863,38 @@ export const spawnAsync = ( } }); - child.on('error', (err) => { + child.on('error', (err: Error) => { + if (cleanup) cleanup(); reject(err); }); }); +}; + +/** + * Executes a command synchronously but prepares the sandbox asynchronously. + * This is useful for terminal-based tools that need to inherit stdio. + */ +export const spawnSyncAsync = async ( + command: string, + args: string[], + options?: SpawnSyncOptions & { profile?: SandboxProfile }, +): Promise<{ status: number | null; error?: Error }> => { + const { + program: finalCommand, + args: finalArgs, + cleanup, + } = await prepareSandboxedCommand(command, args, options); + + try { + const result = spawnSync(finalCommand, finalArgs, options); + return { + status: result.status, + error: result.error, + }; + } finally { + if (cleanup) cleanup(); + } +}; /** * Executes a command and yields lines of output as they appear. @@ -782,9 +910,16 @@ export async function* execStreaming( options?: SpawnOptionsWithoutStdio & { signal?: AbortSignal; allowedExitCodes?: number[]; + profile?: SandboxProfile; }, ): AsyncGenerator { - const child = spawn(command, args, { + const { + program: finalCommand, + args: finalArgs, + cleanup, + } = await prepareSandboxedCommand(command, args, options); + + const child = spawn(finalCommand, finalArgs, { ...options, // ensure we don't open a window on windows if possible/relevant windowsHide: true, @@ -845,45 +980,49 @@ export async function* execStreaming( } // Ensure we wait for the process to exit to check codes - await new Promise((resolve, reject) => { - // If an error occurred before we got here (e.g. spawn failure), reject immediately. - if (error) { - reject(error); - return; - } - - function checkExit(code: number | null) { - // If we aborted or killed it manually, we treat it as success (stop waiting) - if (options?.signal?.aborted || killedByGenerator) { - resolve(); + try { + await new Promise((resolve, reject) => { + // If an error occurred before we got here (e.g. spawn failure), reject immediately. + if (error) { + reject(error); return; } - const allowed = options?.allowedExitCodes ?? [0]; - if (code !== null && allowed.includes(code)) { - resolve(); - } else { - // If we have an accumulated error or explicit error event - if (error) reject(error); - else { - const stderr = Buffer.concat(errorChunks).toString('utf8'); - const truncatedMsg = - stderrTotalBytes >= MAX_STDERR_BYTES ? '...[truncated]' : ''; - reject( - new Error( - `Process exited with code ${code}: ${stderr}${truncatedMsg}`, - ), - ); + function checkExit(code: number | null) { + // If we aborted or killed it manually, we treat it as success (stop waiting) + if (options?.signal?.aborted || killedByGenerator) { + resolve(); + return; + } + + const allowed = options?.allowedExitCodes ?? [0]; + if (code !== null && allowed.includes(code)) { + resolve(); + } else { + // If we have an accumulated error or explicit error event + if (error) reject(error); + else { + const stderr = Buffer.concat(errorChunks).toString('utf8'); + const truncatedMsg = + stderrTotalBytes >= MAX_STDERR_BYTES ? '...[truncated]' : ''; + reject( + new Error( + `Process exited with code ${code}: ${stderr}${truncatedMsg}`, + ), + ); + } } } - } - if (child.exitCode !== null) { - checkExit(child.exitCode); - } else { - child.on('close', (code) => checkExit(code)); - child.on('error', (err) => reject(err)); - } - }); + if (child.exitCode !== null) { + checkExit(child.exitCode); + } else { + child.on('close', (code) => checkExit(code)); + child.on('error', (err) => reject(err)); + } + }); + } finally { + if (cleanup) cleanup(); + } } }