From 1b052df52f768889204a2d62f5f75c6dadce5632 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Wed, 25 Mar 2026 17:54:45 +0000 Subject: [PATCH] feat(core): implement Windows sandbox dynamic expansion Phase 1 and 2.1 (#23691) --- packages/core/src/config/config.ts | 10 +- .../core/src/policy/policy-engine.test.ts | 40 +- packages/core/src/policy/policy-engine.ts | 46 +- packages/core/src/policy/types.ts | 9 +- .../src/sandbox/linux/LinuxSandboxManager.ts | 13 + .../sandbox/macos/MacOsSandboxManager.test.ts | 2 +- .../src/sandbox/macos/MacOsSandboxManager.ts | 76 +-- .../core/src/sandbox/macos/commandSafety.ts | 74 ++- .../windows/WindowsSandboxManager.test.ts | 579 ++++++++++++------ .../sandbox/windows/WindowsSandboxManager.ts | 121 +++- .../src/sandbox/windows/commandSafety.test.ts | 50 ++ .../core/src/sandbox/windows/commandSafety.ts | 148 +++++ .../core/src/services/sandboxManager.test.ts | 416 +++++++------ packages/core/src/services/sandboxManager.ts | 38 ++ .../src/services/sandboxManagerFactory.ts | 27 +- .../sandboxedFileSystemService.test.ts | 8 + .../services/shellExecutionService.test.ts | 2 + packages/core/src/utils/shell-utils.ts | 37 +- 18 files changed, 1168 insertions(+), 528 deletions(-) create mode 100644 packages/core/src/sandbox/windows/commandSafety.test.ts create mode 100644 packages/core/src/sandbox/windows/commandSafety.ts diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 795df747cb..a7af5387d6 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1197,10 +1197,7 @@ export class Config implements McpContext, AgentLoopContext { ...params.policyEngineConfig, approvalMode: engineApprovalMode, disableAlwaysAllow: this.disableAlwaysAllow, - toolSandboxEnabled: this.getSandboxEnabled(), - sandboxApprovedTools: - this.sandboxPolicyManager?.getModeConfig(engineApprovalMode) - ?.approvedTools ?? [], + sandboxManager: this._sandboxManager, }, checkerRunner, ); @@ -2392,10 +2389,7 @@ export class Config implements McpContext, AgentLoopContext { ); } - this.policyEngine.setApprovalMode( - mode, - this.sandboxPolicyManager?.getModeConfig(mode)?.approvedTools ?? [], - ); + this.policyEngine.setApprovalMode(mode); this.refreshSandboxManager(); const isPlanModeTransition = diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index 805e4cef70..137ca76aa1 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -22,6 +22,11 @@ import { SafetyCheckDecision } from '../safety/protocol.js'; import type { CheckerRunner } from '../safety/checker-runner.js'; import { initializeShellParsers } from '../utils/shell-utils.js'; import { buildArgsPatterns } from './utils.js'; +import { + NoopSandboxManager, + LocalSandboxManager, + type SandboxManager, +} from '../services/sandboxManager.js'; // Mock shell-utils to ensure consistent behavior across platforms (especially Windows CI) // We want to test PolicyEngine logic, not the shell parser's ability to parse commands @@ -96,7 +101,10 @@ describe('PolicyEngine', () => { runChecker: vi.fn(), } as unknown as CheckerRunner; engine = new PolicyEngine( - { approvalMode: ApprovalMode.DEFAULT }, + { + approvalMode: ApprovalMode.DEFAULT, + sandboxManager: new NoopSandboxManager(), + }, mockCheckerRunner, ); }); @@ -332,7 +340,7 @@ describe('PolicyEngine', () => { engine = new PolicyEngine({ rules, approvalMode: ApprovalMode.AUTO_EDIT, - toolSandboxEnabled: true, + sandboxManager: new LocalSandboxManager(), }); expect((await engine.check({ name: 'edit' }, undefined)).decision).toBe( PolicyDecision.ALLOW, @@ -345,6 +353,29 @@ describe('PolicyEngine', () => { ); }); + it('should respect tools approved by the SandboxManager', async () => { + const mockSandboxManager = { + enabled: true, + prepareCommand: vi.fn(), + isDangerousCommand: vi.fn().mockReturnValue(false), + isKnownSafeCommand: vi + .fn() + .mockImplementation((args) => args[0] === 'npm'), + } as unknown as SandboxManager; + + engine = new PolicyEngine({ + sandboxManager: mockSandboxManager, + defaultDecision: PolicyDecision.ASK_USER, + }); + + const { decision } = await engine.check( + { name: 'run_shell_command', args: { command: 'npm install' } }, + undefined, + ); + + expect(decision).toBe(PolicyDecision.ALLOW); + }); + it('should return ALLOW by default in YOLO mode when no rules match', async () => { engine = new PolicyEngine({ approvalMode: ApprovalMode.YOLO }); @@ -1576,7 +1607,10 @@ describe('PolicyEngine', () => { }, ]; - engine = new PolicyEngine({ rules, toolSandboxEnabled: true }); + engine = new PolicyEngine({ + rules, + sandboxManager: new LocalSandboxManager(), + }); engine.setApprovalMode(ApprovalMode.AUTO_EDIT); const result = await engine.check( diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 4a1dc879af..18ab20bb14 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -6,9 +6,12 @@ import { type FunctionCall } from '@google/genai'; import { - isDangerousCommand, - isKnownSafeCommand, -} from '../sandbox/macos/commandSafety.js'; + SHELL_TOOL_NAMES, + initializeShellParsers, + splitCommands, + hasRedirection, + extractStringFromParseEntry, +} from '../utils/shell-utils.js'; import { parse as shellParse } from 'shell-quote'; import { PolicyDecision, @@ -24,12 +27,6 @@ import { stableStringify } from './stable-stringify.js'; import { debugLogger } from '../utils/debugLogger.js'; import type { CheckerRunner } from '../safety/checker-runner.js'; import { SafetyCheckDecision } from '../safety/protocol.js'; -import { - SHELL_TOOL_NAMES, - initializeShellParsers, - splitCommands, - hasRedirection, -} from '../utils/shell-utils.js'; import { getToolAliases } from '../tools/tool-names.js'; import { MCP_TOOL_PREFIX, @@ -38,6 +35,10 @@ import { formatMcpToolName, isMcpToolName, } from '../tools/mcp-tool.js'; +import { + type SandboxManager, + NoopSandboxManager, +} from '../services/sandboxManager.js'; function isWildcardPattern(name: string): boolean { return name === '*' || name.includes('*'); @@ -197,8 +198,7 @@ export class PolicyEngine { private readonly disableAlwaysAllow: boolean; private readonly checkerRunner?: CheckerRunner; private approvalMode: ApprovalMode; - private toolSandboxEnabled: boolean; - private sandboxApprovedTools: string[]; + private readonly sandboxManager: SandboxManager; constructor(config: PolicyEngineConfig = {}, checkerRunner?: CheckerRunner) { this.rules = (config.rules ?? []).sort( @@ -249,18 +249,14 @@ export class PolicyEngine { this.disableAlwaysAllow = config.disableAlwaysAllow ?? false; this.checkerRunner = checkerRunner; this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT; - this.toolSandboxEnabled = config.toolSandboxEnabled ?? false; - this.sandboxApprovedTools = config.sandboxApprovedTools ?? []; + this.sandboxManager = config.sandboxManager ?? new NoopSandboxManager(); } /** * Update the current approval mode. */ - setApprovalMode(mode: ApprovalMode, sandboxApprovedTools?: string[]): void { + setApprovalMode(mode: ApprovalMode): void { this.approvalMode = mode; - if (sandboxApprovedTools !== undefined) { - this.sandboxApprovedTools = sandboxApprovedTools; - } } /** @@ -285,8 +281,9 @@ export class PolicyEngine { if (!hasRedirection(command)) return false; // Do not downgrade (do not ask user) if sandboxing is enabled and in AUTO_EDIT or YOLO + const sandboxEnabled = !(this.sandboxManager instanceof NoopSandboxManager); if ( - this.toolSandboxEnabled && + sandboxEnabled && (this.approvalMode === ApprovalMode.AUTO_EDIT || this.approvalMode === ApprovalMode.YOLO) ) { @@ -299,7 +296,6 @@ export class PolicyEngine { /** * Check if a shell command is allowed. */ - private async applyShellHeuristics( command: string, decision: PolicyDecision, @@ -307,19 +303,17 @@ export class PolicyEngine { await initializeShellParsers(); try { const parsedObjArgs = shellParse(command); - if (parsedObjArgs.some((arg) => typeof arg === 'object')) return decision; - const parsedArgs = parsedObjArgs.map(String); - if (isDangerousCommand(parsedArgs)) { + const parsedArgs = parsedObjArgs.map(extractStringFromParseEntry); + + if (this.sandboxManager.isDangerousCommand(parsedArgs)) { debugLogger.debug( `[PolicyEngine.check] Command evaluated as dangerous, forcing ASK_USER: ${command}`, ); return PolicyDecision.ASK_USER; } - const isApprovedBySandbox = - this.toolSandboxEnabled && - this.sandboxApprovedTools.includes(parsedArgs[0]); + if ( - (isKnownSafeCommand(parsedArgs) || isApprovedBySandbox) && + this.sandboxManager.isKnownSafeCommand(parsedArgs) && decision === PolicyDecision.ASK_USER ) { debugLogger.debug( diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index 0fcf682767..2366ec3fe1 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -5,6 +5,7 @@ */ import type { SafetyCheckInput } from '../safety/protocol.js'; +import type { SandboxManager } from '../services/sandboxManager.js'; export enum PolicyDecision { ALLOW = 'allow', @@ -311,13 +312,9 @@ export interface PolicyEngineConfig { approvalMode?: ApprovalMode; /** - * Whether tool sandboxing is enabled. + * The sandbox manager instance. */ - toolSandboxEnabled?: boolean; - /** - * List of tools approved by the sandbox policy for the current mode. - */ - sandboxApprovedTools?: string[]; + sandboxManager?: SandboxManager; } export interface PolicySettings { diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index 8dd1154846..2b3e8cc7c9 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -99,12 +99,25 @@ function touch(filePath: string, isDirectory: boolean) { } } +import { + isKnownSafeCommand, + isDangerousCommand, +} from '../macos/commandSafety.js'; + /** * A SandboxManager implementation for Linux that uses Bubblewrap (bwrap). */ export class LinuxSandboxManager implements SandboxManager { constructor(private readonly options: GlobalSandboxOptions) {} + isKnownSafeCommand(args: string[]): boolean { + return isKnownSafeCommand(args); + } + + isDangerousCommand(args: string[]): boolean { + return isDangerousCommand(args); + } + async prepareCommand(req: SandboxRequest): Promise { const sanitizationConfig = getSecureSanitizationConfig( req.policy?.sanitizationConfig, diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index 7d9bd57cae..0c7e83ecfe 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -69,7 +69,7 @@ describe('MacOsSandboxManager', () => { allowedPaths: mockAllowedPaths, networkAccess: mockNetworkAccess, forbiddenPaths: undefined, - workspaceWrite: false, + workspaceWrite: true, additionalPermissions: { fileSystem: { read: [], diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index 10828083a5..c767c18b82 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -14,23 +14,20 @@ import { import { sanitizeEnvironment, getSecureSanitizationConfig, - type EnvironmentSanitizationConfig, } from '../../services/environmentSanitization.js'; import { buildSeatbeltArgs } from './seatbeltArgsBuilder.js'; import { - getCommandRoots, initializeShellParsers, - splitCommands, - stripShellWrapper, + getCommandName, } from '../../utils/shell-utils.js'; -import { isKnownSafeCommand } from './commandSafety.js'; -import { parse as shellParse } from 'shell-quote'; +import { + isKnownSafeCommand, + isDangerousCommand, + isStrictlyApproved, +} from './commandSafety.js'; import { type SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js'; -import path from 'node:path'; export interface MacOsSandboxOptions extends GlobalSandboxOptions { - /** Optional base sanitization config. */ - sanitizationConfig?: EnvironmentSanitizationConfig; /** The current sandbox mode behavior from config. */ modeConfig?: { readonly?: boolean; @@ -48,52 +45,17 @@ export interface MacOsSandboxOptions extends GlobalSandboxOptions { export class MacOsSandboxManager implements SandboxManager { constructor(private readonly options: MacOsSandboxOptions) {} - private async isStrictlyApproved(req: SandboxRequest): Promise { - const approvedTools = this.options.modeConfig?.approvedTools; - if (!approvedTools || approvedTools.length === 0) { - return false; - } - - await initializeShellParsers(); - - const fullCmd = [req.command, ...req.args].join(' '); - const stripped = stripShellWrapper(fullCmd); - - const roots = getCommandRoots(stripped); - if (roots.length === 0) return false; - - const allRootsApproved = roots.every((root) => - approvedTools.includes(root), - ); - if (allRootsApproved) { + isKnownSafeCommand(args: string[]): boolean { + const toolName = args[0]; + const approvedTools = this.options.modeConfig?.approvedTools ?? []; + if (toolName && approvedTools.includes(toolName)) { return true; } - - const pipelineCommands = splitCommands(stripped); - if (pipelineCommands.length === 0) return false; - - // For safety, every command in the pipeline must be considered safe. - for (const cmdString of pipelineCommands) { - const parsedArgs = shellParse(cmdString).map(String); - if (!isKnownSafeCommand(parsedArgs)) { - return false; - } - } - - return true; + return isKnownSafeCommand(args); } - private async getCommandName(req: SandboxRequest): Promise { - await initializeShellParsers(); - const fullCmd = [req.command, ...req.args].join(' '); - const stripped = stripShellWrapper(fullCmd); - const roots = getCommandRoots(stripped).filter( - (r) => r !== 'shopt' && r !== 'set', - ); - if (roots.length > 0) { - return roots[0]; - } - return path.basename(req.command); + isDangerousCommand(args: string[]): boolean { + return isDangerousCommand(args); } async prepareCommand(req: SandboxRequest): Promise { @@ -122,15 +84,19 @@ export class MacOsSandboxManager implements SandboxManager { // If not in readonly mode OR it's a strictly approved pipeline, allow workspace writes const isApproved = allowOverrides - ? await this.isStrictlyApproved(req) + ? await isStrictlyApproved( + req.command, + req.args, + this.options.modeConfig?.approvedTools, + ) : false; const workspaceWrite = !isReadonlyMode || isApproved; - const networkAccess = + const defaultNetwork = this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; // Fetch persistent approvals for this command - const commandName = await this.getCommandName(req); + const commandName = await getCommandName(req.command, req.args); const persistentPermissions = allowOverrides ? this.options.policyManager?.getCommandPermissions(commandName) : undefined; @@ -148,7 +114,7 @@ export class MacOsSandboxManager implements SandboxManager { ], }, network: - networkAccess || + defaultNetwork || persistentPermissions?.network || req.policy?.additionalPermissions?.network || false, diff --git a/packages/core/src/sandbox/macos/commandSafety.ts b/packages/core/src/sandbox/macos/commandSafety.ts index a9911932fc..c57f77512b 100644 --- a/packages/core/src/sandbox/macos/commandSafety.ts +++ b/packages/core/src/sandbox/macos/commandSafety.ts @@ -4,6 +4,57 @@ * SPDX-License-Identifier: Apache-2.0 */ import { parse as shellParse } from 'shell-quote'; +import { + extractStringFromParseEntry, + initializeShellParsers, + splitCommands, + stripShellWrapper, +} from '../../utils/shell-utils.js'; + +/** + * Determines if a command is strictly approved for execution on macOS. + * A command is approved if it's composed entirely of tools explicitly listed in `approvedTools` + * OR if it's composed of known safe, read-only POSIX commands. + * + * @param command - The full command string to execute. + * @param args - The arguments for the command. + * @param approvedTools - A list of explicitly approved tool names (e.g., ['npm', 'git']). + * @returns true if the command is strictly approved, false otherwise. + */ +export async function isStrictlyApproved( + command: string, + args: string[], + approvedTools?: string[], +): Promise { + const tools = approvedTools ?? []; + + await initializeShellParsers(); + + const fullCmd = [command, ...args].join(' '); + const stripped = stripShellWrapper(fullCmd); + + const pipelineCommands = splitCommands(stripped); + + // Fallback for simple commands or parsing failures + if (pipelineCommands.length === 0) { + // For simple commands, we check the root command. + // If it's explicitly approved OR it's a known safe POSIX command, we allow it. + return tools.includes(command) || isKnownSafeCommand([command, ...args]); + } + + // Check every segment of the pipeline + return pipelineCommands.every((cmdString) => { + const trimmed = cmdString.trim(); + if (!trimmed) return true; + + const parsedArgs = shellParse(trimmed).map(extractStringFromParseEntry); + if (parsedArgs.length === 0) return true; + + const root = parsedArgs[0]; + // The segment is approved if the root tool is in the allowlist OR if the whole segment is safe. + return tools.includes(root) || isKnownSafeCommand(parsedArgs); + }); +} /** * Checks if a command with its arguments is known to be safe to execute @@ -45,25 +96,18 @@ export function isKnownSafeCommand(args: string[]): boolean { return false; } - const commands = script.split(/&&|\|\||\||;/); + const commands = splitCommands(script); + if (commands.length === 0) return false; - let allSafe = true; - for (const cmd of commands) { + return commands.every((cmd) => { const trimmed = cmd.trim(); - if (!trimmed) continue; + if (!trimmed) return true; - const parsed = shellParse(trimmed).map(String); - if (parsed.length === 0) continue; + const parsed = shellParse(trimmed).map(extractStringFromParseEntry); + if (parsed.length === 0) return true; - if (!isSafeToCallWithExec(parsed)) { - allSafe = false; - break; - } - } - - if (allSafe && commands.length > 0) { - return true; - } + return isSafeToCallWithExec(parsed); + }); } catch { return false; } diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 0abd3dd56b..8f9b9d617c 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -12,10 +12,18 @@ import { WindowsSandboxManager } from './WindowsSandboxManager.js'; import * as sandboxManager from '../../services/sandboxManager.js'; import type { SandboxRequest } from '../../services/sandboxManager.js'; import { spawnAsync } from '../../utils/shell-utils.js'; +import type { SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js'; -vi.mock('../../utils/shell-utils.js', () => ({ - spawnAsync: vi.fn(), -})); +vi.mock('../../utils/shell-utils.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + spawnAsync: vi.fn(), + initializeShellParsers: vi.fn(), + isStrictlyApproved: vi.fn().mockResolvedValue(true), + }; +}); describe('WindowsSandboxManager', () => { let manager: WindowsSandboxManager; @@ -27,7 +35,10 @@ describe('WindowsSandboxManager', () => { p.toString(), ); testCwd = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-cli-test-')); - manager = new WindowsSandboxManager({ workspace: testCwd }); + manager = new WindowsSandboxManager({ + workspace: testCwd, + modeConfig: { readonly: false, allowOverrides: true }, + }); }); afterEach(() => { @@ -35,240 +46,406 @@ describe('WindowsSandboxManager', () => { fs.rmSync(testCwd, { recursive: true, force: true }); }); - describe('prepareCommand', () => { - it('should correctly format the base command and args', async () => { - const req: SandboxRequest = { - command: 'whoami', - args: ['/groups'], - cwd: testCwd, - env: { TEST_VAR: 'test_value' }, - policy: { - networkAccess: false, + it('should prepare a GeminiSandbox.exe command', async () => { + const req: SandboxRequest = { + command: 'whoami', + args: ['/groups'], + cwd: testCwd, + env: { TEST_VAR: 'test_value' }, + policy: { + networkAccess: false, + }, + }; + + const result = await manager.prepareCommand(req); + + expect(result.program).toContain('GeminiSandbox.exe'); + expect(result.args).toEqual(['0', testCwd, 'whoami', '/groups']); + }); + + it('should handle networkAccess from config', async () => { + const req: SandboxRequest = { + command: 'whoami', + args: [], + cwd: testCwd, + env: {}, + policy: { + networkAccess: true, + }, + }; + + const result = await manager.prepareCommand(req); + expect(result.args[0]).toBe('1'); + }); + + it('should handle network access from additionalPermissions', async () => { + const req: SandboxRequest = { + command: 'whoami', + args: [], + cwd: testCwd, + env: {}, + policy: { + additionalPermissions: { + network: true, }, - }; + }, + }; - const result = await manager.prepareCommand(req); + const result = await manager.prepareCommand(req); + expect(result.args[0]).toBe('1'); + }); - expect(result.program).toContain('GeminiSandbox.exe'); - expect(result.args).toEqual(['0', testCwd, 'whoami', '/groups']); + it('should reject network access in Plan mode', async () => { + const planManager = new WindowsSandboxManager({ + workspace: testCwd, + modeConfig: { readonly: true, allowOverrides: false }, + }); + const req: SandboxRequest = { + command: 'curl', + args: ['google.com'], + cwd: testCwd, + env: {}, + policy: { + additionalPermissions: { network: true }, + }, + }; + + await expect(planManager.prepareCommand(req)).rejects.toThrow( + 'Sandbox request rejected: Cannot override readonly/network restrictions in Plan mode.', + ); + }); + + it('should handle persistent permissions from policyManager', async () => { + const persistentPath = path.resolve('/persistent/path'); + const mockPolicyManager = { + getCommandPermissions: vi.fn().mockReturnValue({ + fileSystem: { write: [persistentPath] }, + network: true, + }), + } as unknown as SandboxPolicyManager; + + const managerWithPolicy = new WindowsSandboxManager({ + workspace: testCwd, + modeConfig: { allowOverrides: true, network: false }, + policyManager: mockPolicyManager, }); - it('should correctly pass through the cwd to the resulting command', async () => { - const req: SandboxRequest = { - command: 'whoami', - args: [], - cwd: '/different/cwd', - env: {}, - }; + const req: SandboxRequest = { + command: 'test-cmd', + args: [], + cwd: testCwd, + env: {}, + }; - const result = await manager.prepareCommand(req); + const result = await managerWithPolicy.prepareCommand(req); + expect(result.args[0]).toBe('1'); // Network allowed by persistent policy - expect(result.cwd).toBe('/different/cwd'); - }); + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); - it('should apply environment sanitization via the default mechanisms', async () => { + expect(icaclsArgs).toContainEqual([ + persistentPath, + '/setintegritylevel', + 'Low', + ]); + }); + + it('should sanitize environment variables', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: { + API_KEY: 'secret', + PATH: '/usr/bin', + }, + policy: { + sanitizationConfig: { + allowedEnvironmentVariables: ['PATH'], + blockedEnvironmentVariables: ['API_KEY'], + enableEnvironmentVariableRedaction: true, + }, + }, + }; + + const result = await manager.prepareCommand(req); + expect(result.env['PATH']).toBe('/usr/bin'); + expect(result.env['API_KEY']).toBeUndefined(); + }); + + it('should ensure governance files exist', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + }; + + await manager.prepareCommand(req); + + expect(fs.existsSync(path.join(testCwd, '.gitignore'))).toBe(true); + expect(fs.existsSync(path.join(testCwd, '.geminiignore'))).toBe(true); + expect(fs.existsSync(path.join(testCwd, '.git'))).toBe(true); + expect(fs.lstatSync(path.join(testCwd, '.git')).isDirectory()).toBe(true); + }); + + it('should grant Low Integrity access to the workspace and allowed paths', async () => { + const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); + if (!fs.existsSync(allowedPath)) { + fs.mkdirSync(allowedPath); + } + try { const req: SandboxRequest = { command: 'test', args: [], cwd: testCwd, - env: { - API_KEY: 'secret', - PATH: '/usr/bin', - }, + env: {}, policy: { - sanitizationConfig: { - allowedEnvironmentVariables: ['PATH'], - blockedEnvironmentVariables: ['API_KEY'], - enableEnvironmentVariableRedaction: true, - }, + allowedPaths: [allowedPath], }, }; - const result = await manager.prepareCommand(req); - expect(result.env['PATH']).toBe('/usr/bin'); - expect(result.env['API_KEY']).toBeUndefined(); - }); + await manager.prepareCommand(req); - it('should allow network when networkAccess is true', async () => { + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); + + expect(icaclsArgs).toContainEqual([ + path.resolve(testCwd), + '/setintegritylevel', + 'Low', + ]); + + expect(icaclsArgs).toContainEqual([ + path.resolve(allowedPath), + '/setintegritylevel', + 'Low', + ]); + } finally { + fs.rmSync(allowedPath, { recursive: true, force: true }); + } + }); + + it('should grant Low Integrity access to additional write paths', async () => { + const extraWritePath = path.join( + os.tmpdir(), + 'gemini-cli-test-extra-write', + ); + if (!fs.existsSync(extraWritePath)) { + fs.mkdirSync(extraWritePath); + } + try { const req: SandboxRequest = { - command: 'whoami', + command: 'test', args: [], cwd: testCwd, env: {}, policy: { - networkAccess: true, + additionalPermissions: { + fileSystem: { + write: [extraWritePath], + }, + }, }, }; - const result = await manager.prepareCommand(req); - expect(result.args[0]).toBe('1'); - }); + await manager.prepareCommand(req); - describe('governance files', () => { - it('should ensure governance files exist', async () => { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - }; + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); - await manager.prepareCommand(req); + expect(icaclsArgs).toContainEqual([ + path.resolve(extraWritePath), + '/setintegritylevel', + 'Low', + ]); + } finally { + fs.rmSync(extraWritePath, { recursive: true, force: true }); + } + }); - expect(fs.existsSync(path.join(testCwd, '.gitignore'))).toBe(true); - expect(fs.existsSync(path.join(testCwd, '.geminiignore'))).toBe(true); - expect(fs.existsSync(path.join(testCwd, '.git'))).toBe(true); - expect(fs.lstatSync(path.join(testCwd, '.git')).isDirectory()).toBe( - true, - ); - }); - }); - - describe('allowedPaths', () => { - it('should parameterize allowed paths and normalize them', async () => { - const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); - if (!fs.existsSync(allowedPath)) { - fs.mkdirSync(allowedPath); - } - try { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - allowedPaths: [allowedPath], + it.runIf(process.platform === 'win32')( + 'should reject UNC paths in grantLowIntegrityAccess', + async () => { + const uncPath = '\\\\attacker\\share\\malicious.txt'; + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + additionalPermissions: { + fileSystem: { + write: [uncPath], }, - }; - - await manager.prepareCommand(req); - - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(testCwd), - '/setintegritylevel', - 'Low', - ]); - - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(allowedPath), - '/setintegritylevel', - 'Low', - ]); - } finally { - fs.rmSync(allowedPath, { recursive: true, force: true }); - } - }); - }); - - describe('forbiddenPaths', () => { - it('should parameterize forbidden paths and explicitly deny them', async () => { - const forbiddenPath = path.join( - os.tmpdir(), - 'gemini-cli-test-forbidden', - ); - if (!fs.existsSync(forbiddenPath)) { - fs.mkdirSync(forbiddenPath); - } - try { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - forbiddenPaths: [forbiddenPath], - }, - }; - - await manager.prepareCommand(req); - - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(forbiddenPath), - '/deny', - '*S-1-16-4096:(OI)(CI)(F)', - ]); - } finally { - fs.rmSync(forbiddenPath, { recursive: true, force: true }); - } - }); - - it('explicitly denies non-existent forbidden paths to prevent creation', async () => { - const missingPath = path.join( - os.tmpdir(), - 'gemini-cli-test-missing', - 'does-not-exist.txt', - ); - - // Ensure it definitely doesn't exist - if (fs.existsSync(missingPath)) { - fs.rmSync(missingPath, { recursive: true, force: true }); - } - - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - forbiddenPaths: [missingPath], }, - }; + }, + }; - await manager.prepareCommand(req); + await manager.prepareCommand(req); - // Should NOT have called icacls to deny the missing path - expect(spawnAsync).not.toHaveBeenCalledWith('icacls', [ - path.resolve(missingPath), - '/deny', - '*S-1-16-4096:(OI)(CI)(F)', - ]); - }); + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); - it('should override allowed paths if a path is also in forbidden paths', async () => { - const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); - if (!fs.existsSync(conflictPath)) { - fs.mkdirSync(conflictPath); - } - try { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - allowedPaths: [conflictPath], - forbiddenPaths: [conflictPath], + expect(icaclsArgs).not.toContainEqual([ + uncPath, + '/setintegritylevel', + 'Low', + ]); + }, + ); + + it.runIf(process.platform === 'win32')( + 'should allow extended-length and local device paths', + async () => { + const longPath = '\\\\?\\C:\\very\\long\\path'; + const devicePath = '\\\\.\\PhysicalDrive0'; + + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + additionalPermissions: { + fileSystem: { + write: [longPath, devicePath], }, - }; + }, + }, + }; - await manager.prepareCommand(req); + await manager.prepareCommand(req); - const spawnMock = vi.mocked(spawnAsync); - const allowCallIndex = spawnMock.mock.calls.findIndex( - (call) => - call[1] && - call[1].includes('/setintegritylevel') && - call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), - ); - const denyCallIndex = spawnMock.mock.calls.findIndex( - (call) => - call[1] && - call[1].includes('/deny') && - call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), - ); + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); - // Both should have been called - expect(allowCallIndex).toBeGreaterThan(-1); - expect(denyCallIndex).toBeGreaterThan(-1); + expect(icaclsArgs).toContainEqual([ + longPath, + '/setintegritylevel', + 'Low', + ]); + expect(icaclsArgs).toContainEqual([ + devicePath, + '/setintegritylevel', + 'Low', + ]); + }, + ); - // Verify order: explicitly denying must happen after the explicit allow - expect(allowCallIndex).toBeLessThan(denyCallIndex); - } finally { - fs.rmSync(conflictPath, { recursive: true, force: true }); - } - }); - }); + it('skips denying access to non-existent forbidden paths to prevent icacls failure', async () => { + const missingPath = path.join( + os.tmpdir(), + 'gemini-cli-test-missing', + 'does-not-exist.txt', + ); + + // Ensure it definitely doesn't exist + if (fs.existsSync(missingPath)) { + fs.rmSync(missingPath, { recursive: true, force: true }); + } + + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + forbiddenPaths: [missingPath], + }, + }; + + await manager.prepareCommand(req); + + // Should NOT have called icacls to deny the missing path + expect(spawnAsync).not.toHaveBeenCalledWith('icacls', [ + path.resolve(missingPath), + '/deny', + '*S-1-16-4096:(OI)(CI)(F)', + ]); + }); + + it('should deny Low Integrity access to forbidden paths', async () => { + const forbiddenPath = path.join(os.tmpdir(), 'gemini-cli-test-forbidden'); + if (!fs.existsSync(forbiddenPath)) { + fs.mkdirSync(forbiddenPath); + } + try { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + forbiddenPaths: [forbiddenPath], + }, + }; + + await manager.prepareCommand(req); + + expect(spawnAsync).toHaveBeenCalledWith('icacls', [ + path.resolve(forbiddenPath), + '/deny', + '*S-1-16-4096:(OI)(CI)(F)', + ]); + } finally { + fs.rmSync(forbiddenPath, { recursive: true, force: true }); + } + }); + + it('should override allowed paths if a path is also in forbidden paths', async () => { + const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); + if (!fs.existsSync(conflictPath)) { + fs.mkdirSync(conflictPath); + } + try { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + allowedPaths: [conflictPath], + forbiddenPaths: [conflictPath], + }, + }; + + await manager.prepareCommand(req); + + const spawnMock = vi.mocked(spawnAsync); + const allowCallIndex = spawnMock.mock.calls.findIndex( + (call) => + call[1] && + call[1].includes('/setintegritylevel') && + call[0] === 'icacls' && + call[1][0] === path.resolve(conflictPath), + ); + const denyCallIndex = spawnMock.mock.calls.findIndex( + (call) => + call[1] && + call[1].includes('/deny') && + call[0] === 'icacls' && + call[1][0] === path.resolve(conflictPath), + ); + + // Both should have been called + expect(allowCallIndex).toBeGreaterThan(-1); + expect(denyCallIndex).toBeGreaterThan(-1); + + // Verify order: explicitly denying must happen after the explicit allow + expect(allowCallIndex).toBeLessThan(denyCallIndex); + } finally { + fs.rmSync(conflictPath, { recursive: true, force: true }); + } }); }); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 0a1bc2a95f..0a5d08637c 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -16,18 +16,37 @@ import { type GlobalSandboxOptions, sanitizePaths, tryRealpath, + type SandboxPermissions, } from '../../services/sandboxManager.js'; import { sanitizeEnvironment, getSecureSanitizationConfig, } from '../../services/environmentSanitization.js'; import { debugLogger } from '../../utils/debugLogger.js'; -import { spawnAsync } from '../../utils/shell-utils.js'; +import { spawnAsync, getCommandName } from '../../utils/shell-utils.js'; import { isNodeError } from '../../utils/errors.js'; +import { + isKnownSafeCommand, + isDangerousCommand, + isStrictlyApproved, +} from './commandSafety.js'; +import { type SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); +export interface WindowsSandboxOptions extends GlobalSandboxOptions { + /** The current sandbox mode behavior from config. */ + modeConfig?: { + readonly?: boolean; + network?: boolean; + approvedTools?: string[]; + allowOverrides?: boolean; + }; + /** The policy manager for persistent approvals. */ + policyManager?: SandboxPolicyManager; +} + /** * A SandboxManager implementation for Windows that uses Restricted Tokens, * Job Objects, and Low Integrity levels for process isolation. @@ -39,10 +58,23 @@ export class WindowsSandboxManager implements SandboxManager { private readonly allowedCache = new Set(); private readonly deniedCache = new Set(); - constructor(private readonly options: GlobalSandboxOptions) { + constructor(private readonly options: WindowsSandboxOptions) { this.helperPath = path.resolve(__dirname, 'GeminiSandbox.exe'); } + isKnownSafeCommand(args: string[]): boolean { + const toolName = args[0]?.toLowerCase(); + const approvedTools = this.options.modeConfig?.approvedTools ?? []; + if (toolName && approvedTools.some((t) => t.toLowerCase() === toolName)) { + return true; + } + return isKnownSafeCommand(args); + } + + isDangerousCommand(args: string[]): boolean { + return isDangerousCommand(args); + } + /** * Ensures a file or directory exists. */ @@ -178,9 +210,60 @@ export class WindowsSandboxManager implements SandboxManager { const sanitizedEnv = sanitizeEnvironment(req.env, sanitizationConfig); + const isReadonlyMode = this.options.modeConfig?.readonly ?? true; + const allowOverrides = this.options.modeConfig?.allowOverrides ?? true; + + // Reject override attempts in plan mode + if (!allowOverrides && req.policy?.additionalPermissions) { + const perms = req.policy.additionalPermissions; + if ( + perms.network || + (perms.fileSystem?.write && perms.fileSystem.write.length > 0) + ) { + throw new Error( + 'Sandbox request rejected: Cannot override readonly/network restrictions in Plan mode.', + ); + } + } + + // Fetch persistent approvals for this command + const commandName = await getCommandName(req.command, req.args); + const persistentPermissions = allowOverrides + ? this.options.policyManager?.getCommandPermissions(commandName) + : undefined; + + // Merge all permissions + const mergedAdditional: SandboxPermissions = { + fileSystem: { + read: [ + ...(persistentPermissions?.fileSystem?.read ?? []), + ...(req.policy?.additionalPermissions?.fileSystem?.read ?? []), + ], + write: [ + ...(persistentPermissions?.fileSystem?.write ?? []), + ...(req.policy?.additionalPermissions?.fileSystem?.write ?? []), + ], + }, + network: + persistentPermissions?.network || + req.policy?.additionalPermissions?.network || + false, + }; + // 1. Handle filesystem permissions for Low Integrity // Grant "Low Mandatory Level" write access to the workspace. - await this.grantLowIntegrityAccess(this.options.workspace); + // If not in readonly mode OR it's a strictly approved pipeline, allow workspace writes + const isApproved = allowOverrides + ? await isStrictlyApproved( + req.command, + req.args, + this.options.modeConfig?.approvedTools, + ) + : false; + + if (!isReadonlyMode || isApproved) { + await this.grantLowIntegrityAccess(this.options.workspace); + } // Grant "Low Mandatory Level" read access to allowedPaths. const allowedPaths = sanitizePaths(req.policy?.allowedPaths) || []; @@ -188,6 +271,13 @@ export class WindowsSandboxManager implements SandboxManager { await this.grantLowIntegrityAccess(allowedPath); } + // Grant "Low Mandatory Level" write access to additional permissions write paths. + const additionalWritePaths = + sanitizePaths(mergedAdditional.fileSystem?.write) || []; + for (const writePath of additionalWritePaths) { + await this.grantLowIntegrityAccess(writePath); + } + // Denies access to forbiddenPaths for Low Integrity processes. const forbiddenPaths = sanitizePaths(req.policy?.forbiddenPaths) || []; for (const forbiddenPath of forbiddenPaths) { @@ -219,13 +309,12 @@ export class WindowsSandboxManager implements SandboxManager { // GeminiSandbox.exe [args...] const program = this.helperPath; + const defaultNetwork = + this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; + const networkAccess = defaultNetwork || mergedAdditional.network; + // If the command starts with __, it's an internal command for the sandbox helper itself. - const args = [ - req.policy?.networkAccess ? '1' : '0', - req.cwd, - req.command, - ...req.args, - ]; + const args = [networkAccess ? '1' : '0', req.cwd, req.command, ...req.args]; return { program, @@ -248,6 +337,20 @@ export class WindowsSandboxManager implements SandboxManager { return; } + // Explicitly reject UNC paths to prevent credential theft/SSRF, + // but allow local extended-length and device paths. + if ( + resolvedPath.startsWith('\\\\') && + !resolvedPath.startsWith('\\\\?\\') && + !resolvedPath.startsWith('\\\\.\\') + ) { + debugLogger.log( + 'WindowsSandboxManager: Rejecting UNC path for Low Integrity grant:', + resolvedPath, + ); + return; + } + // Never modify integrity levels for system directories const systemRoot = process.env['SystemRoot'] || 'C:\\Windows'; const programFiles = process.env['ProgramFiles'] || 'C:\\Program Files'; diff --git a/packages/core/src/sandbox/windows/commandSafety.test.ts b/packages/core/src/sandbox/windows/commandSafety.test.ts new file mode 100644 index 0000000000..82077b2690 --- /dev/null +++ b/packages/core/src/sandbox/windows/commandSafety.test.ts @@ -0,0 +1,50 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { isKnownSafeCommand, isDangerousCommand } from './commandSafety.js'; + +describe('Windows commandSafety', () => { + describe('isKnownSafeCommand', () => { + it('should identify known safe commands', () => { + expect(isKnownSafeCommand(['dir'])).toBe(true); + expect(isKnownSafeCommand(['echo', 'hello'])).toBe(true); + expect(isKnownSafeCommand(['whoami'])).toBe(true); + }); + + it('should strip .exe extension for safe commands', () => { + expect(isKnownSafeCommand(['dir.exe'])).toBe(true); + expect(isKnownSafeCommand(['ECHO.EXE', 'hello'])).toBe(true); + expect(isKnownSafeCommand(['WHOAMI.exe'])).toBe(true); + }); + + it('should reject unknown commands', () => { + expect(isKnownSafeCommand(['unknown'])).toBe(false); + expect(isKnownSafeCommand(['npm', 'install'])).toBe(false); + }); + }); + + describe('isDangerousCommand', () => { + it('should identify dangerous commands', () => { + expect(isDangerousCommand(['del', 'file.txt'])).toBe(true); + expect(isDangerousCommand(['powershell', '-Command', 'echo'])).toBe(true); + expect(isDangerousCommand(['cmd', '/c', 'dir'])).toBe(true); + }); + + it('should strip .exe extension for dangerous commands', () => { + expect(isDangerousCommand(['del.exe', 'file.txt'])).toBe(true); + expect(isDangerousCommand(['POWERSHELL.EXE', '-Command', 'echo'])).toBe( + true, + ); + expect(isDangerousCommand(['cmd.exe', '/c', 'dir'])).toBe(true); + }); + + it('should not flag safe commands as dangerous', () => { + expect(isDangerousCommand(['dir'])).toBe(false); + expect(isDangerousCommand(['echo', 'hello'])).toBe(false); + }); + }); +}); diff --git a/packages/core/src/sandbox/windows/commandSafety.ts b/packages/core/src/sandbox/windows/commandSafety.ts new file mode 100644 index 0000000000..bff2976e62 --- /dev/null +++ b/packages/core/src/sandbox/windows/commandSafety.ts @@ -0,0 +1,148 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ +import { parse as shellParse } from 'shell-quote'; +import { + extractStringFromParseEntry, + initializeShellParsers, + splitCommands, + stripShellWrapper, +} from '../../utils/shell-utils.js'; + +/** + * Determines if a command is strictly approved for execution on Windows. + * A command is approved if it's composed entirely of tools explicitly listed in `approvedTools` + * OR if it's composed of known safe, read-only Windows commands. + * + * @param command - The full command string to execute. + * @param args - The arguments for the command. + * @param approvedTools - A list of explicitly approved tool names (e.g., ['npm', 'git']). + * @returns true if the command is strictly approved, false otherwise. + */ +export async function isStrictlyApproved( + command: string, + args: string[], + approvedTools?: string[], +): Promise { + const tools = approvedTools ?? []; + + await initializeShellParsers(); + + const fullCmd = [command, ...args].join(' '); + const stripped = stripShellWrapper(fullCmd); + + const pipelineCommands = splitCommands(stripped); + + // Fallback for simple commands or parsing failures + if (pipelineCommands.length === 0) { + return tools.includes(command) || isKnownSafeCommand([command, ...args]); + } + + // Check every segment of the pipeline + return pipelineCommands.every((cmdString) => { + const trimmed = cmdString.trim(); + if (!trimmed) return true; + + const parsedArgs = shellParse(trimmed).map(extractStringFromParseEntry); + if (parsedArgs.length === 0) return true; + + let root = parsedArgs[0].toLowerCase(); + if (root.endsWith('.exe')) { + root = root.slice(0, -4); + } + // The segment is approved if the root tool is in the allowlist OR if the whole segment is safe. + return ( + tools.some((t) => t.toLowerCase() === root) || + isKnownSafeCommand(parsedArgs) + ); + }); +} + +/** + * Checks if a Windows command is known to be safe (read-only). + */ +export function isKnownSafeCommand(args: string[]): boolean { + if (!args || args.length === 0) return false; + let cmd = args[0].toLowerCase(); + if (cmd.endsWith('.exe')) { + cmd = cmd.slice(0, -4); + } + + // Native Windows/PowerShell safe commands + const safeCommands = new Set([ + 'dir', + 'type', + 'echo', + 'cd', + 'pwd', + 'whoami', + 'hostname', + 'ver', + 'vol', + 'systeminfo', + 'attrib', + 'findstr', + 'where', + 'sort', + 'more', + 'get-childitem', + 'get-content', + 'get-location', + 'get-help', + 'get-process', + 'get-service', + 'get-eventlog', + 'select-string', + ]); + + if (safeCommands.has(cmd)) { + return true; + } + + // We allow git on Windows if it's read-only, using the same logic as POSIX + if (cmd === 'git') { + // For simplicity in this branch, we'll allow standard git read operations + // In a full implementation, we'd port the sub-command validation too. + const sub = args[1]?.toLowerCase(); + return ['status', 'log', 'diff', 'show', 'branch'].includes(sub); + } + + return false; +} + +/** + * Checks if a Windows command is explicitly dangerous. + */ +export function isDangerousCommand(args: string[]): boolean { + if (!args || args.length === 0) return false; + let cmd = args[0].toLowerCase(); + if (cmd.endsWith('.exe')) { + cmd = cmd.slice(0, -4); + } + + const dangerous = new Set([ + 'del', + 'erase', + 'rd', + 'rmdir', + 'net', + 'reg', + 'sc', + 'format', + 'mklink', + 'takeown', + 'icacls', + 'powershell', // prevent shell escapes + 'pwsh', + 'cmd', + 'remove-item', + 'stop-process', + 'stop-service', + 'set-item', + 'new-item', + ]); + + return dangerous.has(cmd); +} diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 411b49636b..1f3cfa089e 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -3,13 +3,13 @@ * Copyright 2025 Google LLC * SPDX-License-Identifier: Apache-2.0 */ - import os from 'node:os'; import path from 'node:path'; import fs from 'node:fs/promises'; -import { describe, expect, it, vi, beforeEach } from 'vitest'; +import { afterEach, describe, expect, it, vi, beforeEach } from 'vitest'; import { NoopSandboxManager, + LocalSandboxManager, sanitizePaths, tryRealpath, } from './sandboxManager.js'; @@ -18,225 +18,265 @@ import { LinuxSandboxManager } from '../sandbox/linux/LinuxSandboxManager.js'; import { MacOsSandboxManager } from '../sandbox/macos/MacOsSandboxManager.js'; import { WindowsSandboxManager } from '../sandbox/windows/WindowsSandboxManager.js'; -describe('sanitizePaths', () => { - it('should return undefined if no paths are provided', () => { - expect(sanitizePaths(undefined)).toBeUndefined(); - }); +describe('SandboxManager', () => { + afterEach(() => vi.restoreAllMocks()); - it('should deduplicate paths and return them', () => { - const paths = ['/workspace/foo', '/workspace/bar', '/workspace/foo']; - expect(sanitizePaths(paths)).toEqual(['/workspace/foo', '/workspace/bar']); - }); - - it('should throw an error if a path is not absolute', () => { - const paths = ['/workspace/foo', 'relative/path']; - expect(() => sanitizePaths(paths)).toThrow( - 'Sandbox path must be absolute: relative/path', - ); - }); -}); - -describe('tryRealpath', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it('should return the realpath if the file exists', async () => { - vi.spyOn(fs, 'realpath').mockResolvedValue('/real/path/to/file.txt'); - const result = await tryRealpath('/some/symlink/to/file.txt'); - expect(result).toBe('/real/path/to/file.txt'); - expect(fs.realpath).toHaveBeenCalledWith('/some/symlink/to/file.txt'); - }); - - it('should fallback to parent directory if file does not exist (ENOENT)', async () => { - vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { - if (p === '/workspace/nonexistent.txt') { - throw Object.assign(new Error('ENOENT: no such file or directory'), { - code: 'ENOENT', - }); - } - if (p === '/workspace') { - return '/real/workspace'; - } - throw new Error(`Unexpected path: ${p}`); + describe('sanitizePaths', () => { + it('should return undefined if no paths are provided', () => { + expect(sanitizePaths(undefined)).toBeUndefined(); }); - const result = await tryRealpath('/workspace/nonexistent.txt'); - - // It should combine the real path of the parent with the original basename - expect(result).toBe(path.join('/real/workspace', 'nonexistent.txt')); - }); - - it('should recursively fallback up the directory tree on multiple ENOENT errors', async () => { - vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { - if (p === '/workspace/missing_dir/missing_file.txt') { - throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); - } - if (p === '/workspace/missing_dir') { - throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); - } - if (p === '/workspace') { - return '/real/workspace'; - } - throw new Error(`Unexpected path: ${p}`); + it('should deduplicate paths and return them', () => { + const paths = ['/workspace/foo', '/workspace/bar', '/workspace/foo']; + expect(sanitizePaths(paths)).toEqual([ + '/workspace/foo', + '/workspace/bar', + ]); }); - const result = await tryRealpath('/workspace/missing_dir/missing_file.txt'); - - // It should resolve '/workspace' to '/real/workspace' and append the missing parts - expect(result).toBe( - path.join('/real/workspace', 'missing_dir', 'missing_file.txt'), - ); + it('should throw an error if a path is not absolute', () => { + const paths = ['/workspace/foo', 'relative/path']; + expect(() => sanitizePaths(paths)).toThrow( + 'Sandbox path must be absolute: relative/path', + ); + }); }); - it('should return the path unchanged if it reaches the root directory and it still does not exist', async () => { - const rootPath = path.resolve('/'); - vi.spyOn(fs, 'realpath').mockImplementation(async () => { - throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + describe('tryRealpath', () => { + beforeEach(() => { + vi.clearAllMocks(); }); - const result = await tryRealpath(rootPath); - expect(result).toBe(rootPath); - }); + it('should return the realpath if the file exists', async () => { + vi.spyOn(fs, 'realpath').mockResolvedValue('/real/path/to/file.txt'); + const result = await tryRealpath('/some/symlink/to/file.txt'); + expect(result).toBe('/real/path/to/file.txt'); + expect(fs.realpath).toHaveBeenCalledWith('/some/symlink/to/file.txt'); + }); - it('should throw an error if realpath fails with a non-ENOENT error (e.g. EACCES)', async () => { - vi.spyOn(fs, 'realpath').mockImplementation(async () => { - throw Object.assign(new Error('EACCES: permission denied'), { - code: 'EACCES', + it('should fallback to parent directory if file does not exist (ENOENT)', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { + if (p === '/workspace/nonexistent.txt') { + throw Object.assign(new Error('ENOENT: no such file or directory'), { + code: 'ENOENT', + }); + } + if (p === '/workspace') { + return '/real/workspace'; + } + throw new Error(`Unexpected path: ${p}`); }); + + const result = await tryRealpath('/workspace/nonexistent.txt'); + + // It should combine the real path of the parent with the original basename + expect(result).toBe(path.join('/real/workspace', 'nonexistent.txt')); }); - await expect(tryRealpath('/secret/file.txt')).rejects.toThrow( - 'EACCES: permission denied', - ); - }); -}); + it('should recursively fallback up the directory tree on multiple ENOENT errors', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { + if (p === '/workspace/missing_dir/missing_file.txt') { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + } + if (p === '/workspace/missing_dir') { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + } + if (p === '/workspace') { + return '/real/workspace'; + } + throw new Error(`Unexpected path: ${p}`); + }); -describe('NoopSandboxManager', () => { - const sandboxManager = new NoopSandboxManager(); + const result = await tryRealpath( + '/workspace/missing_dir/missing_file.txt', + ); - it('should pass through the command and arguments unchanged', async () => { - const req = { - command: 'ls', - args: ['-la'], - cwd: '/tmp', - env: { PATH: '/usr/bin' }, - }; + // It should resolve '/workspace' to '/real/workspace' and append the missing parts + expect(result).toBe( + path.join('/real/workspace', 'missing_dir', 'missing_file.txt'), + ); + }); - const result = await sandboxManager.prepareCommand(req); + it('should return the path unchanged if it reaches the root directory and it still does not exist', async () => { + const rootPath = path.resolve('/'); + vi.spyOn(fs, 'realpath').mockImplementation(async () => { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); - expect(result.program).toBe('ls'); - expect(result.args).toEqual(['-la']); + const result = await tryRealpath(rootPath); + expect(result).toBe(rootPath); + }); + + it('should throw an error if realpath fails with a non-ENOENT error (e.g. EACCES)', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async () => { + throw Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }); + }); + + await expect(tryRealpath('/secret/file.txt')).rejects.toThrow( + 'EACCES: permission denied', + ); + }); }); - it('should sanitize the environment variables', async () => { - const req = { - command: 'echo', - args: ['hello'], - cwd: '/tmp', - env: { - PATH: '/usr/bin', - GITHUB_TOKEN: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', - MY_SECRET: 'super-secret', - SAFE_VAR: 'is-safe', - }, - }; + describe('NoopSandboxManager', () => { + const sandboxManager = new NoopSandboxManager(); - const result = await sandboxManager.prepareCommand(req); + it('should pass through the command and arguments unchanged', async () => { + const req = { + command: 'ls', + args: ['-la'], + cwd: '/tmp', + env: { PATH: '/usr/bin' }, + }; - expect(result.env['PATH']).toBe('/usr/bin'); - expect(result.env['SAFE_VAR']).toBe('is-safe'); - expect(result.env['GITHUB_TOKEN']).toBeUndefined(); - expect(result.env['MY_SECRET']).toBeUndefined(); - }); + const result = await sandboxManager.prepareCommand(req); - it('should NOT allow disabling environment variable redaction if requested in config (vulnerability fix)', async () => { - const req = { - command: 'echo', - args: ['hello'], - cwd: '/tmp', - env: { - API_KEY: 'sensitive-key', - }, - policy: { - sanitizationConfig: { - enableEnvironmentVariableRedaction: false, + expect(result.program).toBe('ls'); + expect(result.args).toEqual(['-la']); + }); + + it('should sanitize the environment variables', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + PATH: '/usr/bin', + GITHUB_TOKEN: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', + MY_SECRET: 'super-secret', + SAFE_VAR: 'is-safe', }, - }, - }; + }; - const result = await sandboxManager.prepareCommand(req); + const result = await sandboxManager.prepareCommand(req); - // API_KEY should be redacted because SandboxManager forces redaction and API_KEY matches NEVER_ALLOWED_NAME_PATTERNS - expect(result.env['API_KEY']).toBeUndefined(); - }); + expect(result.env['PATH']).toBe('/usr/bin'); + expect(result.env['SAFE_VAR']).toBe('is-safe'); + expect(result.env['GITHUB_TOKEN']).toBeUndefined(); + expect(result.env['MY_SECRET']).toBeUndefined(); + }); - it('should respect allowedEnvironmentVariables in config but filter sensitive ones', async () => { - const req = { - command: 'echo', - args: ['hello'], - cwd: '/tmp', - env: { - MY_SAFE_VAR: 'safe-value', - MY_TOKEN: 'secret-token', - }, - policy: { - sanitizationConfig: { - allowedEnvironmentVariables: ['MY_SAFE_VAR', 'MY_TOKEN'], + it('should NOT allow disabling environment variable redaction if requested in config (vulnerability fix)', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + API_KEY: 'sensitive-key', }, - }, - }; - - const result = await sandboxManager.prepareCommand(req); - - expect(result.env['MY_SAFE_VAR']).toBe('safe-value'); - // MY_TOKEN matches /TOKEN/i so it should be redacted despite being allowed in config - expect(result.env['MY_TOKEN']).toBeUndefined(); - }); - - it('should respect blockedEnvironmentVariables in config', async () => { - const req = { - command: 'echo', - args: ['hello'], - cwd: '/tmp', - env: { - SAFE_VAR: 'safe-value', - BLOCKED_VAR: 'blocked-value', - }, - policy: { - sanitizationConfig: { - blockedEnvironmentVariables: ['BLOCKED_VAR'], + policy: { + sanitizationConfig: { + enableEnvironmentVariableRedaction: false, + }, }, - }, - }; + }; - const result = await sandboxManager.prepareCommand(req); + const result = await sandboxManager.prepareCommand(req); - expect(result.env['SAFE_VAR']).toBe('safe-value'); - expect(result.env['BLOCKED_VAR']).toBeUndefined(); - }); -}); + // API_KEY should be redacted because SandboxManager forces redaction and API_KEY matches NEVER_ALLOWED_NAME_PATTERNS + expect(result.env['API_KEY']).toBeUndefined(); + }); -describe('createSandboxManager', () => { - it('should return NoopSandboxManager if sandboxing is disabled', () => { - const manager = createSandboxManager({ enabled: false }, '/workspace'); - expect(manager).toBeInstanceOf(NoopSandboxManager); + it('should respect allowedEnvironmentVariables in config but filter sensitive ones', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + MY_SAFE_VAR: 'safe-value', + MY_TOKEN: 'secret-token', + }, + policy: { + sanitizationConfig: { + allowedEnvironmentVariables: ['MY_SAFE_VAR', 'MY_TOKEN'], + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.env['MY_SAFE_VAR']).toBe('safe-value'); + // MY_TOKEN matches /TOKEN/i so it should be redacted despite being allowed in config + expect(result.env['MY_TOKEN']).toBeUndefined(); + }); + + it('should respect blockedEnvironmentVariables in config', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + SAFE_VAR: 'safe-value', + BLOCKED_VAR: 'blocked-value', + }, + policy: { + sanitizationConfig: { + blockedEnvironmentVariables: ['BLOCKED_VAR'], + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.env['SAFE_VAR']).toBe('safe-value'); + expect(result.env['BLOCKED_VAR']).toBeUndefined(); + }); + + it('should delegate isKnownSafeCommand to platform specific checkers', () => { + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + expect(sandboxManager.isKnownSafeCommand(['ls'])).toBe(true); + expect(sandboxManager.isKnownSafeCommand(['dir'])).toBe(false); + + vi.spyOn(os, 'platform').mockReturnValue('win32'); + expect(sandboxManager.isKnownSafeCommand(['dir'])).toBe(true); + }); + + it('should delegate isDangerousCommand to platform specific checkers', () => { + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + expect(sandboxManager.isDangerousCommand(['rm', '-rf', '.'])).toBe(true); + expect(sandboxManager.isDangerousCommand(['del'])).toBe(false); + + vi.spyOn(os, 'platform').mockReturnValue('win32'); + expect(sandboxManager.isDangerousCommand(['del'])).toBe(true); + }); }); - it.each([ - { platform: 'linux', expected: LinuxSandboxManager }, - { platform: 'darwin', expected: MacOsSandboxManager }, - { platform: 'win32', expected: WindowsSandboxManager }, - ] as const)( - 'should return $expected.name if sandboxing is enabled and platform is $platform', - ({ platform, expected }) => { - const osSpy = vi.spyOn(os, 'platform').mockReturnValue(platform); - try { + describe('createSandboxManager', () => { + it('should return NoopSandboxManager if sandboxing is disabled', () => { + const manager = createSandboxManager({ enabled: false }, '/workspace'); + expect(manager).toBeInstanceOf(NoopSandboxManager); + }); + + it.each([ + { platform: 'linux', expected: LinuxSandboxManager }, + { platform: 'darwin', expected: MacOsSandboxManager }, + ] as const)( + 'should return $expected.name if sandboxing is enabled and platform is $platform', + ({ platform, expected }) => { + vi.spyOn(os, 'platform').mockReturnValue(platform); const manager = createSandboxManager({ enabled: true }, '/workspace'); expect(manager).toBeInstanceOf(expected); - } finally { - osSpy.mockRestore(); - } - }, - ); + }, + ); + + it("should return WindowsSandboxManager if sandboxing is enabled with 'windows-native' command on win32", () => { + vi.spyOn(os, 'platform').mockReturnValue('win32'); + const manager = createSandboxManager( + { enabled: true, command: 'windows-native' }, + '/workspace', + ); + expect(manager).toBeInstanceOf(WindowsSandboxManager); + }); + + it('should return LocalSandboxManager on win32 if command is not windows-native', () => { + vi.spyOn(os, 'platform').mockReturnValue('win32'); + const manager = createSandboxManager( + { enabled: true, command: 'docker' as unknown as 'windows-native' }, + '/workspace', + ); + expect(manager).toBeInstanceOf(LocalSandboxManager); + }); + }); }); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index c2f5a4c623..0e282b0748 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -7,6 +7,14 @@ import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; +import { + isKnownSafeCommand as isMacSafeCommand, + isDangerousCommand as isMacDangerousCommand, +} from '../sandbox/macos/commandSafety.js'; +import { + isKnownSafeCommand as isWindowsSafeCommand, + isDangerousCommand as isWindowsDangerousCommand, +} from '../sandbox/windows/commandSafety.js'; import { isNodeError } from '../utils/errors.js'; import { sanitizeEnvironment, @@ -90,6 +98,16 @@ export interface SandboxManager { * Prepares a command to run in a sandbox, including environment sanitization. */ prepareCommand(req: SandboxRequest): Promise; + + /** + * Checks if a command with its arguments is known to be safe for this sandbox. + */ + isKnownSafeCommand(args: string[]): boolean; + + /** + * Checks if a command with its arguments is explicitly known to be dangerous for this sandbox. + */ + isDangerousCommand(args: string[]): boolean; } /** @@ -124,6 +142,18 @@ export class NoopSandboxManager implements SandboxManager { env: sanitizedEnv, }; } + + isKnownSafeCommand(args: string[]): boolean { + return os.platform() === 'win32' + ? isWindowsSafeCommand(args) + : isMacSafeCommand(args); + } + + isDangerousCommand(args: string[]): boolean { + return os.platform() === 'win32' + ? isWindowsDangerousCommand(args) + : isMacDangerousCommand(args); + } } /** @@ -133,6 +163,14 @@ export class LocalSandboxManager implements SandboxManager { async prepareCommand(_req: SandboxRequest): Promise { throw new Error('Tool sandboxing is not yet implemented.'); } + + isKnownSafeCommand(_args: string[]): boolean { + return false; + } + + isDangerousCommand(_args: string[]): boolean { + return false; + } } /** diff --git a/packages/core/src/services/sandboxManagerFactory.ts b/packages/core/src/services/sandboxManagerFactory.ts index 669257b7b0..bb8cea4752 100644 --- a/packages/core/src/services/sandboxManagerFactory.ts +++ b/packages/core/src/services/sandboxManagerFactory.ts @@ -29,24 +29,21 @@ export function createSandboxManager( return new NoopSandboxManager(); } - const isWindows = os.platform() === 'win32'; - - if ( - isWindows && - (sandbox?.enabled || sandbox?.command === 'windows-native') - ) { - return new WindowsSandboxManager({ workspace }); - } + const modeConfig = + policyManager && approvalMode + ? policyManager.getModeConfig(approvalMode) + : undefined; if (sandbox?.enabled) { - if (os.platform() === 'linux') { + if (os.platform() === 'win32' && sandbox?.command === 'windows-native') { + return new WindowsSandboxManager({ + workspace, + modeConfig, + policyManager, + }); + } else if (os.platform() === 'linux') { return new LinuxSandboxManager({ workspace }); - } - if (os.platform() === 'darwin') { - const modeConfig = - policyManager && approvalMode - ? policyManager.getModeConfig(approvalMode) - : undefined; + } else if (os.platform() === 'darwin') { return new MacOsSandboxManager({ workspace, modeConfig, diff --git a/packages/core/src/services/sandboxedFileSystemService.test.ts b/packages/core/src/services/sandboxedFileSystemService.test.ts index 9983bcfca7..046aadb132 100644 --- a/packages/core/src/services/sandboxedFileSystemService.test.ts +++ b/packages/core/src/services/sandboxedFileSystemService.test.ts @@ -35,6 +35,14 @@ class MockSandboxManager implements SandboxManager { env: req.env || {}, }; } + + isKnownSafeCommand(): boolean { + return false; + } + + isDangerousCommand(): boolean { + return false; + } } describe('SandboxedFileSystemService', () => { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index a828771c25..6a0371b68d 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -1918,6 +1918,8 @@ describe('ShellExecutionService environment variables', () => { args: ['-c', 'ls'], env: { SANDBOXED: 'true' }, }), + isKnownSafeCommand: vi.fn().mockReturnValue(false), + isDangerousCommand: vi.fn().mockReturnValue(false), }; const configWithSandbox: ShellExecutionConfig = { diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 119e8cd7f8..11e17ca358 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -7,12 +7,47 @@ import os from 'node:os'; import fs from 'node:fs'; import path from 'node:path'; -import { quote } from 'shell-quote'; +import { quote, type ParseEntry } from 'shell-quote'; import { spawn, spawnSync, type SpawnOptionsWithoutStdio, } from 'node:child_process'; + +/** + * Extracts the primary command name from a potentially wrapped shell command. + * Strips shell wrappers and handles shopt/set/etc. + * + * @param command - The full command string. + * @param args - The arguments for the command. + * @returns The primary command name. + */ +export async function getCommandName( + command: string, + args: string[], +): Promise { + await initializeShellParsers(); + const fullCmd = [command, ...args].join(' '); + const stripped = stripShellWrapper(fullCmd); + const roots = getCommandRoots(stripped).filter( + (r) => r !== 'shopt' && r !== 'set', + ); + if (roots.length > 0) { + return roots[0]; + } + return path.basename(command); +} + +/** + * Extracts a string representation from a shell-quote ParseEntry. + */ +export function extractStringFromParseEntry(entry: ParseEntry): string { + if (typeof entry === 'string') return entry; + if ('pattern' in entry) return entry.pattern; + if ('op' in entry) return entry.op; + if ('comment' in entry) return ''; // We can typically ignore comments for safety checks + return ''; +} import * as readline from 'node:readline'; import { Language, Parser, Query, type Node, type Tree } from 'web-tree-sitter'; import { loadWasmBinary } from './fileUtils.js';