diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index c7bdd351a7..3e1862998e 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -64,20 +64,12 @@ describe('MacOsSandboxManager', () => { policy: mockPolicy, }); - expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith({ - workspace: mockWorkspace, - allowedPaths: mockAllowedPaths, - forbiddenPaths: [], - networkAccess: mockNetworkAccess, - workspaceWrite: false, - additionalPermissions: { - fileSystem: { - read: [], - write: [], - }, - network: true, - }, - }); + expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( + expect.objectContaining({ + networkAccess: true, + workspaceWrite: false, + }), + ); expect(result.program).toBe('/usr/bin/sandbox-exec'); expect(result.args[0]).toBe('-f'); @@ -155,11 +147,10 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - additionalPermissions: expect.objectContaining({ - fileSystem: expect.objectContaining({ - read: expect.not.arrayContaining(['/']), - write: expect.not.arrayContaining(['/']), - }), + workspaceWrite: true, + resolvedPaths: expect.objectContaining({ + policyRead: expect.not.arrayContaining(['/']), + policyWrite: expect.not.arrayContaining(['/']), }), }), ); @@ -213,7 +204,11 @@ describe('MacOsSandboxManager', () => { // The seatbelt builder internally handles governance files, so we simply verify // it is invoked correctly with the right workspace. expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( - expect.objectContaining({ workspace: mockWorkspace }), + expect.objectContaining({ + resolvedPaths: expect.objectContaining({ + workspace: { resolved: mockWorkspace, original: mockWorkspace }, + }), + }), ); }); }); @@ -233,10 +228,12 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - allowedPaths: expect.arrayContaining([ - '/tmp/allowed1', - '/tmp/allowed2', - ]), + resolvedPaths: expect.objectContaining({ + policyAllowed: expect.arrayContaining([ + '/tmp/allowed1', + '/tmp/allowed2', + ]), + }), }), ); }); @@ -258,7 +255,9 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - forbiddenPaths: expect.arrayContaining(['/tmp/forbidden1']), + resolvedPaths: expect.objectContaining({ + forbidden: expect.arrayContaining(['/tmp/forbidden1']), + }), }), ); }); @@ -278,7 +277,9 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - forbiddenPaths: expect.arrayContaining(['/tmp/does-not-exist']), + resolvedPaths: expect.objectContaining({ + forbidden: expect.arrayContaining(['/tmp/does-not-exist']), + }), }), ); }); @@ -301,8 +302,10 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - allowedPaths: [], - forbiddenPaths: expect.arrayContaining(['/tmp/conflict']), + resolvedPaths: expect.objectContaining({ + policyAllowed: [], + forbidden: expect.arrayContaining(['/tmp/conflict']), + }), }), ); }); diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index 27e6867030..f87dc0289c 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -133,28 +133,26 @@ export class MacOsSandboxManager implements SandboxManager { false, }; + const { command: finalCommand, args: finalArgs } = handleReadWriteCommands( + req, + mergedAdditional, + this.options.workspace, + [ + ...(req.policy?.allowedPaths || []), + ...(this.options.includeDirectories || []), + ], + ); + const resolvedPaths = await resolveSandboxPaths( this.options, req, mergedAdditional, ); - const { command: finalCommand, args: finalArgs } = handleReadWriteCommands( - req, - mergedAdditional, - this.options.workspace, - req.policy?.allowedPaths, - ); const sandboxArgs = buildSeatbeltProfile({ - workspace: this.options.workspace, - allowedPaths: [ - ...resolvedPaths.policyAllowed, - ...(this.options.includeDirectories || []), - ], - forbiddenPaths: resolvedPaths.forbidden, + resolvedPaths, networkAccess: mergedAdditional.network, workspaceWrite, - additionalPermissions: mergedAdditional, }); const tempFile = this.writeProfileToTempFile(sandboxArgs); diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts index 7102fde2f7..19ba8303ae 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts @@ -8,18 +8,21 @@ import { buildSeatbeltProfile, escapeSchemeString, } from './seatbeltArgsBuilder.js'; -import * as fsUtils from '../utils/fsUtils.js'; +import type { ResolvedSandboxPaths } from '../../services/sandboxManager.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'); - return { - ...actual, - tryRealpath: vi.fn((p) => p), - resolveGitWorktreePaths: vi.fn(() => ({})), - }; -}); +const defaultResolvedPaths: ResolvedSandboxPaths = { + workspace: { + resolved: '/Users/test/workspace', + original: '/Users/test/raw-workspace', + }, + forbidden: [], + globalIncludes: [], + policyAllowed: [], + policyRead: [], + policyWrite: [], +}; describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { afterEach(() => { @@ -35,12 +38,8 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { describe('buildSeatbeltProfile', () => { it('should build a strict allowlist profile allowing the workspace', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const profile = buildSeatbeltProfile({ - workspace: '/Users/test/workspace', - allowedPaths: [], - forbiddenPaths: [], + resolvedPaths: defaultResolvedPaths, }); expect(profile).toContain('(version 1)'); @@ -51,11 +50,11 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { }); it('should allow network when networkAccess is true', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); const profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: [], - forbiddenPaths: [], + resolvedPaths: { + ...defaultResolvedPaths, + workspace: { resolved: '/test', original: '/test' }, + }, networkAccess: true, }); expect(profile).toContain('(allow network-outbound)'); @@ -63,7 +62,6 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { describe('governance files', () => { it('should inject explicit deny rules for governance files', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p.toString()); vi.spyOn(fs, 'existsSync').mockReturnValue(true); vi.spyOn(fs, 'lstatSync').mockImplementation( (p) => @@ -74,9 +72,13 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { ); const profile = buildSeatbeltProfile({ - workspace: '/test/workspace', - allowedPaths: [], - forbiddenPaths: [], + resolvedPaths: { + ...defaultResolvedPaths, + workspace: { + resolved: '/test/workspace', + original: '/test/workspace', + }, + }, }); expect(profile).toContain( @@ -87,48 +89,16 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { `(deny file-write* (subpath "/test/workspace/.git"))`, ); }); - - it('should protect both the symlink and the real path if they differ', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => { - if (p === '/test/workspace/.gitignore') - return '/test/real/.gitignore'; - return p.toString(); - }); - vi.spyOn(fs, 'existsSync').mockReturnValue(true); - vi.spyOn(fs, 'lstatSync').mockImplementation( - () => - ({ - isDirectory: () => false, - isFile: () => true, - }) as unknown as fs.Stats, - ); - - const profile = buildSeatbeltProfile({ - workspace: '/test/workspace', - allowedPaths: [], - forbiddenPaths: [], - }); - - expect(profile).toContain( - `(deny file-write* (literal "/test/workspace/.gitignore"))`, - ); - expect(profile).toContain( - `(deny file-write* (literal "/test/real/.gitignore"))`, - ); - }); }); describe('allowedPaths', () => { - it('should embed allowed paths and normalize them', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => { - if (p === '/test/symlink') return '/test/real_path'; - return p; - }); - + it('should embed allowed paths', () => { const profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: ['/custom/path1', '/test/symlink'], - forbiddenPaths: [], + resolvedPaths: { + ...defaultResolvedPaths, + workspace: { resolved: '/test', original: '/test' }, + policyAllowed: ['/custom/path1', '/test/real_path'], + }, }); expect(profile).toContain(`(subpath "/custom/path1")`); @@ -138,12 +108,12 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { describe('forbiddenPaths', () => { it('should explicitly deny forbidden paths', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); - const profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: [], - forbiddenPaths: ['/secret/path'], + resolvedPaths: { + ...defaultResolvedPaths, + workspace: { resolved: '/test', original: '/test' }, + forbidden: ['/secret/path'], + }, }); expect(profile).toContain( @@ -151,46 +121,14 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { ); }); - it('resolves forbidden symlink paths to their real paths', () => { - vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => { - if (p === '/test/symlink' || p === '/test/missing-dir') { - return '/test/real_path'; - } - return p; - }); - - const profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: [], - forbiddenPaths: ['/test/symlink'], - }); - - expect(profile).toContain( - `(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 profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: [], - forbiddenPaths: ['/test/missing-dir/missing-file.txt'], - }); - - expect(profile).toContain( - `(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 profile = buildSeatbeltProfile({ - workspace: '/test', - allowedPaths: ['/custom/path1'], - forbiddenPaths: ['/custom/path1'], + resolvedPaths: { + ...defaultResolvedPaths, + workspace: { resolved: '/test', original: '/test' }, + policyAllowed: ['/custom/path1'], + forbidden: ['/custom/path1'], + }, }); const allowString = `(allow file-read* file-write* (subpath "/custom/path1"))`; diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts index e5430d1471..967cd8f183 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts @@ -12,9 +12,9 @@ import { NETWORK_SEATBELT_PROFILE, } from './baseProfile.js'; import { - type SandboxPermissions, GOVERNANCE_FILES, SECRET_FILES, + type ResolvedSandboxPaths, } from '../../services/sandboxManager.js'; import { tryRealpath, resolveGitWorktreePaths } from '../utils/fsUtils.js'; @@ -22,16 +22,10 @@ import { tryRealpath, resolveGitWorktreePaths } from '../utils/fsUtils.js'; * Options for building macOS Seatbelt profile. */ export interface SeatbeltArgsOptions { - /** The primary workspace path to allow access to. */ - workspace: string; - /** Additional paths to allow access to. */ - allowedPaths: string[]; - /** Absolute paths to explicitly deny read/write access to (overrides allowlists). */ - forbiddenPaths: string[]; + /** Fully resolved paths for the sandbox execution. */ + resolvedPaths: ResolvedSandboxPaths; /** Whether to allow network access. */ networkAccess?: boolean; - /** Granular additional permissions. */ - additionalPermissions?: SandboxPermissions; /** Whether to allow write access to the workspace. */ workspaceWrite?: boolean; } @@ -49,72 +43,22 @@ export function escapeSchemeString(str: string): string { */ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { let profile = BASE_SEATBELT_PROFILE + '\n'; + const { resolvedPaths, networkAccess, workspaceWrite } = options; - const workspacePath = tryRealpath(options.workspace); - 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 "${escapeSchemeString(options.workspace)}"))\n`; - profile += `(allow file-write* (subpath "${escapeSchemeString(workspacePath)}"))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(resolvedPaths.workspace.original)}"))\n`; + profile += `(allow file-read* (subpath "${escapeSchemeString(resolvedPaths.workspace.resolved)}"))\n`; + if (workspaceWrite) { + profile += `(allow file-write* (subpath "${escapeSchemeString(resolvedPaths.workspace.original)}"))\n`; + profile += `(allow file-write* (subpath "${escapeSchemeString(resolvedPaths.workspace.resolved)}"))\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 - // (Seatbelt evaluates rules in order, later rules win for same path). - for (let i = 0; i < GOVERNANCE_FILES.length; i++) { - const governanceFile = path.join(workspacePath, GOVERNANCE_FILES[i].path); - const realGovernanceFile = tryRealpath(governanceFile); - - // Determine if it should be treated as a directory (subpath) or a file (literal). - // .git is generally a directory, while ignore files are literals. - let isDirectory = GOVERNANCE_FILES[i].isDirectory; - try { - if (fs.existsSync(realGovernanceFile)) { - isDirectory = fs.lstatSync(realGovernanceFile).isDirectory(); - } - } catch { - // Ignore errors, use default guess - } - - const ruleType = isDirectory ? 'subpath' : 'literal'; - - profile += `(deny file-write* (${ruleType} "${escapeSchemeString(governanceFile)}"))\n`; - - if (realGovernanceFile !== governanceFile) { - profile += `(deny file-write* (${ruleType} "${escapeSchemeString(realGovernanceFile)}"))\n`; - } - } - - // Add explicit deny rules for secret files (.env, .env.*) in the workspace and allowed paths. - // We use regex rules to avoid expensive file discovery scans. - // Anchoring to workspace/allowed paths to avoid over-blocking. - const searchPaths = [options.workspace, ...options.allowedPaths]; - - for (const basePath of searchPaths) { - const resolvedBase = tryRealpath(basePath); - for (const secret of SECRET_FILES) { - // Map pattern to Seatbelt regex - let regexPattern: string; - const escapedBase = escapeRegex(resolvedBase); - if (secret.pattern.endsWith('*')) { - // .env.* -> .env\..+ (match .env followed by dot and something) - // We anchor the secret file name to either a directory separator or the start of the relative path. - const basePattern = secret.pattern.slice(0, -1).replace(/\./g, '\\\\.'); - regexPattern = `^${escapedBase}/(.*/)?${basePattern}[^/]+$`; - } else { - // .env -> \.env$ - const basePattern = secret.pattern.replace(/\./g, '\\\\.'); - regexPattern = `^${escapedBase}/(.*/)?${basePattern}$`; - } - profile += `(deny file-read* file-write* (regex #"${regexPattern}"))\n`; - } - } - // Auto-detect and support git worktrees by granting read and write access to the underlying git directory - const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths(workspacePath); + const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths( + resolvedPaths.workspace.resolved, + ); if (worktreeGitDir) { profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`; } @@ -154,58 +98,115 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { } } - // Handle allowedPaths - const allowedPaths = options.allowedPaths; + // Handle allowedPaths and globalIncludes + const allowedPaths = [ + ...resolvedPaths.policyAllowed, + ...resolvedPaths.globalIncludes, + ]; for (let i = 0; i < allowedPaths.length; i++) { - const allowedPath = tryRealpath(allowedPaths[i]); + const allowedPath = allowedPaths[i]; profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(allowedPath)}"))\n`; } - // Handle granular additional permissions - if (options.additionalPermissions?.fileSystem) { - const { read, write } = options.additionalPermissions.fileSystem; - if (read) { - for (let i = 0; i < read.length; i++) { - const resolved = tryRealpath(read[i]); - let isFile = false; - try { - isFile = fs.statSync(resolved).isFile(); - } catch { - // Ignore error - } - if (isFile) { - profile += `(allow file-read* (literal "${escapeSchemeString(resolved)}"))\n`; - } else { - profile += `(allow file-read* (subpath "${escapeSchemeString(resolved)}"))\n`; - } - } + // Handle granular additional read permissions + for (let i = 0; i < resolvedPaths.policyRead.length; i++) { + const resolved = resolvedPaths.policyRead[i]; + let isFile = false; + try { + isFile = fs.statSync(resolved).isFile(); + } catch { + // Ignore error } - if (write) { - for (let i = 0; i < write.length; i++) { - const resolved = tryRealpath(write[i]); - let isFile = false; - try { - isFile = fs.statSync(resolved).isFile(); - } catch { - // Ignore error - } - if (isFile) { - profile += `(allow file-read* file-write* (literal "${escapeSchemeString(resolved)}"))\n`; - } else { - profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(resolved)}"))\n`; - } + if (isFile) { + profile += `(allow file-read* (literal "${escapeSchemeString(resolved)}"))\n`; + } else { + profile += `(allow file-read* (subpath "${escapeSchemeString(resolved)}"))\n`; + } + } + + // Handle granular additional write permissions + for (let i = 0; i < resolvedPaths.policyWrite.length; i++) { + const resolved = resolvedPaths.policyWrite[i]; + let isFile = false; + try { + isFile = fs.statSync(resolved).isFile(); + } catch { + // Ignore error + } + if (isFile) { + profile += `(allow file-read* file-write* (literal "${escapeSchemeString(resolved)}"))\n`; + } else { + profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(resolved)}"))\n`; + } + } + + // Add explicit deny rules for governance files in the workspace. + // These are added after the workspace allow rule to ensure they take precedence + // (Seatbelt evaluates rules in order, later rules win for same path). + for (let i = 0; i < GOVERNANCE_FILES.length; i++) { + const governanceFile = path.join( + resolvedPaths.workspace.resolved, + GOVERNANCE_FILES[i].path, + ); + const realGovernanceFile = tryRealpath(governanceFile); + + // Determine if it should be treated as a directory (subpath) or a file (literal). + // .git is generally a directory, while ignore files are literals. + let isDirectory = GOVERNANCE_FILES[i].isDirectory; + try { + if (fs.existsSync(realGovernanceFile)) { + isDirectory = fs.lstatSync(realGovernanceFile).isDirectory(); } + } catch { + // Ignore errors, use default guess + } + + const ruleType = isDirectory ? 'subpath' : 'literal'; + + profile += `(deny file-write* (${ruleType} "${escapeSchemeString(governanceFile)}"))\n`; + + if (realGovernanceFile !== governanceFile) { + profile += `(deny file-write* (${ruleType} "${escapeSchemeString(realGovernanceFile)}"))\n`; + } + } + + // Add explicit deny rules for secret files (.env, .env.*) in the workspace and allowed paths. + // We use regex rules to avoid expensive file discovery scans. + // Anchoring to workspace/allowed paths to avoid over-blocking. + const searchPaths = [ + resolvedPaths.workspace.resolved, + resolvedPaths.workspace.original, + ...resolvedPaths.policyAllowed, + ...resolvedPaths.globalIncludes, + ]; + + for (const basePath of searchPaths) { + for (const secret of SECRET_FILES) { + // Map pattern to Seatbelt regex + let regexPattern: string; + const escapedBase = escapeRegex(basePath); + if (secret.pattern.endsWith('*')) { + // .env.* -> .env\..+ (match .env followed by dot and something) + // We anchor the secret file name to either a directory separator or the start of the relative path. + const basePattern = secret.pattern.slice(0, -1).replace(/\./g, '\\\\.'); + regexPattern = `^${escapedBase}/(.*/)?${basePattern}[^/]+$`; + } else { + // .env -> \.env$ + const basePattern = secret.pattern.replace(/\./g, '\\\\.'); + regexPattern = `^${escapedBase}/(.*/)?${basePattern}$`; + } + profile += `(deny file-read* file-write* (regex #"${regexPattern}"))\n`; } } // Handle forbiddenPaths - const forbiddenPaths = options.forbiddenPaths; + const forbiddenPaths = resolvedPaths.forbidden; for (let i = 0; i < forbiddenPaths.length; i++) { - const forbiddenPath = tryRealpath(forbiddenPaths[i]); + const forbiddenPath = forbiddenPaths[i]; profile += `(deny file-read* file-write* (subpath "${escapeSchemeString(forbiddenPath)}"))\n`; } - if (options.networkAccess || options.additionalPermissions?.network) { + if (networkAccess) { profile += NETWORK_SEATBELT_PROFILE; }