From 0d6d5d90b9f30e082d67251beced335e89346453 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Mon, 13 Apr 2026 11:43:13 -0700 Subject: [PATCH] refactor(core): extract and centralize sandbox path utilities (#25305) Co-authored-by: David Pierce --- .../core/src/policy/sandboxPolicyManager.ts | 8 +-- .../core/src/services/sandboxManager.test.ts | 62 +------------------ packages/core/src/services/sandboxManager.ts | 52 +++------------- packages/core/src/tools/shell.ts | 13 ++-- packages/core/src/utils/paths.test.ts | 60 ++++++++++++++++++ packages/core/src/utils/paths.ts | 42 +++++++++++++ 6 files changed, 121 insertions(+), 116 deletions(-) diff --git a/packages/core/src/policy/sandboxPolicyManager.ts b/packages/core/src/policy/sandboxPolicyManager.ts index 84b627eb92..7498a1c73b 100644 --- a/packages/core/src/policy/sandboxPolicyManager.ts +++ b/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), diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 7ff8525f77..81864416f4 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/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'); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index 0191207b16..a4dc356984 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/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 { /** * 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(); - 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'; diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index e299d88e4c..44f0c85316 100644 --- a/packages/core/src/tools/shell.ts +++ b/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) diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 1a4266834d..09b58d4187 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/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')); + }); + }); }); diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index b83860eadb..dae7c5c4e8 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -454,3 +454,45 @@ function robustRealpath(p: string, visited = new Set()): 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(); + 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; +}