From 5b7f7b30a7281d50c41f6411d5756d420896cfe0 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Tue, 24 Mar 2026 22:37:32 -0400 Subject: [PATCH] refactor(core): standardize OS-specific sandbox tests and extract linux helper methods (#23715) --- .../sandbox/linux/LinuxSandboxManager.test.ts | 563 ++++++++++-------- .../src/sandbox/linux/LinuxSandboxManager.ts | 201 ++++--- .../sandbox/macos/MacOsSandboxManager.test.ts | 116 +++- .../sandbox/macos/seatbeltArgsBuilder.test.ts | 387 ++++++------ .../windows/WindowsSandboxManager.test.ts | 386 ++++++------ .../sandbox/windows/WindowsSandboxManager.ts | 1 + 6 files changed, 967 insertions(+), 687 deletions(-) diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts index 36811a44b1..5bde6a44da 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.test.ts @@ -95,272 +95,343 @@ describe('LinuxSandboxManager', () => { expect(dynamicBinds).toEqual(expectedDynamicBinds); }; - it('correctly outputs bwrap as the program with appropriate isolation flags', async () => { - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: ['-la'], - cwd: workspace, - env: {}, + describe('prepareCommand', () => { + it('should correctly format the base command and args', async () => { + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + }); + + expect(bwrapArgs).toEqual([ + '--unshare-all', + '--new-session', + '--die-with-parent', + '--ro-bind', + '/', + '/', + '--dev', + '/dev', + '--proc', + '/proc', + '--tmpfs', + '/tmp', + '--bind', + workspace, + workspace, + '--ro-bind', + `${workspace}/.gitignore`, + `${workspace}/.gitignore`, + '--ro-bind', + `${workspace}/.geminiignore`, + `${workspace}/.geminiignore`, + '--ro-bind', + `${workspace}/.git`, + `${workspace}/.git`, + '--seccomp', + '9', + '--', + 'ls', + '-la', + ]); }); - expect(bwrapArgs).toEqual([ - '--unshare-all', - '--new-session', - '--die-with-parent', - '--ro-bind', - '/', - '/', - '--dev', - '/dev', - '--proc', - '/proc', - '--tmpfs', - '/tmp', - '--bind', - workspace, - workspace, - '--ro-bind', - `${workspace}/.gitignore`, - `${workspace}/.gitignore`, - '--ro-bind', - `${workspace}/.geminiignore`, - `${workspace}/.geminiignore`, - '--ro-bind', - `${workspace}/.git`, - `${workspace}/.git`, - '--seccomp', - '9', - '--', - 'ls', - '-la', - ]); - }); + it('should correctly pass through the cwd to the resulting command', async () => { + const req: SandboxRequest = { + command: 'ls', + args: [], + cwd: '/different/cwd', + env: {}, + }; - it('maps allowedPaths to bwrap binds', async () => { - const bwrapArgs = await getBwrapArgs({ - command: 'node', - args: ['script.js'], - cwd: workspace, - env: {}, - policy: { - allowedPaths: ['/tmp/cache', '/opt/tools', workspace], - }, + const result = await manager.prepareCommand(req); + + expect(result.cwd).toBe('/different/cwd'); }); - // Verify the specific bindings were added correctly - expectDynamicBinds(bwrapArgs, [ - '--bind-try', - '/tmp/cache', - '/tmp/cache', - '--bind-try', - '/opt/tools', - '/opt/tools', - ]); - }); + it('should apply environment sanitization via the default mechanisms', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: workspace, + env: { + API_KEY: 'secret', + PATH: '/usr/bin', + }, + policy: { + sanitizationConfig: { + allowedEnvironmentVariables: ['PATH'], + blockedEnvironmentVariables: ['API_KEY'], + enableEnvironmentVariableRedaction: true, + }, + }, + }; - it('protects real paths of governance files if they are symlinks', async () => { - vi.mocked(fs.realpathSync).mockImplementation((p) => { - if (p.toString() === `${workspace}/.gitignore`) - return '/shared/global.gitignore'; - return p.toString(); + const result = await manager.prepareCommand(req); + expect(result.env['PATH']).toBe('/usr/bin'); + expect(result.env['API_KEY']).toBeUndefined(); }); - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: [], - cwd: workspace, - env: {}, + it('should allow network when networkAccess is true', async () => { + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + policy: { + networkAccess: true, + }, + }); + + expect(bwrapArgs).toContain('--unshare-user'); + expect(bwrapArgs).toContain('--unshare-ipc'); + expect(bwrapArgs).toContain('--unshare-pid'); + expect(bwrapArgs).toContain('--unshare-uts'); + expect(bwrapArgs).toContain('--unshare-cgroup'); + expect(bwrapArgs).not.toContain('--unshare-all'); }); - expect(bwrapArgs).toContain('--ro-bind'); - expect(bwrapArgs).toContain(`${workspace}/.gitignore`); - expect(bwrapArgs).toContain('/shared/global.gitignore'); + describe('governance files', () => { + it('should ensure governance files exist', async () => { + vi.mocked(fs.existsSync).mockReturnValue(false); - // Check that both are bound - const gitignoreIndex = bwrapArgs.indexOf(`${workspace}/.gitignore`); - expect(bwrapArgs[gitignoreIndex - 1]).toBe('--ro-bind'); - expect(bwrapArgs[gitignoreIndex + 1]).toBe(`${workspace}/.gitignore`); + await getBwrapArgs({ + command: 'ls', + args: [], + cwd: workspace, + env: {}, + }); - const realGitignoreIndex = bwrapArgs.indexOf('/shared/global.gitignore'); - expect(bwrapArgs[realGitignoreIndex - 1]).toBe('--ro-bind'); - expect(bwrapArgs[realGitignoreIndex + 1]).toBe('/shared/global.gitignore'); - }); + expect(fs.mkdirSync).toHaveBeenCalled(); + expect(fs.openSync).toHaveBeenCalled(); + }); - it('touches governance files if they do not exist', async () => { - vi.mocked(fs.existsSync).mockReturnValue(false); + it('should protect both the symlink and the real path if they differ', async () => { + vi.mocked(fs.realpathSync).mockImplementation((p) => { + if (p.toString() === `${workspace}/.gitignore`) + return '/shared/global.gitignore'; + return p.toString(); + }); - await getBwrapArgs({ - command: 'ls', - args: [], - cwd: workspace, - env: {}, + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: [], + cwd: workspace, + env: {}, + }); + + expect(bwrapArgs).toContain('--ro-bind'); + expect(bwrapArgs).toContain(`${workspace}/.gitignore`); + expect(bwrapArgs).toContain('/shared/global.gitignore'); + + // Check that both are bound + const gitignoreIndex = bwrapArgs.indexOf(`${workspace}/.gitignore`); + expect(bwrapArgs[gitignoreIndex - 1]).toBe('--ro-bind'); + expect(bwrapArgs[gitignoreIndex + 1]).toBe(`${workspace}/.gitignore`); + + const realGitignoreIndex = bwrapArgs.indexOf( + '/shared/global.gitignore', + ); + expect(bwrapArgs[realGitignoreIndex - 1]).toBe('--ro-bind'); + expect(bwrapArgs[realGitignoreIndex + 1]).toBe( + '/shared/global.gitignore', + ); + }); }); - expect(fs.mkdirSync).toHaveBeenCalled(); - expect(fs.openSync).toHaveBeenCalled(); - }); + describe('allowedPaths', () => { + it('should parameterize allowed paths and normalize them', async () => { + const bwrapArgs = await getBwrapArgs({ + command: 'node', + args: ['script.js'], + cwd: workspace, + env: {}, + policy: { + allowedPaths: ['/tmp/cache', '/opt/tools', workspace], + }, + }); - it('should not bind the workspace twice even if it has a trailing slash in allowedPaths', async () => { - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: ['-la'], - cwd: workspace, - env: {}, - policy: { - allowedPaths: [workspace + '/'], - }, + // Verify the specific bindings were added correctly + expectDynamicBinds(bwrapArgs, [ + '--bind-try', + '/tmp/cache', + '/tmp/cache', + '--bind-try', + '/opt/tools', + '/opt/tools', + ]); + }); + + it('should not bind the workspace twice even if it has a trailing slash in allowedPaths', async () => { + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + policy: { + allowedPaths: [workspace + '/'], + }, + }); + + // Should only contain the primary workspace bind and governance files, not the second workspace bind with a trailing slash + expectDynamicBinds(bwrapArgs, []); + }); }); - // Should only contain the primary workspace bind and governance files, not the second workspace bind with a trailing slash - expectDynamicBinds(bwrapArgs, []); - }); + describe('forbiddenPaths', () => { + it('should parameterize forbidden paths and explicitly deny them', async () => { + vi.spyOn(fs.promises, 'stat').mockImplementation(async (p) => { + // Mock /tmp/cache as a directory, and /opt/secret.txt as a file + if (p.toString().includes('cache')) { + return { isDirectory: () => true } as fs.Stats; + } + return { isDirectory: () => false } as fs.Stats; + }); + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => + p.toString(), + ); - it('maps forbiddenPaths to empty mounts', async () => { - vi.spyOn(fs.promises, 'stat').mockImplementation(async (p) => { - // Mock /tmp/cache as a directory, and /opt/secret.txt as a file - if (p.toString().includes('cache')) { - return { isDirectory: () => true } as fs.Stats; - } - return { isDirectory: () => false } as fs.Stats; + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + policy: { + forbiddenPaths: ['/tmp/cache', '/opt/secret.txt'], + }, + }); + + expectDynamicBinds(bwrapArgs, [ + '--tmpfs', + '/tmp/cache', + '--remount-ro', + '/tmp/cache', + '--ro-bind-try', + '/dev/null', + '/opt/secret.txt', + ]); + }); + + it('resolves forbidden symlink paths to their real paths', async () => { + vi.spyOn(fs.promises, 'stat').mockImplementation( + async () => ({ isDirectory: () => false }) as fs.Stats, + ); + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => { + if (p === '/tmp/forbidden-symlink') return '/opt/real-target.txt'; + return p.toString(); + }, + ); + + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + policy: { + forbiddenPaths: ['/tmp/forbidden-symlink'], + }, + }); + + // Should explicitly mask both the resolved path and the original symlink path + expectDynamicBinds(bwrapArgs, [ + '--ro-bind-try', + '/dev/null', + '/opt/real-target.txt', + '--ro-bind-try', + '/dev/null', + '/tmp/forbidden-symlink', + ]); + }); + + it('explicitly denies non-existent forbidden paths to prevent creation', async () => { + const error = new Error('File not found') as NodeJS.ErrnoException; + error.code = 'ENOENT'; + vi.spyOn(fs.promises, 'stat').mockRejectedValue(error); + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => + p.toString(), + ); + + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: [], + cwd: workspace, + env: {}, + policy: { + forbiddenPaths: ['/tmp/not-here.txt'], + }, + }); + + expectDynamicBinds(bwrapArgs, [ + '--symlink', + '/.forbidden', + '/tmp/not-here.txt', + ]); + }); + + it('masks directory symlinks with tmpfs for both paths', async () => { + vi.spyOn(fs.promises, 'stat').mockImplementation( + async () => ({ isDirectory: () => true }) as fs.Stats, + ); + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => { + if (p === '/tmp/dir-link') return '/opt/real-dir'; + return p.toString(); + }, + ); + + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: [], + cwd: workspace, + env: {}, + policy: { + forbiddenPaths: ['/tmp/dir-link'], + }, + }); + + expectDynamicBinds(bwrapArgs, [ + '--tmpfs', + '/opt/real-dir', + '--remount-ro', + '/opt/real-dir', + '--tmpfs', + '/tmp/dir-link', + '--remount-ro', + '/tmp/dir-link', + ]); + }); + + it('should override allowed paths if a path is also in forbidden paths', async () => { + vi.spyOn(fs.promises, 'stat').mockImplementation( + async () => ({ isDirectory: () => true }) as fs.Stats, + ); + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => + p.toString(), + ); + + const bwrapArgs = await getBwrapArgs({ + command: 'ls', + args: ['-la'], + cwd: workspace, + env: {}, + policy: { + allowedPaths: ['/tmp/conflict'], + forbiddenPaths: ['/tmp/conflict'], + }, + }); + + expectDynamicBinds(bwrapArgs, [ + '--bind-try', + '/tmp/conflict', + '/tmp/conflict', + '--tmpfs', + '/tmp/conflict', + '--remount-ro', + '/tmp/conflict', + ]); + }); }); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => - p.toString(), - ); - - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: ['-la'], - cwd: workspace, - env: {}, - policy: { - forbiddenPaths: ['/tmp/cache', '/opt/secret.txt'], - }, - }); - - expectDynamicBinds(bwrapArgs, [ - '--tmpfs', - '/tmp/cache', - '--remount-ro', - '/tmp/cache', - '--ro-bind-try', - '/dev/null', - '/opt/secret.txt', - ]); - }); - - it('overrides allowedPaths if a path is also in forbiddenPaths', async () => { - vi.spyOn(fs.promises, 'stat').mockImplementation( - async () => ({ isDirectory: () => true }) as fs.Stats, - ); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => - p.toString(), - ); - - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: ['-la'], - cwd: workspace, - env: {}, - policy: { - allowedPaths: ['/tmp/conflict'], - forbiddenPaths: ['/tmp/conflict'], - }, - }); - - expectDynamicBinds(bwrapArgs, [ - '--bind-try', - '/tmp/conflict', - '/tmp/conflict', - '--tmpfs', - '/tmp/conflict', - '--remount-ro', - '/tmp/conflict', - ]); - }); - - it('protects both the resolved path and the original path for forbidden symlinks', async () => { - vi.spyOn(fs.promises, 'stat').mockImplementation( - async () => ({ isDirectory: () => false }) as fs.Stats, - ); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => { - if (p === '/tmp/forbidden-symlink') return '/opt/real-target.txt'; - return p.toString(); - }); - - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: ['-la'], - cwd: workspace, - env: {}, - policy: { - forbiddenPaths: ['/tmp/forbidden-symlink'], - }, - }); - - // Should explicitly mask both the resolved path and the original symlink path - expectDynamicBinds(bwrapArgs, [ - '--ro-bind-try', - '/dev/null', - '/opt/real-target.txt', - '--ro-bind-try', - '/dev/null', - '/tmp/forbidden-symlink', - ]); - }); - - it('masks non-existent forbidden paths with a broken symlink', async () => { - const error = new Error('File not found') as NodeJS.ErrnoException; - error.code = 'ENOENT'; - vi.spyOn(fs.promises, 'stat').mockRejectedValue(error); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => - p.toString(), - ); - - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: [], - cwd: workspace, - env: {}, - policy: { - forbiddenPaths: ['/tmp/not-here.txt'], - }, - }); - - expectDynamicBinds(bwrapArgs, [ - '--symlink', - '/.forbidden', - '/tmp/not-here.txt', - ]); - }); - - it('masks directory symlinks with tmpfs for both paths', async () => { - vi.spyOn(fs.promises, 'stat').mockImplementation( - async () => ({ isDirectory: () => true }) as fs.Stats, - ); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => { - if (p === '/tmp/dir-link') return '/opt/real-dir'; - return p.toString(); - }); - - const bwrapArgs = await getBwrapArgs({ - command: 'ls', - args: [], - cwd: workspace, - env: {}, - policy: { - forbiddenPaths: ['/tmp/dir-link'], - }, - }); - - expectDynamicBinds(bwrapArgs, [ - '--tmpfs', - '/opt/real-dir', - '--remount-ro', - '/opt/real-dir', - '--tmpfs', - '/tmp/dir-link', - '--remount-ro', - '/tmp/dir-link', - ]); }); }); diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index cd653061b8..8dd1154846 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -113,78 +113,13 @@ export class LinuxSandboxManager implements SandboxManager { const sanitizedEnv = sanitizeEnvironment(req.env, sanitizationConfig); const bwrapArgs: string[] = [ - ...(req.policy?.networkAccess - ? [ - '--unshare-user', - '--unshare-ipc', - '--unshare-pid', - '--unshare-uts', - '--unshare-cgroup', - ] - : ['--unshare-all']), - '--new-session', // Isolate session - '--die-with-parent', // Prevent orphaned runaway processes - '--ro-bind', - '/', - '/', - '--dev', // Creates a safe, minimal /dev (replaces --dev-bind) - '/dev', - '--proc', // Creates a fresh procfs for the unshared PID namespace - '/proc', - '--tmpfs', // Provides an isolated, writable /tmp directory - '/tmp', - // Note: --dev /dev sets up /dev/pts automatically - '--bind', - this.options.workspace, - this.options.workspace, + ...this.getNetworkArgs(req), + ...this.getBaseArgs(), + ...this.getGovernanceArgs(), + ...this.getAllowedPathsArgs(req.policy?.allowedPaths), + ...(await this.getForbiddenPathsArgs(req.policy?.forbiddenPaths)), ]; - // Protected governance files are bind-mounted as read-only, even if the workspace is RW. - // We ensure they exist on the host and resolve real paths to prevent symlink bypasses. - // In bwrap, later binds override earlier ones for the same path. - for (const file of GOVERNANCE_FILES) { - const filePath = join(this.options.workspace, file.path); - touch(filePath, file.isDirectory); - - const realPath = fs.realpathSync(filePath); - - bwrapArgs.push('--ro-bind', filePath, filePath); - if (realPath !== filePath) { - bwrapArgs.push('--ro-bind', realPath, realPath); - } - } - - const allowedPaths = sanitizePaths(req.policy?.allowedPaths) || []; - const normalizedWorkspace = this.normalizePath(this.options.workspace); - for (const p of allowedPaths) { - if (this.normalizePath(p) !== normalizedWorkspace) { - bwrapArgs.push('--bind-try', p, p); - } - } - - const forbiddenPaths = sanitizePaths(req.policy?.forbiddenPaths) || []; - for (const p of forbiddenPaths) { - try { - const originalPath = this.normalizePath(p); - const resolvedPath = await tryRealpath(originalPath); - - // Mask the resolved path to prevent access to the underlying file. - await this.applyMasking(bwrapArgs, resolvedPath); - - // If the original path was a symlink, mask it as well to prevent access - // through the link itself. - if (resolvedPath !== originalPath) { - await this.applyMasking(bwrapArgs, originalPath); - } - } catch (e) { - throw new Error( - `Failed to deny access to forbidden path: ${p}. ${ - e instanceof Error ? e.message : String(e) - }`, - ); - } - } - const bpfPath = getSeccompBpfPath(); bwrapArgs.push('--seccomp', '9'); @@ -202,29 +137,139 @@ export class LinuxSandboxManager implements SandboxManager { program: 'sh', args: shArgs, env: sanitizedEnv, + cwd: req.cwd, }; } /** - * Applies bubblewrap arguments to mask a forbidden path. + * Generates arguments for network isolation. */ - private async applyMasking(args: string[], path: string) { + private getNetworkArgs(req: SandboxRequest): string[] { + return req.policy?.networkAccess + ? [ + '--unshare-user', + '--unshare-ipc', + '--unshare-pid', + '--unshare-uts', + '--unshare-cgroup', + ] + : ['--unshare-all']; + } + + /** + * Generates the base bubblewrap arguments for isolation. + */ + private getBaseArgs(): string[] { + return [ + '--new-session', // Isolate session + '--die-with-parent', // Prevent orphaned runaway processes + '--ro-bind', + '/', + '/', + '--dev', // Creates a safe, minimal /dev (replaces --dev-bind) + '/dev', + '--proc', // Creates a fresh procfs for the unshared PID namespace + '/proc', + '--tmpfs', // Provides an isolated, writable /tmp directory + '/tmp', + // Note: --dev /dev sets up /dev/pts automatically + '--bind', + this.options.workspace, + this.options.workspace, + ]; + } + + /** + * Generates arguments for protected governance files. + */ + private getGovernanceArgs(): string[] { + const args: string[] = []; + // Protected governance files are bind-mounted as read-only, even if the workspace is RW. + // We ensure they exist on the host and resolve real paths to prevent symlink bypasses. + // In bwrap, later binds override earlier ones for the same path. + for (const file of GOVERNANCE_FILES) { + const filePath = join(this.options.workspace, file.path); + touch(filePath, file.isDirectory); + + const realPath = fs.realpathSync(filePath); + + args.push('--ro-bind', filePath, filePath); + if (realPath !== filePath) { + args.push('--ro-bind', realPath, realPath); + } + } + return args; + } + + /** + * Generates arguments for allowed paths. + */ + private getAllowedPathsArgs(allowedPaths?: string[]): string[] { + const args: string[] = []; + const paths = sanitizePaths(allowedPaths) || []; + const normalizedWorkspace = this.normalizePath(this.options.workspace); + + for (const p of paths) { + if (this.normalizePath(p) !== normalizedWorkspace) { + args.push('--bind-try', p, p); + } + } + return args; + } + + /** + * Generates arguments for forbidden paths. + */ + private async getForbiddenPathsArgs( + forbiddenPaths?: string[], + ): Promise { + const args: string[] = []; + const paths = sanitizePaths(forbiddenPaths) || []; + + for (const p of paths) { + try { + const originalPath = this.normalizePath(p); + const resolvedPath = await tryRealpath(originalPath); + + // Mask the resolved path to prevent access to the underlying file. + const resolvedMask = await this.getMaskArgs(resolvedPath); + args.push(...resolvedMask); + + // If the original path was a symlink, mask it as well to prevent access + // through the link itself. + if (resolvedPath !== originalPath) { + const originalMask = await this.getMaskArgs(originalPath); + args.push(...originalMask); + } + } catch (e) { + throw new Error( + `Failed to deny access to forbidden path: ${p}. ${ + e instanceof Error ? e.message : String(e) + }`, + ); + } + } + return args; + } + + /** + * Generates bubblewrap arguments to mask a forbidden path. + */ + private async getMaskArgs(path: string): Promise { try { const stats = await fs.promises.stat(path); if (stats.isDirectory()) { // Directories are masked by mounting an empty, read-only tmpfs. - args.push('--tmpfs', path, '--remount-ro', path); - } else { - // Existing files are masked by binding them to /dev/null. - args.push('--ro-bind-try', '/dev/null', path); + return ['--tmpfs', path, '--remount-ro', path]; } + // Existing files are masked by binding them to /dev/null. + return ['--ro-bind-try', '/dev/null', path]; } catch (e) { if (isNodeError(e) && e.code === 'ENOENT') { // Non-existent paths are masked by a broken symlink. This prevents // creation within the sandbox while avoiding host remnants. - args.push('--symlink', '/.forbidden', path); - return; + return ['--symlink', '/.forbidden', path]; } throw e; } diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index 1f0f1d44fd..7d9bd57cae 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -55,7 +55,7 @@ describe('MacOsSandboxManager', () => { }); describe('prepareCommand', () => { - it('should correctly orchestrate Seatbelt args and format the final command', async () => { + it('should correctly format the base command and args', async () => { const result = await manager.prepareCommand({ command: 'echo', args: ['hello'], @@ -118,5 +118,119 @@ describe('MacOsSandboxManager', () => { expect(result.env['SAFE_VAR']).toBe('1'); expect(result.env['GITHUB_TOKEN']).toBeUndefined(); }); + + it('should allow network when networkAccess is true', async () => { + await manager.prepareCommand({ + command: 'echo', + args: ['hello'], + cwd: mockWorkspace, + env: {}, + policy: { ...mockPolicy, networkAccess: true }, + }); + + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ networkAccess: true }), + ); + }); + + describe('governance files', () => { + it('should ensure governance files exist', async () => { + await manager.prepareCommand({ + command: 'echo', + args: [], + cwd: mockWorkspace, + env: {}, + policy: mockPolicy, + }); + + // The seatbelt builder internally handles governance files, so we simply verify + // it is invoked correctly with the right workspace. + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ workspace: mockWorkspace }), + ); + }); + }); + + describe('allowedPaths', () => { + it('should parameterize allowed paths and normalize them', async () => { + await manager.prepareCommand({ + command: 'echo', + args: [], + cwd: mockWorkspace, + env: {}, + policy: { + ...mockPolicy, + allowedPaths: ['/tmp/allowed1', '/tmp/allowed2'], + }, + }); + + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ + allowedPaths: ['/tmp/allowed1', '/tmp/allowed2'], + }), + ); + }); + }); + + describe('forbiddenPaths', () => { + it('should parameterize forbidden paths and explicitly deny them', async () => { + await manager.prepareCommand({ + command: 'echo', + args: [], + cwd: mockWorkspace, + env: {}, + policy: { + ...mockPolicy, + forbiddenPaths: ['/tmp/forbidden1'], + }, + }); + + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ + forbiddenPaths: ['/tmp/forbidden1'], + }), + ); + }); + + it('explicitly denies non-existent forbidden paths to prevent creation', async () => { + await manager.prepareCommand({ + command: 'echo', + args: [], + cwd: mockWorkspace, + env: {}, + policy: { + ...mockPolicy, + forbiddenPaths: ['/tmp/does-not-exist'], + }, + }); + + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ + forbiddenPaths: ['/tmp/does-not-exist'], + }), + ); + }); + + it('should override allowed paths if a path is also in forbidden paths', async () => { + await manager.prepareCommand({ + command: 'echo', + args: [], + cwd: mockWorkspace, + env: {}, + policy: { + ...mockPolicy, + allowedPaths: ['/tmp/conflict'], + forbiddenPaths: ['/tmp/conflict'], + }, + }); + + expect(seatbeltArgsBuilder.buildSeatbeltArgs).toHaveBeenCalledWith( + expect.objectContaining({ + allowedPaths: ['/tmp/conflict'], + forbiddenPaths: ['/tmp/conflict'], + }), + ); + }); + }); }); }); diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts index 88cd04acff..dd2c95235e 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts @@ -14,201 +14,224 @@ describe('seatbeltArgsBuilder', () => { vi.restoreAllMocks(); }); - it('should build a strict allowlist profile allowing the workspace via param', async () => { - // Mock tryRealpath to just return the path for testing - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p); - - const args = await buildSeatbeltArgs({ - 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).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', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p); - const args = await buildSeatbeltArgs({ - workspace: '/test', - networkAccess: true, - }); - const profile = args[1]; - expect(profile).toContain('(allow network-outbound)'); - }); - - it('should parameterize allowed paths and normalize them', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => { - if (p === '/test/symlink') return '/test/real_path'; - return p; - }); - - const args = await buildSeatbeltArgs({ - 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'); - }); - - it('should parameterize forbidden paths and explicitly deny them', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p); - - const args = await buildSeatbeltArgs({ - 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")))', - ); - }); - - it('explicitly denies non-existent forbidden paths to prevent creation', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p); - - const args = await buildSeatbeltArgs({ - 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")))', - ); - }); - - it('resolves forbidden symlink paths to their real paths', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => { - if (p === '/test/symlink') return '/test/real_path'; - return p; - }); - - const args = await buildSeatbeltArgs({ - workspace: '/test', - forbiddenPaths: ['/test/symlink'], - }); - - const profile = args[1]; - - // The builder should resolve the symlink and explicitly deny the real target path - 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")))', - ); - }); - - it('should override allowed paths if a path is also in forbidden paths', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p); - - const args = await buildSeatbeltArgs({ - 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")))'; - - expect(profile).toContain(allowString); - expect(profile).toContain(denyString); - - // Verify ordering: The explicit deny must appear AFTER the explicit allow in the profile string - // Seatbelt rules are evaluated in order where the latest rule matching a path wins - const allowIndex = profile.indexOf(allowString); - const denyIndex = profile.indexOf(denyString); - expect(denyIndex).toBeGreaterThan(allowIndex); - }); - - describe('governance files', () => { - it('should inject explicit deny rules for governance files', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => - p.toString(), - ); - vi.spyOn(fs, 'existsSync').mockReturnValue(true); - vi.spyOn(fs, 'lstatSync').mockImplementation( - (p) => - ({ - isDirectory: () => p.toString().endsWith('.git'), - isFile: () => !p.toString().endsWith('.git'), - }) as unknown as fs.Stats, + describe('buildSeatbeltArgs', () => { + it('should build a strict allowlist profile allowing the workspace via param', async () => { + // Mock tryRealpath to just return the path for testing + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => p, ); const args = await buildSeatbeltArgs({ 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).not.toContain('(allow network*)'); - // .gitignore should be a literal deny expect(args).toContain('-D'); - expect(args).toContain( - 'GOVERNANCE_FILE_0=/Users/test/workspace/.gitignore', - ); - expect(profile).toContain( - '(deny file-write* (literal (param "GOVERNANCE_FILE_0")))', - ); - - // .git should be a subpath deny - expect(args).toContain('GOVERNANCE_FILE_2=/Users/test/workspace/.git'); - expect(profile).toContain( - '(deny file-write* (subpath (param "GOVERNANCE_FILE_2")))', - ); + expect(args).toContain('WORKSPACE=/Users/test/workspace'); + expect(args).toContain(`TMPDIR=${os.tmpdir()}`); }); - it('should protect both the symlink and the real path if they differ', async () => { - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => { - if (p === '/test/workspace/.gitignore') return '/test/real/.gitignore'; - return p.toString(); + it('should allow network when networkAccess is true', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => p, + ); + const args = await buildSeatbeltArgs({ + workspace: '/test', + networkAccess: true, }); - vi.spyOn(fs, 'existsSync').mockReturnValue(true); - vi.spyOn(fs, 'lstatSync').mockImplementation( - () => - ({ - isDirectory: () => false, - isFile: () => true, - }) as unknown as fs.Stats, - ); - - const args = await buildSeatbeltArgs({ workspace: '/test/workspace' }); const profile = args[1]; + expect(profile).toContain('(allow network-outbound)'); + }); - 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")))', - ); - expect(profile).toContain( - '(deny file-write* (literal (param "REAL_GOVERNANCE_FILE_0")))', - ); + describe('governance files', () => { + it('should inject explicit deny rules for governance files', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => + p.toString(), + ); + vi.spyOn(fs, 'existsSync').mockReturnValue(true); + vi.spyOn(fs, 'lstatSync').mockImplementation( + (p) => + ({ + isDirectory: () => p.toString().endsWith('.git'), + isFile: () => !p.toString().endsWith('.git'), + }) as unknown as fs.Stats, + ); + + const args = await buildSeatbeltArgs({ + workspace: '/Users/test/workspace', + }); + const profile = args[1]; + + // .gitignore should be a literal deny + expect(args).toContain('-D'); + expect(args).toContain( + 'GOVERNANCE_FILE_0=/Users/test/workspace/.gitignore', + ); + expect(profile).toContain( + '(deny file-write* (literal (param "GOVERNANCE_FILE_0")))', + ); + + // .git should be a subpath deny + expect(args).toContain('GOVERNANCE_FILE_2=/Users/test/workspace/.git'); + expect(profile).toContain( + '(deny file-write* (subpath (param "GOVERNANCE_FILE_2")))', + ); + }); + + it('should protect both the symlink and the real path if they differ', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (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 args = await buildSeatbeltArgs({ workspace: '/test/workspace' }); + const profile = args[1]; + + 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")))', + ); + expect(profile).toContain( + '(deny file-write* (literal (param "REAL_GOVERNANCE_FILE_0")))', + ); + }); + }); + + describe('allowedPaths', () => { + it('should parameterize allowed paths and normalize them', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => { + if (p === '/test/symlink') return '/test/real_path'; + return p; + }, + ); + + const args = await buildSeatbeltArgs({ + 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'); + }); + }); + + describe('forbiddenPaths', () => { + it('should parameterize forbidden paths and explicitly deny them', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => p, + ); + + const args = await buildSeatbeltArgs({ + 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")))', + ); + }); + + it('resolves forbidden symlink paths to their real paths', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => { + if (p === '/test/symlink') return '/test/real_path'; + return p; + }, + ); + + const args = await buildSeatbeltArgs({ + workspace: '/test', + forbiddenPaths: ['/test/symlink'], + }); + + const profile = args[1]; + + // The builder should resolve the symlink and explicitly deny the real target path + 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")))', + ); + }); + + it('explicitly denies non-existent forbidden paths to prevent creation', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => p, + ); + + const args = await buildSeatbeltArgs({ + 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")))', + ); + }); + + it('should override allowed paths if a path is also in forbidden paths', async () => { + vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation( + async (p) => p, + ); + + const args = await buildSeatbeltArgs({ + 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")))'; + + expect(profile).toContain(allowString); + expect(profile).toContain(denyString); + + // Verify ordering: The explicit deny must appear AFTER the explicit allow in the profile string + // Seatbelt rules are evaluated in order where the latest rule matching a path wins + const allowIndex = profile.indexOf(allowString); + const denyIndex = profile.indexOf(denyString); + expect(denyIndex).toBeGreaterThan(allowIndex); + }); }); }); }); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 6bfe6d581a..0abd3dd56b 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -35,214 +35,240 @@ describe('WindowsSandboxManager', () => { fs.rmSync(testCwd, { recursive: true, force: true }); }); - it('should prepare a GeminiSandbox.exe command', async () => { - const req: SandboxRequest = { - command: 'whoami', - args: ['/groups'], - cwd: testCwd, - env: { TEST_VAR: 'test_value' }, - policy: { - networkAccess: false, - }, - }; - - const result = await manager.prepareCommand(req); - - expect(result.program).toContain('GeminiSandbox.exe'); - expect(result.args).toEqual(['0', testCwd, 'whoami', '/groups']); - }); - - it('should handle networkAccess from config', async () => { - const req: SandboxRequest = { - command: 'whoami', - args: [], - cwd: testCwd, - env: {}, - policy: { - networkAccess: true, - }, - }; - - const result = await manager.prepareCommand(req); - expect(result.args[0]).toBe('1'); - }); - - it('should sanitize environment variables', async () => { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: { - API_KEY: 'secret', - PATH: '/usr/bin', - }, - policy: { - sanitizationConfig: { - allowedEnvironmentVariables: ['PATH'], - blockedEnvironmentVariables: ['API_KEY'], - enableEnvironmentVariableRedaction: true, + describe('prepareCommand', () => { + it('should correctly format the base command and args', async () => { + const req: SandboxRequest = { + command: 'whoami', + args: ['/groups'], + cwd: testCwd, + env: { TEST_VAR: 'test_value' }, + policy: { + networkAccess: false, }, - }, - }; + }; - const result = await manager.prepareCommand(req); - expect(result.env['PATH']).toBe('/usr/bin'); - expect(result.env['API_KEY']).toBeUndefined(); - }); + const result = await manager.prepareCommand(req); - it('should ensure governance files exist', async () => { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - }; + expect(result.program).toContain('GeminiSandbox.exe'); + expect(result.args).toEqual(['0', testCwd, 'whoami', '/groups']); + }); - await manager.prepareCommand(req); + it('should correctly pass through the cwd to the resulting command', async () => { + const req: SandboxRequest = { + command: 'whoami', + args: [], + cwd: '/different/cwd', + env: {}, + }; - expect(fs.existsSync(path.join(testCwd, '.gitignore'))).toBe(true); - expect(fs.existsSync(path.join(testCwd, '.geminiignore'))).toBe(true); - expect(fs.existsSync(path.join(testCwd, '.git'))).toBe(true); - expect(fs.lstatSync(path.join(testCwd, '.git')).isDirectory()).toBe(true); - }); + const result = await manager.prepareCommand(req); - it('should grant Low Integrity access to the workspace and allowed paths', async () => { - const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); - if (!fs.existsSync(allowedPath)) { - fs.mkdirSync(allowedPath); - } - try { + expect(result.cwd).toBe('/different/cwd'); + }); + + it('should apply environment sanitization via the default mechanisms', async () => { const req: SandboxRequest = { command: 'test', args: [], cwd: testCwd, - env: {}, + env: { + API_KEY: 'secret', + PATH: '/usr/bin', + }, policy: { - allowedPaths: [allowedPath], + sanitizationConfig: { + allowedEnvironmentVariables: ['PATH'], + blockedEnvironmentVariables: ['API_KEY'], + enableEnvironmentVariableRedaction: true, + }, }, }; - await manager.prepareCommand(req); + const result = await manager.prepareCommand(req); + expect(result.env['PATH']).toBe('/usr/bin'); + expect(result.env['API_KEY']).toBeUndefined(); + }); - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(testCwd), - '/setintegritylevel', - 'Low', - ]); - - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(allowedPath), - '/setintegritylevel', - 'Low', - ]); - } finally { - fs.rmSync(allowedPath, { recursive: true, force: true }); - } - }); - - it('skips denying access to non-existent forbidden paths to prevent icacls failure', async () => { - const missingPath = path.join( - os.tmpdir(), - 'gemini-cli-test-missing', - 'does-not-exist.txt', - ); - - // Ensure it definitely doesn't exist - if (fs.existsSync(missingPath)) { - fs.rmSync(missingPath, { recursive: true, force: true }); - } - - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - forbiddenPaths: [missingPath], - }, - }; - - await manager.prepareCommand(req); - - // Should NOT have called icacls to deny the missing path - expect(spawnAsync).not.toHaveBeenCalledWith('icacls', [ - path.resolve(missingPath), - '/deny', - '*S-1-16-4096:(OI)(CI)(F)', - ]); - }); - - it('should deny Low Integrity access to forbidden paths', async () => { - const forbiddenPath = path.join(os.tmpdir(), 'gemini-cli-test-forbidden'); - if (!fs.existsSync(forbiddenPath)) { - fs.mkdirSync(forbiddenPath); - } - try { + it('should allow network when networkAccess is true', async () => { const req: SandboxRequest = { - command: 'test', + command: 'whoami', args: [], cwd: testCwd, env: {}, policy: { - forbiddenPaths: [forbiddenPath], + networkAccess: true, }, }; - await manager.prepareCommand(req); + const result = await manager.prepareCommand(req); + expect(result.args[0]).toBe('1'); + }); - expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(forbiddenPath), - '/deny', - '*S-1-16-4096:(OI)(CI)(F)', - ]); - } finally { - fs.rmSync(forbiddenPath, { recursive: true, force: true }); - } - }); + describe('governance files', () => { + it('should ensure governance files exist', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + }; - it('should override allowed paths if a path is also in forbidden paths', async () => { - const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); - if (!fs.existsSync(conflictPath)) { - fs.mkdirSync(conflictPath); - } - try { - const req: SandboxRequest = { - command: 'test', - args: [], - cwd: testCwd, - env: {}, - policy: { - allowedPaths: [conflictPath], - forbiddenPaths: [conflictPath], - }, - }; + await manager.prepareCommand(req); - await manager.prepareCommand(req); + expect(fs.existsSync(path.join(testCwd, '.gitignore'))).toBe(true); + expect(fs.existsSync(path.join(testCwd, '.geminiignore'))).toBe(true); + expect(fs.existsSync(path.join(testCwd, '.git'))).toBe(true); + expect(fs.lstatSync(path.join(testCwd, '.git')).isDirectory()).toBe( + true, + ); + }); + }); - const spawnMock = vi.mocked(spawnAsync); - const allowCallIndex = spawnMock.mock.calls.findIndex( - (call) => - call[1] && - call[1].includes('/setintegritylevel') && - call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), - ); - const denyCallIndex = spawnMock.mock.calls.findIndex( - (call) => - call[1] && - call[1].includes('/deny') && - call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), - ); + describe('allowedPaths', () => { + it('should parameterize allowed paths and normalize them', async () => { + const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); + if (!fs.existsSync(allowedPath)) { + fs.mkdirSync(allowedPath); + } + try { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + allowedPaths: [allowedPath], + }, + }; - // Both should have been called - expect(allowCallIndex).toBeGreaterThan(-1); - expect(denyCallIndex).toBeGreaterThan(-1); + await manager.prepareCommand(req); - // Verify order: explicitly denying must happen after the explicit allow - expect(allowCallIndex).toBeLessThan(denyCallIndex); - } finally { - fs.rmSync(conflictPath, { recursive: true, force: true }); - } + expect(spawnAsync).toHaveBeenCalledWith('icacls', [ + path.resolve(testCwd), + '/setintegritylevel', + 'Low', + ]); + + expect(spawnAsync).toHaveBeenCalledWith('icacls', [ + path.resolve(allowedPath), + '/setintegritylevel', + 'Low', + ]); + } finally { + fs.rmSync(allowedPath, { recursive: true, force: true }); + } + }); + }); + + describe('forbiddenPaths', () => { + it('should parameterize forbidden paths and explicitly deny them', async () => { + const forbiddenPath = path.join( + os.tmpdir(), + 'gemini-cli-test-forbidden', + ); + if (!fs.existsSync(forbiddenPath)) { + fs.mkdirSync(forbiddenPath); + } + try { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + forbiddenPaths: [forbiddenPath], + }, + }; + + await manager.prepareCommand(req); + + expect(spawnAsync).toHaveBeenCalledWith('icacls', [ + path.resolve(forbiddenPath), + '/deny', + '*S-1-16-4096:(OI)(CI)(F)', + ]); + } finally { + fs.rmSync(forbiddenPath, { recursive: true, force: true }); + } + }); + + it('explicitly denies non-existent forbidden paths to prevent creation', async () => { + const missingPath = path.join( + os.tmpdir(), + 'gemini-cli-test-missing', + 'does-not-exist.txt', + ); + + // Ensure it definitely doesn't exist + if (fs.existsSync(missingPath)) { + fs.rmSync(missingPath, { recursive: true, force: true }); + } + + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + forbiddenPaths: [missingPath], + }, + }; + + await manager.prepareCommand(req); + + // Should NOT have called icacls to deny the missing path + expect(spawnAsync).not.toHaveBeenCalledWith('icacls', [ + path.resolve(missingPath), + '/deny', + '*S-1-16-4096:(OI)(CI)(F)', + ]); + }); + + it('should override allowed paths if a path is also in forbidden paths', async () => { + const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); + if (!fs.existsSync(conflictPath)) { + fs.mkdirSync(conflictPath); + } + try { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + policy: { + allowedPaths: [conflictPath], + forbiddenPaths: [conflictPath], + }, + }; + + await manager.prepareCommand(req); + + const spawnMock = vi.mocked(spawnAsync); + const allowCallIndex = spawnMock.mock.calls.findIndex( + (call) => + call[1] && + call[1].includes('/setintegritylevel') && + call[0] === 'icacls' && + call[1][0] === path.resolve(conflictPath), + ); + const denyCallIndex = spawnMock.mock.calls.findIndex( + (call) => + call[1] && + call[1].includes('/deny') && + call[0] === 'icacls' && + call[1][0] === path.resolve(conflictPath), + ); + + // Both should have been called + expect(allowCallIndex).toBeGreaterThan(-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 1ca027d018..0a1bc2a95f 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -231,6 +231,7 @@ export class WindowsSandboxManager implements SandboxManager { program, args, env: sanitizedEnv, + cwd: req.cwd, }; }