diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 1e5a5d5267..d79f218744 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -253,6 +253,10 @@ vi.mock('../core/tokenLimits.js', () => ({ vi.mock('../code_assist/codeAssist.js'); vi.mock('../code_assist/experiments/experiments.js'); +afterEach(() => { + vi.clearAllMocks(); +}); + describe('Server Config (config.ts)', () => { const MODEL = DEFAULT_GEMINI_MODEL; const SANDBOX: SandboxConfig = createMockSandboxConfig({ @@ -1613,6 +1617,31 @@ describe('Server Config (config.ts)', () => { expect(config.getSandboxAllowedPaths()).toEqual(['/only/this']); expect(config.getSandboxNetworkAccess()).toBe(false); }); + + it('lazily resolves forbidden paths when first accessed', async () => { + const config = new Config({ + ...baseParams, + sandbox: { enabled: true, command: 'docker' }, + }); + + const fileService = config.getFileService(); + vi.spyOn(fileService, 'getIgnoredPaths').mockResolvedValue([ + '/tmp/forbidden', + ]); + + await config.initialize(); + expect(fileService.getIgnoredPaths).not.toHaveBeenCalled(); + + // Access resolved paths via the internal resolver + const resolved = await ( + config as unknown as { + getSandboxForbiddenPaths: () => Promise; + } + ).getSandboxForbiddenPaths(); + + expect(fileService.getIgnoredPaths).toHaveBeenCalled(); + expect(resolved).toEqual(['/tmp/forbidden']); + }); }); it('should have independent TopicState across instances', () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index b2d6e32d85..5a70e2e7ff 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -758,6 +758,7 @@ export class Config implements McpContext, AgentLoopContext { readonly modelConfigService: ModelConfigService; private readonly embeddingModel: string; private readonly sandbox: SandboxConfig | undefined; + private _sandboxForbiddenPaths: string[] | undefined; private readonly targetDir: string; private workspaceContext: WorkspaceContext; private readonly debugMode: boolean; @@ -997,6 +998,7 @@ export class Config implements McpContext, AgentLoopContext { this.sandbox, { workspace: this.targetDir, + forbiddenPaths: this.getSandboxForbiddenPaths.bind(this), includeDirectories: this.pendingIncludeDirectories, policyManager: this._sandboxPolicyManager, }, @@ -1678,11 +1680,25 @@ export class Config implements McpContext, AgentLoopContext { return this._geminiClient; } + private async getSandboxForbiddenPaths(): Promise { + if (this._sandboxForbiddenPaths) { + return this._sandboxForbiddenPaths; + } + + this._sandboxForbiddenPaths = await this.getFileService().getIgnoredPaths({ + respectGitIgnore: false, + respectGeminiIgnore: true, + }); + + return this._sandboxForbiddenPaths; + } + private refreshSandboxManager(): void { this._sandboxManager = createSandboxManager( this.sandbox, { workspace: this.targetDir, + forbiddenPaths: this.getSandboxForbiddenPaths.bind(this), policyManager: this._sandboxPolicyManager, }, this.getApprovalMode(), diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts index 1a687b9154..55d11e0ce6 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts @@ -420,7 +420,7 @@ describe('LinuxSandboxManager', () => { const customManager = new LinuxSandboxManager({ workspace, - forbiddenPaths: ['/tmp/cache', '/opt/secret.txt'], + forbiddenPaths: async () => ['/tmp/cache', '/opt/secret.txt'], }); const bwrapArgs = await getBwrapArgs( @@ -452,7 +452,7 @@ describe('LinuxSandboxManager', () => { const customManager = new LinuxSandboxManager({ workspace, - forbiddenPaths: ['/tmp/forbidden-symlink'], + forbiddenPaths: async () => ['/tmp/forbidden-symlink'], }); const bwrapArgs = await getBwrapArgs( @@ -480,7 +480,7 @@ describe('LinuxSandboxManager', () => { const customManager = new LinuxSandboxManager({ workspace, - forbiddenPaths: ['/tmp/not-here.txt'], + forbiddenPaths: async () => ['/tmp/not-here.txt'], }); const bwrapArgs = await getBwrapArgs( @@ -509,7 +509,7 @@ describe('LinuxSandboxManager', () => { const customManager = new LinuxSandboxManager({ workspace, - forbiddenPaths: ['/tmp/dir-link'], + forbiddenPaths: async () => ['/tmp/dir-link'], }); const bwrapArgs = await getBwrapArgs( @@ -534,7 +534,7 @@ describe('LinuxSandboxManager', () => { const customManager = new LinuxSandboxManager({ workspace, - forbiddenPaths: ['/tmp/conflict'], + forbiddenPaths: async () => ['/tmp/conflict'], }); const bwrapArgs = await getBwrapArgs( @@ -550,12 +550,14 @@ describe('LinuxSandboxManager', () => { customManager, ); - const bindTryIdx = bwrapArgs.indexOf('--bind-try'); - const tmpfsIdx = bwrapArgs.lastIndexOf('--tmpfs'); + // Conflict should have been filtered out of allow list (--bind-try) + expect(bwrapArgs).not.toContain('--bind-try'); + expect(bwrapArgs).not.toContain('--bind-try-ro'); - expect(bwrapArgs[bindTryIdx + 1]).toBe('/tmp/conflict'); - expect(bwrapArgs[tmpfsIdx + 1]).toBe('/tmp/conflict'); - expect(tmpfsIdx).toBeGreaterThan(bindTryIdx); + // It should only appear as a forbidden path (via --tmpfs) + const conflictIdx = bwrapArgs.indexOf('/tmp/conflict'); + expect(conflictIdx).toBeGreaterThan(0); + expect(bwrapArgs[conflictIdx - 1]).toBe('--tmpfs'); }); }); }); diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index 71d42c90e5..44c3e69647 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -17,6 +17,7 @@ import { getSecretFileFindArgs, sanitizePaths, type ParsedSandboxDenial, + resolveSandboxPaths, } from '../../services/sandboxManager.js'; import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { @@ -294,7 +295,7 @@ export class LinuxSandboxManager implements SandboxManager { bwrapArgs.push(bindFlag, mainGitDir, mainGitDir); } - const includeDirs = sanitizePaths(this.options.includeDirectories) || []; + const includeDirs = sanitizePaths(this.options.includeDirectories); for (const includeDir of includeDirs) { try { const resolved = tryRealpath(includeDir); @@ -304,7 +305,8 @@ export class LinuxSandboxManager implements SandboxManager { } } - const allowedPaths = sanitizePaths(req.policy?.allowedPaths) || []; + const { allowed: allowedPaths, forbidden: forbiddenPaths } = + await resolveSandboxPaths(this.options, req); const normalizedWorkspace = normalize(workspacePath).replace(/\/$/, ''); for (const allowedPath of allowedPaths) { @@ -330,8 +332,7 @@ export class LinuxSandboxManager implements SandboxManager { } } - const additionalReads = - sanitizePaths(mergedAdditional.fileSystem?.read) || []; + const additionalReads = sanitizePaths(mergedAdditional.fileSystem?.read); for (const p of additionalReads) { try { const safeResolvedPath = tryRealpath(p); @@ -341,8 +342,7 @@ export class LinuxSandboxManager implements SandboxManager { } } - const additionalWrites = - sanitizePaths(mergedAdditional.fileSystem?.write) || []; + const additionalWrites = sanitizePaths(mergedAdditional.fileSystem?.write); for (const p of additionalWrites) { try { const safeResolvedPath = tryRealpath(p); @@ -362,7 +362,6 @@ export class LinuxSandboxManager implements SandboxManager { } } - const forbiddenPaths = sanitizePaths(this.options.forbiddenPaths) || []; for (const p of forbiddenPaths) { let resolved: string; try { diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index e49a062d15..c0fdcbab63 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -67,7 +67,7 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith({ workspace: mockWorkspace, allowedPaths: mockAllowedPaths, - forbiddenPaths: undefined, + forbiddenPaths: [], networkAccess: mockNetworkAccess, workspaceWrite: false, additionalPermissions: { @@ -218,7 +218,7 @@ describe('MacOsSandboxManager', () => { it('should parameterize forbidden paths and explicitly deny them', async () => { const customManager = new MacOsSandboxManager({ workspace: mockWorkspace, - forbiddenPaths: ['/tmp/forbidden1'], + forbiddenPaths: async () => ['/tmp/forbidden1'], }); await customManager.prepareCommand({ command: 'echo', @@ -238,7 +238,7 @@ describe('MacOsSandboxManager', () => { it('explicitly denies non-existent forbidden paths to prevent creation', async () => { const customManager = new MacOsSandboxManager({ workspace: mockWorkspace, - forbiddenPaths: ['/tmp/does-not-exist'], + forbiddenPaths: async () => ['/tmp/does-not-exist'], }); await customManager.prepareCommand({ command: 'echo', @@ -258,7 +258,7 @@ describe('MacOsSandboxManager', () => { it('should override allowed paths if a path is also in forbidden paths', async () => { const customManager = new MacOsSandboxManager({ workspace: mockWorkspace, - forbiddenPaths: ['/tmp/conflict'], + forbiddenPaths: async () => ['/tmp/conflict'], }); await customManager.prepareCommand({ command: 'echo', @@ -273,7 +273,7 @@ describe('MacOsSandboxManager', () => { expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( expect.objectContaining({ - allowedPaths: ['/tmp/conflict'], + allowedPaths: [], forbiddenPaths: ['/tmp/conflict'], }), ); diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts index b6332be6d2..51a2651c47 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.ts @@ -14,6 +14,7 @@ import { type SandboxPermissions, type GlobalSandboxOptions, type ParsedSandboxDenial, + resolveSandboxPaths, } from '../../services/sandboxManager.js'; import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { @@ -93,6 +94,9 @@ export class MacOsSandboxManager implements SandboxManager { const defaultNetwork = this.options.modeConfig?.network || req.policy?.networkAccess || false; + const { allowed: allowedPaths, forbidden: forbiddenPaths } = + await resolveSandboxPaths(this.options, req); + // Fetch persistent approvals for this command const commandName = await getFullCommandName(currentReq); const persistentPermissions = allowOverrides @@ -128,10 +132,10 @@ export class MacOsSandboxManager implements SandboxManager { const sandboxArgs = buildSeatbeltProfile({ workspace: this.options.workspace, allowedPaths: [ - ...(req.policy?.allowedPaths || []), + ...allowedPaths, ...(this.options.includeDirectories || []), ], - forbiddenPaths: this.options.forbiddenPaths, + forbiddenPaths, networkAccess: mergedAdditional.network, workspaceWrite, additionalPermissions: mergedAdditional, diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts index 647b7cb859..45f1c67728 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts @@ -38,6 +38,8 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/Users/test/workspace', + allowedPaths: [], + forbiddenPaths: [], }); expect(profile).toContain('(version 1)'); @@ -51,6 +53,8 @@ describe('seatbeltArgsBuilder', () => { vi.mocked(fsUtils.tryRealpath).mockImplementation((p) => p); const profile = buildSeatbeltProfile({ workspace: '/test', + allowedPaths: [], + forbiddenPaths: [], networkAccess: true, }); expect(profile).toContain('(allow network-outbound)'); @@ -70,6 +74,8 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/test/workspace', + allowedPaths: [], + forbiddenPaths: [], }); expect(profile).toContain( @@ -96,7 +102,11 @@ describe('seatbeltArgsBuilder', () => { }) as unknown as fs.Stats, ); - const profile = buildSeatbeltProfile({ workspace: '/test/workspace' }); + const profile = buildSeatbeltProfile({ + workspace: '/test/workspace', + allowedPaths: [], + forbiddenPaths: [], + }); expect(profile).toContain( `(deny file-write* (literal "/test/workspace/.gitignore"))`, @@ -117,6 +127,7 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/test', allowedPaths: ['/custom/path1', '/test/symlink'], + forbiddenPaths: [], }); expect(profile).toContain(`(subpath "/custom/path1")`); @@ -130,6 +141,7 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/test', + allowedPaths: [], forbiddenPaths: ['/secret/path'], }); @@ -148,6 +160,7 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/test', + allowedPaths: [], forbiddenPaths: ['/test/symlink'], }); @@ -161,6 +174,7 @@ describe('seatbeltArgsBuilder', () => { const profile = buildSeatbeltProfile({ workspace: '/test', + allowedPaths: [], forbiddenPaths: ['/test/missing-dir/missing-file.txt'], }); diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts index 084704b69f..c229632daa 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts @@ -13,7 +13,6 @@ import { } from './baseProfile.js'; import { type SandboxPermissions, - sanitizePaths, GOVERNANCE_FILES, SECRET_FILES, } from '../../services/sandboxManager.js'; @@ -26,9 +25,9 @@ export interface SeatbeltArgsOptions { /** The primary workspace path to allow access to. */ workspace: string; /** Additional paths to allow access to. */ - allowedPaths?: string[]; + allowedPaths: string[]; /** Absolute paths to explicitly deny read/write access to (overrides allowlists). */ - forbiddenPaths?: string[]; + forbiddenPaths: string[]; /** Whether to allow network access. */ networkAccess?: boolean; /** Granular additional permissions. */ @@ -92,10 +91,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { // 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 = sanitizePaths([ - options.workspace, - ...(options.allowedPaths || []), - ]) || [options.workspace]; + const searchPaths = [options.workspace, ...options.allowedPaths]; for (const basePath of searchPaths) { const resolvedBase = tryRealpath(basePath); @@ -159,7 +155,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { } // Handle allowedPaths - const allowedPaths = sanitizePaths(options.allowedPaths) || []; + const allowedPaths = options.allowedPaths; for (let i = 0; i < allowedPaths.length; i++) { const allowedPath = tryRealpath(allowedPaths[i]); profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(allowedPath)}"))\n`; @@ -203,7 +199,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { } // Handle forbiddenPaths - const forbiddenPaths = sanitizePaths(options.forbiddenPaths) || []; + const forbiddenPaths = options.forbiddenPaths; for (let i = 0; i < forbiddenPaths.length; i++) { const forbiddenPath = tryRealpath(forbiddenPaths[i]); profile += `(deny file-read* file-write* (subpath "${escapeSchemeString(forbiddenPath)}"))\n`; diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 1279c1708e..fe1d59550b 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -38,7 +38,7 @@ describe('WindowsSandboxManager', () => { manager = new WindowsSandboxManager({ workspace: testCwd, modeConfig: { readonly: false, allowOverrides: true }, - forbiddenPaths: [], + forbiddenPaths: async () => [], }); }); @@ -107,7 +107,7 @@ describe('WindowsSandboxManager', () => { const planManager = new WindowsSandboxManager({ workspace: testCwd, modeConfig: { readonly: true, allowOverrides: false }, - forbiddenPaths: [], + forbiddenPaths: async () => [], }); const req: SandboxRequest = { command: 'curl', @@ -139,7 +139,7 @@ describe('WindowsSandboxManager', () => { workspace: testCwd, modeConfig: { allowOverrides: true, network: false }, policyManager: mockPolicyManager, - forbiddenPaths: [], + forbiddenPaths: async () => [], }); const req: SandboxRequest = { @@ -369,7 +369,7 @@ describe('WindowsSandboxManager', () => { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, - forbiddenPaths: [missingPath], + forbiddenPaths: async () => [missingPath], }); const req: SandboxRequest = { @@ -397,7 +397,7 @@ describe('WindowsSandboxManager', () => { try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, - forbiddenPaths: [forbiddenPath], + forbiddenPaths: async () => [forbiddenPath], }); const req: SandboxRequest = { @@ -427,7 +427,7 @@ describe('WindowsSandboxManager', () => { try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, - forbiddenPaths: [conflictPath], + forbiddenPaths: async () => [conflictPath], }); const req: SandboxRequest = { @@ -458,12 +458,9 @@ describe('WindowsSandboxManager', () => { call[1][0] === path.resolve(conflictPath), ); - // Both should have been called - expect(allowCallIndex).toBeGreaterThan(-1); + // Conflict should have been filtered out of allow calls + expect(allowCallIndex).toBe(-1); expect(denyCallIndex).toBeGreaterThan(-1); - - // Verify order: explicitly denying must happen after the explicit allow - expect(allowCallIndex).toBeLessThan(denyCallIndex); } finally { fs.rmSync(conflictPath, { recursive: true, force: true }); } diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 1085921959..c828d46fa7 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -19,6 +19,7 @@ import { tryRealpath, type SandboxPermissions, type ParsedSandboxDenial, + resolveSandboxPaths, } from '../../services/sandboxManager.js'; import type { ShellExecutionResult } from '../../services/shellExecutionService.js'; import { @@ -288,14 +289,16 @@ export class WindowsSandboxManager implements SandboxManager { await this.grantLowIntegrityAccess(this.options.workspace); } + const { allowed: allowedPaths, forbidden: forbiddenPaths } = + await resolveSandboxPaths(this.options, req); + // Grant "Low Mandatory Level" access to includeDirectories. - const includeDirs = sanitizePaths(this.options.includeDirectories) || []; + const includeDirs = sanitizePaths(this.options.includeDirectories); for (const includeDir of includeDirs) { await this.grantLowIntegrityAccess(includeDir); } // Grant "Low Mandatory Level" read/write access to allowedPaths. - const allowedPaths = sanitizePaths(req.policy?.allowedPaths) || []; for (const allowedPath of allowedPaths) { const resolved = await tryRealpath(allowedPath); if (!fs.existsSync(resolved)) { @@ -308,8 +311,9 @@ export class WindowsSandboxManager implements SandboxManager { } // Grant "Low Mandatory Level" write access to additional permissions write paths. - const additionalWritePaths = - sanitizePaths(mergedAdditional.fileSystem?.write) || []; + const additionalWritePaths = sanitizePaths( + mergedAdditional.fileSystem?.write, + ); for (const writePath of additionalWritePaths) { const resolved = await tryRealpath(writePath); if (!fs.existsSync(resolved)) { @@ -358,7 +362,6 @@ 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. - const forbiddenPaths = sanitizePaths(this.options.forbiddenPaths) || []; for (const forbiddenPath of forbiddenPaths) { try { await this.denyLowIntegrityAccess(forbiddenPath); diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index cf68f255b9..7dab4edd2f 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -274,7 +274,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [forbiddenDir] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, ); const { command, args } = Platform.touch(testFile); @@ -306,7 +309,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [forbiddenDir] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, ); const { command, args } = Platform.cat(nestedFile); @@ -335,7 +341,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [conflictDir] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [conflictDir], + }, ); const { command, args } = Platform.touch(testFile); @@ -365,7 +374,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [nonExistentPath] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentPath], + }, ); const { command, args } = Platform.echo('survived'); const sandboxed = await osManager.prepareCommand({ @@ -397,7 +409,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [nonExistentFile] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentFile], + }, ); // We use touch to attempt creation of the file @@ -436,7 +451,10 @@ describe('SandboxManager Integration', () => { try { const osManager = createSandboxManager( { enabled: true }, - { workspace: tempWorkspace, forbiddenPaths: [symlinkFile] }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [symlinkFile], + }, ); // Attempt to read the target file directly diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 0454581aac..8b2da94b63 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -14,6 +14,9 @@ import { findSecretFiles, isSecretFile, tryRealpath, + resolveSandboxPaths, + getPathIdentity, + type SandboxRequest, } from './sandboxManager.js'; import { createSandboxManager } from './sandboxManagerFactory.js'; import { LinuxSandboxManager } from '../sandbox/linux/LinuxSandboxManager.js'; @@ -121,8 +124,10 @@ describe('SandboxManager', () => { afterEach(() => vi.restoreAllMocks()); describe('sanitizePaths', () => { - it('should return undefined if no paths are provided', () => { - expect(sanitizePaths(undefined)).toBeUndefined(); + it('should return an empty array if no paths are provided', () => { + expect(sanitizePaths(undefined)).toEqual([]); + expect(sanitizePaths(null)).toEqual([]); + expect(sanitizePaths([])).toEqual([]); }); it('should deduplicate paths and return them', () => { @@ -133,6 +138,20 @@ describe('SandboxManager', () => { ]); }); + it('should deduplicate case-insensitively on Windows and macOS', () => { + vi.spyOn(os, 'platform').mockReturnValue('win32'); + const paths = ['/workspace/foo', '/WORKSPACE/FOO']; + expect(sanitizePaths(paths)).toEqual(['/workspace/foo']); + + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + const macPaths = ['/tmp/foo', '/tmp/FOO']; + expect(sanitizePaths(macPaths)).toEqual(['/tmp/foo']); + + vi.spyOn(os, 'platform').mockReturnValue('linux'); + const linuxPaths = ['/tmp/foo', '/tmp/FOO']; + expect(sanitizePaths(linuxPaths)).toEqual(['/tmp/foo', '/tmp/FOO']); + }); + it('should throw an error if a path is not absolute', () => { const paths = ['/workspace/foo', 'relative/path']; expect(() => sanitizePaths(paths)).toThrow( @@ -141,6 +160,110 @@ describe('SandboxManager', () => { }); }); + describe('getPathIdentity', () => { + it('should normalize slashes and strip trailing slashes', () => { + expect(getPathIdentity('/foo/bar//baz/')).toBe( + path.normalize('/foo/bar/baz'), + ); + }); + + it('should handle case sensitivity correctly per platform', () => { + vi.spyOn(os, 'platform').mockReturnValue('win32'); + expect(getPathIdentity('/Workspace/Foo')).toBe('/workspace/foo'); + + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + expect(getPathIdentity('/Tmp/Foo')).toBe('/tmp/foo'); + + vi.spyOn(os, 'platform').mockReturnValue('linux'); + expect(getPathIdentity('/Tmp/Foo')).toBe('/Tmp/Foo'); + }); + }); + + describe('resolveSandboxPaths', () => { + it('should resolve allowed and forbidden paths', async () => { + const options = { + workspace: '/workspace', + forbiddenPaths: async () => ['/workspace/forbidden'], + }; + const req = { + command: 'ls', + args: [], + cwd: '/workspace', + env: {}, + policy: { + allowedPaths: ['/workspace/allowed'], + }, + }; + + const result = await resolveSandboxPaths(options, req as SandboxRequest); + + expect(result.allowed).toEqual(['/workspace/allowed']); + expect(result.forbidden).toEqual(['/workspace/forbidden']); + }); + + it('should filter out workspace from allowed paths', async () => { + const options = { + workspace: '/workspace', + }; + const req = { + command: 'ls', + args: [], + cwd: '/workspace', + env: {}, + policy: { + allowedPaths: ['/workspace', '/workspace/', '/other/path'], + }, + }; + + const result = await resolveSandboxPaths(options, req as SandboxRequest); + + expect(result.allowed).toEqual(['/other/path']); + }); + + it('should prioritize forbidden paths over allowed paths', async () => { + const options = { + workspace: '/workspace', + forbiddenPaths: async () => ['/workspace/secret'], + }; + const req = { + command: 'ls', + args: [], + cwd: '/workspace', + env: {}, + policy: { + allowedPaths: ['/workspace/secret', '/workspace/normal'], + }, + }; + + const result = await resolveSandboxPaths(options, req as SandboxRequest); + + expect(result.allowed).toEqual(['/workspace/normal']); + expect(result.forbidden).toEqual(['/workspace/secret']); + }); + + it('should handle case-insensitive conflicts on supported platforms', async () => { + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + const options = { + workspace: '/workspace', + forbiddenPaths: async () => ['/workspace/SECRET'], + }; + const req = { + command: 'ls', + args: [], + cwd: '/workspace', + env: {}, + policy: { + allowedPaths: ['/workspace/secret'], + }, + }; + + const result = await resolveSandboxPaths(options, req as SandboxRequest); + + expect(result.allowed).toEqual([]); + expect(result.forbidden).toEqual(['/workspace/SECRET']); + }); + }); + describe('tryRealpath', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index f359eb5cf1..6313c09eeb 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -63,15 +63,12 @@ export interface SandboxModeConfig { * Global configuration options used to initialize a SandboxManager. */ export interface GlobalSandboxOptions { - /** - * The primary workspace path the sandbox is anchored to. - * This directory is granted full read and write access. - */ + /** The absolute path to the primary workspace directory, granted full read/write access. */ workspace: string; /** Absolute paths to explicitly include in the workspace context. */ includeDirectories?: string[]; - /** Absolute paths to explicitly deny read/write access to (overrides allowlists). */ - forbiddenPaths?: string[]; + /** An optional asynchronous resolver function for paths that should be explicitly denied. */ + forbiddenPaths?: () => Promise; /** The current sandbox mode behavior from config. */ modeConfig?: SandboxModeConfig; /** The policy manager for persistent approvals. */ @@ -298,29 +295,47 @@ export class LocalSandboxManager implements SandboxManager { } /** - * Sanitizes an array of paths by deduplicating them and ensuring they are absolute. + * Resolves sanitized allowed and forbidden paths for a request. + * Filters the workspace from allowed paths and ensures forbidden paths take precedence. */ -export function sanitizePaths(paths?: string[]): string[] | undefined { - if (!paths) return undefined; +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); + + const workspaceIdentity = getPathIdentity(options.workspace); + const forbiddenIdentities = new Set(forbidden.map(getPathIdentity)); + + const filteredAllowed = allowed.filter((p) => { + const identity = getPathIdentity(p); + return identity !== workspaceIdentity && !forbiddenIdentities.has(identity); + }); + + return { + allowed: filteredAllowed, + forbidden, + }; +} + +/** + * Sanitizes an array of paths by deduplicating them and ensuring they are absolute. + * Always returns an array (empty if input is null/undefined). + */ +export function sanitizePaths(paths?: string[] | null): string[] { + if (!paths || paths.length === 0) return []; - // We use a Map to deduplicate paths based on their normalized, - // platform-specific identity e.g. handling case-insensitivity on Windows) - // while preserving the original string casing. const uniquePathsMap = new Map(); for (const p of paths) { if (!path.isAbsolute(p)) { throw new Error(`Sandbox path must be absolute: ${p}`); } - // Normalize the path (resolves slashes and redundant components) - let key = path.normalize(p); - - // Windows file systems are case-insensitive, so we lowercase the key for - // deduplication - if (os.platform() === 'win32') { - key = key.toLowerCase(); - } - + const key = getPathIdentity(p); if (!uniquePathsMap.has(key)) { uniquePathsMap.set(key, p); } @@ -329,6 +344,20 @@ export function sanitizePaths(paths?: string[]): string[] | undefined { return Array.from(uniquePathsMap.values()); } +/** Returns a normalized identity for a path, stripping trailing slashes and handling case sensitivity. */ +export function getPathIdentity(p: string): string { + let norm = path.normalize(p); + + // Strip trailing slashes (except for root paths) + if (norm.length > 1 && (norm.endsWith('/') || norm.endsWith('\\'))) { + norm = norm.slice(0, -1); + } + + const platform = os.platform(); + const isCaseInsensitive = platform === 'win32' || platform === 'darwin'; + return isCaseInsensitive ? norm.toLowerCase() : norm; +} + /** * Resolves symlinks for a given path to prevent sandbox escapes. * If a file does not exist (ENOENT), it recursively resolves the parent directory.