From 65ee6171e73235ef94bb505fecb87074f8092fbd Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Tue, 31 Mar 2026 13:35:13 -0400 Subject: [PATCH] fix(core): batch macOS seatbelt rules into a profile file to prevent ARG_MAX errors (#24255) --- .../sandbox/macos/MacOsSandboxManager.test.ts | 72 +++++++------ .../src/sandbox/macos/MacOsSandboxManager.ts | 30 ++++-- .../core/src/sandbox/macos/baseProfile.ts | 6 -- .../sandbox/macos/seatbeltArgsBuilder.test.ts | 101 +++++++----------- .../src/sandbox/macos/seatbeltArgsBuilder.ts | 80 ++++++-------- packages/core/src/services/sandboxManager.ts | 2 + .../src/services/shellExecutionService.ts | 8 +- 7 files changed, 136 insertions(+), 163 deletions(-) diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index cb1fe3c03d..a4254eb650 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -17,7 +17,7 @@ describe('MacOsSandboxManager', () => { const mockNetworkAccess = true; let mockPolicy: ExecutionPolicy; - let manager: MacOsSandboxManager; + let manager: MacOsSandboxManager | undefined; beforeEach(() => { mockWorkspace = fs.mkdtempSync( @@ -35,18 +35,10 @@ describe('MacOsSandboxManager', () => { networkAccess: mockNetworkAccess, }; - manager = new MacOsSandboxManager({ - workspace: mockWorkspace, - forbiddenPaths: [], - }); - // Mock the seatbelt args builder to isolate manager tests - vi.spyOn(seatbeltArgsBuilder, 'buildSeatbeltArgs').mockReturnValue([ - '-p', + vi.spyOn(seatbeltArgsBuilder, 'buildSeatbeltProfile').mockReturnValue( '(mock profile)', - '-D', - 'MOCK_VAR=value', - ]); + ); }); afterEach(() => { @@ -59,6 +51,7 @@ describe('MacOsSandboxManager', () => { describe('prepareCommand', () => { it('should correctly format the base command and args', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); const result = await manager.prepareCommand({ command: 'echo', args: ['hello'], @@ -67,11 +60,11 @@ describe('MacOsSandboxManager', () => { policy: mockPolicy, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith({ + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith({ workspace: mockWorkspace, allowedPaths: mockAllowedPaths, - networkAccess: mockNetworkAccess, - forbiddenPaths: [], + forbiddenPaths: undefined, + networkAccess: true, workspaceWrite: true, additionalPermissions: { fileSystem: { @@ -83,18 +76,23 @@ describe('MacOsSandboxManager', () => { }); expect(result.program).toBe('/usr/bin/sandbox-exec'); - expect(result.args).toEqual([ - '-p', - '(mock profile)', - '-D', - 'MOCK_VAR=value', - '--', - 'echo', - 'hello', - ]); + expect(result.args[0]).toBe('-f'); + expect(result.args[1]).toMatch(/gemini-cli-seatbelt-.*\.sb$/); + expect(result.args.slice(2)).toEqual(['--', 'echo', 'hello']); + + // Verify temp file was written + const tempFile = result.args[1]; + expect(fs.existsSync(tempFile)).toBe(true); + expect(fs.readFileSync(tempFile, 'utf8')).toBe('(mock profile)'); + + // Verify cleanup callback deletes the file + expect(result.cleanup).toBeDefined(); + result.cleanup!(); + expect(fs.existsSync(tempFile)).toBe(false); }); it('should correctly pass through the cwd to the resulting command', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); const result = await manager.prepareCommand({ command: 'echo', args: ['hello'], @@ -107,6 +105,7 @@ describe('MacOsSandboxManager', () => { }); it('should apply environment sanitization via the default mechanisms', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); const result = await manager.prepareCommand({ command: 'echo', args: ['hello'], @@ -126,6 +125,7 @@ describe('MacOsSandboxManager', () => { }); it('should allow network when networkAccess is true', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); await manager.prepareCommand({ command: 'echo', args: ['hello'], @@ -134,13 +134,14 @@ describe('MacOsSandboxManager', () => { policy: { ...mockPolicy, networkAccess: true }, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ networkAccess: true }), ); }); describe('governance files', () => { it('should ensure governance files exist', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); await manager.prepareCommand({ command: 'echo', args: [], @@ -151,7 +152,7 @@ describe('MacOsSandboxManager', () => { // The seatbelt builder internally handles governance files, so we simply verify // it is invoked correctly with the right workspace. - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ workspace: mockWorkspace }), ); }); @@ -159,6 +160,7 @@ describe('MacOsSandboxManager', () => { describe('allowedPaths', () => { it('should parameterize allowed paths and normalize them', async () => { + manager = new MacOsSandboxManager({ workspace: mockWorkspace }); await manager.prepareCommand({ command: 'echo', args: [], @@ -170,7 +172,7 @@ describe('MacOsSandboxManager', () => { }, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ allowedPaths: ['/tmp/allowed1', '/tmp/allowed2'], }), @@ -180,11 +182,11 @@ describe('MacOsSandboxManager', () => { describe('forbiddenPaths', () => { it('should parameterize forbidden paths and explicitly deny them', async () => { - const managerWithForbidden = new MacOsSandboxManager({ + manager = new MacOsSandboxManager({ workspace: mockWorkspace, forbiddenPaths: ['/tmp/forbidden1'], }); - await managerWithForbidden.prepareCommand({ + await manager.prepareCommand({ command: 'echo', args: [], cwd: mockWorkspace, @@ -192,7 +194,7 @@ describe('MacOsSandboxManager', () => { policy: mockPolicy, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ forbiddenPaths: ['/tmp/forbidden1'], }), @@ -200,11 +202,11 @@ describe('MacOsSandboxManager', () => { }); it('explicitly denies non-existent forbidden paths to prevent creation', async () => { - const managerWithForbidden = new MacOsSandboxManager({ + manager = new MacOsSandboxManager({ workspace: mockWorkspace, forbiddenPaths: ['/tmp/does-not-exist'], }); - await managerWithForbidden.prepareCommand({ + await manager.prepareCommand({ command: 'echo', args: [], cwd: mockWorkspace, @@ -212,7 +214,7 @@ describe('MacOsSandboxManager', () => { policy: mockPolicy, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ forbiddenPaths: ['/tmp/does-not-exist'], }), @@ -220,11 +222,11 @@ describe('MacOsSandboxManager', () => { }); it('should override allowed paths if a path is also in forbidden paths', async () => { - const managerWithForbidden = new MacOsSandboxManager({ + manager = new MacOsSandboxManager({ workspace: mockWorkspace, forbiddenPaths: ['/tmp/conflict'], }); - await managerWithForbidden.prepareCommand({ + await manager.prepareCommand({ command: 'echo', args: [], cwd: mockWorkspace, @@ -235,7 +237,7 @@ describe('MacOsSandboxManager', () => { }, }); - expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ allowedPaths: ['/tmp/conflict'], forbiddenPaths: ['/tmp/conflict'], diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index 212fafed83..e3a6a7216a 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -4,6 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { type SandboxManager, type SandboxRequest, @@ -17,7 +20,7 @@ import { sanitizeEnvironment, getSecureSanitizationConfig, } from '../../services/environmentSanitization.js'; -import { buildSeatbeltArgs } from './seatbeltArgsBuilder.js'; +import { buildSeatbeltProfile } from './seatbeltArgsBuilder.js'; import { initializeShellParsers, getCommandName, @@ -30,9 +33,6 @@ import { import { verifySandboxOverrides } from '../utils/commandUtils.js'; import { parsePosixSandboxDenials } from '../utils/sandboxDenialUtils.js'; -/** - * A SandboxManager implementation for macOS that uses Seatbelt. - */ export class MacOsSandboxManager implements SandboxManager { constructor(private readonly options: GlobalSandboxOptions) {} @@ -105,7 +105,7 @@ export class MacOsSandboxManager implements SandboxManager { false, }; - const sandboxArgs = buildSeatbeltArgs({ + const sandboxArgs = buildSeatbeltProfile({ workspace: this.options.workspace, allowedPaths: [...(req.policy?.allowedPaths || [])], forbiddenPaths: this.options.forbiddenPaths, @@ -114,11 +114,29 @@ export class MacOsSandboxManager implements SandboxManager { additionalPermissions: mergedAdditional, }); + const tempFile = this.writeProfileToTempFile(sandboxArgs); + return { program: '/usr/bin/sandbox-exec', - args: [...sandboxArgs, '--', req.command, ...req.args], + args: ['-f', tempFile, '--', req.command, ...req.args], env: sanitizedEnv, cwd: req.cwd, + cleanup: () => { + try { + fs.unlinkSync(tempFile); + } catch { + // Ignore cleanup errors + } + }, }; } + + private writeProfileToTempFile(profile: string): string { + const tempFile = path.join( + os.tmpdir(), + `gemini-cli-seatbelt-${Date.now()}-${Math.random().toString(36).slice(2)}.sb`, + ); + fs.writeFileSync(tempFile, profile, { mode: 0o600 }); + return tempFile; + } } diff --git a/packages/core/src/sandbox/macos/baseProfile.ts b/packages/core/src/sandbox/macos/baseProfile.ts index 4c712b2f1b..f4bd331889 100644 --- a/packages/core/src/sandbox/macos/baseProfile.ts +++ b/packages/core/src/sandbox/macos/baseProfile.ts @@ -134,12 +134,6 @@ export const BASE_SEATBELT_PROFILE = `(version 1) (literal "/dev/zero") (subpath "/tmp") (subpath "/private/tmp") - (subpath (param "TMPDIR")) -) - -; Workspace access using parameterized paths -(allow file-read* - (subpath (param "WORKSPACE")) ) `; diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts index fcab494059..647b7cb859 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts @@ -4,10 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ import { describe, it, expect, vi, afterEach } from 'vitest'; -import { buildSeatbeltArgs } from './seatbeltArgsBuilder.js'; +import { + buildSeatbeltProfile, + escapeSchemeString, +} from './seatbeltArgsBuilder.js'; import * as fsUtils from '../utils/fsUtils.js'; import fs from 'node:fs'; -import os from 'node:os'; vi.mock('../utils/fsUtils.js', async () => { const actual = await vi.importActual('../utils/fsUtils.js'); @@ -23,34 +25,34 @@ describe('seatbeltArgsBuilder', () => { vi.restoreAllMocks(); }); - describe('buildSeatbeltArgs', () => { - it('should build a strict allowlist profile allowing the workspace via param', () => { + describe('escapeSchemeString', () => { + it('escapes quotes and backslashes', () => { + expect(escapeSchemeString('path/to/"file"')).toBe('path/to/\\"file\\"'); + expect(escapeSchemeString('path\\to\\file')).toBe('path\\\\to\\\\file'); + }); + }); + + describe('buildSeatbeltProfile', () => { + it('should build a strict allowlist profile allowing the workspace', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/Users/test/workspace', }); - expect(args[0]).toBe('-p'); - const profile = args[1]; expect(profile).toContain('(version 1)'); expect(profile).toContain('(deny default)'); expect(profile).toContain('(allow process-exec)'); - expect(profile).toContain('(subpath (param "WORKSPACE"))'); + expect(profile).toContain(`(subpath "/Users/test/workspace")`); expect(profile).not.toContain('(allow network*)'); - - expect(args).toContain('-D'); - expect(args).toContain('WORKSPACE=/Users/test/workspace'); - expect(args).toContain(`TMPDIR=${os.tmpdir()}`); }); it('should allow network when networkAccess is true', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', networkAccess: true, }); - const profile = args[1]; expect(profile).toContain('(allow network-outbound)'); }); @@ -66,20 +68,16 @@ describe('seatbeltArgsBuilder', () => { }) as unknown as fs.Stats, ); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test/workspace', }); - const profile = args[1]; - expect(args).toContain('-D'); - expect(args).toContain('GOVERNANCE_FILE_0=/test/workspace/.gitignore'); expect(profile).toContain( - '(deny file-write* (literal (param "GOVERNANCE_FILE_0")))', + `(deny file-write* (literal "/test/workspace/.gitignore"))`, ); - expect(args).toContain('GOVERNANCE_FILE_2=/test/workspace/.git'); expect(profile).toContain( - '(deny file-write* (subpath (param "GOVERNANCE_FILE_2")))', + `(deny file-write* (subpath "/test/workspace/.git"))`, ); }); @@ -98,58 +96,45 @@ describe('seatbeltArgsBuilder', () => { }) as unknown as fs.Stats, ); - const args = buildSeatbeltArgs({ workspace: '/test/workspace' }); - const profile = args[1]; + const profile = buildSeatbeltProfile({ workspace: '/test/workspace' }); - expect(args).toContain('GOVERNANCE_FILE_0=/test/workspace/.gitignore'); - expect(args).toContain('REAL_GOVERNANCE_FILE_0=/test/real/.gitignore'); expect(profile).toContain( - '(deny file-write* (literal (param "GOVERNANCE_FILE_0")))', + `(deny file-write* (literal "/test/workspace/.gitignore"))`, ); expect(profile).toContain( - '(deny file-write* (literal (param "REAL_GOVERNANCE_FILE_0")))', + `(deny file-write* (literal "/test/real/.gitignore"))`, ); }); }); describe('allowedPaths', () => { - it('should parameterize allowed paths and normalize them', () => { + it('should embed allowed paths and normalize them', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => { if (p === '/test/symlink') return '/test/real_path'; return p; }); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', allowedPaths: ['/custom/path1', '/test/symlink'], }); - const profile = args[1]; - expect(profile).toContain('(subpath (param "ALLOWED_PATH_0"))'); - expect(profile).toContain('(subpath (param "ALLOWED_PATH_1"))'); - - expect(args).toContain('-D'); - expect(args).toContain('ALLOWED_PATH_0=/custom/path1'); - expect(args).toContain('ALLOWED_PATH_1=/test/real_path'); + expect(profile).toContain(`(subpath "/custom/path1")`); + expect(profile).toContain(`(subpath "/test/real_path")`); }); }); describe('forbiddenPaths', () => { - it('should parameterize forbidden paths and explicitly deny them', () => { + it('should explicitly deny forbidden paths', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', forbiddenPaths: ['/secret/path'], }); - const profile = args[1]; - - expect(args).toContain('-D'); - expect(args).toContain('FORBIDDEN_PATH_0=/secret/path'); - expect(profile).toContain( - '(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_0")))', + `(deny file-read* file-write* (subpath "/secret/path"))`, ); }); @@ -161,54 +146,40 @@ describe('seatbeltArgsBuilder', () => { return p; }); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', forbiddenPaths: ['/test/symlink'], }); - const profile = args[1]; - - expect(args).toContain('-D'); - expect(args).toContain('FORBIDDEN_PATH_0=/test/real_path'); expect(profile).toContain( - '(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_0")))', + `(deny file-read* file-write* (subpath "/test/real_path"))`, ); }); it('explicitly denies non-existent forbidden paths to prevent creation', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', forbiddenPaths: ['/test/missing-dir/missing-file.txt'], }); - const profile = args[1]; - - expect(args).toContain('-D'); - expect(args).toContain( - 'FORBIDDEN_PATH_0=/test/missing-dir/missing-file.txt', - ); expect(profile).toContain( - '(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_0")))', + `(deny file-read* file-write* (subpath "/test/missing-dir/missing-file.txt"))`, ); }); it('should override allowed paths if a path is also in forbidden paths', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const args = buildSeatbeltArgs({ + const profile = buildSeatbeltProfile({ workspace: '/test', allowedPaths: ['/custom/path1'], forbiddenPaths: ['/custom/path1'], }); - const profile = args[1]; - - const allowString = - '(allow file-read* file-write* (subpath (param "ALLOWED_PATH_0")))'; - const denyString = - '(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_0")))'; + const allowString = `(allow file-read* file-write* (subpath "/custom/path1"))`; + const denyString = `(deny file-read* file-write* (subpath "/custom/path1"))`; expect(profile).toContain(allowString); expect(profile).toContain(denyString); diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts index a610331d88..084704b69f 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts @@ -20,7 +20,7 @@ import { import { tryRealpath, resolveGitWorktreePaths } from '../utils/fsUtils.js'; /** - * Options for building macOS Seatbelt arguments. + * Options for building macOS Seatbelt profile. */ export interface SeatbeltArgsOptions { /** The primary workspace path to allow access to. */ @@ -38,28 +38,29 @@ export interface SeatbeltArgsOptions { } /** - * Builds the arguments array for sandbox-exec using a strict allowlist profile. - * It relies on parameters passed to sandbox-exec via the -D flag to avoid - * string interpolation vulnerabilities, and normalizes paths against symlink escapes. - * - * Returns arguments up to the end of sandbox-exec configuration (e.g. ['-p', '', '-D', ...]) - * Does not include the final '--' separator or the command to run. + * Escapes a string for use within a Scheme string literal "..." */ -export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { +export function escapeSchemeString(str: string): string { + return str.replace(/[\\"]/g, '\\$&'); +} + +/** + * Builds a complete macOS Seatbelt profile string using a strict allowlist. + * It embeds paths directly into the profile, properly escaped for Scheme. + */ +export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { let profile = BASE_SEATBELT_PROFILE + '\n'; - const args: string[] = []; const workspacePath = tryRealpath(options.workspace); - args.push('-D', `WORKSPACE=${workspacePath}`); - args.push('-D', `WORKSPACE_RAW=${options.workspace}`); - profile += `(allow file-read* (subpath (param "WORKSPACE_RAW")))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(options.workspace)}"))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(workspacePath)}"))\n`; if (options.workspaceWrite) { - profile += `(allow file-write* (subpath (param "WORKSPACE_RAW")))\n`; + profile += `(allow file-write* (subpath "${escapeSchemeString(options.workspace)}"))\n`; + profile += `(allow file-write* (subpath "${escapeSchemeString(workspacePath)}"))\n`; } - if (options.workspaceWrite) { - profile += `(allow file-write* (subpath (param "WORKSPACE")))\n`; - } + const tmpPath = tryRealpath(os.tmpdir()); + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(tmpPath)}"))\n`; // Add explicit deny rules for governance files in the workspace. // These are added after the workspace allow rule to ensure they take precedence @@ -81,12 +82,10 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { const ruleType = isDirectory ? 'subpath' : 'literal'; - args.push('-D', `GOVERNANCE_FILE_${i}=${governanceFile}`); - profile += `(deny file-write* (${ruleType} (param "GOVERNANCE_FILE_${i}")))\n`; + profile += `(deny file-write* (${ruleType} "${escapeSchemeString(governanceFile)}"))\n`; if (realGovernanceFile !== governanceFile) { - args.push('-D', `REAL_GOVERNANCE_FILE_${i}=${realGovernanceFile}`); - profile += `(deny file-write* (${ruleType} (param "REAL_GOVERNANCE_FILE_${i}")))\n`; + profile += `(deny file-write* (${ruleType} "${escapeSchemeString(realGovernanceFile)}"))\n`; } } @@ -121,27 +120,20 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { // Auto-detect and support git worktrees by granting read and write access to the underlying git directory const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths(workspacePath); if (worktreeGitDir) { - args.push('-D', `WORKTREE_GIT_DIR=${worktreeGitDir}`); - profile += `(allow file-read* file-write* (subpath (param "WORKTREE_GIT_DIR")))\n`; + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`; } if (mainGitDir) { - args.push('-D', `MAIN_GIT_DIR=${mainGitDir}`); - profile += `(allow file-read* file-write* (subpath (param "MAIN_GIT_DIR")))\n`; + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(mainGitDir)}"))\n`; } - const tmpPath = tryRealpath(os.tmpdir()); - args.push('-D', `TMPDIR=${tmpPath}`); - const nodeRootPath = tryRealpath( path.dirname(path.dirname(process.execPath)), ); - args.push('-D', `NODE_ROOT=${nodeRootPath}`); - profile += `(allow file-read* (subpath (param "NODE_ROOT")))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(nodeRootPath)}"))\n`; // Add PATH directories as read-only to support nvm, homebrew, etc. if (process.env['PATH']) { const paths = process.env['PATH'].split(':'); - let pathIndex = 0; const addedPaths = new Set(); for (const p of paths) { @@ -158,9 +150,7 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { if (!addedPaths.has(resolved)) { addedPaths.add(resolved); - args.push('-D', `SYS_PATH_${pathIndex}=${resolved}`); - profile += `(allow file-read* (subpath (param "SYS_PATH_${pathIndex}")))\n`; - pathIndex++; + profile += `(allow file-read* (subpath "${escapeSchemeString(resolved)}"))\n`; } } catch (_e) { // Ignore paths that do not exist or are inaccessible @@ -170,12 +160,9 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { // Handle allowedPaths const allowedPaths = sanitizePaths(options.allowedPaths) || []; - const resolvedAllowedPaths: string[] = []; for (let i = 0; i < allowedPaths.length; i++) { const allowedPath = tryRealpath(allowedPaths[i]); - resolvedAllowedPaths.push(allowedPath); - args.push('-D', `ALLOWED_PATH_${i}=${allowedPath}`); - profile += `(allow file-read* file-write* (subpath (param "ALLOWED_PATH_${i}")))\n`; + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(allowedPath)}"))\n`; } // Handle granular additional permissions @@ -184,8 +171,6 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { if (read) { for (let i = 0; i < read.length; i++) { const resolved = tryRealpath(read[i]); - const paramName = `ADDITIONAL_READ_${i}`; - args.push('-D', `${paramName}=${resolved}`); let isFile = false; try { isFile = fs.statSync(resolved).isFile(); @@ -193,17 +178,15 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { // Ignore error } if (isFile) { - profile += `(allow file-read* (literal (param "${paramName}")))\n`; + profile += `(allow file-read* (literal "${escapeSchemeString(resolved)}"))\n`; } else { - profile += `(allow file-read* (subpath (param "${paramName}")))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(resolved)}"))\n`; } } } if (write) { for (let i = 0; i < write.length; i++) { const resolved = tryRealpath(write[i]); - const paramName = `ADDITIONAL_WRITE_${i}`; - args.push('-D', `${paramName}=${resolved}`); let isFile = false; try { isFile = fs.statSync(resolved).isFile(); @@ -211,9 +194,9 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { // Ignore error } if (isFile) { - profile += `(allow file-read* file-write* (literal (param "${paramName}")))\n`; + profile += `(allow file-read* file-write* (literal "${escapeSchemeString(resolved)}"))\n`; } else { - profile += `(allow file-read* file-write* (subpath (param "${paramName}")))\n`; + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(resolved)}"))\n`; } } } @@ -223,17 +206,14 @@ export function buildSeatbeltArgs(options: SeatbeltArgsOptions): string[] { const forbiddenPaths = sanitizePaths(options.forbiddenPaths) || []; for (let i = 0; i < forbiddenPaths.length; i++) { const forbiddenPath = tryRealpath(forbiddenPaths[i]); - args.push('-D', `FORBIDDEN_PATH_${i}=${forbiddenPath}`); - profile += `(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_${i}")))\n`; + profile += `(deny file-read* file-write* (subpath "${escapeSchemeString(forbiddenPath)}"))\n`; } if (options.networkAccess || options.additionalPermissions?.network) { profile += NETWORK_SEATBELT_PROFILE; } - args.unshift('-p', profile); - - return args; + return profile; } /** diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index 88b3718dc2..b66e4db422 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -104,6 +104,8 @@ export interface SandboxedCommand { env: NodeJS.ProcessEnv; /** The working directory. */ cwd?: string; + /** An optional cleanup function to be called after the command terminates. */ + cleanup?: () => void; } /** diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index b757bdd793..0bd825db17 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -367,8 +367,9 @@ export class ShellExecutionService { ): Promise<{ program: string; args: string[]; - env: Record; + env: NodeJS.ProcessEnv; cwd: string; + cleanup?: () => void; }> { const sandboxManager = shellExecutionConfig.sandboxManager ?? new NoopSandboxManager(); @@ -474,6 +475,7 @@ export class ShellExecutionService { args: sandboxedCommand.args, env: sandboxedCommand.env, cwd: sandboxedCommand.cwd ?? cwd, + cleanup: sandboxedCommand.cleanup, }; } @@ -493,6 +495,7 @@ export class ShellExecutionService { args: finalArgs, env: finalEnv, cwd: finalCwd, + cleanup: cmdCleanup, } = await this.prepareExecution( commandToExecute, cwd, @@ -661,6 +664,7 @@ export class ShellExecutionService { signal: NodeJS.Signals | null, ) => { cleanup(); + cmdCleanup?.(); let combinedOutput = state.output; if (state.truncated) { @@ -810,6 +814,7 @@ export class ShellExecutionService { args: finalArgs, env: finalEnv, cwd: finalCwd, + cleanup: cmdCleanup, } = await this.prepareExecution( commandToExecute, cwd, @@ -1104,6 +1109,7 @@ export class ShellExecutionService { const finalize = () => { render(true); + cmdCleanup?.(); const event: ShellOutputEvent = { type: 'exit',