test(core): improve sandbox integration test coverage and fix OS-specific failures (#25307)

Co-authored-by: David Pierce <davidapierce@google.com>

packages/core/src/sandbox/linux/LinuxSandboxManager.ts

@@ -37,6 +37,7 @@ import {
   createSandboxDenialCache,
   type SandboxDenialCache,
 } from '../utils/sandboxDenialUtils.js';
+import { isErrnoException } from '../utils/fsUtils.js';
 import { handleReadWriteCommands } from '../utils/sandboxReadWriteUtils.js';
 import { buildBwrapArgs } from './bwrapArgsBuilder.js';
 
@@ -116,9 +117,12 @@ function touch(filePath: string, isDirectory: boolean) {
   assertValidPathString(filePath);
   try {
     // If it exists (even as a broken symlink), do nothing
-    if (fs.lstatSync(filePath)) return;
-  } catch {
-    // Ignore ENOENT
+    fs.lstatSync(filePath);
+    return;
+  } catch (e: unknown) {
+    if (isErrnoException(e) && e.code !== 'ENOENT') {
+      throw e;
+    }
   }
 
   if (isDirectory) {
@@ -136,9 +140,22 @@ function touch(filePath: string, isDirectory: boolean) {
 export class LinuxSandboxManager implements SandboxManager {
   private static maskFilePath: string | undefined;
   private readonly denialCache: SandboxDenialCache = createSandboxDenialCache();
+  private governanceFilesInitialized = false;
 
   constructor(private readonly options: GlobalSandboxOptions) {}
 
+  private ensureGovernanceFilesExist(workspace: string): void {
+    if (this.governanceFilesInitialized) return;
+
+    // These must exist on the host before running the sandbox to ensure they are protected.
+    for (const file of GOVERNANCE_FILES) {
+      const filePath = join(workspace, file.path);
+      touch(filePath, file.isDirectory);
+    }
+
+    this.governanceFilesInitialized = true;
+  }
+
   isKnownSafeCommand(args: string[]): boolean {
     return isKnownSafeCommand(args);
   }
@@ -258,17 +275,14 @@ export class LinuxSandboxManager implements SandboxManager {
       mergedAdditional,
     );
 
-    for (const file of GOVERNANCE_FILES) {
-      const filePath = join(this.options.workspace, file.path);
-      touch(filePath, file.isDirectory);
-    }
+    this.ensureGovernanceFilesExist(resolvedPaths.workspace.resolved);
 
     const bwrapArgs = await buildBwrapArgs({
       resolvedPaths,
       workspaceWrite,
       networkAccess: mergedAdditional.network ?? false,
       maskFilePath: this.getMaskFilePath(),
-      isWriteCommand: req.command === '__write',
+      isReadOnlyCommand: req.command === '__read',
     });
 
     const bpfPath = getSeccompBpfPath();

packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts

@@ -92,7 +92,7 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => {
     workspaceWrite: false,
     networkAccess: false,
     maskFilePath: '/tmp/mask',
-    isWriteCommand: false,
+    isReadOnlyCommand: false,
   };
 
   it('should correctly format the base arguments', async () => {
@@ -188,7 +188,7 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => {
     expect(args[args.indexOf('/opt/tools') - 1]).toBe('--bind-try');
   });
 
-  it('should bind the parent directory of a non-existent path', async () => {
+  it('should bind the parent directory of a non-existent path with --bind-try when isReadOnlyCommand is false', async () => {
     vi.mocked(fs.existsSync).mockImplementation((p) => {
       if (p === '/home/user/workspace/new-file.txt') return false;
       return true;
@@ -196,10 +196,10 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => {
 
     const args = await buildBwrapArgs({
       ...defaultOptions,
+      isReadOnlyCommand: false,
       resolvedPaths: createResolvedPaths({
         policyAllowed: ['/home/user/workspace/new-file.txt'],
       }),
-      isWriteCommand: true,
     });
 
     const parentDir = '/home/user/workspace';
@@ -208,6 +208,26 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => {
     expect(args[bindIndex - 2]).toBe('--bind-try');
   });
 
+  it('should bind the parent directory of a non-existent path with --ro-bind-try when isReadOnlyCommand is true', async () => {
+    vi.mocked(fs.existsSync).mockImplementation((p) => {
+      if (p === '/home/user/workspace/new-file.txt') return false;
+      return true;
+    });
+
+    const args = await buildBwrapArgs({
+      ...defaultOptions,
+      isReadOnlyCommand: true,
+      resolvedPaths: createResolvedPaths({
+        policyAllowed: ['/home/user/workspace/new-file.txt'],
+      }),
+    });
+
+    const parentDir = '/home/user/workspace';
+    const bindIndex = args.lastIndexOf(parentDir);
+    expect(bindIndex).not.toBe(-1);
+    expect(args[bindIndex - 2]).toBe('--ro-bind-try');
+  });
+
   it('should parameterize forbidden paths and explicitly deny them', async () => {
     vi.mocked(fs.statSync).mockImplementation((p) => {
       if (p.toString().includes('cache')) {

packages/core/src/sandbox/linux/bwrapArgsBuilder.ts

@@ -23,7 +23,7 @@ export interface BwrapArgsOptions {
   workspaceWrite: boolean;
   networkAccess: boolean;
   maskFilePath: string;
-  isWriteCommand: boolean;
+  isReadOnlyCommand: boolean;
 }
 
 /**
@@ -37,7 +37,7 @@ export async function buildBwrapArgs(
     workspaceWrite,
     networkAccess,
     maskFilePath,
-    isWriteCommand,
+    isReadOnlyCommand,
   } = options;
   const { workspace } = resolvedPaths;
 
@@ -79,10 +79,13 @@ export async function buildBwrapArgs(
       bwrapArgs.push('--bind-try', allowedPath, allowedPath);
     } else {
       // If the path doesn't exist, we still want to allow access to its parent
-      // to enable creating it. Since allowedPath is already resolved by resolveSandboxPaths,
-      // its parent is also correctly resolved.
+      // to enable creating it.
       const parent = dirname(allowedPath);
-      bwrapArgs.push(isWriteCommand ? '--bind-try' : bindFlag, parent, parent);
+      bwrapArgs.push(
+        isReadOnlyCommand ? '--ro-bind-try' : '--bind-try',
+        parent,
+        parent,
+      );
     }
   }
 

packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts

@@ -5,7 +5,7 @@
  */
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 import { MacOsSandboxManager } from './MacOsSandboxManager.js';
-import type { ExecutionPolicy } from '../../services/sandboxManager.js';
+import { type ExecutionPolicy } from '../../services/sandboxManager.js';
 import * as seatbeltArgsBuilder from './seatbeltArgsBuilder.js';
 import fs from 'node:fs';
 import os from 'node:os';
@@ -191,28 +191,6 @@ describe('MacOsSandboxManager', () => {
       });
     });
 
-    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.buildSeatbeltProfile).toHaveBeenCalledWith(
-          expect.objectContaining({
-            resolvedPaths: expect.objectContaining({
-              workspace: { resolved: mockWorkspace, original: mockWorkspace },
-            }),
-          }),
-        );
-      });
-    });
-
     describe('allowedPaths', () => {
       it('should parameterize allowed paths and normalize them', async () => {
         await manager.prepareCommand({

packages/core/src/sandbox/windows/WindowsSandboxManager.ts

@@ -5,7 +5,7 @@
  */
 
 import fs from 'node:fs';
-import path from 'node:path';
+import path, { join } from 'node:path';
 import os from 'node:os';
 import { fileURLToPath } from 'node:url';
 import {
@@ -33,6 +33,7 @@ import {
 } from './commandSafety.js';
 import { verifySandboxOverrides } from '../utils/commandUtils.js';
 import { parseWindowsSandboxDenials } from './windowsSandboxDenialUtils.js';
+import { isErrnoException } from '../utils/fsUtils.js';
 import {
   isSubpath,
   resolveToRealPath,
@@ -53,10 +54,13 @@ const __dirname = path.dirname(__filename);
  */
 export class WindowsSandboxManager implements SandboxManager {
   static readonly HELPER_EXE = 'GeminiSandbox.exe';
+
   private readonly helperPath: string;
-  private initialized = false;
   private readonly denialCache: SandboxDenialCache = createSandboxDenialCache();
 
+  private static helperCompiled = false;
+  private governanceFilesInitialized = false;
+
   constructor(private readonly options: GlobalSandboxOptions) {
     this.helperPath = path.resolve(__dirname, WindowsSandboxManager.HELPER_EXE);
   }
@@ -86,33 +90,20 @@ export class WindowsSandboxManager implements SandboxManager {
     return this.options;
   }
 
-  /**
-   * Ensures a file or directory exists.
-   */
-  private touch(filePath: string, isDirectory: boolean): void {
-    assertValidPathString(filePath);
-    try {
-      // If it exists (even as a broken symlink), do nothing
-      if (fs.lstatSync(filePath)) return;
-    } catch {
-      // Ignore ENOENT
+  private ensureGovernanceFilesExist(workspace: string): void {
+    if (this.governanceFilesInitialized) return;
+
+    // These must exist on the host before running the sandbox to ensure they are protected.
+    for (const file of GOVERNANCE_FILES) {
+      const filePath = join(workspace, file.path);
+      touch(filePath, file.isDirectory);
     }
 
-    if (isDirectory) {
-      fs.mkdirSync(filePath, { recursive: true });
-    } else {
-      const dir = path.dirname(filePath);
-      if (!fs.existsSync(dir)) {
-        fs.mkdirSync(dir, { recursive: true });
-      }
-      fs.closeSync(fs.openSync(filePath, 'a'));
-    }
+    this.governanceFilesInitialized = true;
   }
 
-  private async ensureInitialized(): Promise<void> {
-    if (this.initialized) return;
-    if (os.platform() !== 'win32') {
-      this.initialized = true;
+  private async ensureHelperCompiled(): Promise<void> {
+    if (WindowsSandboxManager.helperCompiled || os.platform() !== 'win32') {
       return;
     }
 
@@ -207,14 +198,14 @@ export class WindowsSandboxManager implements SandboxManager {
       );
     }
 
-    this.initialized = true;
+    WindowsSandboxManager.helperCompiled = true;
   }
 
   /**
    * Prepares a command for sandboxed execution on Windows.
    */
   async prepareCommand(req: SandboxRequest): Promise<SandboxedCommand> {
-    await this.ensureInitialized();
+    await this.ensureHelperCompiled();
 
     const sanitizationConfig = getSecureSanitizationConfig(
       req.policy?.sanitizationConfig,
@@ -276,6 +267,8 @@ export class WindowsSandboxManager implements SandboxManager {
       mergedAdditional,
     );
 
+    this.ensureGovernanceFilesExist(resolvedPaths.workspace.resolved);
+
     // 1. Collect all forbidden paths.
     // We start with explicitly forbidden paths from the options and request.
     const forbiddenManifest = new Set(
@@ -402,14 +395,6 @@ export class WindowsSandboxManager implements SandboxManager {
       // No-op for read access on Windows.
     }
 
-    // 4. Protected governance files
-    // These must exist on the host before running the sandbox to prevent
-    // the sandboxed process from creating them with Low integrity.
-    for (const file of GOVERNANCE_FILES) {
-      const filePath = path.join(resolvedPaths.workspace.resolved, file.path);
-      this.touch(filePath, file.isDirectory);
-    }
-
     // 5. Generate Manifests
     const tempDir = await fs.promises.mkdtemp(
       path.join(os.tmpdir(), 'gemini-cli-sandbox-'),
@@ -471,3 +456,29 @@ export class WindowsSandboxManager implements SandboxManager {
     );
   }
 }
+
+/**
+ * Ensures a file or directory exists.
+ */
+function touch(filePath: string, isDirectory: boolean): void {
+  assertValidPathString(filePath);
+  try {
+    // If it exists (even as a broken symlink), do nothing
+    fs.lstatSync(filePath);
+    return;
+  } catch (e: unknown) {
+    if (isErrnoException(e) && e.code !== 'ENOENT') {
+      throw e;
+    }
+  }
+
+  if (isDirectory) {
+    fs.mkdirSync(filePath, { recursive: true });
+  } else {
+    const dir = path.dirname(filePath);
+    if (!fs.existsSync(dir)) {
+      fs.mkdirSync(dir, { recursive: true });
+    }
+    fs.closeSync(fs.openSync(filePath, 'a'));
+  }
+}