refactor(core): extract and centralize sandbox path utilities (#25305)

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

packages/core/src/policy/sandboxPolicyManager.ts

@@ -12,7 +12,7 @@ import { z } from 'zod';
 import { fileURLToPath } from 'node:url';
 import { debugLogger } from '../utils/debugLogger.js';
 import { type SandboxPermissions } from '../services/sandboxManager.js';
-import { sanitizePaths } from '../services/sandboxManager.js';
+import { deduplicateAbsolutePaths } from '../utils/paths.js';
 import { normalizeCommand } from '../utils/shell-utils.js';
 
 export const SandboxModeConfigSchema = z.object({
@@ -199,11 +199,11 @@ export class SandboxPolicyManager {
 
     this.sessionApprovals[normalized] = {
       fileSystem: {
-        read: sanitizePaths([
+        read: deduplicateAbsolutePaths([
           ...(existing.fileSystem?.read ?? []),
           ...(permissions.fileSystem?.read ?? []),
         ]),
-        write: sanitizePaths([
+        write: deduplicateAbsolutePaths([
           ...(existing.fileSystem?.write ?? []),
           ...(permissions.fileSystem?.write ?? []),
         ]),
@@ -230,7 +230,7 @@ export class SandboxPolicyManager {
       ...(permissions.fileSystem?.read ?? []),
       ...(permissions.fileSystem?.write ?? []),
     ];
-    const newPaths = new Set(sanitizePaths(newPathsArray));
+    const newPaths = new Set(deduplicateAbsolutePaths(newPathsArray));
 
     this.config.commands[normalized] = {
       allowed_paths: Array.from(newPaths),

packages/core/src/services/sandboxManager.test.ts

@@ -10,11 +10,9 @@ import fsPromises from 'node:fs/promises';
 import { afterEach, describe, expect, it, vi, beforeEach } from 'vitest';
 import {
   NoopSandboxManager,
-  sanitizePaths,
   findSecretFiles,
   isSecretFile,
   resolveSandboxPaths,
-  getPathIdentity,
   type SandboxRequest,
 } from './sandboxManager.js';
 import { createSandboxManager } from './sandboxManagerFactory.js';
@@ -139,64 +137,6 @@ describe('findSecretFiles', () => {
 describe('SandboxManager', () => {
   afterEach(() => vi.restoreAllMocks());
 
-  describe('sanitizePaths', () => {
-    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', () => {
-      const paths = ['/workspace/foo', '/workspace/bar', '/workspace/foo'];
-      expect(sanitizePaths(paths)).toEqual([
-        '/workspace/foo',
-        '/workspace/bar',
-      ]);
-    });
-
-    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(
-        'Sandbox path must be absolute: relative/path',
-      );
-    });
-  });
-
-  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(
-        path.normalize('/workspace/foo'),
-      );
-
-      vi.spyOn(os, 'platform').mockReturnValue('darwin');
-      expect(getPathIdentity('/Tmp/Foo')).toBe(path.normalize('/tmp/foo'));
-
-      vi.spyOn(os, 'platform').mockReturnValue('linux');
-      expect(getPathIdentity('/Tmp/Foo')).toBe(path.normalize('/Tmp/Foo'));
-    });
-  });
-
   describe('resolveSandboxPaths', () => {
     it('should resolve allowed and forbidden paths', async () => {
       const workspace = path.resolve('/workspace');
@@ -268,7 +208,7 @@ describe('SandboxManager', () => {
     });
 
     it('should handle case-insensitive conflicts on supported platforms', async () => {
-      vi.spyOn(os, 'platform').mockReturnValue('darwin');
+      vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin');
       const workspace = path.resolve('/workspace');
       const secretUpper = path.join(workspace, 'SECRET');
       const secretLower = path.join(workspace, 'secret');

packages/core/src/services/sandboxManager.ts

@@ -22,7 +22,11 @@ import {
 } from './environmentSanitization.js';
 import type { ShellExecutionResult } from './shellExecutionService.js';
 import type { SandboxPolicyManager } from '../policy/sandboxPolicyManager.js';
-import { resolveToRealPath } from '../utils/paths.js';
+import {
+  toPathKey,
+  deduplicateAbsolutePaths,
+  resolveToRealPath,
+} from '../utils/paths.js';
 import { resolveGitWorktreePaths } from '../sandbox/utils/fsUtils.js';
 
 /**
@@ -369,7 +373,7 @@ export async function resolveSandboxPaths(
 ): Promise<ResolvedSandboxPaths> {
   /**
    * Helper that expands each path to include its realpath (if it's a symlink)
-   * and pipes the result through sanitizePaths for deduplication and absolute path enforcement.
+   * and pipes the result through deduplicateAbsolutePaths for deduplication and absolute path enforcement.
    */
   const expand = (paths?: string[] | null): string[] => {
     if (!paths || paths.length === 0) return [];
@@ -381,7 +385,7 @@ export async function resolveSandboxPaths(
         return [p];
       }
     });
-    return sanitizePaths(expanded);
+    return deduplicateAbsolutePaths(expanded);
   };
 
   const forbidden = expand(await options.forbiddenPaths?.());
@@ -395,9 +399,9 @@ export async function resolveSandboxPaths(
   const resolvedWorkspace = resolveToRealPath(options.workspace);
 
   const workspaceIdentities = new Set(
-    [options.workspace, resolvedWorkspace].map(getPathIdentity),
+    [options.workspace, resolvedWorkspace].map(toPathKey),
   );
-  const forbiddenIdentities = new Set(forbidden.map(getPathIdentity));
+  const forbiddenIdentities = new Set(forbidden.map(toPathKey));
 
   const { worktreeGitDir, mainGitDir } =
     await resolveGitWorktreePaths(resolvedWorkspace);
@@ -410,7 +414,7 @@ export async function resolveSandboxPaths(
    */
   const filter = (paths: string[]) =>
     paths.filter((p) => {
-      const identity = getPathIdentity(p);
+      const identity = toPathKey(p);
       return (
         !workspaceIdentities.has(identity) && !forbiddenIdentities.has(identity)
       );
@@ -430,40 +434,4 @@ export async function resolveSandboxPaths(
   };
 }
 
-/**
- * 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 [];
-
-  const uniquePathsMap = new Map<string, string>();
-  for (const p of paths) {
-    if (!path.isAbsolute(p)) {
-      throw new Error(`Sandbox path must be absolute: ${p}`);
-    }
-
-    const key = getPathIdentity(p);
-    if (!uniquePathsMap.has(key)) {
-      uniquePathsMap.set(key, p);
-    }
-  }
-
-  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;
-}
-
 export { createSandboxManager } from './sandboxManagerFactory.js';

packages/core/src/tools/shell.ts

@@ -10,10 +10,7 @@ import path from 'node:path';
 import os from 'node:os';
 import crypto from 'node:crypto';
 import { debugLogger } from '../index.js';
-import {
-  type SandboxPermissions,
-  getPathIdentity,
-} from '../services/sandboxManager.js';
+import { type SandboxPermissions } from '../services/sandboxManager.js';
 import { ToolErrorType } from './tool-error.js';
 import {
   BaseDeclarativeTool,
@@ -52,7 +49,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js';
 import { getShellDefinition } from './definitions/coreTools.js';
 import { resolveToolDeclaration } from './definitions/resolver.js';
 import type { AgentLoopContext } from '../config/agent-loop-context.js';
-import { isSubpath, resolveToRealPath } from '../utils/paths.js';
+import { toPathKey, isSubpath, resolveToRealPath } from '../utils/paths.js';
 import {
   getProactiveToolSuggestions,
   isNetworkReliantCommand,
@@ -307,15 +304,13 @@ export class ShellToolInvocation extends BaseToolInvocation<
               approvedPaths?: string[],
             ): boolean => {
               if (!approvedPaths || approvedPaths.length === 0) return false;
-              const requestedRealIdentity = getPathIdentity(
+              const requestedRealIdentity = toPathKey(
                 resolveToRealPath(requestedPath),
               );
 
               // Identity check is fast, subpath check is slower
               return approvedPaths.some((p) => {
-                const approvedRealIdentity = getPathIdentity(
-                  resolveToRealPath(p),
-                );
+                const approvedRealIdentity = toPathKey(resolveToRealPath(p));
                 return (
                   requestedRealIdentity === approvedRealIdentity ||
                   isSubpath(approvedRealIdentity, requestedRealIdentity)

packages/core/src/utils/paths.test.ts

@@ -16,6 +16,8 @@ import {
   normalizePath,
   resolveToRealPath,
   makeRelative,
+  deduplicateAbsolutePaths,
+  toPathKey,
 } from './paths.js';
 
 vi.mock('node:fs', async (importOriginal) => {
@@ -702,4 +704,62 @@ describe('normalizePath', () => {
       expect(result).toBe('/usr/local/bin');
     });
   });
+
+  describe('deduplicateAbsolutePaths', () => {
+    it('should return an empty array if no paths are provided', () => {
+      expect(deduplicateAbsolutePaths(undefined)).toEqual([]);
+      expect(deduplicateAbsolutePaths(null)).toEqual([]);
+      expect(deduplicateAbsolutePaths([])).toEqual([]);
+    });
+
+    it('should deduplicate paths using their normalized identity', () => {
+      const paths = ['/workspace/foo', '/workspace/foo/'];
+      expect(deduplicateAbsolutePaths(paths)).toEqual(['/workspace/foo']);
+    });
+
+    it('should handle case-insensitivity on Windows and macOS', () => {
+      mockPlatform('win32');
+      const paths = ['/workspace/foo', '/Workspace/Foo'];
+      expect(deduplicateAbsolutePaths(paths)).toEqual(['/workspace/foo']);
+
+      mockPlatform('darwin');
+      const macPaths = ['/tmp/foo', '/Tmp/Foo'];
+      expect(deduplicateAbsolutePaths(macPaths)).toEqual(['/tmp/foo']);
+
+      mockPlatform('linux');
+      const linuxPaths = ['/tmp/foo', '/tmp/FOO'];
+      expect(deduplicateAbsolutePaths(linuxPaths)).toEqual([
+        '/tmp/foo',
+        '/tmp/FOO',
+      ]);
+    });
+
+    it('should throw an error if a path is not absolute', () => {
+      const paths = ['relative/path'];
+      expect(() => deduplicateAbsolutePaths(paths)).toThrow(
+        'Path must be absolute: relative/path',
+      );
+    });
+  });
+
+  describe('toPathKey', () => {
+    it('should normalize paths and strip trailing slashes', () => {
+      expect(toPathKey('/foo/bar//baz/')).toBe(path.normalize('/foo/bar/baz'));
+    });
+
+    it('should convert paths to lowercase on Windows and macOS', () => {
+      mockPlatform('win32');
+      expect(toPathKey('/Workspace/Foo')).toBe(
+        path.normalize('/workspace/foo'),
+      );
+      // Ensure drive roots are preserved
+      expect(toPathKey('C:\\')).toBe('c:\\');
+
+      mockPlatform('darwin');
+      expect(toPathKey('/Tmp/Foo')).toBe(path.normalize('/tmp/foo'));
+
+      mockPlatform('linux');
+      expect(toPathKey('/Tmp/Foo')).toBe(path.normalize('/Tmp/Foo'));
+    });
+  });
 });

packages/core/src/utils/paths.ts

@@ -454,3 +454,45 @@ function robustRealpath(p: string, visited = new Set<string>()): string {
     throw e;
   }
 }
+
+/**
+ * Deduplicates an array of paths and ensures all paths are absolute.
+ */
+export function deduplicateAbsolutePaths(paths?: string[] | null): string[] {
+  if (!paths || paths.length === 0) return [];
+
+  const uniquePathsMap = new Map<string, string>();
+  for (const p of paths) {
+    if (!path.isAbsolute(p)) {
+      throw new Error(`Path must be absolute: ${p}`);
+    }
+
+    const key = toPathKey(p);
+    if (!uniquePathsMap.has(key)) {
+      uniquePathsMap.set(key, p);
+    }
+  }
+
+  return Array.from(uniquePathsMap.values());
+}
+
+/**
+ * Returns a stable string key for a path to be used in comparisons or Map lookups.
+ */
+export function toPathKey(p: string): string {
+  // Normalize path segments
+  let norm = path.normalize(p);
+
+  // Strip trailing slashes (except for root paths)
+  if (norm.length > 1 && (norm.endsWith('/') || norm.endsWith('\\'))) {
+    // On Windows, don't strip the slash from a drive root (e.g., "C:\\")
+    if (!/^[a-zA-Z]:[\\/]$/.test(norm)) {
+      norm = norm.slice(0, -1);
+    }
+  }
+
+  // Convert to lowercase on case-insensitive platforms
+  const platform = process.platform;
+  const isCaseInsensitive = platform === 'win32' || platform === 'darwin';
+  return isCaseInsensitive ? norm.toLowerCase() : norm;
+}