From 893ae4d29aee5ede898276c664db67aef98b2845 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:48:18 -0700 Subject: [PATCH] fix(core): ensure sandbox approvals are correctly persisted and matched for proactive expansions (#24577) --- .../src/policy/sandboxPolicyManager.test.ts | 72 +++++++ .../core/src/policy/sandboxPolicyManager.ts | 52 +++-- .../src/sandbox/utils/proactivePermissions.ts | 5 +- packages/core/src/tools/shell.test.ts | 43 +++- packages/core/src/tools/shell.ts | 184 ++++++++++-------- .../core/src/tools/shell_proactive.test.ts | 180 +++++++++++++++++ packages/core/src/utils/paths.test.ts | 79 +++++++- packages/core/src/utils/paths.ts | 29 ++- packages/core/src/utils/shell-utils.test.ts | 18 ++ packages/core/src/utils/shell-utils.ts | 14 ++ 10 files changed, 572 insertions(+), 104 deletions(-) create mode 100644 packages/core/src/policy/sandboxPolicyManager.test.ts create mode 100644 packages/core/src/tools/shell_proactive.test.ts diff --git a/packages/core/src/policy/sandboxPolicyManager.test.ts b/packages/core/src/policy/sandboxPolicyManager.test.ts new file mode 100644 index 0000000000..034ab68735 --- /dev/null +++ b/packages/core/src/policy/sandboxPolicyManager.test.ts @@ -0,0 +1,72 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SandboxPolicyManager } from './sandboxPolicyManager.js'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +describe('SandboxPolicyManager', () => { + const tempDir = path.join(os.tmpdir(), 'gemini-test-sandbox-policy'); + const configPath = path.join(tempDir, 'sandbox.toml'); + + beforeEach(() => { + if (!fs.existsSync(tempDir)) { + fs.mkdirSync(tempDir, { recursive: true }); + } + }); + + afterEach(() => { + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + it('should add and retrieve session approvals', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('ls', { + fileSystem: { read: ['/tmp'], write: [] }, + network: false, + }); + + const perms = manager.getCommandPermissions('ls'); + expect(perms.fileSystem?.read).toContain('/tmp'); + }); + + it('should protect against prototype pollution (session)', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('__proto__', { + fileSystem: { read: ['/POLLUTED'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('any-command'); + expect(perms.fileSystem?.read).not.toContain('/POLLUTED'); + }); + + it('should protect against prototype pollution (persistent)', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addPersistentApproval('constructor', { + fileSystem: { read: ['/POLLUTED_PERSISTENT'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('constructor'); + expect(perms.fileSystem?.read).not.toContain('/POLLUTED_PERSISTENT'); + }); + + it('should lowercase command names for normalization', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('NPM', { + fileSystem: { read: ['/node_modules'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('npm'); + expect(perms.fileSystem?.read).toContain('/node_modules'); + }); +}); diff --git a/packages/core/src/policy/sandboxPolicyManager.ts b/packages/core/src/policy/sandboxPolicyManager.ts index c8a4d2f8df..8b3d9a5744 100644 --- a/packages/core/src/policy/sandboxPolicyManager.ts +++ b/packages/core/src/policy/sandboxPolicyManager.ts @@ -13,6 +13,7 @@ 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 { normalizeCommand } from '../utils/shell-utils.js'; export const SandboxModeConfigSchema = z.object({ network: z.boolean(), @@ -104,6 +105,10 @@ export class SandboxPolicyManager { this.config = this.loadConfig(); } + private isProtectedKey(key: string): boolean { + return key === '__proto__' || key === 'constructor' || key === 'prototype'; + } + private loadConfig(): SandboxTomlSchemaType { if (!fs.existsSync(this.configPath)) { return SandboxPolicyManager.DEFAULT_CONFIG; @@ -154,8 +159,15 @@ export class SandboxPolicyManager { } getCommandPermissions(commandName: string): SandboxPermissions { - const persistent = this.config.commands[commandName]; - const session = this.sessionApprovals[commandName]; + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return { + fileSystem: { read: [], write: [] }, + network: false, + }; + } + const persistent = this.config.commands[normalized]; + const session = this.sessionApprovals[normalized]; return { fileSystem: { @@ -176,25 +188,25 @@ export class SandboxPolicyManager { commandName: string, permissions: SandboxPermissions, ): void { - const existing = this.sessionApprovals[commandName] || { + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return; + } + const existing = this.sessionApprovals[normalized] || { fileSystem: { read: [], write: [] }, network: false, }; - this.sessionApprovals[commandName] = { + this.sessionApprovals[normalized] = { fileSystem: { - read: Array.from( - new Set([ - ...(existing.fileSystem?.read ?? []), - ...(permissions.fileSystem?.read ?? []), - ]), - ), - write: Array.from( - new Set([ - ...(existing.fileSystem?.write ?? []), - ...(permissions.fileSystem?.write ?? []), - ]), - ), + read: sanitizePaths([ + ...(existing.fileSystem?.read ?? []), + ...(permissions.fileSystem?.read ?? []), + ]), + write: sanitizePaths([ + ...(existing.fileSystem?.write ?? []), + ...(permissions.fileSystem?.write ?? []), + ]), }, network: existing.network || permissions.network || false, }; @@ -204,7 +216,11 @@ export class SandboxPolicyManager { commandName: string, permissions: SandboxPermissions, ): void { - const existing = this.config.commands[commandName] || { + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return; + } + const existing = this.config.commands[normalized] || { allowed_paths: [], allow_network: false, }; @@ -216,7 +232,7 @@ export class SandboxPolicyManager { ]; const newPaths = new Set(sanitizePaths(newPathsArray)); - this.config.commands[commandName] = { + this.config.commands[normalized] = { allowed_paths: Array.from(newPaths), allow_network: existing.allow_network || permissions.network || false, }; diff --git a/packages/core/src/sandbox/utils/proactivePermissions.ts b/packages/core/src/sandbox/utils/proactivePermissions.ts index a5e11e2c3c..c4ec0c1520 100644 --- a/packages/core/src/sandbox/utils/proactivePermissions.ts +++ b/packages/core/src/sandbox/utils/proactivePermissions.ts @@ -8,6 +8,7 @@ import os from 'node:os'; import path from 'node:path'; import fs from 'node:fs'; import { type SandboxPermissions } from '../../services/sandboxManager.js'; +import { normalizeCommand } from '../../utils/shell-utils.js'; const NETWORK_RELIANT_TOOLS = new Set([ 'npm', @@ -45,7 +46,7 @@ export function isNetworkReliantCommand( commandName: string, subCommand?: string, ): boolean { - const normalizedCommand = commandName.toLowerCase().replace(/\.exe$/, ''); + const normalizedCommand = normalizeCommand(commandName); if (!NETWORK_RELIANT_TOOLS.has(normalizedCommand)) { return false; } @@ -82,7 +83,7 @@ export function isNetworkReliantCommand( export async function getProactiveToolSuggestions( commandName: string, ): Promise { - const normalizedCommand = commandName.toLowerCase().replace(/\.exe$/, ''); + const normalizedCommand = normalizeCommand(commandName); if (!NETWORK_RELIANT_TOOLS.has(normalizedCommand)) { return undefined; } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 4a3ac48f00..245b7f0eee 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -154,7 +154,11 @@ describe('ShellTool', () => { return mockSandboxManager; }, sandboxPolicyManager: { - getCommandPermissions: vi.fn().mockReturnValue(undefined), + getCommandPermissions: vi.fn().mockReturnValue({ + fileSystem: { read: [], write: [] }, + network: false, + }), + getModeConfig: vi.fn().mockReturnValue({ readonly: false }), addPersistentApproval: vi.fn(), addSessionApproval: vi.fn(), @@ -708,6 +712,39 @@ describe('ShellTool', () => { it('should throw an error if validation fails', () => { expect(() => shellTool.build({ command: '' })).toThrow(); }); + + it('should NOT return a sandbox expansion prompt for npm install when sandboxing is disabled', async () => { + const bus = (shellTool as unknown as { messageBus: MessageBus }) + .messageBus; + const mockBus = getMockMessageBusInstance( + bus, + ) as unknown as TestableMockMessageBus; + mockBus.defaultToolDecision = 'allow'; + + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(false); + const params = { command: 'npm install' }; + const invocation = shellTool.build(params); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + // Should be false because standard confirm mode is 'allow' + expect(confirmation).toBe(false); + }); + + it('should return a sandbox expansion prompt for npm install when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + const params = { command: 'npm install' }; + const invocation = shellTool.build(params); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + expect(confirmation).not.toBe(false); + expect(confirmation && confirmation.type).toBe('sandbox_expansion'); + }); }); describe('getDescription', () => { @@ -950,6 +987,10 @@ describe('ShellTool', () => { describe('sandbox heuristics', () => { const mockAbortSignal = new AbortController().signal; + beforeEach(() => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + }); + it('should suggest proactive permissions for npm commands', async () => { const homeDir = path.join(tempRootDir, 'home'); fs.mkdirSync(homeDir); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index a467ef4c63..81ac9d9a32 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -10,7 +10,10 @@ import path from 'node:path'; import os from 'node:os'; import crypto from 'node:crypto'; import { debugLogger } from '../index.js'; -import type { SandboxPermissions } from '../services/sandboxManager.js'; +import { + type SandboxPermissions, + getPathIdentity, +} from '../services/sandboxManager.js'; import { ToolErrorType } from './tool-error.js'; import { BaseDeclarativeTool, @@ -42,6 +45,7 @@ import { stripShellWrapper, parseCommandDetails, hasRedirection, + normalizeCommand, } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; import { PARAM_ADDITIONAL_PERMISSIONS } from './definitions/base-declarations.js'; @@ -49,7 +53,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 } from '../utils/paths.js'; +import { isSubpath, resolveToRealPath } from '../utils/paths.js'; import { getProactiveToolSuggestions, isNetworkReliantCommand, @@ -247,77 +251,103 @@ export class ShellToolInvocation extends BaseToolInvocation< return this.getConfirmationDetails(abortSignal); } - // Proactively suggest expansion for known network-heavy Node.js ecosystem tools - // (npm install, etc.) to avoid hangs when network is restricted by default. - // We do this even if the command is "allowed" by policy because the DEFAULT - // permissions are usually insufficient for these commands. - const command = stripShellWrapper(this.params.command); - const rootCommands = getCommandRoots(command); - const rootCommand = rootCommands[0]; + if (this.context.config.getSandboxEnabled()) { + const command = stripShellWrapper(this.params.command); + const rootCommands = getCommandRoots(command); + const rawRootCommand = rootCommands[0]; - if (rootCommand) { - const proactive = await getProactiveToolSuggestions(rootCommand); - if (proactive) { - const approved = - this.context.config.sandboxPolicyManager.getCommandPermissions( - rootCommand, - ); - const missingNetwork = !!proactive.network && !approved?.network; - - // Detect commands or sub-commands that definitely need network - const parsed = parseCommandDetails(command); - const subCommand = parsed?.details[0]?.args?.[0]; - const needsNetwork = isNetworkReliantCommand(rootCommand, subCommand); - - if (needsNetwork) { - // Add write permission to the current directory if we are in readonly mode + if (rawRootCommand) { + const rootCommand = normalizeCommand(rawRootCommand); + const proactive = await getProactiveToolSuggestions(rootCommand); + if (proactive) { const mode = this.context.config.getApprovalMode(); - const isReadonlyMode = - this.context.config.sandboxPolicyManager.getModeConfig(mode) - ?.readonly ?? false; + const modeConfig = + this.context.config.sandboxPolicyManager.getModeConfig(mode); + const approved = + this.context.config.sandboxPolicyManager.getCommandPermissions( + rootCommand, + ); - if (isReadonlyMode) { - const cwd = - this.params.dir_path || this.context.config.getTargetDir(); - proactive.fileSystem = proactive.fileSystem || { - read: [], - write: [], - }; - proactive.fileSystem.write = proactive.fileSystem.write || []; - if (!proactive.fileSystem.write.includes(cwd)) { - proactive.fileSystem.write.push(cwd); - proactive.fileSystem.read = proactive.fileSystem.read || []; - if (!proactive.fileSystem.read.includes(cwd)) { - proactive.fileSystem.read.push(cwd); + const hasNetwork = modeConfig.network || approved.network; + const missingNetwork = !!proactive.network && !hasNetwork; + + // Detect commands or sub-commands that definitely need network + const parsed = parseCommandDetails(command); + const subCommand = parsed?.details[0]?.args?.[0]; + const needsNetwork = isNetworkReliantCommand(rootCommand, subCommand); + + if (needsNetwork) { + // Add write permission to the current directory if we are in readonly mode + const isReadonlyMode = modeConfig.readonly ?? false; + + if (isReadonlyMode) { + const cwd = + this.params.dir_path || this.context.config.getTargetDir(); + proactive.fileSystem = proactive.fileSystem || { + read: [], + write: [], + }; + proactive.fileSystem.write = proactive.fileSystem.write || []; + if (!proactive.fileSystem.write.includes(cwd)) { + proactive.fileSystem.write.push(cwd); + proactive.fileSystem.read = proactive.fileSystem.read || []; + if (!proactive.fileSystem.read.includes(cwd)) { + proactive.fileSystem.read.push(cwd); + } } } - } - const missingRead = (proactive.fileSystem?.read || []).filter( - (p) => !approved?.fileSystem?.read?.includes(p), - ); - const missingWrite = (proactive.fileSystem?.write || []).filter( - (p) => !approved?.fileSystem?.write?.includes(p), - ); + const isApproved = ( + requestedPath: string, + approvedPaths?: string[], + ): boolean => { + if (!approvedPaths || approvedPaths.length === 0) return false; + const requestedRealIdentity = getPathIdentity( + resolveToRealPath(requestedPath), + ); - const needsExpansion = - missingRead.length > 0 || missingWrite.length > 0 || missingNetwork; + // Identity check is fast, subpath check is slower + return approvedPaths.some((p) => { + const approvedRealIdentity = getPathIdentity( + resolveToRealPath(p), + ); + return ( + requestedRealIdentity === approvedRealIdentity || + isSubpath(approvedRealIdentity, requestedRealIdentity) + ); + }); + }; - if (needsExpansion) { - const details = await this.getConfirmationDetails( - abortSignal, - proactive, + const missingRead = (proactive.fileSystem?.read || []).filter( + (p) => !isApproved(p, approved.fileSystem?.read), ); - if (details && details.type === 'sandbox_expansion') { - const originalOnConfirm = details.onConfirm; - details.onConfirm = async (outcome: ToolConfirmationOutcome) => { - await originalOnConfirm(outcome); - if (outcome !== ToolConfirmationOutcome.Cancel) { - this.proactivePermissionsConfirmed = proactive; - } - }; + const missingWrite = (proactive.fileSystem?.write || []).filter( + (p) => !isApproved(p, approved.fileSystem?.write), + ); + + const needsExpansion = + missingRead.length > 0 || + missingWrite.length > 0 || + missingNetwork; + + if (needsExpansion) { + const details = await this.getConfirmationDetails( + abortSignal, + proactive, + ); + if (details && details.type === 'sandbox_expansion') { + const originalOnConfirm = details.onConfirm; + details.onConfirm = async ( + outcome: ToolConfirmationOutcome, + ) => { + await originalOnConfirm(outcome); + if (outcome !== ToolConfirmationOutcome.Cancel) { + this.proactivePermissionsConfirmed = proactive; + } + }; + } + return details; } - return details; } } } @@ -742,20 +772,22 @@ export class ShellToolInvocation extends BaseToolInvocation< ); // Proactive permission suggestions for Node ecosystem tools - const proactive = - await getProactiveToolSuggestions(rootCommandDisplay); - if (proactive) { - if (proactive.network) { - sandboxDenial.network = true; - } - if (proactive.fileSystem?.read) { - for (const p of proactive.fileSystem.read) { - readPaths.add(p); + if (this.context.config.getSandboxEnabled()) { + const proactive = + await getProactiveToolSuggestions(rootCommandDisplay); + if (proactive) { + if (proactive.network) { + sandboxDenial.network = true; } - } - if (proactive.fileSystem?.write) { - for (const p of proactive.fileSystem.write) { - writePaths.add(p); + if (proactive.fileSystem?.read) { + for (const p of proactive.fileSystem.read) { + readPaths.add(p); + } + } + if (proactive.fileSystem?.write) { + for (const p of proactive.fileSystem.write) { + writePaths.add(p); + } } } } diff --git a/packages/core/src/tools/shell_proactive.test.ts b/packages/core/src/tools/shell_proactive.test.ts new file mode 100644 index 0000000000..c2327789de --- /dev/null +++ b/packages/core/src/tools/shell_proactive.test.ts @@ -0,0 +1,180 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + vi, + describe, + it, + expect, + beforeEach, + beforeAll, + afterEach, +} from 'vitest'; +import os from 'node:os'; +import type _fs from 'node:fs'; +import { ShellTool } from './shell.js'; +import { type Config } from '../config/config.js'; +import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import * as proactivePermissions from '../sandbox/utils/proactivePermissions.js'; + +import { initializeShellParsers } from '../utils/shell-utils.js'; + +vi.mock('node:fs', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + default: { + ...original, + realpathSync: vi.fn((p) => p), + }, + realpathSync: vi.fn((p) => p), + }; +}); + +vi.mock('../sandbox/utils/proactivePermissions.js', () => ({ + getProactiveToolSuggestions: vi.fn(), + isNetworkReliantCommand: vi.fn(), +})); + +const mockPlatform = (platform: string) => { + vi.stubGlobal( + 'process', + Object.create(process, { + platform: { + get: () => platform, + }, + }), + ); + vi.spyOn(os, 'platform').mockReturnValue(platform as NodeJS.Platform); +}; + +describe('ShellTool Proactive Expansion', () => { + let mockConfig: Config; + let shellTool: ShellTool; + + beforeAll(async () => { + await initializeShellParsers(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + beforeEach(() => { + vi.clearAllMocks(); + mockPlatform('darwin'); + + mockConfig = { + get config() { + return this; + }, + getSandboxEnabled: vi.fn().mockReturnValue(false), + getTargetDir: vi.fn().mockReturnValue('/tmp'), + getApprovalMode: vi.fn().mockReturnValue('strict'), + sandboxPolicyManager: { + getCommandPermissions: vi.fn().mockReturnValue({ + fileSystem: { read: [], write: [] }, + network: false, + }), + getModeConfig: vi.fn().mockReturnValue({ readonly: false }), + }, + getEnableInteractiveShell: vi.fn().mockReturnValue(false), + getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), + getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000), + } as unknown as Config; + + const bus = createMockMessageBus(); + shellTool = new ShellTool(mockConfig, bus); + }); + + it('should NOT call getProactiveToolSuggestions when sandboxing is disabled', async () => { + const invocation = shellTool.build({ command: 'npm install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).not.toHaveBeenCalled(); + }); + + it('should call getProactiveToolSuggestions when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + network: true, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + const invocation = shellTool.build({ command: 'npm install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).toHaveBeenCalledWith('npm'); + }); + + it('should normalize command names (lowercase and strip .exe) when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + network: true, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + const invocation = shellTool.build({ command: 'NPM.EXE install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).toHaveBeenCalledWith('npm'); + }); + + it('should NOT request expansion if paths are already approved (case-insensitive subpath)', async () => { + // This test assumes Darwin or Windows for case-insensitivity + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + fileSystem: { read: ['/project/src'], write: [] }, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + // Current approval is for the parent dir, with different casing + vi.mocked( + mockConfig.sandboxPolicyManager.getCommandPermissions, + ).mockReturnValue({ + fileSystem: { read: ['/PROJECT'], write: [] }, + network: false, + }); + + const invocation = shellTool.build({ command: 'npm install' }); + const result = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + // If it's correctly approved, result should be false (no expansion needed) + // or a normal 'exec' confirmation, but NOT 'sandbox_expansion'. + if (result) { + expect(result.type).not.toBe('sandbox_expansion'); + } else { + expect(result).toBe(false); + } + }); +}); diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 590f3aab58..1a4266834d 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -15,6 +15,7 @@ import { shortenPath, normalizePath, resolveToRealPath, + makeRelative, } from './paths.js'; vi.mock('node:fs', async (importOriginal) => { @@ -215,7 +216,7 @@ describe('isSubpath', () => { }); }); -describe('isSubpath on Windows', () => { +describe.skipIf(process.platform !== 'win32')('isSubpath on Windows', () => { afterEach(() => vi.unstubAllGlobals()); beforeEach(() => mockPlatform('win32')); @@ -268,6 +269,20 @@ describe('isSubpath on Windows', () => { }); }); +describe.skipIf(process.platform !== 'darwin')('isSubpath on Darwin', () => { + afterEach(() => vi.unstubAllGlobals()); + + beforeEach(() => mockPlatform('darwin')); + + it('should be case-insensitive for path components on Darwin', () => { + expect(isSubpath('/PROJECT', '/project/src')).toBe(true); + }); + + it('should return true for a direct subpath on Darwin', () => { + expect(isSubpath('/Users/Test', '/Users/Test/file.txt')).toBe(true); + }); +}); + describe('shortenPath', () => { describe.skipIf(process.platform === 'win32')('on POSIX', () => { it('should not shorten a path that is shorter than maxLen', () => { @@ -586,6 +601,54 @@ describe('resolveToRealPath', () => { }); }); +describe('makeRelative', () => { + describe.skipIf(process.platform === 'win32')('on POSIX', () => { + it('should return relative path if targetPath is already relative', () => { + expect(makeRelative('foo/bar', '/root')).toBe('foo/bar'); + }); + + it('should return relative path from root to target', () => { + const root = '/Users/test/project'; + const target = '/Users/test/project/src/file.ts'; + expect(makeRelative(target, root)).toBe('src/file.ts'); + }); + + it('should return "." if target and root are the same', () => { + const root = '/Users/test/project'; + expect(makeRelative(root, root)).toBe('.'); + }); + + it('should handle parent directories with ..', () => { + const root = '/Users/test/project/src'; + const target = '/Users/test/project/docs/readme.md'; + expect(makeRelative(target, root)).toBe('../docs/readme.md'); + }); + }); + + describe.skipIf(process.platform !== 'win32')('on Windows', () => { + it('should return relative path if targetPath is already relative', () => { + expect(makeRelative('foo/bar', 'C:\\root')).toBe('foo/bar'); + }); + + it('should return relative path from root to target', () => { + const root = 'C:\\Users\\test\\project'; + const target = 'C:\\Users\\test\\project\\src\\file.ts'; + expect(makeRelative(target, root)).toBe('src\\file.ts'); + }); + + it('should return "." if target and root are the same', () => { + const root = 'C:\\Users\\test\\project'; + expect(makeRelative(root, root)).toBe('.'); + }); + + it('should handle parent directories with ..', () => { + const root = 'C:\\Users\\test\\project\\src'; + const target = 'C:\\Users\\test\\project\\docs\\readme.md'; + expect(makeRelative(target, root)).toBe('..\\docs\\readme.md'); + }); + }); +}); + describe('normalizePath', () => { it('should resolve a relative path to an absolute path', () => { const result = normalizePath('some/relative/path'); @@ -615,7 +678,19 @@ describe('normalizePath', () => { }); }); - describe.skipIf(process.platform === 'win32')('on POSIX', () => { + describe.skipIf(process.platform !== 'darwin')('on Darwin', () => { + beforeEach(() => mockPlatform('darwin')); + afterEach(() => vi.unstubAllGlobals()); + + it('should lowercase the entire path', () => { + const result = normalizePath('/Users/TEST'); + expect(result).toBe('/users/test'); + }); + }); + + describe.skipIf( + process.platform === 'win32' || process.platform === 'darwin', + )('on Linux', () => { it('should preserve case', () => { const result = normalizePath('/usr/Local/Bin'); expect(result).toContain('Local'); diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 312bacd7ea..135e047530 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -325,9 +325,14 @@ export function getProjectHash(projectRoot: string): string { * - On Windows, converts to lowercase for case-insensitivity. */ export function normalizePath(p: string): string { - const resolved = path.resolve(p); + const platform = process.platform; + const isWindows = platform === 'win32'; + const pathModule = isWindows ? path.win32 : path; + + const resolved = pathModule.resolve(p); const normalized = resolved.replace(/\\/g, '/'); - return process.platform === 'win32' ? normalized.toLowerCase() : normalized; + const isCaseInsensitive = isWindows || platform === 'darwin'; + return isCaseInsensitive ? normalized.toLowerCase() : normalized; } /** @@ -337,11 +342,25 @@ export function normalizePath(p: string): string { * @returns True if childPath is a subpath of parentPath, false otherwise. */ export function isSubpath(parentPath: string, childPath: string): boolean { - const isWindows = process.platform === 'win32'; + const platform = process.platform; + const isWindows = platform === 'win32'; + const isDarwin = platform === 'darwin'; const pathModule = isWindows ? path.win32 : path; - // On Windows, path.relative is case-insensitive. On POSIX, it's case-sensitive. - const relative = pathModule.relative(parentPath, childPath); + // Resolve both paths to absolute to ensure consistent comparison, + // especially when mixing relative and absolute paths or when casing differs. + let p = pathModule.resolve(parentPath); + let c = pathModule.resolve(childPath); + + // On Windows, path.relative is case-insensitive. + // On POSIX (including Darwin), path.relative is case-sensitive. + // We want it to be case-insensitive on Darwin to match user expectation and sandbox policy. + if (isDarwin) { + p = p.toLowerCase(); + c = c.toLowerCase(); + } + + const relative = pathModule.relative(p, c); return ( !relative.startsWith(`..${pathModule.sep}`) && diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 2370aa25c4..0dda7c4881 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -21,6 +21,7 @@ import { parseCommandDetails, splitCommands, stripShellWrapper, + normalizeCommand, hasRedirection, resolveExecutable, } from './shell-utils.js'; @@ -115,6 +116,23 @@ const mockPowerShellResult = ( }); }; +describe('normalizeCommand', () => { + it('should lowercase the command', () => { + expect(normalizeCommand('NPM')).toBe('npm'); + }); + + it('should remove .exe extension', () => { + expect(normalizeCommand('node.exe')).toBe('node'); + }); + + it('should handle absolute paths', () => { + expect(normalizeCommand('/usr/bin/npm')).toBe('npm'); + expect(normalizeCommand('C:\\Program Files\\nodejs\\node.exe')).toBe( + 'node', + ); + }); +}); + describe('getCommandRoots', () => { it('should return a single command', () => { expect(getCommandRoots('ls -l')).toEqual(['ls']); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 2ca3068e50..8486be0de9 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -310,6 +310,20 @@ function normalizeCommandName(raw: string): string { return raw.trim(); } +/** + * Normalizes a command name for sandbox policy lookups. + * Converts to lowercase and removes the .exe extension for cross-platform consistency. + * + * @param commandName - The command name to normalize. + * @returns The normalized command name. + */ +export function normalizeCommand(commandName: string): string { + // Split by both separators and get the last non-empty part + const parts = commandName.split(/[\\/]/).filter(Boolean); + const base = parts.length > 0 ? parts[parts.length - 1] : ''; + return base.toLowerCase().replace(/\.exe$/, ''); +} + function extractNameFromNode(node: Node): string | null { switch (node.type) { case 'command': {