diff --git a/integration-tests/ripgrep-real.test.ts b/integration-tests/ripgrep-real.test.ts index ac8a3eb169..9c83e32270 100644 --- a/integration-tests/ripgrep-real.test.ts +++ b/integration-tests/ripgrep-real.test.ts @@ -31,6 +31,10 @@ class MockConfig { getFileFilteringRespectGeminiIgnore() { return true; } + + validatePathAccess() { + return null; + } } describe('ripgrep-real-direct', () => { diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index 5e86c9b27a..509ce0e4b3 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -75,15 +75,46 @@ describe('handleAtCommand', () => { getFileSystemService: () => new StandardFileSystemService(), getEnableRecursiveFileSearch: vi.fn(() => true), getWorkspaceContext: () => ({ - isPathWithinWorkspace: () => true, + isPathWithinWorkspace: (p: string) => + p.startsWith(testRootDir) || p.startsWith('/private' + testRootDir), getDirectories: () => [testRootDir], }), + storage: { + getProjectTempDir: () => path.join(os.tmpdir(), 'gemini-cli-temp'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + if (this.interactive && path.isAbsolute(absolutePath)) { + return true; + } + + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + const resolvedProjectTempDir = path.resolve(projectTempDir); + return ( + absolutePath.startsWith(resolvedProjectTempDir + path.sep) || + absolutePath === resolvedProjectTempDir + ); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path validation failed: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, getPromptRegistry: () => ({ getPromptsByServer: () => [], }), getDebugMode: () => false, + getWorkingDir: () => '/working/dir', getFileExclusions: () => ({ getCoreIgnorePatterns: () => COMMON_IGNORE_PATTERNS, getDefaultExcludePatterns: () => [], diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index 708c950907..91ef84642e 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -230,8 +230,11 @@ export async function handleAtCommand({ continue; } - const workspaceContext = config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(pathName)) { + const resolvedPathName = path.isAbsolute(pathName) + ? pathName + : path.resolve(config.getTargetDir(), pathName); + + if (!config.isPathAllowed(resolvedPathName)) { onDebugMessage( `Path ${pathName} is not in the workspace and will be skipped.`, ); diff --git a/packages/cli/src/ui/hooks/atCommandProcessor_agents.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor_agents.test.ts index 0364cf94f6..90267e64c0 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor_agents.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor_agents.test.ts @@ -77,15 +77,46 @@ describe('handleAtCommand with Agents', () => { getFileSystemService: () => new StandardFileSystemService(), getEnableRecursiveFileSearch: vi.fn(() => true), getWorkspaceContext: () => ({ - isPathWithinWorkspace: () => true, + isPathWithinWorkspace: (p: string) => + p.startsWith(testRootDir) || p.startsWith('/private' + testRootDir), getDirectories: () => [testRootDir], }), + storage: { + getProjectTempDir: () => path.join(os.tmpdir(), 'gemini-cli-temp'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + if (this.interactive && path.isAbsolute(absolutePath)) { + return true; + } + + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + const resolvedProjectTempDir = path.resolve(projectTempDir); + return ( + absolutePath.startsWith(resolvedProjectTempDir + path.sep) || + absolutePath === resolvedProjectTempDir + ); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path validation failed: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, getPromptRegistry: () => ({ getPromptsByServer: () => [], }), getDebugMode: () => false, + getWorkingDir: () => '/working/dir', getFileExclusions: () => ({ getCoreIgnorePatterns: () => COMMON_IGNORE_PATTERNS, getDefaultExcludePatterns: () => [], @@ -102,8 +133,9 @@ describe('handleAtCommand with Agents', () => { getMcpClientManager: () => ({ getClient: () => undefined, }), - getAgentRegistry: () => mockAgentRegistry, getMessageBus: () => mockMessageBus, + interactive: true, + getAgentRegistry: () => mockAgentRegistry, } as unknown as Config; const registry = new ToolRegistry(mockConfig, mockMessageBus); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 730ea32e5e..ff7664a9c0 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -56,6 +56,7 @@ const MockedGeminiClientClass = vi.hoisted(() => this.startChat = mockStartChat; this.sendMessageStream = mockSendMessageStream; this.addHistory = vi.fn(); + this.getCurrentSequenceModel = vi.fn(); this.getChat = vi.fn().mockReturnValue({ recordCompletedToolCalls: vi.fn(), }); @@ -75,6 +76,13 @@ const MockedUserPromptEvent = vi.hoisted(() => ); const mockParseAndFormatApiError = vi.hoisted(() => vi.fn()); +const MockValidationRequiredError = vi.hoisted( + () => + class extends Error { + userHandled = false; + }, +); + vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actualCoreModule = (await importOriginal()) as any; return { @@ -82,6 +90,7 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { GitService: vi.fn(), GeminiClient: MockedGeminiClientClass, UserPromptEvent: MockedUserPromptEvent, + ValidationRequiredError: MockValidationRequiredError, parseAndFormatApiError: mockParseAndFormatApiError, tokenLimit: vi.fn().mockReturnValue(100), // Mock tokenLimit }; @@ -221,6 +230,7 @@ describe('useGeminiStream', () => { getApprovalMode: () => ApprovalMode.DEFAULT, getUsageStatisticsEnabled: () => true, getDebugMode: () => false, + getWorkingDir: () => '/working/dir', addHistory: vi.fn(), getSessionId() { return 'test-session-id'; @@ -228,6 +238,7 @@ describe('useGeminiStream', () => { setQuotaErrorOccurred: vi.fn(), getQuotaErrorOccurred: vi.fn(() => false), getModel: vi.fn(() => 'gemini-2.5-pro'), + getContentGenerator: vi.fn(), getContentGeneratorConfig: vi .fn() .mockReturnValue(contentGeneratorConfig), @@ -1671,6 +1682,7 @@ describe('useGeminiStream', () => { const testConfig = { ...mockConfig, + getContentGenerator: vi.fn(), getContentGeneratorConfig: vi.fn(() => ({ authType: mockAuthType, })), diff --git a/packages/cli/src/ui/hooks/useToolScheduler.test.ts b/packages/cli/src/ui/hooks/useToolScheduler.test.ts index 1978198b37..051d0e057f 100644 --- a/packages/cli/src/ui/hooks/useToolScheduler.test.ts +++ b/packages/cli/src/ui/hooks/useToolScheduler.test.ts @@ -65,6 +65,7 @@ const mockConfig = { getSessionId: () => 'test-session-id', getUsageStatisticsEnabled: () => true, getDebugMode: () => false, + getWorkingDir: () => '/working/dir', storage: { getProjectTempDir: () => '/tmp', }, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index ab83dc595d..6302607d45 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -6,6 +6,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; +import * as os from 'node:os'; import { inspect } from 'node:util'; import process from 'node:process'; import type { @@ -116,6 +117,7 @@ import { logApprovalModeDuration, } from '../telemetry/loggers.js'; import { fetchAdminControls } from '../code_assist/admin/admin_controls.js'; +import { isSubpath } from '../utils/paths.js'; export interface AccessibilitySettings { enableLoadingPhrases?: boolean; @@ -496,7 +498,8 @@ export class Config { private readonly importFormat: 'tree' | 'flat'; private readonly discoveryMaxDirs: number; private readonly compressionThreshold: number | undefined; - private readonly interactive: boolean; + /** Public for testing only */ + readonly interactive: boolean; private readonly ptyInfo: string; private readonly trustedFolder: boolean | undefined; private readonly useRipgrep: boolean; @@ -1688,6 +1691,57 @@ export class Config { return this.fileSystemService; } + /** + * Checks if a given absolute path is allowed for file system operations. + * A path is allowed if it's within the workspace context or the project's temporary directory. + * + * @param absolutePath The absolute path to check. + * @returns true if the path is allowed, false otherwise. + */ + isPathAllowed(absolutePath: string): boolean { + if (this.interactive && path.isAbsolute(absolutePath)) { + return true; + } + + const realpath = (p: string) => { + let resolved: string; + try { + resolved = fs.realpathSync(p); + } catch { + resolved = path.resolve(p); + } + return os.platform() === 'win32' ? resolved.toLowerCase() : resolved; + }; + + const resolvedPath = realpath(absolutePath); + + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(resolvedPath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + const resolvedTempDir = realpath(projectTempDir); + + return isSubpath(resolvedTempDir, resolvedPath); + } + + /** + * Validates if a path is allowed and returns a detailed error message if not. + * + * @param absolutePath The absolute path to validate. + * @returns An error message string if the path is disallowed, null otherwise. + */ + validatePathAccess(absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + } + /** * Set a custom FileSystemService */ diff --git a/packages/core/src/tools/confirmation-policy.test.ts b/packages/core/src/tools/confirmation-policy.test.ts index 1d04896a10..30213ac4d9 100644 --- a/packages/core/src/tools/confirmation-policy.test.ts +++ b/packages/core/src/tools/confirmation-policy.test.ts @@ -16,6 +16,7 @@ import { MessageBusType } from '../confirmation-bus/types.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import type { Config } from '../config/config.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs'; import os from 'node:os'; @@ -70,6 +71,27 @@ describe('Tool Confirmation Policy Updates', () => { isPathWithinWorkspace: () => true, getDirectories: () => [rootDir], }), + storage: { + getProjectTempDir: () => path.join(os.tmpdir(), 'gemini-cli-temp'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, }; }); diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 142bbde364..445e048202 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -55,6 +55,7 @@ import { getMockMessageBusInstance, } from '../test-utils/mock-message-bus.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs'; import os from 'node:os'; import { ApprovalMode } from '../policy/types.js'; @@ -122,6 +123,27 @@ describe('EditTool', () => { isInteractive: () => false, getDisableLLMCorrection: vi.fn(() => true), getExperiments: () => {}, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; (mockConfig.getApprovalMode as Mock).mockClear(); @@ -370,9 +392,7 @@ describe('EditTool', () => { old_string: 'old', new_string: 'new', }; - expect(tool.validateToolParams(params)).toMatch( - /must be within one of the workspace directories/, - ); + expect(tool.validateToolParams(params)).toMatch(/Path not in workspace/); }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 088ec9d0d3..baaefe9a92 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as fs from 'node:fs'; +import * as fsPromises from 'node:fs/promises'; import * as path from 'node:path'; import * as crypto from 'node:crypto'; import * as Diff from 'diff'; @@ -763,6 +763,22 @@ class EditToolInvocation * @returns Result of the edit operation */ async execute(signal: AbortSignal): Promise { + const resolvedPath = path.resolve( + this.config.getTargetDir(), + this.params.file_path, + ); + const validationError = this.config.validatePathAccess(resolvedPath); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Error: Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + let editData: CalculatedEdit; try { editData = await this.calculateEdit(this.params, signal); @@ -793,7 +809,7 @@ class EditToolInvocation } try { - this.ensureParentDirectoriesExist(this.params.file_path); + await this.ensureParentDirectoriesExistAsync(this.params.file_path); let finalContent = editData.newContent; // Restore original line endings if they were CRLF @@ -868,10 +884,14 @@ class EditToolInvocation /** * Creates parent directories if they don't exist */ - private ensureParentDirectoriesExist(filePath: string): void { + private async ensureParentDirectoriesExistAsync( + filePath: string, + ): Promise { const dirName = path.dirname(filePath); - if (!fs.existsSync(dirName)) { - fs.mkdirSync(dirName, { recursive: true }); + try { + await fsPromises.access(dirName); + } catch { + await fsPromises.mkdir(dirName, { recursive: true }); } } } @@ -978,13 +998,7 @@ A good instruction should concisely answer: } params.file_path = filePath; - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(params.file_path)) { - const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join(', ')}`; - } - - return null; + return this.config.validatePathAccess(params.file_path); } protected createInvocation( diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index d015c37e59..e96b3e7650 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -8,6 +8,7 @@ import type { GlobToolParams, GlobPath } from './glob.js'; import { GlobTool, sortFileEntries } from './glob.js'; import { partListUnionToString } from '../core/geminiRequest.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs/promises'; import os from 'node:os'; import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -17,6 +18,7 @@ import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.j import { ToolErrorType } from './tool-error.js'; import * as glob from 'glob'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; vi.mock('glob', { spy: true }); @@ -24,26 +26,48 @@ describe('GlobTool', () => { let tempRootDir: string; // This will be the rootDirectory for the GlobTool instance let globTool: GlobTool; const abortSignal = new AbortController().signal; - - // Mock config for testing - const mockConfig = { - getFileService: () => new FileDiscoveryService(tempRootDir), - getFileFilteringRespectGitIgnore: () => true, - getFileFilteringOptions: () => ({ - respectGitIgnore: true, - respectGeminiIgnore: true, - }), - getTargetDir: () => tempRootDir, - getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), - getFileExclusions: () => ({ - getGlobExcludes: () => [], - }), - } as unknown as Config; + let mockConfig: Config; beforeEach(async () => { // Create a unique root directory for each test run tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-')); await fs.writeFile(path.join(tempRootDir, '.git'), ''); // Fake git repo + + const rootDir = tempRootDir; + const workspaceContext = createMockWorkspaceContext(rootDir); + const fileDiscovery = new FileDiscoveryService(rootDir); + + const mockStorage = { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }; + + mockConfig = { + getTargetDir: () => rootDir, + getWorkspaceContext: () => workspaceContext, + getFileService: () => fileDiscovery, + getFileFilteringOptions: () => DEFAULT_FILE_FILTERING_OPTIONS, + getFileExclusions: () => ({ getGlobExcludes: () => [] }), + storage: mockStorage, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; + globTool = new GlobTool(mockConfig, createMockMessageBus()); // Create some test files and directories within this root @@ -73,6 +97,7 @@ describe('GlobTool', () => { afterEach(async () => { // Clean up the temporary root directory await fs.rm(tempRootDir, { recursive: true, force: true }); + vi.resetAllMocks(); }); describe('execute', () => { @@ -198,341 +223,286 @@ describe('GlobTool', () => { const invocation = globTool.build(params); const result = await invocation.execute(abortSignal); const llmContent = partListUnionToString(result.llmContent); - - expect(llmContent).toContain('Found 2 file(s)'); - // Ensure llmContent is a string for TypeScript type checking - expect(typeof llmContent).toBe('string'); - - const filesListed = llmContent - .trim() - .split(/\r?\n/) - .slice(1) - .map((line) => line.trim()) - .filter(Boolean); - - expect(filesListed).toHaveLength(2); - expect(path.resolve(filesListed[0])).toBe( - path.resolve(tempRootDir, 'newer.sortme'), - ); - expect(path.resolve(filesListed[1])).toBe( - path.resolve(tempRootDir, 'older.sortme'), - ); + const newerIndex = llmContent.indexOf('newer.sortme'); + const olderIndex = llmContent.indexOf('older.sortme'); + expect(newerIndex).toBeLessThan(olderIndex); }, 30000); it('should return a PATH_NOT_IN_WORKSPACE error if path is outside workspace', async () => { - // Bypassing validation to test execute method directly - vi.spyOn(globTool, 'validateToolParams').mockReturnValue(null); - const params: GlobToolParams = { pattern: '*.txt', dir_path: '/etc' }; - const invocation = globTool.build(params); - const result = await invocation.execute(abortSignal); - expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); - expect(result.returnDisplay).toBe('Path is not within workspace'); - }, 30000); + const params: GlobToolParams = { pattern: '*', dir_path: '/etc' }; + expect(() => globTool.build(params)).toThrow(/Path not in workspace/); + }); it('should return a GLOB_EXECUTION_ERROR on glob failure', async () => { vi.mocked(glob.glob).mockRejectedValue(new Error('Glob failed')); - const params: GlobToolParams = { pattern: '*.txt' }; + const params: GlobToolParams = { pattern: '*' }; const invocation = globTool.build(params); const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.GLOB_EXECUTION_ERROR); - expect(result.llmContent).toContain( - 'Error during glob search operation: Glob failed', - ); - // Reset glob. - vi.mocked(glob.glob).mockReset(); }, 30000); }); describe('validateToolParams', () => { - it.each([ - { - name: 'should return null for valid parameters (pattern only)', - params: { pattern: '*.js' }, - expected: null, - }, - { - name: 'should return null for valid parameters (pattern and dir_path)', - params: { pattern: '*.js', dir_path: 'sub' }, - expected: null, - }, - { - name: 'should return null for valid parameters (pattern, dir_path, and case_sensitive)', - params: { pattern: '*.js', dir_path: 'sub', case_sensitive: true }, - expected: null, - }, - { - name: 'should return error if pattern is missing (schema validation)', - params: { dir_path: '.' }, - expected: `params must have required property 'pattern'`, - }, - { - name: 'should return error if pattern is an empty string', - params: { pattern: '' }, - expected: "The 'pattern' parameter cannot be empty.", - }, - { - name: 'should return error if pattern is only whitespace', - params: { pattern: ' ' }, - expected: "The 'pattern' parameter cannot be empty.", - }, - { - name: 'should return error if dir_path is not a string (schema validation)', - params: { pattern: '*.ts', dir_path: 123 }, - expected: 'params/dir_path must be string', - }, - { - name: 'should return error if case_sensitive is not a boolean (schema validation)', - params: { pattern: '*.ts', case_sensitive: 'true' }, - expected: 'params/case_sensitive must be boolean', - }, - { - name: "should return error if search path resolves outside the tool's root directory", - params: { - pattern: '*.txt', - dir_path: '../../../../../../../../../../tmp', - }, - expected: 'resolves outside the allowed workspace directories', - }, - { - name: 'should return error if specified search path does not exist', - params: { pattern: '*.txt', dir_path: 'nonexistent_subdir' }, - expected: 'Search path does not exist', - }, - { - name: 'should return error if specified search path is a file, not a directory', - params: { pattern: '*.txt', dir_path: 'fileA.txt' }, - expected: 'Search path is not a directory', - }, - ])('$name', ({ params, expected }) => { - // @ts-expect-error - We're intentionally creating invalid params for testing - const result = globTool.validateToolParams(params); - if (expected === null) { - expect(result).toBeNull(); - } else { - expect(result).toContain(expected); - } + it('should return null for valid parameters', () => { + const params: GlobToolParams = { pattern: '*.txt' }; + expect(globTool.validateToolParams(params)).toBeNull(); + }); + + it('should return null for valid parameters with dir_path', () => { + const params: GlobToolParams = { pattern: '*.txt', dir_path: 'sub' }; + expect(globTool.validateToolParams(params)).toBeNull(); + }); + + it('should return null for valid parameters with absolute dir_path within workspace', async () => { + const params: GlobToolParams = { + pattern: '*.txt', + dir_path: tempRootDir, + }; + expect(globTool.validateToolParams(params)).toBeNull(); + }); + + it('should return error if pattern is missing', () => { + const params = {} as unknown as GlobToolParams; + expect(globTool.validateToolParams(params)).toContain( + "params must have required property 'pattern'", + ); + }); + + it('should return error if pattern is an empty string', () => { + const params: GlobToolParams = { pattern: '' }; + expect(globTool.validateToolParams(params)).toContain( + "The 'pattern' parameter cannot be empty", + ); + }); + + it('should return error if pattern is only whitespace', () => { + const params: GlobToolParams = { pattern: ' ' }; + expect(globTool.validateToolParams(params)).toContain( + "The 'pattern' parameter cannot be empty", + ); + }); + + it('should return error if dir_path is not a string', () => { + const params = { + pattern: '*', + dir_path: 123, + } as unknown as GlobToolParams; + expect(globTool.validateToolParams(params)).toContain( + 'params/dir_path must be string', + ); + }); + + it('should return error if case_sensitive is not a boolean', () => { + const params = { + pattern: '*', + case_sensitive: 'true', + } as unknown as GlobToolParams; + expect(globTool.validateToolParams(params)).toContain( + 'params/case_sensitive must be boolean', + ); + }); + + it('should return error if search path resolves outside workspace', () => { + const params: GlobToolParams = { pattern: '*', dir_path: '../' }; + expect(globTool.validateToolParams(params)).toContain( + 'resolves outside the allowed workspace directories', + ); + }); + + it('should return error if specified search path does not exist', () => { + const params: GlobToolParams = { + pattern: '*', + dir_path: 'non-existent', + }; + expect(globTool.validateToolParams(params)).toContain( + 'Search path does not exist', + ); + }); + + it('should return error if specified search path is not a directory', async () => { + await fs.writeFile(path.join(tempRootDir, 'not-a-dir'), 'content'); + const params: GlobToolParams = { pattern: '*', dir_path: 'not-a-dir' }; + expect(globTool.validateToolParams(params)).toContain( + 'Search path is not a directory', + ); }); }); describe('workspace boundary validation', () => { it('should validate search paths are within workspace boundaries', () => { - const validPath = { pattern: '*.ts', dir_path: 'sub' }; - const invalidPath = { pattern: '*.ts', dir_path: '../..' }; + expect(globTool.validateToolParams({ pattern: '*' })).toBeNull(); + expect( + globTool.validateToolParams({ pattern: '*', dir_path: '.' }), + ).toBeNull(); + expect( + globTool.validateToolParams({ pattern: '*', dir_path: tempRootDir }), + ).toBeNull(); - expect(globTool.validateToolParams(validPath)).toBeNull(); - expect(globTool.validateToolParams(invalidPath)).toContain( - 'resolves outside the allowed workspace directories', - ); + expect( + globTool.validateToolParams({ pattern: '*', dir_path: '..' }), + ).toContain('resolves outside the allowed workspace directories'); + expect( + globTool.validateToolParams({ pattern: '*', dir_path: '/' }), + ).toContain('resolves outside the allowed workspace directories'); }); it('should provide clear error messages when path is outside workspace', () => { - const invalidPath = { pattern: '*.ts', dir_path: '/etc' }; - const error = globTool.validateToolParams(invalidPath); - - expect(error).toContain( + const result = globTool.validateToolParams({ + pattern: '*', + dir_path: '/tmp/outside', + }); + expect(result).toContain( 'resolves outside the allowed workspace directories', ); - expect(error).toContain(tempRootDir); }); it('should work with paths in workspace subdirectories', async () => { - const params: GlobToolParams = { pattern: '*.md', dir_path: 'sub' }; - const invocation = globTool.build(params); - const result = await invocation.execute(abortSignal); - - expect(result.llmContent).toContain('Found 2 file(s)'); - expect(result.llmContent).toContain('fileC.md'); - expect(result.llmContent).toContain('FileD.MD'); + const subDir = path.join(tempRootDir, 'allowed-sub'); + await fs.mkdir(subDir); + expect( + globTool.validateToolParams({ pattern: '*', dir_path: 'allowed-sub' }), + ).toBeNull(); }); }); describe('ignore file handling', () => { - interface IgnoreFileTestCase { - name: string; - ignoreFile: { name: string; content: string }; - filesToCreate: string[]; - globToolParams: GlobToolParams; - expectedCountMessage: string; - expectedToContain?: string[]; - notExpectedToContain?: string[]; - } + it('should respect .gitignore files by default', async () => { + await fs.writeFile( + path.join(tempRootDir, '.gitignore'), + 'ignored_test.txt', + ); + await fs.writeFile(path.join(tempRootDir, 'ignored_test.txt'), 'content'); + await fs.writeFile(path.join(tempRootDir, 'visible_test.txt'), 'content'); - it.each([ - { - name: 'should respect .gitignore files by default', - ignoreFile: { name: '.gitignore', content: '*.ignored.txt' }, - filesToCreate: ['a.ignored.txt', 'b.notignored.txt'], - globToolParams: { pattern: '*.txt' }, - expectedCountMessage: 'Found 3 file(s)', - notExpectedToContain: ['a.ignored.txt'], - }, - { - name: 'should respect .geminiignore files by default', - ignoreFile: { name: '.geminiignore', content: '*.geminiignored.txt' }, - filesToCreate: ['a.geminiignored.txt', 'b.notignored.txt'], - globToolParams: { pattern: '*.txt' }, - expectedCountMessage: 'Found 3 file(s)', - notExpectedToContain: ['a.geminiignored.txt'], - }, - { - name: 'should not respect .gitignore when respect_git_ignore is false', - ignoreFile: { name: '.gitignore', content: '*.ignored.txt' }, - filesToCreate: ['a.ignored.txt'], - globToolParams: { pattern: '*.txt', respect_git_ignore: false }, - expectedCountMessage: 'Found 3 file(s)', - expectedToContain: ['a.ignored.txt'], - }, - { - name: 'should not respect .geminiignore when respect_gemini_ignore is false', - ignoreFile: { name: '.geminiignore', content: '*.geminiignored.txt' }, - filesToCreate: ['a.geminiignored.txt'], - globToolParams: { pattern: '*.txt', respect_gemini_ignore: false }, - expectedCountMessage: 'Found 3 file(s)', - expectedToContain: ['a.geminiignored.txt'], - }, - ])( - '$name', - async ({ - ignoreFile, - filesToCreate, - globToolParams, - expectedCountMessage, - expectedToContain, - notExpectedToContain, - }) => { - await fs.writeFile( - path.join(tempRootDir, ignoreFile.name), - ignoreFile.content, - ); - for (const file of filesToCreate) { - await fs.writeFile(path.join(tempRootDir, file), 'content'); - } + const params: GlobToolParams = { pattern: '*_test.txt' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); - const invocation = globTool.build(globToolParams); - const result = await invocation.execute(abortSignal); + expect(result.llmContent).toContain('Found 1 file(s)'); + expect(result.llmContent).toContain('visible_test.txt'); + expect(result.llmContent).not.toContain('ignored_test.txt'); + }, 30000); - expect(result.llmContent).toContain(expectedCountMessage); + it('should respect .geminiignore files by default', async () => { + await fs.writeFile( + path.join(tempRootDir, '.geminiignore'), + 'gemini-ignored_test.txt', + ); + await fs.writeFile( + path.join(tempRootDir, 'gemini-ignored_test.txt'), + 'content', + ); + await fs.writeFile(path.join(tempRootDir, 'visible_test.txt'), 'content'); - if (expectedToContain) { - for (const file of expectedToContain) { - expect(result.llmContent).toContain(file); - } - } - if (notExpectedToContain) { - for (const file of notExpectedToContain) { - expect(result.llmContent).not.toContain(file); - } - } - }, - ); + const params: GlobToolParams = { pattern: 'visible_test.txt' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 1 file(s)'); + expect(result.llmContent).toContain('visible_test.txt'); + expect(result.llmContent).not.toContain('gemini-ignored_test.txt'); + }, 30000); + + it('should not respect .gitignore when respect_git_ignore is false', async () => { + await fs.writeFile( + path.join(tempRootDir, '.gitignore'), + 'ignored_test.txt', + ); + await fs.writeFile(path.join(tempRootDir, 'ignored_test.txt'), 'content'); + + const params: GlobToolParams = { + pattern: 'ignored_test.txt', + respect_git_ignore: false, + }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 1 file(s)'); + expect(result.llmContent).toContain('ignored_test.txt'); + }, 30000); + + it('should not respect .geminiignore when respect_gemini_ignore is false', async () => { + await fs.writeFile( + path.join(tempRootDir, '.geminiignore'), + 'gemini-ignored_test.txt', + ); + await fs.writeFile( + path.join(tempRootDir, 'gemini-ignored_test.txt'), + 'content', + ); + + const params: GlobToolParams = { + pattern: 'gemini-ignored_test.txt', + respect_gemini_ignore: false, + }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 1 file(s)'); + expect(result.llmContent).toContain('gemini-ignored_test.txt'); + }, 30000); }); }); describe('sortFileEntries', () => { - const nowTimestamp = new Date('2024-01-15T12:00:00.000Z').getTime(); - const oneDayInMs = 24 * 60 * 60 * 1000; + const now = 1000000; + const threshold = 10000; - const createFileEntry = (fullpath: string, mtimeDate: Date): GlobPath => ({ - fullpath: () => fullpath, - mtimeMs: mtimeDate.getTime(), + it('should sort a mix of recent and older files correctly', () => { + const entries: GlobPath[] = [ + { fullpath: () => 'older-b.txt', mtimeMs: now - 20000 }, + { fullpath: () => 'recent-b.txt', mtimeMs: now - 1000 }, + { fullpath: () => 'recent-a.txt', mtimeMs: now - 500 }, + { fullpath: () => 'older-a.txt', mtimeMs: now - 30000 }, + ]; + + const sorted = sortFileEntries(entries, now, threshold); + expect(sorted.map((e) => e.fullpath())).toEqual([ + 'recent-a.txt', // Recent, newest first + 'recent-b.txt', + 'older-a.txt', // Older, alphabetical + 'older-b.txt', + ]); }); - const testCases = [ - { - name: 'should sort a mix of recent and older files correctly', - entries: [ - { - name: 'older_zebra.txt', - mtime: new Date(nowTimestamp - (oneDayInMs + 2 * 60 * 60 * 1000)), - }, - { - name: 'recent_alpha.txt', - mtime: new Date(nowTimestamp - 1 * 60 * 60 * 1000), - }, - { - name: 'older_apple.txt', - mtime: new Date(nowTimestamp - (oneDayInMs + 1 * 60 * 60 * 1000)), - }, - { - name: 'recent_beta.txt', - mtime: new Date(nowTimestamp - 2 * 60 * 60 * 1000), - }, - { - name: 'older_banana.txt', - mtime: new Date(nowTimestamp - (oneDayInMs + 1 * 60 * 60 * 1000)), - }, - ], - expected: [ - 'recent_alpha.txt', - 'recent_beta.txt', - 'older_apple.txt', - 'older_banana.txt', - 'older_zebra.txt', - ], - }, - { - name: 'should sort only recent files by mtime descending', - entries: [ - { name: 'c.txt', mtime: new Date(nowTimestamp - 2000) }, - { name: 'a.txt', mtime: new Date(nowTimestamp - 3000) }, - { name: 'b.txt', mtime: new Date(nowTimestamp - 1000) }, - ], - expected: ['b.txt', 'c.txt', 'a.txt'], - }, - { - name: 'should sort only older files alphabetically by path', - entries: [ - { name: 'zebra.txt', mtime: new Date(nowTimestamp - 2 * oneDayInMs) }, - { name: 'apple.txt', mtime: new Date(nowTimestamp - 2 * oneDayInMs) }, - { name: 'banana.txt', mtime: new Date(nowTimestamp - 2 * oneDayInMs) }, - ], - expected: ['apple.txt', 'banana.txt', 'zebra.txt'], - }, - { - name: 'should handle an empty array', - entries: [], - expected: [], - }, - { - name: 'should correctly sort files when mtimes are identical for recent files', - entries: [ - { name: 'b.txt', mtime: new Date(nowTimestamp - 1000) }, - { name: 'a.txt', mtime: new Date(nowTimestamp - 1000) }, - ], - expectedUnordered: ['a.txt', 'b.txt'], - }, - { - name: 'should use recencyThresholdMs parameter correctly', - recencyThresholdMs: 1000, - entries: [ - { name: 'older_file.txt', mtime: new Date(nowTimestamp - 1001) }, - { name: 'recent_file.txt', mtime: new Date(nowTimestamp - 999) }, - ], - expected: ['recent_file.txt', 'older_file.txt'], - }, - ]; + it('should sort only recent files by mtime descending', () => { + const entries: GlobPath[] = [ + { fullpath: () => 'a.txt', mtimeMs: now - 2000 }, + { fullpath: () => 'b.txt', mtimeMs: now - 1000 }, + ]; + const sorted = sortFileEntries(entries, now, threshold); + expect(sorted.map((e) => e.fullpath())).toEqual(['b.txt', 'a.txt']); + }); - it.each(testCases)( - '$name', - ({ entries, expected, expectedUnordered, recencyThresholdMs }) => { - const globPaths = entries.map((e) => createFileEntry(e.name, e.mtime)); - const sorted = sortFileEntries( - globPaths, - nowTimestamp, - recencyThresholdMs ?? oneDayInMs, - ); - const sortedPaths = sorted.map((e) => e.fullpath()); + it('should sort only older files alphabetically', () => { + const entries: GlobPath[] = [ + { fullpath: () => 'b.txt', mtimeMs: now - 20000 }, + { fullpath: () => 'a.txt', mtimeMs: now - 30000 }, + ]; + const sorted = sortFileEntries(entries, now, threshold); + expect(sorted.map((e) => e.fullpath())).toEqual(['a.txt', 'b.txt']); + }); - if (expected) { - expect(sortedPaths).toEqual(expected); - } else if (expectedUnordered) { - expect(sortedPaths).toHaveLength(expectedUnordered.length); - for (const path of expectedUnordered) { - expect(sortedPaths).toContain(path); - } - } else { - throw new Error('Test case must have expected or expectedUnordered'); - } - }, - ); + it('should handle an empty array', () => { + expect(sortFileEntries([], now, threshold)).toEqual([]); + }); + + it('should correctly sort files when mtimeMs is missing', () => { + const entries: GlobPath[] = [ + { fullpath: () => 'b.txt' }, + { fullpath: () => 'a.txt' }, + ]; + const sorted = sortFileEntries(entries, now, threshold); + expect(sorted.map((e) => e.fullpath())).toEqual(['a.txt', 'b.txt']); + }); + + it('should use recencyThresholdMs parameter', () => { + const customThreshold = 5000; + const entries: GlobPath[] = [ + { fullpath: () => 'old.txt', mtimeMs: now - 8000 }, + { fullpath: () => 'new.txt', mtimeMs: now - 3000 }, + ]; + const sorted = sortFileEntries(entries, now, customThreshold); + expect(sorted.map((e) => e.fullpath())).toEqual(['new.txt', 'old.txt']); + }); }); diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 7a98d8e3e2..23c38871f7 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -123,13 +123,14 @@ class GlobToolInvocation extends BaseToolInvocation< this.config.getTargetDir(), this.params.dir_path, ); - if (!workspaceContext.isPathWithinWorkspace(searchDirAbsolute)) { - const rawError = `Error: Path "${this.params.dir_path}" is not within any workspace directory`; + const validationError = + this.config.validatePathAccess(searchDirAbsolute); + if (validationError) { return { - llmContent: rawError, - returnDisplay: `Path is not within workspace`, + llmContent: validationError, + returnDisplay: 'Path not in workspace.', error: { - message: rawError, + message: validationError, type: ToolErrorType.PATH_NOT_IN_WORKSPACE, }, }; @@ -317,10 +318,9 @@ export class GlobTool extends BaseDeclarativeTool { params.dir_path || '.', ); - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(searchDirAbsolute)) { - const directories = workspaceContext.getDirectories(); - return `Search path ("${searchDirAbsolute}") resolves outside the allowed workspace directories: ${directories.join(', ')}`; + const validationError = this.config.validatePathAccess(searchDirAbsolute); + if (validationError) { + return validationError; } const targetDir = searchDirAbsolute || this.config.getTargetDir(); diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 0f0db86665..3f1f023faf 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -9,6 +9,7 @@ import type { GrepToolParams } from './grep.js'; import { GrepTool } from './grep.js'; import type { ToolResult } from './tools.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs/promises'; import os from 'node:os'; import type { Config } from '../config/config.js'; @@ -42,17 +43,40 @@ describe('GrepTool', () => { let tempRootDir: string; let grepTool: GrepTool; const abortSignal = new AbortController().signal; - - const mockConfig = { - getTargetDir: () => tempRootDir, - getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), - getFileExclusions: () => ({ - getGlobExcludes: () => [], - }), - } as unknown as Config; + let mockConfig: Config; beforeEach(async () => { tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); + + mockConfig = { + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), + getFileExclusions: () => ({ + getGlobExcludes: () => [], + }), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; + grepTool = new GrepTool(mockConfig, createMockMessageBus()); // Create some test files and directories @@ -120,7 +144,7 @@ describe('GrepTool', () => { }; // Check for the core error message, as the full path might vary expect(grepTool.validateToolParams(params)).toContain( - 'Failed to access path stats for', + 'Path does not exist', ); expect(grepTool.validateToolParams(params)).toContain('nonexistent'); }); @@ -311,6 +335,27 @@ describe('GrepTool', () => { getFileExclusions: () => ({ getGlobExcludes: () => [], }), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; const multiDirGrepTool = new GrepTool( @@ -367,6 +412,27 @@ describe('GrepTool', () => { getFileExclusions: () => ({ getGlobExcludes: () => [], }), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; const multiDirGrepTool = new GrepTool( diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index ed4fdcb93a..f1a0d413fe 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -74,47 +74,6 @@ class GrepToolInvocation extends BaseToolInvocation< this.fileExclusions = config.getFileExclusions(); } - /** - * Checks if a path is within the root directory and resolves it. - * @param relativePath Path relative to the root directory (or undefined for root). - * @returns The absolute path if valid and exists, or null if no path specified (to search all directories). - * @throws {Error} If path is outside root, doesn't exist, or isn't a directory. - */ - private resolveAndValidatePath(relativePath?: string): string | null { - // If no path specified, return null to indicate searching all workspace directories - if (!relativePath) { - return null; - } - - const targetPath = path.resolve(this.config.getTargetDir(), relativePath); - - // Security Check: Ensure the resolved path is within workspace boundaries - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(targetPath)) { - const directories = workspaceContext.getDirectories(); - throw new Error( - `Path validation failed: Attempted path "${relativePath}" resolves outside the allowed workspace directories: ${directories.join(', ')}`, - ); - } - - // Check existence and type after resolving - try { - const stats = fs.statSync(targetPath); - if (!stats.isDirectory()) { - throw new Error(`Path is not a directory: ${targetPath}`); - } - } catch (error: unknown) { - if (isNodeError(error) && error.code !== 'ENOENT') { - throw new Error(`Path does not exist: ${targetPath}`); - } - throw new Error( - `Failed to access path stats for ${targetPath}: ${error}`, - ); - } - - return targetPath; - } - /** * Parses a single line of grep-like output (git grep, system grep). * Expects format: filePath:lineNumber:lineContent @@ -159,8 +118,59 @@ class GrepToolInvocation extends BaseToolInvocation< async execute(signal: AbortSignal): Promise { try { const workspaceContext = this.config.getWorkspaceContext(); - const searchDirAbs = this.resolveAndValidatePath(this.params.dir_path); - const searchDirDisplay = this.params.dir_path || '.'; + const pathParam = this.params.dir_path; + + let searchDirAbs: string | null = null; + if (pathParam) { + searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam); + const validationError = this.config.validatePathAccess(searchDirAbs); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Error: Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + + try { + const stats = await fsPromises.stat(searchDirAbs); + if (!stats.isDirectory()) { + return { + llmContent: `Path is not a directory: ${searchDirAbs}`, + returnDisplay: 'Error: Path is not a directory.', + error: { + message: `Path is not a directory: ${searchDirAbs}`, + type: ToolErrorType.PATH_IS_NOT_A_DIRECTORY, + }, + }; + } + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { + llmContent: `Path does not exist: ${searchDirAbs}`, + returnDisplay: 'Error: Path does not exist.', + error: { + message: `Path does not exist: ${searchDirAbs}`, + type: ToolErrorType.FILE_NOT_FOUND, + }, + }; + } + const errorMessage = getErrorMessage(error); + return { + llmContent: `Failed to access path stats for ${searchDirAbs}: ${errorMessage}`, + returnDisplay: 'Error: Failed to access path.', + error: { + message: `Failed to access path stats for ${searchDirAbs}: ${errorMessage}`, + type: ToolErrorType.GREP_EXECUTION_ERROR, + }, + }; + } + } + + const searchDirDisplay = pathParam || '.'; // Determine which directories to search let searchDirectories: readonly string[]; @@ -256,7 +266,8 @@ class GrepToolInvocation extends BaseToolInvocation< let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n---\n`; for (const filePath in matchesByFile) { - llmContent += `File: ${filePath}\n`; + llmContent += `File: ${filePath} +`; matchesByFile[filePath].forEach((match) => { const trimmedLine = match.line.trim(); llmContent += `L${match.lineNumber}: ${trimmedLine}\n`; @@ -586,47 +597,6 @@ export class GrepTool extends BaseDeclarativeTool { ); } - /** - * Checks if a path is within the root directory and resolves it. - * @param relativePath Path relative to the root directory (or undefined for root). - * @returns The absolute path if valid and exists, or null if no path specified (to search all directories). - * @throws {Error} If path is outside root, doesn't exist, or isn't a directory. - */ - private resolveAndValidatePath(relativePath?: string): string | null { - // If no path specified, return null to indicate searching all workspace directories - if (!relativePath) { - return null; - } - - const targetPath = path.resolve(this.config.getTargetDir(), relativePath); - - // Security Check: Ensure the resolved path is within workspace boundaries - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(targetPath)) { - const directories = workspaceContext.getDirectories(); - throw new Error( - `Path validation failed: Attempted path "${relativePath}" resolves outside the allowed workspace directories: ${directories.join(', ')}`, - ); - } - - // Check existence and type after resolving - try { - const stats = fs.statSync(targetPath); - if (!stats.isDirectory()) { - throw new Error(`Path is not a directory: ${targetPath}`); - } - } catch (error: unknown) { - if (isNodeError(error) && error.code !== 'ENOENT') { - throw new Error(`Path does not exist: ${targetPath}`); - } - throw new Error( - `Failed to access path stats for ${targetPath}: ${error}`, - ); - } - - return targetPath; - } - /** * Validates the parameters for the tool * @param params Parameters to validate @@ -643,10 +613,26 @@ export class GrepTool extends BaseDeclarativeTool { // Only validate dir_path if one is provided if (params.dir_path) { + const resolvedPath = path.resolve( + this.config.getTargetDir(), + params.dir_path, + ); + const validationError = this.config.validatePathAccess(resolvedPath); + if (validationError) { + return validationError; + } + + // We still want to check if it's a directory try { - this.resolveAndValidatePath(params.dir_path); - } catch (error) { - return getErrorMessage(error); + const stats = fs.statSync(resolvedPath); + if (!stats.isDirectory()) { + return `Path is not a directory: ${resolvedPath}`; + } + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return `Path does not exist: ${resolvedPath}`; + } + return `Failed to access path stats for ${resolvedPath}: ${getErrorMessage(error)}`; } } diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 06e8da264e..e358d24186 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import fs from 'node:fs/promises'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import os from 'node:os'; import { LSTool } from './ls.js'; import type { Config } from '../config/config.js'; @@ -29,6 +30,10 @@ describe('LSTool', () => { path.join(realTmp, 'ls-tool-secondary-'), ); + const mockStorage = { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }; + mockConfig = { getTargetDir: () => tempRootDir, getWorkspaceContext: () => @@ -38,6 +43,25 @@ describe('LSTool', () => { respectGitIgnore: true, respectGeminiIgnore: true, }), + storage: mockStorage, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; lsTool = new LSTool(mockConfig, createMockMessageBus()); @@ -70,7 +94,7 @@ describe('LSTool', () => { it('should reject paths outside workspace with clear error message', () => { expect(() => lsTool.build({ dir_path: '/etc/passwd' })).toThrow( - `Path must be within one of the workspace directories: ${tempRootDir}, ${tempSecondaryDir}`, + /Path not in workspace: Attempted path ".*" resolves outside the allowed workspace directories: .*/, ); }); @@ -297,7 +321,7 @@ describe('LSTool', () => { it('should reject paths outside all workspace directories', () => { const params = { dir_path: '/etc/passwd' }; expect(() => lsTool.build(params)).toThrow( - 'Path must be within one of the workspace directories', + /Path not in workspace: Attempted path ".*" resolves outside the allowed workspace directories: .*/, ); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 80a5ecbc0d..6241d28793 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -142,6 +142,19 @@ class LSToolInvocation extends BaseToolInvocation { this.config.getTargetDir(), this.params.dir_path, ); + + const validationError = this.config.validatePathAccess(resolvedDirPath); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + try { const stats = await fs.stat(resolvedDirPath); if (!stats) { @@ -318,14 +331,7 @@ export class LSTool extends BaseDeclarativeTool { this.config.getTargetDir(), params.dir_path, ); - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(resolvedPath)) { - const directories = workspaceContext.getDirectories(); - return `Path must be within one of the workspace directories: ${directories.join( - ', ', - )}`; - } - return null; + return this.config.validatePathAccess(resolvedPath); } protected createInvocation( diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 0194e18288..275b8fb25c 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -9,6 +9,7 @@ import type { ReadFileToolParams } from './read-file.js'; import { ReadFileTool } from './read-file.js'; import { ToolErrorType } from './tool-error.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import os from 'node:os'; import fs from 'node:fs'; import fsp from 'node:fs/promises'; @@ -46,6 +47,24 @@ describe('ReadFileTool', () => { getProjectTempDir: () => path.join(tempRootDir, '.temp'), }, isInteractive: () => false, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; tool = new ReadFileTool(mockConfigInstance, createMockMessageBus()); }); @@ -82,9 +101,7 @@ describe('ReadFileTool', () => { const params: ReadFileToolParams = { file_path: '/outside/root.txt', }; - expect(() => tool.build(params)).toThrow( - /File path must be within one of the workspace directories/, - ); + expect(() => tool.build(params)).toThrow(/Path not in workspace/); }); it('should allow access to files in project temp directory', () => { @@ -100,9 +117,7 @@ describe('ReadFileTool', () => { const params: ReadFileToolParams = { file_path: '/completely/outside/path.txt', }; - expect(() => tool.build(params)).toThrow( - /File path must be within one of the workspace directories.*or within the project temp directory/, - ); + expect(() => tool.build(params)).toThrow(/Path not in workspace/); }); it('should throw error if path is empty', () => { @@ -438,6 +453,27 @@ describe('ReadFileTool', () => { storage: { getProjectTempDir: () => path.join(tempRootDir, '.temp'), }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess( + this: Config, + absolutePath: string, + ): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; tool = new ReadFileTool(mockConfigInstance, createMockMessageBus()); }); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index f748bf8b45..d7a3684b51 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -9,6 +9,7 @@ import path from 'node:path'; import { makeRelative, shortenPath } from '../utils/paths.js'; import type { ToolInvocation, ToolLocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; import type { PartUnion } from '@google/genai'; import { @@ -74,6 +75,18 @@ class ReadFileToolInvocation extends BaseToolInvocation< } async execute(): Promise { + const validationError = this.config.validatePathAccess(this.resolvedPath); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + const result = await processSingleFileContent( this.resolvedPath, this.config.getTargetDir(), @@ -189,24 +202,16 @@ export class ReadFileTool extends BaseDeclarativeTool< return "The 'file_path' parameter must be non-empty."; } - const workspaceContext = this.config.getWorkspaceContext(); - const projectTempDir = this.config.storage.getProjectTempDir(); const resolvedPath = path.resolve( this.config.getTargetDir(), params.file_path, ); - const resolvedProjectTempDir = path.resolve(projectTempDir); - const isWithinTempDir = - resolvedPath.startsWith(resolvedProjectTempDir + path.sep) || - resolvedPath === resolvedProjectTempDir; - if ( - !workspaceContext.isPathWithinWorkspace(resolvedPath) && - !isWithinTempDir - ) { - const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join(', ')} or within the project temp directory: ${projectTempDir}`; + const validationError = this.config.validatePathAccess(resolvedPath); + if (validationError) { + return validationError; } + if (params.offset !== undefined && params.offset < 0) { return 'Offset must be a non-negative number'; } diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index e092b6d6a5..6b2c13fdee 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -10,6 +10,7 @@ import { mockControl } from '../__mocks__/fs/promises.js'; import { ReadManyFilesTool } from './read-many-files.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs'; // Actual fs for setup import os from 'node:os'; import type { Config } from '../config/config.js'; @@ -90,7 +91,28 @@ describe('ReadManyFilesTool', () => { getReadManyFilesExcludes: () => DEFAULT_FILE_EXCLUDES, }), isInteractive: () => false, - } as Partial as Config; + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; tool = new ReadManyFilesTool(mockConfig, createMockMessageBus()); mockReadFileFn = mockControl.mockReadFile; @@ -505,7 +527,28 @@ describe('ReadManyFilesTool', () => { getReadManyFilesExcludes: () => [], }), isInteractive: () => false, - } as Partial as Config; + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; tool = new ReadManyFilesTool(mockConfig, createMockMessageBus()); fs.writeFileSync(path.join(tempDir1, 'file1.txt'), 'Content1'); diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 26ddf673a8..ab90e86a90 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -8,7 +8,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { getErrorMessage } from '../utils/errors.js'; -import * as fs from 'node:fs'; +import * as fsPromises from 'node:fs/promises'; import * as path from 'node:path'; import { glob, escape } from 'glob'; import type { ProcessedFileReadResult } from '../utils/fileUtils.js'; @@ -169,7 +169,15 @@ ${finalExclusionPatternsForDescription for (const p of include) { const normalizedP = p.replace(/\\/g, '/'); const fullPath = path.join(dir, normalizedP); - if (fs.existsSync(fullPath)) { + let exists = false; + try { + await fsPromises.access(fullPath); + exists = true; + } catch { + exists = false; + } + + if (exists) { processedPatterns.push(escape(normalizedP)); } else { // The path does not exist or is not a file, so we treat it as a glob pattern. @@ -195,6 +203,7 @@ ${finalExclusionPatternsForDescription ); const fileDiscovery = this.config.getFileService(); + const { filteredPaths, ignoredCount } = fileDiscovery.filterFilesWithReport(relativeEntries, { respectGitIgnore: @@ -211,12 +220,12 @@ ${finalExclusionPatternsForDescription // Security check: ensure the glob library didn't return something outside the workspace. const fullPath = path.resolve(this.config.getTargetDir(), relativePath); - if ( - !this.config.getWorkspaceContext().isPathWithinWorkspace(fullPath) - ) { + + const validationError = this.config.validatePathAccess(fullPath); + if (validationError) { skippedFiles.push({ path: fullPath, - reason: `Security: Glob library returned path outside workspace. Path: ${fullPath}`, + reason: 'Security: Path not in workspace', }); continue; } diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 4c27dde5b1..e1f610d3e3 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -16,6 +16,7 @@ import { import type { RipGrepToolParams } from './ripGrep.js'; import { canUseRipgrep, RipGrepTool, ensureRgPath } from './ripGrep.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs/promises'; import os from 'node:os'; import type { Config } from '../config/config.js'; @@ -246,13 +247,7 @@ describe('RipGrepTool', () => { let ripgrepBinaryPath: string; let grepTool: RipGrepTool; const abortSignal = new AbortController().signal; - - const mockConfig = { - getTargetDir: () => tempRootDir, - getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), - getDebugMode: () => false, - getFileFilteringRespectGeminiIgnore: () => true, - } as unknown as Config; + let mockConfig: Config; beforeEach(async () => { downloadRipGrepMock.mockReset(); @@ -266,6 +261,35 @@ describe('RipGrepTool', () => { await fs.writeFile(ripgrepBinaryPath, ''); storageSpy.mockImplementation(() => binDir); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); + + mockConfig = { + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), + getDebugMode: () => false, + getFileFilteringRespectGeminiIgnore: () => true, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; + grepTool = new RipGrepTool(mockConfig, createMockMessageBus()); // Create some test files and directories @@ -311,11 +335,6 @@ describe('RipGrepTool', () => { params: { pattern: 'hello', dir_path: '.', include: '*.txt' }, expected: null, }, - { - name: 'invalid regex pattern', - params: { pattern: '[[' }, - expected: null, - }, ])( 'should return null for valid params ($name)', ({ params, expected }) => { @@ -323,6 +342,13 @@ describe('RipGrepTool', () => { }, ); + it('should throw error for invalid regex pattern', () => { + const params: RipGrepToolParams = { pattern: '[[' }; + expect(grepTool.validateToolParams(params)).toMatch( + /Invalid regular expression pattern provided/, + ); + }); + it('should return error if pattern is missing', () => { const params = { dir_path: '.' } as unknown as RipGrepToolParams; expect(grepTool.validateToolParams(params)).toBe( @@ -336,10 +362,9 @@ describe('RipGrepTool', () => { dir_path: 'nonexistent', }; // Check for the core error message, as the full path might vary - expect(grepTool.validateToolParams(params)).toContain( - 'Path does not exist', - ); - expect(grepTool.validateToolParams(params)).toContain('nonexistent'); + const result = grepTool.validateToolParams(params); + expect(result).toMatch(/Path does not exist/); + expect(result).toMatch(/nonexistent/); }); it('should allow path to be a file', async () => { @@ -550,19 +575,10 @@ describe('RipGrepTool', () => { expect(result.returnDisplay).toBe('No matches found'); }); - it('should return an error from ripgrep for invalid regex pattern', async () => { - mockSpawn.mockImplementationOnce( - createMockSpawn({ - exitCode: 2, - }), - ); - + it('should throw error for invalid regex pattern during build', async () => { const params: RipGrepToolParams = { pattern: '[[' }; - const invocation = grepTool.build(params); - const result = await invocation.execute(abortSignal); - expect(result.llmContent).toContain('Process exited with code 2'); - expect(result.returnDisplay).toContain( - 'Error: Process exited with code 2', + expect(() => grepTool.build(params)).toThrow( + /Invalid regular expression pattern provided/, ); }); @@ -763,6 +779,27 @@ describe('RipGrepTool', () => { createMockWorkspaceContext(tempRootDir, [secondDir]), getDebugMode: () => false, getFileFilteringRespectGeminiIgnore: () => true, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; // Setup specific mock for this test - multi-directory search for 'world' @@ -850,6 +887,27 @@ describe('RipGrepTool', () => { createMockWorkspaceContext(tempRootDir, [secondDir]), getDebugMode: () => false, getFileFilteringRespectGeminiIgnore: () => true, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; // Setup specific mock for this test - searching in 'sub' should only return matches from that directory @@ -931,7 +989,7 @@ describe('RipGrepTool', () => { pattern: 'test', dir_path: '../outside', }; - expect(() => grepTool.build(params)).toThrow(/Path validation failed/); + expect(() => grepTool.build(params)).toThrow(/Path not in workspace/); }); it.each([ @@ -1353,6 +1411,27 @@ describe('RipGrepTool', () => { getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), getDebugMode: () => false, getFileFilteringRespectGeminiIgnore: () => true, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; const geminiIgnoreTool = new RipGrepTool( configWithGeminiIgnore, @@ -1393,6 +1472,27 @@ describe('RipGrepTool', () => { getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), getDebugMode: () => false, getFileFilteringRespectGeminiIgnore: () => false, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; const geminiIgnoreTool = new RipGrepTool( configWithoutGeminiIgnore, @@ -1518,6 +1618,27 @@ describe('RipGrepTool', () => { getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir, ['/another/dir']), getDebugMode: () => false, + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, } as unknown as Config; const multiDirGrepTool = new RipGrepTool( diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index c4642ca20e..4752edf9b9 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -6,11 +6,12 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import fs from 'node:fs'; +import fsPromises from 'node:fs/promises'; import path from 'node:path'; import { downloadRipGrep } from '@joshua.litt/get-ripgrep'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; +import { ToolErrorType } from './tool-error.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; @@ -82,51 +83,6 @@ export async function ensureRgPath(): Promise { throw new Error('Cannot use ripgrep.'); } -/** - * Checks if a path is within the root directory and resolves it. - * @param config The configuration object. - * @param relativePath Path relative to the root directory (or undefined for root). - * @returns The absolute path if valid and exists, or null if no path specified. - * @throws {Error} If path is outside root, doesn't exist, or isn't a directory/file. - */ -function resolveAndValidatePath( - config: Config, - relativePath?: string, -): string | null { - if (!relativePath) { - return null; - } - - const targetDir = config.getTargetDir(); - const targetPath = path.resolve(targetDir, relativePath); - - // Ensure the resolved path is within workspace boundaries - const workspaceContext = config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(targetPath)) { - const directories = workspaceContext.getDirectories(); - throw new Error( - `Path validation failed: Attempted path "${relativePath}" resolves outside the allowed workspace directories: ${directories.join(', ')}`, - ); - } - - // Check existence and type after resolving - try { - const stats = fs.statSync(targetPath); - if (!stats.isDirectory() && !stats.isFile()) { - throw new Error( - `Path is not a valid directory or file: ${targetPath} (CWD: ${targetDir})`, - ); - } - } catch (error: unknown) { - if (isNodeError(error) && error.code === 'ENOENT') { - throw new Error(`Path does not exist: ${targetPath} (CWD: ${targetDir})`); - } - throw new Error(`Failed to access path stats for ${targetPath}: ${error}`); - } - - return targetPath; -} - /** * Parameters for the GrepTool */ @@ -207,7 +163,45 @@ class GrepToolInvocation extends BaseToolInvocation< // This forces CWD search instead of 'all workspaces' search by default. const pathParam = this.params.dir_path || '.'; - const searchDirAbs = resolveAndValidatePath(this.config, pathParam); + const searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam); + const validationError = this.config.validatePathAccess(searchDirAbs); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Error: Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + + // Check existence and type asynchronously + try { + const stats = await fsPromises.stat(searchDirAbs); + if (!stats.isDirectory() && !stats.isFile()) { + return { + llmContent: `Path is not a valid directory or file: ${searchDirAbs}`, + returnDisplay: 'Error: Path is not a valid directory or file.', + }; + } + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { + llmContent: `Path does not exist: ${searchDirAbs}`, + returnDisplay: 'Error: Path does not exist.', + error: { + message: `Path does not exist: ${searchDirAbs}`, + type: ToolErrorType.FILE_NOT_FOUND, + }, + }; + } + return { + llmContent: `Failed to access path stats for ${searchDirAbs}: ${getErrorMessage(error)}`, + returnDisplay: 'Error: Failed to access path.', + }; + } + const searchDirDisplay = pathParam; const totalMaxMatches = DEFAULT_TOTAL_MAX_MATCHES; @@ -233,7 +227,7 @@ class GrepToolInvocation extends BaseToolInvocation< try { allMatches = await this.performRipgrepSearch({ pattern: this.params.pattern, - path: searchDirAbs!, + path: searchDirAbs, include: this.params.include, case_sensitive: this.params.case_sensitive, fixed_strings: this.params.fixed_strings, @@ -552,21 +546,37 @@ export class RipGrepTool extends BaseDeclarativeTool< * @param params Parameters to validate * @returns An error message string if invalid, null otherwise */ - override validateToolParams(params: RipGrepToolParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; + protected override validateToolParamValues( + params: RipGrepToolParams, + ): string | null { + try { + new RegExp(params.pattern); + } catch (error) { + return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`; } // Only validate path if one is provided if (params.dir_path) { + const resolvedPath = path.resolve( + this.config.getTargetDir(), + params.dir_path, + ); + const validationError = this.config.validatePathAccess(resolvedPath); + if (validationError) { + return validationError; + } + + // Check existence and type try { - resolveAndValidatePath(this.config, params.dir_path); - } catch (error) { - return getErrorMessage(error); + const stats = fs.statSync(resolvedPath); + if (!stats.isDirectory() && !stats.isFile()) { + return `Path is not a valid directory or file: ${resolvedPath}`; + } + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return `Path does not exist: ${resolvedPath}`; + } + return `Failed to access path stats for ${resolvedPath}: ${getErrorMessage(error)}`; } } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 196c8678e9..7575dcc616 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -47,6 +47,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import { EOL } from 'node:os'; import * as path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import * as crypto from 'node:crypto'; import * as summarizer from '../utils/summarizer.js'; import { ToolErrorType } from './tool-error.js'; @@ -99,10 +100,31 @@ describe('ShellTool', () => { getWorkspaceContext: vi .fn() .mockReturnValue(new WorkspaceContext(tempRootDir)), - getGeminiClient: vi.fn(), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + getGeminiClient: vi.fn().mockReturnValue({}), + getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000), getEnableInteractiveShell: vi.fn().mockReturnValue(false), - isInteractive: vi.fn().mockReturnValue(true), - getShellToolInactivityTimeout: vi.fn().mockReturnValue(300000), + sanitizationConfig: {}, } as unknown as Config; const bus = createMockMessageBus(); @@ -183,9 +205,7 @@ describe('ShellTool', () => { const outsidePath = path.resolve(tempRootDir, '../outside'); expect(() => shellTool.build({ command: 'ls', dir_path: outsidePath }), - ).toThrow( - `Directory '${outsidePath}' is not within any of the registered workspace directories.`, - ); + ).toThrow(/Path not in workspace/); }); it('should return an invocation for a valid absolute directory path', () => { @@ -235,7 +255,7 @@ describe('ShellTool', () => { expect.any(Function), expect.any(AbortSignal), false, - { pager: 'cat' }, + { pager: 'cat', sanitizationConfig: {} }, ); expect(result.llmContent).toContain('Background PIDs: 54322'); // The file should be deleted by the tool @@ -260,7 +280,7 @@ describe('ShellTool', () => { expect.any(Function), expect.any(AbortSignal), false, - { pager: 'cat' }, + { pager: 'cat', sanitizationConfig: {} }, ); }); @@ -281,7 +301,7 @@ describe('ShellTool', () => { expect.any(Function), expect.any(AbortSignal), false, - { pager: 'cat' }, + { pager: 'cat', sanitizationConfig: {} }, ); }); @@ -308,7 +328,7 @@ describe('ShellTool', () => { expect.any(Function), expect.any(AbortSignal), false, - { pager: 'cat' }, + { pager: 'cat', sanitizationConfig: {} }, ); }, 20000, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 28b312dcbf..55575511f0 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import fs from 'node:fs'; +import fsPromises from 'node:fs/promises'; import path from 'node:path'; import os, { EOL } from 'node:os'; import crypto from 'node:crypto'; @@ -183,6 +183,17 @@ export class ShellToolInvocation extends BaseToolInvocation< ? path.resolve(this.config.getTargetDir(), this.params.dir_path) : this.config.getTargetDir(); + const validationError = this.config.validatePathAccess(cwd); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } let cumulativeOutput: string | AnsiOutput = ''; let lastUpdateTime = Date.now(); let isBinaryStream = false; @@ -267,11 +278,17 @@ export class ShellToolInvocation extends BaseToolInvocation< const backgroundPIDs: number[] = []; if (os.platform() !== 'win32') { - if (fs.existsSync(tempFilePath)) { - const pgrepLines = fs - .readFileSync(tempFilePath, 'utf8') - .split(EOL) - .filter(Boolean); + let tempFileExists = false; + try { + await fsPromises.access(tempFilePath); + tempFileExists = true; + } catch { + tempFileExists = false; + } + + if (tempFileExists) { + const pgrepContent = await fsPromises.readFile(tempFilePath, 'utf8'); + const pgrepLines = pgrepContent.split(EOL).filter(Boolean); for (const line of pgrepLines) { if (!/^\d+$/.test(line)) { debugLogger.error(`pgrep: ${line}`); @@ -395,8 +412,10 @@ export class ShellToolInvocation extends BaseToolInvocation< if (timeoutTimer) clearTimeout(timeoutTimer); signal.removeEventListener('abort', onAbort); timeoutController.signal.removeEventListener('abort', onAbort); - if (fs.existsSync(tempFilePath)) { - fs.unlinkSync(tempFilePath); + try { + await fsPromises.unlink(tempFilePath); + } catch { + // Ignore errors during unlink } } } @@ -485,10 +504,7 @@ export class ShellTool extends BaseDeclarativeTool< this.config.getTargetDir(), params.dir_path, ); - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(resolvedPath)) { - return `Directory '${resolvedPath}' is not within any of the registered workspace directories.`; - } + return this.config.validatePathAccess(resolvedPath); } return null; } diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 6bdbab64ed..2a0d3301be 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -28,6 +28,7 @@ import type { Config } from '../config/config.js'; import { ApprovalMode } from '../policy/types.js'; import type { ToolRegistry } from './tool-registry.js'; import path from 'node:path'; +import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs'; import os from 'node:os'; import { GeminiClient } from '../core/client.js'; @@ -59,6 +60,7 @@ vi.mock('../ide/ide-client.js', () => ({ })); let mockGeminiClientInstance: Mocked; let mockBaseLlmClientInstance: Mocked; +let mockConfig: Config; const mockEnsureCorrectEdit = vi.fn(); const mockEnsureCorrectFileContent = vi.fn(); const mockIdeClient = { @@ -108,8 +110,10 @@ const mockConfigInternal = { }) as unknown as ToolRegistry, isInteractive: () => false, getDisableLLMCorrection: vi.fn(() => true), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }, }; -const mockConfig = mockConfigInternal as unknown as Config; vi.mock('../telemetry/loggers.js', () => ({ logFileOperation: vi.fn(), @@ -135,6 +139,35 @@ describe('WriteFileTool', () => { fs.mkdirSync(plansDir, { recursive: true }); } + const workspaceContext = new WorkspaceContext(rootDir, [plansDir]); + const mockStorage = { + getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), + }; + + mockConfig = { + ...mockConfigInternal, + getWorkspaceContext: () => workspaceContext, + storage: mockStorage, + isPathAllowed(this: Config, absolutePath: string): boolean { + const workspaceContext = this.getWorkspaceContext(); + if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + return true; + } + + const projectTempDir = this.storage.getProjectTempDir(); + return isSubpath(path.resolve(projectTempDir), absolutePath); + }, + validatePathAccess(this: Config, absolutePath: string): string | null { + if (this.isPathAllowed(absolutePath)) { + return null; + } + + const workspaceDirs = this.getWorkspaceContext().getDirectories(); + const projectTempDir = this.storage.getProjectTempDir(); + return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; + }, + } as unknown as Config; + // Setup GeminiClient mock mockGeminiClientInstance = new (vi.mocked(GeminiClient))( mockConfig, @@ -243,9 +276,7 @@ describe('WriteFileTool', () => { file_path: outsidePath, content: 'hello', }; - expect(() => tool.build(params)).toThrow( - /File path must be within one of the workspace directories/, - ); + expect(() => tool.build(params)).toThrow(/Path not in workspace/); }); it('should throw an error if path is a directory', () => { @@ -816,9 +847,7 @@ describe('WriteFileTool', () => { file_path: '/etc/passwd', content: 'malicious', }; - expect(() => tool.build(params)).toThrow( - /File path must be within one of the workspace directories/, - ); + expect(() => tool.build(params)).toThrow(/Path not in workspace/); }); it('should allow paths within the plans directory', () => { @@ -834,9 +863,7 @@ describe('WriteFileTool', () => { file_path: path.join(plansDir, '..', 'escaped.txt'), content: 'malicious', }; - expect(() => tool.build(params)).toThrow( - /File path must be within one of the workspace directories/, - ); + expect(() => tool.build(params)).toThrow(/Path not in workspace/); }); }); @@ -874,7 +901,6 @@ describe('WriteFileTool', () => { errorMessage: 'Generic write error', expectedMessagePrefix: 'Error writing to file', mockFsExistsSync: false, - restoreAllMocks: true, }, ])( 'should return $errorType error when write fails with $errorCode', @@ -884,25 +910,22 @@ describe('WriteFileTool', () => { errorMessage, expectedMessagePrefix, mockFsExistsSync, - restoreAllMocks, }) => { const filePath = path.join(rootDir, `${errorType}_file.txt`); const content = 'test content'; - if (restoreAllMocks) { - vi.restoreAllMocks(); - } - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let existsSyncSpy: any; + let existsSyncSpy: // eslint-disable-next-line @typescript-eslint/no-explicit-any + ReturnType> | undefined = undefined; try { if (mockFsExistsSync) { const originalExistsSync = fs.existsSync; existsSyncSpy = vi - .spyOn(fs, 'existsSync') - .mockImplementation((path) => - path === filePath ? false : originalExistsSync(path as string), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .spyOn(fs as any, 'existsSync') + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation((path: any) => + path === filePath ? false : originalExistsSync(path), ); } diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index b496fa6e8e..4cc6f5998d 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -5,6 +5,7 @@ */ import fs from 'node:fs'; +import fsPromises from 'node:fs/promises'; import path from 'node:path'; import * as Diff from 'diff'; import { WRITE_FILE_TOOL_NAME } from './tool-names.js'; @@ -245,6 +246,18 @@ class WriteFileToolInvocation extends BaseToolInvocation< } async execute(abortSignal: AbortSignal): Promise { + const validationError = this.config.validatePathAccess(this.resolvedPath); + if (validationError) { + return { + llmContent: validationError, + returnDisplay: 'Error: Path not in workspace.', + error: { + message: validationError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, + }; + } + const { content, ai_proposed_content, modified_by_user } = this.params; const correctedContentResult = await getCorrectedFileContent( this.config, @@ -282,8 +295,10 @@ class WriteFileToolInvocation extends BaseToolInvocation< try { const dirName = path.dirname(this.resolvedPath); - if (!fs.existsSync(dirName)) { - fs.mkdirSync(dirName, { recursive: true }); + try { + await fsPromises.access(dirName); + } catch { + await fsPromises.mkdir(dirName, { recursive: true }); } await this.config @@ -453,12 +468,9 @@ export class WriteFileTool const resolvedPath = path.resolve(this.config.getTargetDir(), filePath); - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(resolvedPath)) { - const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join( - ', ', - )}`; + const validationError = this.config.validatePathAccess(resolvedPath); + if (validationError) { + return validationError; } try {