From 8868b34c752a965fa2fd3639cc7b5abe78cfe658 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Thu, 26 Mar 2026 15:10:15 -0700 Subject: [PATCH] refactor(core): delegate sandbox denial parsing to SandboxManager (#23928) --- .../core/src/policy/policy-engine.test.ts | 1 + .../src/sandbox/linux/LinuxSandboxManager.ts | 7 + .../src/sandbox/macos/MacOsSandboxManager.ts | 7 + .../sandbox/utils/sandboxDenialUtils.test.ts | 43 ++++ .../src/sandbox/utils/sandboxDenialUtils.ts | 81 ++++++ .../sandbox/windows/WindowsSandboxManager.ts | 6 + packages/core/src/services/sandboxManager.ts | 27 +- .../sandboxedFileSystemService.test.ts | 4 + .../services/shellExecutionService.test.ts | 1 + packages/core/src/tools/shell.ts | 243 +++++++----------- 10 files changed, 272 insertions(+), 148 deletions(-) create mode 100644 packages/core/src/sandbox/utils/sandboxDenialUtils.test.ts create mode 100644 packages/core/src/sandbox/utils/sandboxDenialUtils.ts diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index 95f754bc02..5bbe62fec9 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -375,6 +375,7 @@ describe('PolicyEngine', () => { isKnownSafeCommand: vi .fn() .mockImplementation((args) => args[0] === 'npm'), + parseDenials: vi.fn().mockReturnValue(undefined), } as unknown as SandboxManager; engine = new PolicyEngine({ diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index 28be7ad281..7f9ff599a7 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -16,7 +16,9 @@ import { GOVERNANCE_FILES, getSecretFileFindArgs, sanitizePaths, + type ParsedSandboxDenial, } from '../../services/sandboxManager.js'; +import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { sanitizeEnvironment, getSecureSanitizationConfig, @@ -38,6 +40,7 @@ import { isKnownSafeCommand, isDangerousCommand, } from '../utils/commandSafety.js'; +import { parsePosixSandboxDenials } from '../utils/sandboxDenialUtils.js'; let cachedBpfPath: string | undefined; @@ -154,6 +157,10 @@ export class LinuxSandboxManager implements SandboxManager { return isDangerousCommand(args); } + parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined { + return parsePosixSandboxDenials(result); + } + private getMaskFilePath(): string { if ( LinuxSandboxManager.maskFilePath && diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index db2768d7c6..2d7c7daf8b 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -10,7 +10,9 @@ import { type SandboxedCommand, type SandboxPermissions, type GlobalSandboxOptions, + type ParsedSandboxDenial, } from '../../services/sandboxManager.js'; +import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { sanitizeEnvironment, getSecureSanitizationConfig, @@ -27,6 +29,7 @@ import { } from '../utils/commandSafety.js'; import { type SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js'; import { verifySandboxOverrides } from '../utils/commandUtils.js'; +import { parsePosixSandboxDenials } from '../utils/sandboxDenialUtils.js'; export interface MacOsSandboxOptions extends GlobalSandboxOptions { /** The current sandbox mode behavior from config. */ @@ -59,6 +62,10 @@ export class MacOsSandboxManager implements SandboxManager { return isDangerousCommand(args); } + parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined { + return parsePosixSandboxDenials(result); + } + async prepareCommand(req: SandboxRequest): Promise { await initializeShellParsers(); const sanitizationConfig = getSecureSanitizationConfig( diff --git a/packages/core/src/sandbox/utils/sandboxDenialUtils.test.ts b/packages/core/src/sandbox/utils/sandboxDenialUtils.test.ts new file mode 100644 index 0000000000..3b4585ba69 --- /dev/null +++ b/packages/core/src/sandbox/utils/sandboxDenialUtils.test.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { parsePosixSandboxDenials } from './sandboxDenialUtils.js'; +import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; + +describe('parsePosixSandboxDenials', () => { + it('should detect file system denial and extract paths', () => { + const parsed = parsePosixSandboxDenials({ + output: 'ls: /root: Operation not permitted', + } as unknown as ShellExecutionResult); + expect(parsed).toBeDefined(); + expect(parsed?.filePaths).toContain('/root'); + }); + + it('should detect network denial', () => { + const parsed = parsePosixSandboxDenials({ + output: 'curl: (6) Could not resolve host: google.com', + } as unknown as ShellExecutionResult); + expect(parsed).toBeDefined(); + expect(parsed?.network).toBe(true); + }); + + it('should use fallback heuristic for absolute paths', () => { + const parsed = parsePosixSandboxDenials({ + output: + 'operation not permitted\nsome error happened with /some/path/to/file', + } as unknown as ShellExecutionResult); + expect(parsed).toBeDefined(); + expect(parsed?.filePaths).toContain('/some/path/to/file'); + }); + + it('should return undefined if no denial detected', () => { + const parsed = parsePosixSandboxDenials({ + output: 'hello world', + } as unknown as ShellExecutionResult); + expect(parsed).toBeUndefined(); + }); +}); diff --git a/packages/core/src/sandbox/utils/sandboxDenialUtils.ts b/packages/core/src/sandbox/utils/sandboxDenialUtils.ts new file mode 100644 index 0000000000..d1e2366e76 --- /dev/null +++ b/packages/core/src/sandbox/utils/sandboxDenialUtils.ts @@ -0,0 +1,81 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { type ParsedSandboxDenial } from '../../services/sandboxManager.js'; +import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; + +/** + * Common POSIX-style sandbox denial detection. + * Used by macOS and Linux sandbox managers. + */ +export function parsePosixSandboxDenials( + result: ShellExecutionResult, +): ParsedSandboxDenial | undefined { + const output = result.output || ''; + const errorOutput = result.error?.message; + const combined = (output + ' ' + (errorOutput || '')).toLowerCase(); + + const isFileDenial = [ + 'operation not permitted', + 'vim:e303', + 'should be read/write', + 'sandbox_apply', + 'sandbox: ', + ].some((keyword) => combined.includes(keyword)); + + const isNetworkDenial = [ + 'error connecting to', + 'network is unreachable', + 'could not resolve host', + 'connection refused', + 'no address associated with hostname', + ].some((keyword) => combined.includes(keyword)); + + if (!isFileDenial && !isNetworkDenial) { + return undefined; + } + + const filePaths = new Set(); + + // Extract denied paths (POSIX absolute paths) + const regex = + /(?:^|\s)['"]?(\/[\w.-/]+)['"]?:\s*[Oo]peration not permitted/gi; + let match; + while ((match = regex.exec(output)) !== null) { + filePaths.add(match[1]); + } + if (errorOutput) { + while ((match = regex.exec(errorOutput)) !== null) { + filePaths.add(match[1]); + } + } + + // Fallback heuristic: look for any absolute path in the output if it was a file denial + if (isFileDenial && filePaths.size === 0) { + const fallbackRegex = + /(?:^|[\s"'[\]])(\/[a-zA-Z0-9_.-]+(?:\/[a-zA-Z0-9_.-]+)+)(?:$|[\s"'[\]:])/gi; + let m; + while ((m = fallbackRegex.exec(output)) !== null) { + const p = m[1]; + if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) { + filePaths.add(p); + } + } + if (errorOutput) { + while ((m = fallbackRegex.exec(errorOutput)) !== null) { + const p = m[1]; + if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) { + filePaths.add(p); + } + } + } + } + + return { + network: isNetworkDenial || undefined, + filePaths: filePaths.size > 0 ? Array.from(filePaths) : undefined, + }; +} diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index a07241366a..d1770b094f 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -18,7 +18,9 @@ import { sanitizePaths, tryRealpath, type SandboxPermissions, + type ParsedSandboxDenial, } from '../../services/sandboxManager.js'; +import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { sanitizeEnvironment, getSecureSanitizationConfig, @@ -77,6 +79,10 @@ export class WindowsSandboxManager implements SandboxManager { return isDangerousCommand(args); } + parseDenials(_result: ShellExecutionResult): ParsedSandboxDenial | undefined { + return undefined; // TODO: Implement Windows-specific denial parsing + } + /** * Ensures a file or directory exists. */ diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index 0028ba9f24..41b0ab045d 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -21,7 +21,7 @@ import { getSecureSanitizationConfig, type EnvironmentSanitizationConfig, } from './environmentSanitization.js'; - +import type { ShellExecutionResult } from './shellExecutionService.js'; export interface SandboxPermissions { /** Filesystem permissions. */ fileSystem?: { @@ -91,6 +91,16 @@ export interface SandboxedCommand { cwd?: string; } +/** + * A structured result from parsing sandbox denials. + */ +export interface ParsedSandboxDenial { + /** If the denial is related to file system access, these are the paths that were blocked. */ + filePaths?: string[]; + /** If the denial is related to network access. */ + network?: boolean; +} + /** * Interface for a service that prepares commands for sandboxed execution. */ @@ -109,6 +119,11 @@ export interface SandboxManager { * Checks if a command with its arguments is explicitly known to be dangerous for this sandbox. */ isDangerousCommand(args: string[]): boolean; + + /** + * Parses the output of a command to detect sandbox denials. + */ + parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined; } /** @@ -236,10 +251,14 @@ export class NoopSandboxManager implements SandboxManager { ? isWindowsDangerousCommand(args) : isMacDangerousCommand(args); } + + parseDenials(): undefined { + return undefined; + } } /** - * SandboxManager that implements actual sandboxing. + * A SandboxManager implementation that just runs locally (no sandboxing yet). */ export class LocalSandboxManager implements SandboxManager { async prepareCommand(_req: SandboxRequest): Promise { @@ -253,6 +272,10 @@ export class LocalSandboxManager implements SandboxManager { isDangerousCommand(_args: string[]): boolean { return false; } + + parseDenials(): undefined { + return undefined; + } } /** diff --git a/packages/core/src/services/sandboxedFileSystemService.test.ts b/packages/core/src/services/sandboxedFileSystemService.test.ts index 046aadb132..1070af54d3 100644 --- a/packages/core/src/services/sandboxedFileSystemService.test.ts +++ b/packages/core/src/services/sandboxedFileSystemService.test.ts @@ -43,6 +43,10 @@ class MockSandboxManager implements SandboxManager { isDangerousCommand(): boolean { return false; } + + parseDenials(): undefined { + return undefined; + } } describe('SandboxedFileSystemService', () => { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index adb519d087..465d79fe4b 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -1914,6 +1914,7 @@ describe('ShellExecutionService environment variables', () => { }), isKnownSafeCommand: vi.fn().mockReturnValue(false), isDangerousCommand: vi.fn().mockReturnValue(false), + parseDenials: vi.fn().mockReturnValue(undefined), }; const configWithSandbox: ShellExecutionConfig = { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index f72b6f28fe..0b4760ccc7 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -478,162 +478,113 @@ export class ShellToolInvocation extends BaseToolInvocation< } // Heuristic Sandbox Denial Detection - const lowerOutput = ( - (result.output || '') + - ' ' + - (result.error?.message || '') - ).toLowerCase(); - const isFileDenial = [ - 'operation not permitted', - 'vim:e303', - 'should be read/write', - 'sandbox_apply', - 'sandbox: ', - ].some((keyword) => lowerOutput.includes(keyword)); - - const isNetworkDenial = [ - 'error connecting to', - 'network is unreachable', - 'could not resolve host', - 'connection refused', - 'no address associated with hostname', - ].some((keyword) => lowerOutput.includes(keyword)); - - // Only trigger heuristic if the command actually failed (exit code != 0 or aborted) - const failed = + if ( !!result.error || !!result.signal || (result.exitCode !== undefined && result.exitCode !== 0) || - result.aborted; + result.aborted + ) { + const sandboxDenial = + this.context.config.sandboxManager.parseDenials(result); + if (sandboxDenial) { + const strippedCommand = stripShellWrapper(this.params.command); + const rootCommands = getCommandRoots(strippedCommand).filter( + (r) => r !== 'shopt', + ); + const rootCommandDisplay = + rootCommands.length > 0 ? rootCommands[0] : 'shell'; - if (failed && (isFileDenial || isNetworkDenial)) { - const strippedCommand = stripShellWrapper(this.params.command); - const rootCommands = getCommandRoots(strippedCommand).filter( - (r) => r !== 'shopt', - ); - const rootCommandDisplay = - rootCommands.length > 0 ? rootCommands[0] : 'shell'; - // Extract denied paths - const deniedPaths = new Set(); - const regex = - /(?:^|\s)['"]?(\/[\w.-/]+)['"]?:\s*[Oo]peration not permitted/gi; - let match; - while ((match = regex.exec(result.output || '')) !== null) { - deniedPaths.add(match[1]); - } - while ((match = regex.exec(result.error?.message || '')) !== null) { - deniedPaths.add(match[1]); - } + const readPaths = new Set( + this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.read || [], + ); + const writePaths = new Set( + this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.write || [], + ); - if (isFileDenial && deniedPaths.size === 0) { - // Fallback heuristic: look for any absolute path in the output - // Avoid matching simple commands like /bin/sh - const fallbackRegex = - /(?:^|[\s"'[\]])(\/[a-zA-Z0-9_.-]+(?:\/[a-zA-Z0-9_.-]+)+)(?:$|[\s"'[\]:])/gi; - let m; - while ((m = fallbackRegex.exec(result.output || '')) !== null) { - const p = m[1]; - if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) { - deniedPaths.add(p); - } - } - while ( - (m = fallbackRegex.exec(result.error?.message || '')) !== null - ) { - const p = m[1]; - if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) { - deniedPaths.add(p); - } - } - } - - const readPaths = new Set( - this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.read || [], - ); - const writePaths = new Set( - this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.write || [], - ); - - for (const p of deniedPaths) { - try { - // Find an existing parent directory to add instead of a non-existent file - let currentPath = p; - try { - if ( - fs.existsSync(currentPath) && - fs.statSync(currentPath).isFile() - ) { - currentPath = path.dirname(currentPath); - } - } catch (_e) { - /* ignore */ - } - while (currentPath.length > 1) { - if (fs.existsSync(currentPath)) { - writePaths.add(currentPath); - readPaths.add(currentPath); - break; - } - currentPath = path.dirname(currentPath); - } - } catch (_e) { - // ignore - } - } - - const additionalPermissions = { - network: - isNetworkDenial || - this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network || - undefined, - fileSystem: - isFileDenial || writePaths.size > 0 - ? { - read: Array.from(readPaths), - write: Array.from(writePaths), + if (sandboxDenial.filePaths) { + for (const p of sandboxDenial.filePaths) { + try { + // Find an existing parent directory to add instead of a non-existent file + let currentPath = p; + try { + if ( + fs.existsSync(currentPath) && + fs.statSync(currentPath).isFile() + ) { + currentPath = path.dirname(currentPath); + } + } catch (_e) { + /* ignore */ } - : undefined, - }; + while (currentPath.length > 1) { + if (fs.existsSync(currentPath)) { + writePaths.add(currentPath); + readPaths.add(currentPath); + break; + } + currentPath = path.dirname(currentPath); + } + } catch (_e) { + // ignore + } + } + } - const originalReadSize = - this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.read?.length || - 0; - const originalWriteSize = - this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.write - ?.length || 0; - const originalNetwork = - !!this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network; - - const newReadSize = additionalPermissions.fileSystem?.read?.length || 0; - const newWriteSize = - additionalPermissions.fileSystem?.write?.length || 0; - const newNetwork = !!additionalPermissions.network; - - const hasNewPermissions = - newReadSize > originalReadSize || - newWriteSize > originalWriteSize || - (!originalNetwork && newNetwork); - - if (hasNewPermissions) { - const confirmationDetails = { - type: 'sandbox_expansion', - title: 'Sandbox Expansion Request', - command: this.params.command, - rootCommand: rootCommandDisplay, - additionalPermissions, + const additionalPermissions = { + network: + sandboxDenial.network || + this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network || + undefined, + fileSystem: + sandboxDenial.filePaths?.length || writePaths.size > 0 + ? { + read: Array.from(readPaths), + write: Array.from(writePaths), + } + : undefined, }; - return { - llmContent: 'Sandbox expansion required', - returnDisplay: returnDisplayMessage, - error: { - type: ToolErrorType.SANDBOX_EXPANSION_REQUIRED, - message: JSON.stringify(confirmationDetails), - }, - }; + const originalReadSize = + this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.read + ?.length || 0; + const originalWriteSize = + this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.write + ?.length || 0; + const originalNetwork = + !!this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network; + + const newReadSize = + additionalPermissions.fileSystem?.read?.length || 0; + const newWriteSize = + additionalPermissions.fileSystem?.write?.length || 0; + const newNetwork = !!additionalPermissions.network; + + const hasNewPermissions = + newReadSize > originalReadSize || + newWriteSize > originalWriteSize || + (!originalNetwork && newNetwork); + + if (hasNewPermissions) { + const confirmationDetails = { + type: 'sandbox_expansion', + title: 'Sandbox Expansion Request', + command: this.params.command, + rootCommand: rootCommandDisplay, + additionalPermissions, + }; + + return { + llmContent: 'Sandbox expansion required', + returnDisplay: returnDisplayMessage, + error: { + type: ToolErrorType.SANDBOX_EXPANSION_REQUIRED, + message: JSON.stringify(confirmationDetails), + }, + }; + } + // If no new permissions were found by heuristic, do not intercept. + // Just return the normal execution error so the LLM can try providing explicit paths itself. } - // If no new permissions were found by heuristic, do not intercept. - // Just return the normal execution error so the LLM can try providing explicit paths itself. } const summarizeConfig =