From af3638640c429fec6f77c8aada326bd779e2af33 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Wed, 8 Apr 2026 15:00:50 -0700 Subject: [PATCH] fix(core): resolve windows symlink bypass and stabilize sandbox integration tests (#24834) --- .../src/sandbox/linux/LinuxSandboxManager.ts | 11 +- .../sandbox/macos/MacOsSandboxManager.test.ts | 11 +- .../src/sandbox/macos/MacOsSandboxManager.ts | 13 +- .../windows/WindowsSandboxManager.test.ts | 8 +- .../sandbox/windows/WindowsSandboxManager.ts | 78 +- .../sandboxManager.integration.test.ts | 868 +++++++++--------- .../core/src/services/sandboxManager.test.ts | 8 +- packages/core/src/services/sandboxManager.ts | 92 +- 8 files changed, 586 insertions(+), 503 deletions(-) diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index f210138127..facd2fe46f 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -249,8 +249,11 @@ export class LinuxSandboxManager implements SandboxManager { const sanitizedEnv = sanitizeEnvironment(req.env, sanitizationConfig); - const { allowed: allowedPaths, forbidden: forbiddenPaths } = - await resolveSandboxPaths(this.options, req); + const resolvedPaths = await resolveSandboxPaths( + this.options, + req, + mergedAdditional, + ); for (const file of GOVERNANCE_FILES) { const filePath = join(this.options.workspace, file.path); @@ -261,8 +264,8 @@ export class LinuxSandboxManager implements SandboxManager { workspace: this.options.workspace, workspaceWrite, networkAccess, - allowedPaths, - forbiddenPaths, + allowedPaths: resolvedPaths.policyAllowed, + forbiddenPaths: resolvedPaths.forbidden, additionalPermissions: mergedAdditional, includeDirectories: this.options.includeDirectories || [], maskFilePath: this.getMaskFilePath(), diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index 7b58f70696..c7bdd351a7 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -233,7 +233,10 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - allowedPaths: ['/tmp/allowed1', '/tmp/allowed2'], + allowedPaths: expect.arrayContaining([ + '/tmp/allowed1', + '/tmp/allowed2', + ]), }), ); }); @@ -255,7 +258,7 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - forbiddenPaths: ['/tmp/forbidden1'], + forbiddenPaths: expect.arrayContaining(['/tmp/forbidden1']), }), ); }); @@ -275,7 +278,7 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - forbiddenPaths: ['/tmp/does-not-exist'], + forbiddenPaths: expect.arrayContaining(['/tmp/does-not-exist']), }), ); }); @@ -299,7 +302,7 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ allowedPaths: [], - forbiddenPaths: ['/tmp/conflict'], + forbiddenPaths: expect.arrayContaining(['/tmp/conflict']), }), ); }); diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index 44774e8e82..27e6867030 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -106,13 +106,9 @@ export class MacOsSandboxManager implements SandboxManager { const isYolo = this.options.modeConfig?.yolo ?? false; const workspaceWrite = !isReadonlyMode || isApproved || isYolo; - const defaultNetwork = this.options.modeConfig?.network || req.policy?.networkAccess || isYolo; - const { allowed: allowedPaths, forbidden: forbiddenPaths } = - await resolveSandboxPaths(this.options, req); - // Fetch persistent approvals for this command const commandName = await getFullCommandName(currentReq); const persistentPermissions = allowOverrides @@ -137,6 +133,11 @@ export class MacOsSandboxManager implements SandboxManager { false, }; + const resolvedPaths = await resolveSandboxPaths( + this.options, + req, + mergedAdditional, + ); const { command: finalCommand, args: finalArgs } = handleReadWriteCommands( req, mergedAdditional, @@ -147,10 +148,10 @@ export class MacOsSandboxManager implements SandboxManager { const sandboxArgs = buildSeatbeltProfile({ workspace: this.options.workspace, allowedPaths: [ - ...allowedPaths, + ...resolvedPaths.policyAllowed, ...(this.options.includeDirectories || []), ], - forbiddenPaths, + forbiddenPaths: resolvedPaths.forbidden, networkAccess: mergedAdditional.network, workspaceWrite, additionalPermissions: mergedAdditional, diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index c814f740f7..40902b9121 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -398,16 +398,16 @@ describe('WindowsSandboxManager', () => { expect(icaclsArgs).toContainEqual([ path.resolve(longPath), '/grant', - '*S-1-16-4096:(OI)(CI)(M)', + '*S-1-16-4096:(M)', '/setintegritylevel', - '(OI)(CI)Low', + 'Low', ]); expect(icaclsArgs).toContainEqual([ path.resolve(devicePath), '/grant', - '*S-1-16-4096:(OI)(CI)(M)', + '*S-1-16-4096:(M)', '/setintegritylevel', - '(OI)(CI)Low', + 'Low', ]); }, ); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index a2d6428906..86d1eda641 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -15,7 +15,6 @@ import { GOVERNANCE_FILES, findSecretFiles, type GlobalSandboxOptions, - sanitizePaths, type SandboxPermissions, type ParsedSandboxDenial, resolveSandboxPaths, @@ -51,6 +50,10 @@ const __dirname = path.dirname(__filename); // S-1-16-4096 is the SID for "Low Mandatory Level" (Low Integrity) const LOW_INTEGRITY_SID = '*S-1-16-4096'; +// icacls flags: (OI) Object Inherit, (CI) Container Inherits. +// Omit /T (recursive) for performance; (OI)(CI) ensures inheritance for new items. +const DIRECTORY_FLAGS = '(OI)(CI)'; + /** * A SandboxManager implementation for Windows that uses Restricted Tokens, * Job Objects, and Low Integrity levels for process isolation. @@ -277,8 +280,11 @@ export class WindowsSandboxManager implements SandboxManager { this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; const networkAccess = defaultNetwork || mergedAdditional.network; - const { allowed: allowedPaths, forbidden: forbiddenPaths } = - await resolveSandboxPaths(this.options, req); + const resolvedPaths = await resolveSandboxPaths( + this.options, + req, + mergedAdditional, + ); // Track all roots where Low Integrity write access has been granted. // New files created within these roots will inherit the Low label. @@ -294,51 +300,45 @@ export class WindowsSandboxManager implements SandboxManager { : false; if (!isReadonlyMode || isApproved) { - await this.grantLowIntegrityAccess(this.options.workspace); - writableRoots.push(this.options.workspace); + await this.grantLowIntegrityAccess(resolvedPaths.workspace.resolved); + writableRoots.push(resolvedPaths.workspace.resolved); } // 2. Globally included directories - const includeDirs = sanitizePaths(this.options.includeDirectories); - for (const includeDir of includeDirs) { + for (const includeDir of resolvedPaths.globalIncludes) { await this.grantLowIntegrityAccess(includeDir); writableRoots.push(includeDir); } // 3. Explicitly allowed paths from the request policy - for (const allowedPath of allowedPaths) { - const resolved = resolveToRealPath(allowedPath); + for (const allowedPath of resolvedPaths.policyAllowed) { try { - await fs.promises.access(resolved, fs.constants.F_OK); + await fs.promises.access(allowedPath, fs.constants.F_OK); } catch { throw new Error( - `Sandbox request rejected: Allowed path does not exist: ${resolved}. ` + + `Sandbox request rejected: Allowed path does not exist: ${allowedPath}. ` + 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', ); } - await this.grantLowIntegrityAccess(resolved); - writableRoots.push(resolved); + await this.grantLowIntegrityAccess(allowedPath); + writableRoots.push(allowedPath); } // 4. Additional write paths (e.g. from internal __write command) - const additionalWritePaths = sanitizePaths( - mergedAdditional.fileSystem?.write, - ); - for (const writePath of additionalWritePaths) { - const resolved = resolveToRealPath(writePath); + for (const writePath of resolvedPaths.policyWrite) { try { - await fs.promises.access(resolved, fs.constants.F_OK); - await this.grantLowIntegrityAccess(resolved); + await fs.promises.access(writePath, fs.constants.F_OK); + await this.grantLowIntegrityAccess(writePath); continue; } catch { // If the file doesn't exist, it's only allowed if it resides within a granted root. const isInherited = writableRoots.some((root) => - isSubpath(root, resolved), + isSubpath(root, writePath), ); if (!isInherited) { throw new Error( - `Sandbox request rejected: Additional write path does not exist and its parent directory is not allowed: ${resolved}. ` + + `Sandbox request rejected: Additional write path does not exist and its parent directory is not allowed: ${writePath}. ` + 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', ); } @@ -350,9 +350,9 @@ export class WindowsSandboxManager implements SandboxManager { // processes to ensure they cannot be read or written. const secretsToBlock: string[] = []; const searchDirs = new Set([ - this.options.workspace, - ...allowedPaths, - ...includeDirs, + resolvedPaths.workspace.resolved, + ...resolvedPaths.policyAllowed, + ...resolvedPaths.globalIncludes, ]); for (const dir of searchDirs) { try { @@ -382,7 +382,7 @@ export class WindowsSandboxManager implements SandboxManager { // is restricted to avoid host corruption. External commands rely on // Low Integrity read/write restrictions, while internal commands // use the manifest for enforcement. - for (const forbiddenPath of forbiddenPaths) { + for (const forbiddenPath of resolvedPaths.forbidden) { try { await this.denyLowIntegrityAccess(forbiddenPath); } catch (e) { @@ -398,14 +398,14 @@ export class WindowsSandboxManager implements SandboxManager { // the sandboxed process from creating them with Low integrity. // By being created as Medium integrity, they are write-protected from Low processes. for (const file of GOVERNANCE_FILES) { - const filePath = path.join(this.options.workspace, file.path); + const filePath = path.join(resolvedPaths.workspace.resolved, file.path); this.touch(filePath, file.isDirectory); } // 4. Forbidden paths manifest // We use a manifest file to avoid command-line length limits. const allForbidden = Array.from( - new Set([...secretsToBlock, ...forbiddenPaths]), + new Set([...secretsToBlock, ...resolvedPaths.forbidden]), ); const tempDir = fs.mkdtempSync( path.join(os.tmpdir(), 'gemini-cli-forbidden-'), @@ -475,14 +475,19 @@ export class WindowsSandboxManager implements SandboxManager { } try { + const stats = await fs.promises.stat(resolvedPath); + const isDirectory = stats.isDirectory(); + + const flags = isDirectory ? DIRECTORY_FLAGS : ''; + // 1. Grant explicit Modify access to the Low Integrity SID // 2. Set the Mandatory Label to Low to allow "Write Up" from Low processes await spawnAsync('icacls', [ resolvedPath, '/grant', - `${LOW_INTEGRITY_SID}:(OI)(CI)(M)`, + `${LOW_INTEGRITY_SID}:${flags}(M)`, '/setintegritylevel', - '(OI)(CI)Low', + `${flags}Low`, ]); this.allowedCache.add(resolvedPath); } catch (e) { @@ -512,29 +517,26 @@ export class WindowsSandboxManager implements SandboxManager { return; } - // icacls flags: (OI) Object Inherit, (CI) Container Inherit, (F) Full Access Deny. - // Omit /T (recursive) for performance; (OI)(CI) ensures inheritance for new items. - // Windows dynamically evaluates existing items, though deep explicit Allow ACEs - // could potentially bypass this inherited Deny rule. - const DENY_ALL_INHERIT = '(OI)(CI)(F)'; - // icacls fails on non-existent paths, so we cannot explicitly deny // paths that do not yet exist (unlike macOS/Linux). // Skip to prevent sandbox initialization failure. + let isDirectory = false; try { - await fs.promises.stat(resolvedPath); + const stats = await fs.promises.stat(resolvedPath); + isDirectory = stats.isDirectory(); } catch (e: unknown) { if (isNodeError(e) && e.code === 'ENOENT') { return; } throw e; } + const flags = isDirectory ? DIRECTORY_FLAGS : ''; try { await spawnAsync('icacls', [ resolvedPath, '/deny', - `${LOW_INTEGRITY_SID}:${DENY_ALL_INHERIT}`, + `${LOW_INTEGRITY_SID}:${flags}(F)`, ]); this.deniedCache.add(resolvedPath); } catch (e) { diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index 4923de97bf..1461b6d606 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -1,4 +1,4 @@ -/** +/** * @license * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 @@ -8,11 +8,10 @@ import { createSandboxManager } from './sandboxManagerFactory.js'; import { ShellExecutionService } from './shellExecutionService.js'; import { getSecureSanitizationConfig } from './environmentSanitization.js'; import { + type SandboxManager, type SandboxedCommand, - NoopSandboxManager, - LocalSandboxManager, } from './sandboxManager.js'; -import { execFile, execSync } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import os from 'node:os'; import fs from 'node:fs'; @@ -20,49 +19,59 @@ import path from 'node:path'; import http from 'node:http'; /** - * Abstracts platform-specific shell commands for integration testing. + * Cross-platform command wrappers using Node.js inline scripts. + * Ensures consistent execution behavior and reliable exit codes across + * different host operating systems and restricted sandbox environments. */ const Platform = { isWindows: os.platform() === 'win32', + isMac: os.platform() === 'darwin', /** Returns a command to create an empty file. */ touch(filePath: string) { - return this.isWindows - ? { - command: 'powershell.exe', - args: [ - '-NoProfile', - '-Command', - `New-Item -Path "${filePath}" -ItemType File -Force`, - ], - } - : { command: 'touch', args: [filePath] }; + return { + command: process.execPath, + args: [ + '-e', + `require("node:fs").writeFileSync(${JSON.stringify(filePath)}, "")`, + ], + }; }, /** Returns a command to read a file's content. */ cat(filePath: string) { - return this.isWindows - ? { command: 'cmd.exe', args: ['/c', `type "${filePath}"`] } - : { command: 'cat', args: [filePath] }; + return { + command: process.execPath, + args: [ + '-e', + `console.log(require("node:fs").readFileSync(${JSON.stringify(filePath)}, "utf8"))`, + ], + }; }, /** Returns a command to echo a string. */ echo(text: string) { - return this.isWindows - ? { command: 'cmd.exe', args: ['/c', `echo ${text}`] } - : { command: 'echo', args: [text] }; + return { + command: process.execPath, + args: ['-e', `console.log(${JSON.stringify(text)})`], + }; }, /** Returns a command to perform a network request. */ curl(url: string) { - return { command: 'curl', args: ['-s', '--connect-timeout', '1', url] }; + return { + command: process.execPath, + args: [ + '-e', + `require("node:http").get(${JSON.stringify(url)}, (res) => { res.on("data", (d) => process.stdout.write(d)); res.on("end", () => process.exit(0)); }).on("error", () => process.exit(1));`, + ], + }; }, /** Returns a command that checks if the current terminal is interactive. */ isPty() { - return this.isWindows - ? 'powershell.exe -NoProfile -Command "echo True"' - : 'bash -c "if [ -t 1 ]; then echo True; else echo False; fi"'; + // ShellExecutionService.execute expects a raw shell string + return `"${process.execPath}" -e "console.log(process.stdout.isTTY ? 'True' : 'False')"`; }, /** Returns a path that is strictly outside the workspace and likely blocked. */ @@ -96,462 +105,465 @@ async function runCommand(command: SandboxedCommand) { } /** - * Determines if the system has the necessary binaries to run the sandbox. - * Throws an error if a supported platform is missing its required tools. + * Asserts the result of a sandboxed command execution, and provides detailed + * diagnostics on failure. */ -function ensureSandboxAvailable(): boolean { - const platform = os.platform(); +function assertResult( + result: { status: number; stdout: string; stderr: string }, + command: SandboxedCommand, + expected: 'success' | 'failure', +) { + const isSuccess = result.status === 0; + const shouldBeSuccess = expected === 'success'; - if (platform === 'win32') { - // Windows sandboxing relies on icacls, which is a core system utility and - // always available. - // TODO: reenable once flakiness is addressed - return false; - } - - if (platform === 'darwin') { - if (fs.existsSync('/usr/bin/sandbox-exec')) { - try { - execSync('sandbox-exec -p "(version 1)(allow default)" echo test', { - stdio: 'ignore', - }); - return true; - } catch { - // eslint-disable-next-line no-console - console.warn( - 'sandbox-exec is present but cannot be used (likely running inside a sandbox already). Skipping sandbox tests.', - ); - return false; - } + if (isSuccess === shouldBeSuccess) { + if (shouldBeSuccess) { + expect(result.status).toBe(0); + } else { + expect(result.status).not.toBe(0); } - throw new Error( - 'Sandboxing tests on macOS require /usr/bin/sandbox-exec to be present.', - ); + return; } - if (platform === 'linux') { - try { - execSync('which bwrap', { stdio: 'ignore' }); - return true; - } catch { - throw new Error( - 'Sandboxing tests on Linux require bubblewrap (bwrap) to be installed.', - ); - } - } + const commandLine = `${command.program} ${command.args.join(' ')}`; + const message = `Command ${ + shouldBeSuccess ? 'failed' : 'succeeded' + } unexpectedly. +Command: ${commandLine} +CWD: ${command.cwd || 'N/A'} +Status: ${result.status} (expected ${expected})${ + result.stdout ? `\nStdout: ${result.stdout.trim()}` : '' + }${result.stderr ? `\nStderr: ${result.stderr.trim()}` : ''}`; - return false; + throw new Error(message); } describe('SandboxManager Integration', () => { - const workspace = process.cwd(); - const manager = createSandboxManager({ enabled: true }, { workspace }); + const tempDirectories: string[] = []; - // Skip if we are on an unsupported platform or if it's a NoopSandboxManager - const shouldSkip = - manager instanceof NoopSandboxManager || - manager instanceof LocalSandboxManager || - !ensureSandboxAvailable(); + /** + * Creates a temporary directory. + * - macOS: Created in process.cwd() to avoid the seatbelt profile's global os.tmpdir() whitelist. + * - Win/Linux: Created in os.tmpdir() because enforcing sandbox restrictions inside a large directory can be very slow. + */ + function createTempDir(prefix = 'gemini-sandbox-test-'): string { + const baseDir = Platform.isMac + ? path.join(process.cwd(), `.${prefix}`) + : path.join(os.tmpdir(), prefix); - describe.skipIf(shouldSkip)('Cross-platform Sandbox Behavior', () => { - describe('Basic Execution', () => { - it('executes commands within the workspace', async () => { - const { command, args } = Platform.echo('sandbox test'); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); + const dir = fs.mkdtempSync(baseDir); + tempDirectories.push(dir); + return dir; + } - const result = await runCommand(sandboxed); - expect(result.status).toBe(0); - expect(result.stdout.trim()).toBe('sandbox test'); + let workspace: string; + let manager: SandboxManager; + + beforeAll(() => { + workspace = createTempDir('workspace-'); + manager = createSandboxManager({ enabled: true }, { workspace }); + }); + + afterAll(() => { + for (const dir of tempDirectories) { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // Best-effort cleanup + } + } + }); + + describe('Basic Execution', () => { + it('executes commands within the workspace', async () => { + const { command, args } = Platform.echo('sandbox test'); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, }); - // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes - // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. - it.skipIf(Platform.isWindows)( - 'supports interactive pseudo-terminals (node-pty)', - async () => { - const handle = await ShellExecutionService.execute( - Platform.isPty(), - workspace, - () => {}, - new AbortController().signal, - true, - { - sanitizationConfig: getSecureSanitizationConfig(), - sandboxManager: manager, - }, - ); - - const result = await handle.result; - expect(result.exitCode).toBe(0); - expect(result.output).toContain('True'); - }, - ); + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('sandbox test'); }); - describe('File System Access', () => { - it('blocks access outside the workspace', async () => { - const blockedPath = Platform.getExternalBlockedPath(); - const { command, args } = Platform.touch(blockedPath); + // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes + // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. + it.skipIf(Platform.isWindows)( + 'supports interactive pseudo-terminals (node-pty)', + async () => { + const handle = await ShellExecutionService.execute( + Platform.isPty(), + workspace, + () => {}, + new AbortController().signal, + true, + { + sanitizationConfig: getSecureSanitizationConfig(), + sandboxManager: manager, + }, + ); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(result.output).toContain('True'); + }, + ); + }); - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); + describe('File System Access', () => { + it('blocks access outside the workspace', async () => { + const blockedPath = Platform.getExternalBlockedPath(); + const { command, args } = Platform.touch(blockedPath); + + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, }); - it('allows dynamic expansion of permissions after a failure', async () => { - const tempDir = fs.mkdtempSync( - path.join(workspace, '..', 'expansion-'), - ); - const testFile = path.join(tempDir, 'test.txt'); + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); - try { - const { command, args } = Platform.touch(testFile); + it('allows dynamic expansion of permissions after a failure', async () => { + const tempDir = createTempDir('expansion-'); + const testFile = path.join(tempDir, 'test.txt'); + const { command, args } = Platform.touch(testFile); - // First attempt: fails due to sandbox restrictions - const sandboxed1 = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); - const result1 = await runCommand(sandboxed1); - expect(result1.status).not.toBe(0); - expect(fs.existsSync(testFile)).toBe(false); + // First attempt: fails due to sandbox restrictions + const sandboxed1 = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + const result1 = await runCommand(sandboxed1); + assertResult(result1, sandboxed1, 'failure'); + expect(fs.existsSync(testFile)).toBe(false); - // Second attempt: succeeds with additional permissions - const sandboxed2 = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - policy: { allowedPaths: [tempDir] }, - }); - const result2 = await runCommand(sandboxed2); - expect(result2.status).toBe(0); - expect(fs.existsSync(testFile)).toBe(true); - } finally { - if (fs.existsSync(testFile)) fs.unlinkSync(testFile); - fs.rmSync(tempDir, { recursive: true, force: true }); - } + // Second attempt: succeeds with additional permissions + const sandboxed2 = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [tempDir] }, + }); + const result2 = await runCommand(sandboxed2); + assertResult(result2, sandboxed2, 'success'); + expect(fs.existsSync(testFile)).toBe(true); + }); + + it('grants access to explicitly allowed paths', async () => { + const allowedDir = createTempDir('allowed-'); + const testFile = path.join(allowedDir, 'test.txt'); + + const { command, args } = Platform.touch(testFile); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [allowedDir] }, }); - it('grants access to explicitly allowed paths', async () => { - const allowedDir = fs.mkdtempSync( - path.join(workspace, '..', 'allowed-'), - ); - const testFile = path.join(allowedDir, 'test.txt'); + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(testFile)).toBe(true); + }); - try { - const { command, args } = Platform.touch(testFile); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - policy: { allowedPaths: [allowedDir] }, - }); + it('blocks write access to forbidden paths within the workspace', async () => { + const tempWorkspace = createTempDir('workspace-'); + const forbiddenDir = path.join(tempWorkspace, 'forbidden'); + const testFile = path.join(forbiddenDir, 'test.txt'); + fs.mkdirSync(forbiddenDir); - const result = await runCommand(sandboxed); - expect(result.status).toBe(0); - expect(fs.existsSync(testFile)).toBe(true); - } finally { - if (fs.existsSync(testFile)) fs.unlinkSync(testFile); - fs.rmSync(allowedDir, { recursive: true, force: true }); - } + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, + ); + const { command, args } = Platform.touch(testFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, }); - it('blocks access to forbidden paths within the workspace', async () => { - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), - ); + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); + + // Windows icacls does not reliably block read-up access for Low Integrity + // processes, so we skip read-specific assertions on Windows. The internal + // tool architecture prevents read bypasses via the C# wrapper and __read. + it.skipIf(Platform.isWindows)( + 'blocks read access to forbidden paths within the workspace', + async () => { + const tempWorkspace = createTempDir('workspace-'); const forbiddenDir = path.join(tempWorkspace, 'forbidden'); const testFile = path.join(forbiddenDir, 'test.txt'); fs.mkdirSync(forbiddenDir); + fs.writeFileSync(testFile, 'secret data'); - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [forbiddenDir], - }, - ); - const { command, args } = Platform.touch(testFile); - - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - }); - - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - - it('blocks access to files inside forbidden directories recursively', async () => { - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, ); - const forbiddenDir = path.join(tempWorkspace, 'forbidden'); - const nestedDir = path.join(forbiddenDir, 'nested'); - const nestedFile = path.join(nestedDir, 'test.txt'); - fs.mkdirSync(nestedDir, { recursive: true }); - fs.writeFileSync(nestedFile, 'secret'); + const { command, args } = Platform.cat(testFile); - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [forbiddenDir], - }, - ); - const { command, args } = Platform.cat(nestedFile); - - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - }); - - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - - it('prioritizes forbiddenPaths over allowedPaths', async () => { - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), - ); - const conflictDir = path.join(tempWorkspace, 'conflict'); - const testFile = path.join(conflictDir, 'test.txt'); - fs.mkdirSync(conflictDir); - - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [conflictDir], - }, - ); - const { command, args } = Platform.touch(testFile); - - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - policy: { - allowedPaths: [conflictDir], - }, - }); - - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - - it('gracefully ignores non-existent paths in allowedPaths and forbiddenPaths', async () => { - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), - ); - const nonExistentPath = path.join(tempWorkspace, 'does-not-exist'); - - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [nonExistentPath], - }, - ); - const { command, args } = Platform.echo('survived'); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - policy: { - allowedPaths: [nonExistentPath], - }, - }); - const result = await runCommand(sandboxed); - expect(result.status).toBe(0); - expect(result.stdout.trim()).toBe('survived'); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - - it('prevents creation of non-existent forbidden paths', async () => { - // Windows icacls cannot explicitly protect paths that have not yet been created. - if (Platform.isWindows) return; - - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), - ); - const nonExistentFile = path.join(tempWorkspace, 'never-created.txt'); - - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [nonExistentFile], - }, - ); - - // We use touch to attempt creation of the file - const { command: cmdTouch, args: argsTouch } = - Platform.touch(nonExistentFile); - - const sandboxedCmd = await osManager.prepareCommand({ - command: cmdTouch, - args: argsTouch, - cwd: tempWorkspace, - env: process.env, - }); - - // Execute the command, we expect it to fail (permission denied or read-only file system) - const result = await runCommand(sandboxedCmd); - - expect(result.status).not.toBe(0); - expect(fs.existsSync(nonExistentFile)).toBe(false); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - - it('blocks access to both a symlink and its target when the symlink is forbidden', async () => { - if (Platform.isWindows) return; - - const tempWorkspace = fs.mkdtempSync( - path.join(os.tmpdir(), 'workspace-'), - ); - const targetFile = path.join(tempWorkspace, 'target.txt'); - const symlinkFile = path.join(tempWorkspace, 'link.txt'); - - fs.writeFileSync(targetFile, 'secret data'); - fs.symlinkSync(targetFile, symlinkFile); - - try { - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [symlinkFile], - }, - ); - - // Attempt to read the target file directly - const { command: cmdTarget, args: argsTarget } = - Platform.cat(targetFile); - const commandTarget = await osManager.prepareCommand({ - command: cmdTarget, - args: argsTarget, - cwd: tempWorkspace, - env: process.env, - }); - const resultTarget = await runCommand(commandTarget); - expect(resultTarget.status).not.toBe(0); - - // Attempt to read via the symlink - const { command: cmdLink, args: argsLink } = - Platform.cat(symlinkFile); - const commandLink = await osManager.prepareCommand({ - command: cmdLink, - args: argsLink, - cwd: tempWorkspace, - env: process.env, - }); - const resultLink = await runCommand(commandLink); - expect(resultLink.status).not.toBe(0); - } finally { - fs.rmSync(tempWorkspace, { recursive: true, force: true }); - } - }); - }); - - describe('Network Access', () => { - let server: http.Server; - let url: string; - - beforeAll(async () => { - server = http.createServer((_, res) => { - res.setHeader('Connection', 'close'); - res.writeHead(200); - res.end('ok'); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, }); - await new Promise((resolve, reject) => { - server.on('error', reject); - server.listen(0, '127.0.0.1', () => { - const addr = server.address() as import('net').AddressInfo; - url = `http://127.0.0.1:${addr.port}`; - resolve(); - }); - }); - }); - afterAll(async () => { - if (server) await new Promise((res) => server.close(() => res())); - }); + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }, + ); - // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, - // so this test cannot verify loopback blocking on Windows. - it.skipIf(Platform.isWindows)( - 'blocks network access by default', - async () => { - const { command, args } = Platform.curl(url); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); + it('blocks access to files inside forbidden directories recursively', async () => { + const tempWorkspace = createTempDir('workspace-'); + const forbiddenDir = path.join(tempWorkspace, 'forbidden'); + const nestedDir = path.join(forbiddenDir, 'nested'); + const nestedFile = path.join(nestedDir, 'test.txt'); - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); + // Create the base forbidden directory first so the manager can restrict access to it. + fs.mkdirSync(forbiddenDir); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], }, ); - it('grants network access when explicitly allowed', async () => { + // Execute a dummy command so the manager initializes its restrictions. + const dummyCommand = await osManager.prepareCommand({ + ...Platform.echo('init'), + cwd: tempWorkspace, + env: process.env, + }); + await runCommand(dummyCommand); + + // Now create the nested items. They will inherit the sandbox restrictions from their parent. + fs.mkdirSync(nestedDir, { recursive: true }); + fs.writeFileSync(nestedFile, 'secret'); + + const { command, args } = Platform.touch(nestedFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); + + it('prioritizes forbiddenPaths over allowedPaths', async () => { + const tempWorkspace = createTempDir('workspace-'); + const conflictDir = path.join(tempWorkspace, 'conflict'); + const testFile = path.join(conflictDir, 'test.txt'); + fs.mkdirSync(conflictDir); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [conflictDir], + }, + ); + const { command, args } = Platform.touch(testFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + policy: { + allowedPaths: [conflictDir], + }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); + + it('gracefully ignores non-existent paths in allowedPaths and forbiddenPaths', async () => { + const tempWorkspace = createTempDir('workspace-'); + const nonExistentPath = path.join(tempWorkspace, 'does-not-exist'); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentPath], + }, + ); + const { command, args } = Platform.echo('survived'); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + policy: { + allowedPaths: [nonExistentPath], + }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('survived'); + }); + + it('prevents creation of non-existent forbidden paths', async () => { + const tempWorkspace = createTempDir('workspace-'); + const nonExistentFile = path.join(tempWorkspace, 'never-created.txt'); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentFile], + }, + ); + + // We use touch to attempt creation of the file + const { command: cmdTouch, args: argsTouch } = + Platform.touch(nonExistentFile); + + const sandboxedCmd = await osManager.prepareCommand({ + command: cmdTouch, + args: argsTouch, + cwd: tempWorkspace, + env: process.env, + }); + + // Execute the command, we expect it to fail (permission denied or read-only file system) + const result = await runCommand(sandboxedCmd); + + assertResult(result, sandboxedCmd, 'failure'); + expect(fs.existsSync(nonExistentFile)).toBe(false); + }); + + it('blocks access to both a symlink and its target when the symlink is forbidden', async () => { + const tempWorkspace = createTempDir('workspace-'); + const targetFile = path.join(tempWorkspace, 'target.txt'); + const symlinkFile = path.join(tempWorkspace, 'link.txt'); + + fs.writeFileSync(targetFile, 'secret data'); + fs.symlinkSync(targetFile, symlinkFile); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [symlinkFile], + }, + ); + + // Attempt to write to the target file directly + const { command: cmdTarget, args: argsTarget } = + Platform.touch(targetFile); + const commandTarget = await osManager.prepareCommand({ + command: cmdTarget, + args: argsTarget, + cwd: tempWorkspace, + env: process.env, + }); + + const resultTarget = await runCommand(commandTarget); + assertResult(resultTarget, commandTarget, 'failure'); + + // Attempt to write via the symlink + const { command: cmdLink, args: argsLink } = Platform.touch(symlinkFile); + const commandLink = await osManager.prepareCommand({ + command: cmdLink, + args: argsLink, + cwd: tempWorkspace, + env: process.env, + }); + + const resultLink = await runCommand(commandLink); + assertResult(resultLink, commandLink, 'failure'); + }); + }); + + describe('Network Access', () => { + let server: http.Server; + let url: string; + + beforeAll(async () => { + server = http.createServer((_, res) => { + res.setHeader('Connection', 'close'); + res.writeHead(200); + res.end('ok'); + }); + await new Promise((resolve, reject) => { + server.on('error', reject); + server.listen(0, '127.0.0.1', () => { + const addr = server.address() as import('net').AddressInfo; + url = `http://127.0.0.1:${addr.port}`; + resolve(); + }); + }); + }); + + afterAll(async () => { + if (server) await new Promise((res) => server.close(() => res())); + }); + + // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, + // so this test cannot verify loopback blocking on Windows. + it.skipIf(Platform.isWindows)( + 'blocks network access by default', + async () => { const { command, args } = Platform.curl(url); const sandboxed = await manager.prepareCommand({ command, args, cwd: workspace, env: process.env, - policy: { networkAccess: true }, }); const result = await runCommand(sandboxed); - expect(result.status).toBe(0); - if (!Platform.isWindows) { - expect(result.stdout.trim()).toBe('ok'); - } + assertResult(result, sandboxed, 'failure'); + }, + ); + + it('grants network access when explicitly allowed', async () => { + const { command, args } = Platform.curl(url); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { networkAccess: true }, }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + if (!Platform.isWindows) { + expect(result.stdout.trim()).toBe('ok'); + } }); }); }); diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index d6b026395a..134ef167bd 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -204,7 +204,7 @@ describe('SandboxManager', () => { const result = await resolveSandboxPaths(options, req as SandboxRequest); - expect(result.allowed).toEqual([allowed]); + expect(result.policyAllowed).toEqual([allowed]); expect(result.forbidden).toEqual([forbidden]); }); @@ -226,7 +226,7 @@ describe('SandboxManager', () => { const result = await resolveSandboxPaths(options, req as SandboxRequest); - expect(result.allowed).toEqual([other]); + expect(result.policyAllowed).toEqual([other]); }); it('should prioritize forbidden paths over allowed paths', async () => { @@ -249,7 +249,7 @@ describe('SandboxManager', () => { const result = await resolveSandboxPaths(options, req as SandboxRequest); - expect(result.allowed).toEqual([normal]); + expect(result.policyAllowed).toEqual([normal]); expect(result.forbidden).toEqual([secret]); }); @@ -274,7 +274,7 @@ describe('SandboxManager', () => { const result = await resolveSandboxPaths(options, req as SandboxRequest); - expect(result.allowed).toEqual([]); + expect(result.policyAllowed).toEqual([]); expect(result.forbidden).toEqual([secretUpper]); }); }); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index 673c13b9af..f7f2944fe7 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -23,6 +23,33 @@ import { } from './environmentSanitization.js'; import type { ShellExecutionResult } from './shellExecutionService.js'; import type { SandboxPolicyManager } from '../policy/sandboxPolicyManager.js'; +import { resolveToRealPath } from '../utils/paths.js'; + +/** + * A structured result of fully resolved sandbox paths. + * All paths in this object are absolute, deduplicated, and expanded to include + * both the original path and its real target (if it is a symlink). + */ +export interface ResolvedSandboxPaths { + /** The primary workspace directory. */ + workspace: { + /** The original path provided in the sandbox options. */ + original: string; + /** The real path. */ + resolved: string; + }; + /** Explicitly denied paths. */ + forbidden: string[]; + /** Directories included globally across all commands in this sandbox session. */ + globalIncludes: string[]; + /** Paths explicitly allowed by the policy of the currently executing command. */ + policyAllowed: string[]; + /** Paths granted temporary read access by the current command's dynamic permissions. */ + policyRead: string[]; + /** Paths granted temporary write access by the current command's dynamic permissions. */ + policyWrite: string[]; +} + export interface SandboxPermissions { /** Filesystem permissions. */ fileSystem?: { @@ -326,33 +353,68 @@ export class LocalSandboxManager implements SandboxManager { } /** - * Resolves sanitized allowed and forbidden paths for a request. - * Filters the workspace from allowed paths and ensures forbidden paths take precedence. + * Resolves and sanitizes all path categories for a sandbox request. */ export async function resolveSandboxPaths( options: GlobalSandboxOptions, req: SandboxRequest, -): Promise<{ - allowed: string[]; - forbidden: string[]; -}> { - const forbidden = sanitizePaths(await options.forbiddenPaths?.()); - const allowed = sanitizePaths(req.policy?.allowedPaths); + overridePermissions?: SandboxPermissions, +): Promise { + /** + * Helper that expands each path to include its realpath (if it's a symlink) + * and pipes the result through sanitizePaths for deduplication and absolute path enforcement. + */ + const expand = (paths?: string[] | null): string[] => { + if (!paths || paths.length === 0) return []; + const expanded = paths.flatMap((p) => { + try { + const resolved = resolveToRealPath(p); + return resolved === p ? [p] : [p, resolved]; + } catch { + return [p]; + } + }); + return sanitizePaths(expanded); + }; - const workspaceIdentity = getPathIdentity(options.workspace); + const forbidden = expand(await options.forbiddenPaths?.()); + + const globalIncludes = expand(options.includeDirectories); + const policyAllowed = expand(req.policy?.allowedPaths); + + const policyRead = expand(overridePermissions?.fileSystem?.read); + const policyWrite = expand(overridePermissions?.fileSystem?.write); + + const resolvedWorkspace = resolveToRealPath(options.workspace); + + const workspaceIdentities = new Set( + [options.workspace, resolvedWorkspace].map(getPathIdentity), + ); const forbiddenIdentities = new Set(forbidden.map(getPathIdentity)); - const filteredAllowed = allowed.filter((p) => { - const identity = getPathIdentity(p); - return identity !== workspaceIdentity && !forbiddenIdentities.has(identity); - }); + /** + * Filters out any paths that are explicitly forbidden or match the workspace root (original or resolved). + */ + const filter = (paths: string[]) => + paths.filter((p) => { + const identity = getPathIdentity(p); + return ( + !workspaceIdentities.has(identity) && !forbiddenIdentities.has(identity) + ); + }); return { - allowed: filteredAllowed, + workspace: { + original: options.workspace, + resolved: resolvedWorkspace, + }, forbidden, + globalIncludes: filter(globalIncludes), + policyAllowed: filter(policyAllowed), + policyRead: filter(policyRead), + policyWrite: filter(policyWrite), }; } - /** * Sanitizes an array of paths by deduplicating them and ensuring they are absolute. * Always returns an array (empty if input is null/undefined).