fix(sandbox): centralize async git worktree resolution and enforce read-only security (#25040)

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

@@ -339,4 +339,61 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => {
     const envIndex = args.indexOf(`${includeDir}/.env`);
     expect(args[envIndex - 2]).toBe('--bind');
   });
+
+  it('binds git worktree directories if present', async () => {
+    const worktreeGitDir = '/path/to/worktree/.git';
+    const mainGitDir = '/path/to/main/.git';
+
+    const args = await buildBwrapArgs({
+      ...defaultOptions,
+      resolvedPaths: createResolvedPaths({
+        gitWorktree: {
+          worktreeGitDir,
+          mainGitDir,
+        },
+      }),
+    });
+
+    expect(args).toContain(worktreeGitDir);
+    expect(args).toContain(mainGitDir);
+    expect(args[args.indexOf(worktreeGitDir) - 1]).toBe('--ro-bind-try');
+    expect(args[args.indexOf(mainGitDir) - 1]).toBe('--ro-bind-try');
+  });
+
+  it('enforces read-only binding for git worktrees even if workspaceWrite is true', async () => {
+    const worktreeGitDir = '/path/to/worktree/.git';
+
+    const args = await buildBwrapArgs({
+      ...defaultOptions,
+      workspaceWrite: true,
+      resolvedPaths: createResolvedPaths({
+        gitWorktree: {
+          worktreeGitDir,
+        },
+      }),
+    });
+
+    expect(args[args.indexOf(worktreeGitDir) - 1]).toBe('--ro-bind-try');
+  });
+
+  it('git worktree read-only bindings should override previous policyWrite bindings', async () => {
+    const worktreeGitDir = '/custom/worktree/.git';
+
+    const args = await buildBwrapArgs({
+      ...defaultOptions,
+      resolvedPaths: createResolvedPaths({
+        policyWrite: ['/custom/worktree'],
+        gitWorktree: {
+          worktreeGitDir,
+        },
+      }),
+    });
+
+    const writeBindIndex = args.indexOf('/custom/worktree');
+    const worktreeBindIndex = args.lastIndexOf(worktreeGitDir);
+
+    expect(writeBindIndex).toBeGreaterThan(-1);
+    expect(worktreeBindIndex).toBeGreaterThan(-1);
+    expect(worktreeBindIndex).toBeGreaterThan(writeBindIndex);
+  });
 });

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

@@ -11,7 +11,7 @@ import {
   getSecretFileFindArgs,
   type ResolvedSandboxPaths,
 } from '../../services/sandboxManager.js';
-import { resolveGitWorktreePaths, isErrnoException } from '../utils/fsUtils.js';
+import { isErrnoException } from '../utils/fsUtils.js';
 import { spawnAsync } from '../../utils/shell-utils.js';
 import { debugLogger } from '../../utils/debugLogger.js';
 
@@ -70,16 +70,6 @@ export async function buildBwrapArgs(
     bwrapArgs.push(bindFlag, workspace.resolved, workspace.resolved);
   }
 
-  const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths(
-    workspace.resolved,
-  );
-  if (worktreeGitDir) {
-    bwrapArgs.push(bindFlag, worktreeGitDir, worktreeGitDir);
-  }
-  if (mainGitDir) {
-    bwrapArgs.push(bindFlag, mainGitDir, mainGitDir);
-  }
-
   for (const includeDir of resolvedPaths.globalIncludes) {
     bwrapArgs.push('--ro-bind-try', includeDir, includeDir);
   }
@@ -113,6 +103,18 @@ export async function buildBwrapArgs(
     }
   }
 
+  // Grant read-only access to git worktrees/submodules. We do this last in order to
+  // ensure that these rules aren't overwritten by broader write policies.
+  if (resolvedPaths.gitWorktree) {
+    const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree;
+    if (worktreeGitDir) {
+      bwrapArgs.push('--ro-bind-try', worktreeGitDir, worktreeGitDir);
+    }
+    if (mainGitDir) {
+      bwrapArgs.push('--ro-bind-try', mainGitDir, mainGitDir);
+    }
+  }
+
   for (const p of resolvedPaths.forbidden) {
     if (!fs.existsSync(p)) continue;
     try {

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

@@ -142,5 +142,62 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => {
         expect(denyIndex).toBeGreaterThan(allowIndex);
       });
     });
+
+    describe('git worktree paths', () => {
+      it('enforces read-only binding for git worktrees even if workspaceWrite is true', () => {
+        const worktreeGitDir = '/path/to/worktree/.git';
+        const mainGitDir = '/path/to/main/.git';
+
+        const profile = buildSeatbeltProfile({
+          resolvedPaths: {
+            ...defaultResolvedPaths,
+            gitWorktree: {
+              worktreeGitDir,
+              mainGitDir,
+            },
+          },
+          workspaceWrite: true,
+        });
+
+        // Should grant read access
+        expect(profile).toContain(
+          `(allow file-read* (subpath "${worktreeGitDir}"))`,
+        );
+        expect(profile).toContain(
+          `(allow file-read* (subpath "${mainGitDir}"))`,
+        );
+
+        // Should NOT grant write access
+        expect(profile).not.toContain(
+          `(allow file-read* file-write* (subpath "${worktreeGitDir}"))`,
+        );
+        expect(profile).not.toContain(
+          `(allow file-read* file-write* (subpath "${mainGitDir}"))`,
+        );
+      });
+
+      it('git worktree read-only rules should override previous policyAllowed write paths', () => {
+        const worktreeGitDir = '/custom/worktree/.git';
+        const profile = buildSeatbeltProfile({
+          resolvedPaths: {
+            ...defaultResolvedPaths,
+            policyAllowed: ['/custom/worktree'],
+            gitWorktree: {
+              worktreeGitDir,
+            },
+          },
+        });
+
+        const allowString = `(allow file-read* file-write* (subpath "/custom/worktree"))`;
+        const denyString = `(deny file-write* (subpath "${worktreeGitDir}"))`;
+
+        expect(profile).toContain(allowString);
+        expect(profile).toContain(denyString);
+
+        const allowIndex = profile.indexOf(allowString);
+        const denyIndex = profile.indexOf(denyString);
+        expect(denyIndex).toBeGreaterThan(allowIndex);
+      });
+    });
   });
 });

packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts

@@ -16,7 +16,7 @@ import {
   SECRET_FILES,
   type ResolvedSandboxPaths,
 } from '../../services/sandboxManager.js';
-import { tryRealpath, resolveGitWorktreePaths } from '../utils/fsUtils.js';
+import { resolveToRealPath } from '../../utils/paths.js';
 
 /**
  * Options for building macOS Seatbelt profile.
@@ -52,21 +52,21 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string {
     profile += `(allow file-write* (subpath "${escapeSchemeString(resolvedPaths.workspace.resolved)}"))\n`;
   }
 
-  const tmpPath = tryRealpath(os.tmpdir());
+  const tmpPath = resolveToRealPath(os.tmpdir());
   profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(tmpPath)}"))\n`;
 
-  // Auto-detect and support git worktrees by granting read and write access to the underlying git directory
-  const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths(
-    resolvedPaths.workspace.resolved,
-  );
-  if (worktreeGitDir) {
-    profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`;
-  }
-  if (mainGitDir) {
-    profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(mainGitDir)}"))\n`;
+  // Support git worktrees/submodules; read-only to prevent malicious hook/config modification (RCE).
+  if (resolvedPaths.gitWorktree) {
+    const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree;
+    if (worktreeGitDir) {
+      profile += `(allow file-read* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`;
+    }
+    if (mainGitDir) {
+      profile += `(allow file-read* (subpath "${escapeSchemeString(mainGitDir)}"))\n`;
+    }
   }
 
-  const nodeRootPath = tryRealpath(
+  const nodeRootPath = resolveToRealPath(
     path.dirname(path.dirname(process.execPath)),
   );
   profile += `(allow file-read* (subpath "${escapeSchemeString(nodeRootPath)}"))\n`;
@@ -79,7 +79,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string {
     for (const p of paths) {
       if (!p.trim()) continue;
       try {
-        let resolved = tryRealpath(p);
+        let resolved = resolveToRealPath(p);
 
         // If this is a 'bin' directory (like /usr/local/bin or homebrew/bin),
         // also grant read access to its parent directory so that symlinked
@@ -148,7 +148,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string {
       resolvedPaths.workspace.resolved,
       GOVERNANCE_FILES[i].path,
     );
-    const realGovernanceFile = tryRealpath(governanceFile);
+    const realGovernanceFile = resolveToRealPath(governanceFile);
 
     // Determine if it should be treated as a directory (subpath) or a file (literal).
     // .git is generally a directory, while ignore files are literals.
@@ -170,6 +170,18 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string {
     }
   }
 
+  // Grant read-only access to git worktrees/submodules. We do this last in order to
+  // ensure that these rules aren't overwritten by broader write policies.
+  if (resolvedPaths.gitWorktree) {
+    const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree;
+    if (worktreeGitDir) {
+      profile += `(deny file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`;
+    }
+    if (mainGitDir) {
+      profile += `(deny file-write* (subpath "${escapeSchemeString(mainGitDir)}"))\n`;
+    }
+  }
+
   // Add explicit deny rules for secret files (.env, .env.*) in the workspace and allowed paths.
   // We use regex rules to avoid expensive file discovery scans.
   // Anchoring to workspace/allowed paths to avoid over-blocking.

packages/core/src/sandbox/utils/fsUtils.test.ts

@@ -4,49 +4,117 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { describe, it, expect, beforeAll, afterAll } from 'vitest';
-import fs from 'node:fs';
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+import fsPromises from 'node:fs/promises';
 import path from 'node:path';
-import os from 'node:os';
-import { tryRealpath } from './fsUtils.js';
+import { resolveGitWorktreePaths } from './fsUtils.js';
+
+vi.mock('node:fs/promises', async () => {
+  const actual =
+    await vi.importActual<typeof import('node:fs/promises')>(
+      'node:fs/promises',
+    );
+  return {
+    ...actual,
+    default: {
+      ...actual,
+      lstat: vi.fn(),
+      readFile: vi.fn(),
+    },
+    lstat: vi.fn(),
+    readFile: vi.fn(),
+  };
+});
+
+vi.mock('../../utils/paths.js', async () => {
+  const actual = await vi.importActual<typeof import('../../utils/paths.js')>(
+    '../../utils/paths.js',
+  );
+  return {
+    ...actual,
+    resolveToRealPath: vi.fn((p) => p),
+  };
+});
 
 describe('fsUtils', () => {
-  let tempDir: string;
-  let realTempDir: string;
+  describe('resolveGitWorktreePaths', () => {
+    const workspace = path.resolve('/workspace');
+    const gitPath = path.join(workspace, '.git');
 
-  beforeAll(() => {
-    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-utils-test-'));
-    realTempDir = fs.realpathSync(tempDir);
-  });
+    beforeEach(() => {
+      vi.clearAllMocks();
+    });
 
-  afterAll(() => {
-    fs.rmSync(tempDir, { recursive: true, force: true });
-  });
-
-  describe('tryRealpath', () => {
-    it('should throw error for paths with null bytes', () => {
-      expect(() => tryRealpath(path.join(tempDir, 'foo\0bar'))).toThrow(
-        'Invalid path',
+    it('should return empty if .git does not exist', async () => {
+      vi.mocked(fsPromises.lstat).mockRejectedValue(
+        Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) as never,
       );
+      const result = await resolveGitWorktreePaths(workspace);
+      expect(result).toEqual({});
     });
 
-    it('should resolve existing paths', () => {
-      const resolved = tryRealpath(tempDir);
-      expect(resolved).toBe(realTempDir);
+    it('should return empty if .git is a directory', async () => {
+      vi.mocked(fsPromises.lstat).mockResolvedValue({
+        isFile: () => false,
+      } as never);
+      const result = await resolveGitWorktreePaths(workspace);
+      expect(result).toEqual({});
     });
 
-    it('should handle non-existent paths by resolving parent', () => {
-      const nonExistentPath = path.join(tempDir, 'non-existent-file-12345');
-      const expected = path.join(realTempDir, 'non-existent-file-12345');
-      const resolved = tryRealpath(nonExistentPath);
-      expect(resolved).toBe(expected);
+    it('should resolve worktree paths from .git file', async () => {
+      const mainGitDir = path.resolve('/project/.git');
+      const worktreeGitDir = path.join(mainGitDir, 'worktrees', 'feature');
+
+      vi.mocked(fsPromises.lstat).mockResolvedValue({
+        isFile: () => true,
+      } as never);
+      vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => {
+        if (p === gitPath) return Promise.resolve(`gitdir: ${worktreeGitDir}`);
+        if (p === path.join(worktreeGitDir, 'gitdir'))
+          return Promise.resolve(gitPath);
+        return Promise.reject(new Error('ENOENT'));
+      }) as never);
+
+      const result = await resolveGitWorktreePaths(workspace);
+      expect(result).toEqual({
+        worktreeGitDir,
+        mainGitDir,
+      });
     });
 
-    it('should handle nested non-existent paths', () => {
-      const nonExistentPath = path.join(tempDir, 'dir1', 'dir2', 'file');
-      const expected = path.join(realTempDir, 'dir1', 'dir2', 'file');
-      const resolved = tryRealpath(nonExistentPath);
-      expect(resolved).toBe(expected);
+    it('should reject worktree if backlink is missing or invalid', async () => {
+      const worktreeGitDir = path.resolve('/git/worktrees/feature');
+
+      vi.mocked(fsPromises.lstat).mockResolvedValue({
+        isFile: () => true,
+      } as never);
+      vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => {
+        if (p === gitPath) return Promise.resolve(`gitdir: ${worktreeGitDir}`);
+        return Promise.reject(new Error('ENOENT'));
+      }) as never);
+
+      const result = await resolveGitWorktreePaths(workspace);
+      expect(result).toEqual({});
+    });
+
+    it('should support submodules via config check', async () => {
+      const submoduleGitDir = path.resolve('/project/.git/modules/sub');
+
+      vi.mocked(fsPromises.lstat).mockResolvedValue({
+        isFile: () => true,
+      } as never);
+      vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => {
+        if (p === gitPath) return Promise.resolve(`gitdir: ${submoduleGitDir}`);
+        if (p === path.join(submoduleGitDir, 'config'))
+          return Promise.resolve(`[core]\n\tworktree = ${workspace}`);
+        return Promise.reject(new Error('ENOENT'));
+      }) as never);
+
+      const result = await resolveGitWorktreePaths(workspace);
+      expect(result).toEqual({
+        worktreeGitDir: submoduleGitDir,
+        mainGitDir: path.resolve('/project/.git'),
+      });
     });
   });
 });

packages/core/src/sandbox/utils/fsUtils.ts

@@ -4,68 +4,55 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import fs from 'node:fs';
+import fs from 'node:fs/promises';
 import path from 'node:path';
-import { assertValidPathString } from '../../utils/paths.js';
+import { resolveToRealPath } from '../../utils/paths.js';
 
 export function isErrnoException(e: unknown): e is NodeJS.ErrnoException {
   return e instanceof Error && 'code' in e;
 }
 
-export function tryRealpath(p: string): string {
-  assertValidPathString(p);
-  try {
-    return fs.realpathSync(p);
-  } catch (e) {
-    if (isErrnoException(e) && e.code === 'ENOENT') {
-      const parentDir = path.dirname(p);
-      if (parentDir === p) {
-        return p;
-      }
-      return path.join(tryRealpath(parentDir), path.basename(p));
-    }
-    throw e;
-  }
-}
-
-export function resolveGitWorktreePaths(workspacePath: string): {
+export async function resolveGitWorktreePaths(workspacePath: string): Promise<{
   worktreeGitDir?: string;
   mainGitDir?: string;
-} {
+}> {
   try {
     const gitPath = path.join(workspacePath, '.git');
-    const gitStat = fs.lstatSync(gitPath);
+    const gitStat = await fs.lstat(gitPath);
     if (gitStat.isFile()) {
-      const gitContent = fs.readFileSync(gitPath, 'utf8');
+      const gitContent = await fs.readFile(gitPath, 'utf8');
       const match = gitContent.match(/^gitdir:\s+(.+)$/m);
       if (match && match[1]) {
         let worktreeGitDir = match[1].trim();
         if (!path.isAbsolute(worktreeGitDir)) {
           worktreeGitDir = path.resolve(workspacePath, worktreeGitDir);
         }
-        const resolvedWorktreeGitDir = tryRealpath(worktreeGitDir);
+        const resolvedWorktreeGitDir = resolveToRealPath(worktreeGitDir);
 
         // Security check: Verify the bidirectional link to prevent sandbox escape
         let isValid = false;
         try {
           const backlinkPath = path.join(resolvedWorktreeGitDir, 'gitdir');
-          const backlink = fs.readFileSync(backlinkPath, 'utf8').trim();
+          const backlink = (await fs.readFile(backlinkPath, 'utf8')).trim();
           // The backlink must resolve to the workspace's .git file
-          if (tryRealpath(backlink) === tryRealpath(gitPath)) {
+          if (resolveToRealPath(backlink) === resolveToRealPath(gitPath)) {
             isValid = true;
           }
         } catch {
           // Fallback for submodules: check core.worktree in config
           try {
             const configPath = path.join(resolvedWorktreeGitDir, 'config');
-            const config = fs.readFileSync(configPath, 'utf8');
+            const config = await fs.readFile(configPath, 'utf8');
             const match = config.match(/^\s*worktree\s*=\s*(.+)$/m);
             if (match && match[1]) {
               const worktreePath = path.resolve(
                 resolvedWorktreeGitDir,
                 match[1].trim(),
               );
-              if (tryRealpath(worktreePath) === tryRealpath(workspacePath)) {
+              if (
+                resolveToRealPath(worktreePath) ===
+                resolveToRealPath(workspacePath)
+              ) {
                 isValid = true;
               }
             }
@@ -78,7 +65,7 @@ export function resolveGitWorktreePaths(workspacePath: string): {
           return {}; // Reject: valid worktrees/submodules must have a readable backlink
         }
 
-        const mainGitDir = tryRealpath(
+        const mainGitDir = resolveToRealPath(
           path.dirname(path.dirname(resolvedWorktreeGitDir)),
         );
         return {

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

@@ -10,6 +10,7 @@ import os from 'node:os';
 import path from 'node:path';
 import { WindowsSandboxManager } from './WindowsSandboxManager.js';
 import * as sandboxManager from '../../services/sandboxManager.js';
+import * as paths from '../../utils/paths.js';
 import type { SandboxRequest } from '../../services/sandboxManager.js';
 import { spawnAsync } from '../../utils/shell-utils.js';
 import type { SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js';
@@ -44,9 +45,7 @@ describe('WindowsSandboxManager', () => {
 
   beforeEach(() => {
     vi.spyOn(os, 'platform').mockReturnValue('win32');
-    vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) =>
-      p.toString(),
-    );
+    vi.spyOn(paths, 'resolveToRealPath').mockImplementation((p) => p);
 
     // Mock existsSync to skip the csc.exe auto-compilation of helper during unit tests.
     const originalExistsSync = fs.existsSync;
@@ -299,6 +298,60 @@ describe('WindowsSandboxManager', () => {
     }
   });
 
+  it('should NOT grant Low Integrity access to git worktree paths (enforce read-only)', async () => {
+    const worktreeGitDir = createTempDir('worktree-git');
+    const mainGitDir = createTempDir('main-git');
+
+    try {
+      vi.spyOn(sandboxManager, 'resolveSandboxPaths').mockResolvedValue({
+        workspace: { original: testCwd, resolved: testCwd },
+        forbidden: [],
+        globalIncludes: [],
+        policyAllowed: [],
+        policyRead: [],
+        policyWrite: [],
+        gitWorktree: {
+          worktreeGitDir,
+          mainGitDir,
+        },
+      });
+
+      const req: SandboxRequest = {
+        command: 'test',
+        args: [],
+        cwd: testCwd,
+        env: {},
+      };
+
+      await manager.prepareCommand(req);
+
+      const icaclsArgs = vi
+        .mocked(spawnAsync)
+        .mock.calls.filter((c) => c[0] === 'icacls')
+        .map((c) => c[1]);
+
+      // Verify that no icacls grants were issued for the git directories
+      expect(icaclsArgs).not.toContainEqual([
+        worktreeGitDir,
+        '/grant',
+        '*S-1-16-4096:(OI)(CI)(M)',
+        '/setintegritylevel',
+        '(OI)(CI)Low',
+      ]);
+
+      expect(icaclsArgs).not.toContainEqual([
+        mainGitDir,
+        '/grant',
+        '*S-1-16-4096:(OI)(CI)(M)',
+        '/setintegritylevel',
+        '(OI)(CI)Low',
+      ]);
+    } finally {
+      fs.rmSync(worktreeGitDir, { recursive: true, force: true });
+      fs.rmSync(mainGitDir, { recursive: true, force: true });
+    }
+  });
+
   it('should grant Low Integrity access to additional write paths', async () => {
     const extraWritePath = createTempDir('extra-write');
     try {

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

@@ -299,7 +299,9 @@ export class WindowsSandboxManager implements SandboxManager {
         )
       : false;
 
-    if (!isReadonlyMode || isApproved) {
+    const workspaceWrite = !isReadonlyMode || isApproved || isYolo;
+
+    if (workspaceWrite) {
       await this.grantLowIntegrityAccess(resolvedPaths.workspace.resolved);
       writableRoots.push(resolvedPaths.workspace.resolved);
     }
@@ -345,6 +347,12 @@ export class WindowsSandboxManager implements SandboxManager {
       }
     }
 
+    // Support git worktrees/submodules; read-only to prevent malicious hook/config modification (RCE).
+    // Read access is inherited; skip grantLowIntegrityAccess to ensure write protection.
+    if (resolvedPaths.gitWorktree) {
+      // No-op for read access.
+    }
+
     // 2. Collect secret files and apply protective ACLs
     // On Windows, we explicitly deny access to secret files for Low Integrity
     // processes to ensure they cannot be read or written.