From f741d0328209b92a46571ba1d565e6c56fb4f32d Mon Sep 17 00:00:00 2001 From: luisfelipe-alt Date: Tue, 16 Jun 2026 22:05:03 +0000 Subject: [PATCH] fix(core-tools): resolve defensive path resolution for at-reference files (#27943) --- .../src/tools/at-reference-resolution.test.ts | 591 ++++++++++++++++++ packages/core/src/tools/edit.ts | 120 +++- packages/core/src/tools/read-file.ts | 34 +- packages/core/src/tools/write-file.ts | 98 ++- packages/core/src/utils/pathCorrector.ts | 12 +- packages/core/src/utils/paths.ts | 56 ++ 6 files changed, 882 insertions(+), 29 deletions(-) create mode 100644 packages/core/src/tools/at-reference-resolution.test.ts diff --git a/packages/core/src/tools/at-reference-resolution.test.ts b/packages/core/src/tools/at-reference-resolution.test.ts new file mode 100644 index 0000000000..26f260c226 --- /dev/null +++ b/packages/core/src/tools/at-reference-resolution.test.ts @@ -0,0 +1,591 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { ReadFileTool } from './read-file.js'; +import { WriteFileTool, getCorrectedFileContent } from './write-file.js'; +import { EditTool } from './edit.js'; +import { correctPath } from '../utils/pathCorrector.js'; +import path from 'node:path'; +import os from 'node:os'; +import fs from 'node:fs'; +import fsp from 'node:fs/promises'; +import type { Config } from '../config/config.js'; +import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; +import { StandardFileSystemService } from '../services/fileSystemService.js'; +import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import { isSubpath } from '../utils/paths.js'; + +vi.mock('../telemetry/loggers.js', () => ({ + logFileOperation: vi.fn(), + logEditStrategy: vi.fn(), + logEditCorrectionEvent: vi.fn(), +})); + +vi.mock('./jit-context.js', () => ({ + discoverJitContext: vi.fn().mockResolvedValue(''), + appendJitContext: vi.fn().mockImplementation((content) => content), + appendJitContextToParts: vi.fn().mockImplementation((content) => content), +})); + +describe('Consolidated At-Reference Path Resolution Tests (b-495551283)', () => { + let tempRootDir: string; + let mockConfigInstance: Config; + const abortSignal = new AbortController().signal; + + beforeEach(async () => { + // Create a unique temporary root directory for each test run + const realTmp = await fsp.realpath(os.tmpdir()); + tempRootDir = await fsp.mkdtemp( + path.join(realTmp, 'at-ref-resolution-root-'), + ); + + mockConfigInstance = { + getFileService: () => new FileDiscoveryService(tempRootDir), + getFileSystemService: () => new StandardFileSystemService(), + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), + getFileFilteringOptions: () => ({ + respectGitIgnore: true, + respectGeminiIgnore: true, + }), + storage: { + getProjectTempDir: () => path.join(tempRootDir, '.temp'), + }, + isInteractive: () => false, + isPlanMode: () => false, + getActiveModel: () => undefined, + getBaseLlmClient: () => undefined, + getDisableLLMCorrection: () => true, + 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; + + // Create the policies directory and new-policies.txt file + await fsp.mkdir(path.join(tempRootDir, 'policies'), { recursive: true }); + await fsp.writeFile( + path.join(tempRootDir, 'policies', 'new-policies.txt'), + '[[rule]]\ntoolName = "run_shell_command"\ndecision = "allow"\n', + 'utf8', + ); + }); + + afterEach(async () => { + // Clean up the temporary root directory + if (fs.existsSync(tempRootDir)) { + await fsp.rm(tempRootDir, { recursive: true, force: true }); + } + }); + + it('ReadFileTool successfully reads a file when the path is prefixed with @', async () => { + const readFileTool = new ReadFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = readFileTool.build({ + file_path: '@policies/new-policies.txt', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed because it defensively strips the leading '@' + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('toolName = "run_shell_command"'); + }); + + it('ReadFileTool successfully reads a file when the path is prefixed with @/', async () => { + const readFileTool = new ReadFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = readFileTool.build({ + file_path: '@/policies/new-policies.txt', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed because it defensively strips the leading '@/' + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('toolName = "run_shell_command"'); + }); + + it('WriteFileTool successfully writes to/updates a file when the path is prefixed with @', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@policies/new-policies.txt', + content: '[[rule]]\nupdated_content = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and update the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@policies', + 'new-policies.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'policies', + 'new-policies.txt', + ); + + // It should NOT have created a literal "@policies" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It should have updated the correct file under "policies" + const updatedContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(updatedContent).toContain('updated_content = true'); + }); + + it('WriteFileTool successfully creates a new file when the path is prefixed with @ and the parent directory exists', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@policies/brand-new-file.txt', + content: '[[rule]]\nbrand_new_file = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@policies', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'policies', + 'brand-new-file.txt', + ); + + // It should NOT have created a literal "@policies" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It should have created the correct file under "policies" + const createdContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(createdContent).toContain('brand_new_file = true'); + }); + + it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment exists', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@policies/sub/brand-new-file.txt', + content: '[[rule]]\nnested_brand_new_file = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@policies', + 'sub', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'policies', + 'sub', + 'brand-new-file.txt', + ); + + // It should NOT have created a literal "@policies" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It should have created the correct file under "policies/sub" + const createdContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(createdContent).toContain('nested_brand_new_file = true'); + }); + + it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment does NOT exist', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@new-policies/sub/brand-new-file.txt', + content: '[[rule]]\nnested_brand_new_file = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@new-policies', + 'sub', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'new-policies', + 'sub', + 'brand-new-file.txt', + ); + + // It SHOULD have created a literal "@new-policies" directory because neither exists + expect(fs.existsSync(incorrectFilePath)).toBe(true); + + // It should NOT have created the file under "new-policies/sub" + expect(fs.existsSync(correctFilePath)).toBe(false); + + // Verify the content of the created file + const createdContent = await fsp.readFile(incorrectFilePath, 'utf8'); + expect(createdContent).toContain('nested_brand_new_file = true'); + }); + + it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @/ and the first segment does NOT exist', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@/new-policies-alias/sub/brand-new-file.txt', + content: '[[rule]]\nnested_brand_new_file_alias = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const literalAtFilePath = path.join( + tempRootDir, + '@', + 'new-policies-alias', + 'sub', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'new-policies-alias', + 'sub', + 'brand-new-file.txt', + ); + + // It should NOT have created a literal "@" directory + expect(fs.existsSync(literalAtFilePath)).toBe(false); + expect(fs.existsSync(path.join(tempRootDir, '@'))).toBe(false); + + // It should have created the file under "new-policies-alias/sub" + expect(fs.existsSync(correctFilePath)).toBe(true); + + // Verify the content of the created file + const createdContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(createdContent).toContain('nested_brand_new_file_alias = true'); + }); + + it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @\\ and the first segment does NOT exist', async () => { + const writeFileTool = new WriteFileTool( + mockConfigInstance, + createMockMessageBus(), + ); + const invocation = writeFileTool.build({ + file_path: '@\\new-policies-alias-win\\sub\\brand-new-file.txt', + content: '[[rule]]\nnested_brand_new_file_alias_win = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const isWindows = process.platform === 'win32'; + const literalAtFilePath = isWindows + ? path.join( + tempRootDir, + '@', + 'new-policies-alias-win', + 'sub', + 'brand-new-file.txt', + ) + : path.join( + tempRootDir, + '@\\new-policies-alias-win\\sub\\brand-new-file.txt', + ); + const correctFilePath = isWindows + ? path.join( + tempRootDir, + 'new-policies-alias-win', + 'sub', + 'brand-new-file.txt', + ) + : path.join( + tempRootDir, + 'new-policies-alias-win\\sub\\brand-new-file.txt', + ); + + // It should NOT have created a literal "@" directory + expect(fs.existsSync(literalAtFilePath)).toBe(false); + expect(fs.existsSync(path.join(tempRootDir, '@'))).toBe(false); + + // It should have created the file under "new-policies-alias-win/sub" + expect(fs.existsSync(correctFilePath)).toBe(true); + + // Verify the content of the created file + const createdContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(createdContent).toContain('nested_brand_new_file_alias_win = true'); + }); + + it('getCorrectedFileContent blocks path traversal outside the workspace', async () => { + const result = await getCorrectedFileContent( + mockConfigInstance, + '../../etc/passwd', + 'malicious content', + abortSignal, + ); + + // The utility should fail with a path validation error + expect(result.error).toBeDefined(); + expect(result.error?.message).toContain('Path not in workspace'); + }); + + it('EditTool.getModifyContext blocks path traversal outside the workspace', async () => { + const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); + const modifyContext = editTool.getModifyContext(abortSignal); + + // The getCurrentContent method should throw a path validation error + await expect( + modifyContext.getCurrentContent({ + file_path: '../../etc/passwd', + instruction: 'read file', + old_string: '', + new_string: '', + }), + ).rejects.toThrow('Path not in workspace'); + + // The getProposedContent method should throw a path validation error + await expect( + modifyContext.getProposedContent({ + file_path: '../../etc/passwd', + instruction: 'read file', + old_string: '', + new_string: '', + }), + ).rejects.toThrow('Path not in workspace'); + }); + + it('getCorrectedFileContent handles symlink loops gracefully', async () => { + const symlinkPath1 = path.join(tempRootDir, 'symlink1'); + const symlinkPath2 = path.join(tempRootDir, 'symlink2'); + await fsp.symlink(symlinkPath2, symlinkPath1); + await fsp.symlink(symlinkPath1, symlinkPath2); + + const result = await getCorrectedFileContent( + mockConfigInstance, + 'symlink1', + 'content', + abortSignal, + ); + + // The utility should fail gracefully with a resolution error + expect(result.error).toBeDefined(); + expect(result.error?.message).toContain('Failed to resolve path'); + }); + + it('EditTool.getModifyContext handles symlink loops gracefully by throwing a descriptive error', async () => { + const symlinkPath1 = path.join(tempRootDir, 'symlink1'); + const symlinkPath2 = path.join(tempRootDir, 'symlink2'); + await fsp.symlink(symlinkPath2, symlinkPath1); + await fsp.symlink(symlinkPath1, symlinkPath2); + + const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); + const modifyContext = editTool.getModifyContext(abortSignal); + + // The getCurrentContent method should throw a path resolution error + await expect( + modifyContext.getCurrentContent({ + file_path: 'symlink1', + instruction: 'read file', + old_string: '', + new_string: '', + }), + ).rejects.toThrow('Failed to resolve path'); + + // The getProposedContent method should throw a path resolution error + await expect( + modifyContext.getProposedContent({ + file_path: 'symlink1', + instruction: 'read file', + old_string: '', + new_string: '', + }), + ).rejects.toThrow('Failed to resolve path'); + }); + + it('getCorrectedFileContent successfully resolves paths in Plan Mode', async () => { + const plansDir = path.join(tempRootDir, '.plans'); + await fsp.mkdir(plansDir, { recursive: true }); + await fsp.writeFile( + path.join(plansDir, 'plan-file.txt'), + 'plan content', + 'utf8', + ); + + const planConfigInstance = Object.assign({}, mockConfigInstance, { + isPlanMode: () => true, + getProjectRoot: () => tempRootDir, + storage: { + getProjectTempDir: () => path.join(tempRootDir, '.temp'), + getPlansDir: () => plansDir, + }, + }) as unknown as Config; + + const result = await getCorrectedFileContent( + planConfigInstance, + 'plan-file.txt', + 'new plan content', + abortSignal, + ); + + expect(result.error).toBeUndefined(); + expect(result.originalContent).toBe('plan content'); + }); + + it('EditTool successfully edits an existing file when the path is prefixed with @', async () => { + const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); + const invocation = editTool.build({ + file_path: '@policies/new-policies.txt', + instruction: 'update decision rule', + old_string: 'decision = "allow"', + new_string: 'decision = "deny"', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and update the correct file + expect(result.error).toBeUndefined(); + + const correctFilePath = path.join( + tempRootDir, + 'policies', + 'new-policies.txt', + ); + const updatedContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(updatedContent).toContain('decision = "deny"'); + }); + + it('EditTool successfully creates a new file when the path is prefixed with @ and the parent directory exists', async () => { + const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); + const invocation = editTool.build({ + file_path: '@policies/brand-new-edit-file.txt', + instruction: 'create new file', + old_string: '', + new_string: '[[rule]]\nbrand_new_edit_file = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@policies', + 'brand-new-edit-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'policies', + 'brand-new-edit-file.txt', + ); + + // It should NOT have created a literal "@policies" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It should have created the correct file under "policies" + const createdContent = await fsp.readFile(correctFilePath, 'utf8'); + expect(createdContent).toContain('brand_new_edit_file = true'); + }); + + it('EditTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment does NOT exist', async () => { + const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); + const invocation = editTool.build({ + file_path: '@new-policies-edit/sub/brand-new-file.txt', + instruction: 'create new file in nested subdirectory', + old_string: '', + new_string: '[[rule]]\nnested_brand_new_edit_file = true\n', + }); + + const result = await invocation.execute({ abortSignal }); + + // The tool should succeed and create the correct file + expect(result.error).toBeUndefined(); + + const incorrectFilePath = path.join( + tempRootDir, + '@new-policies-edit', + 'sub', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'new-policies-edit', + 'sub', + 'brand-new-file.txt', + ); + + // It SHOULD have created a literal "@new-policies-edit" directory because neither exists + expect(fs.existsSync(incorrectFilePath)).toBe(true); + + // It should NOT have created the file under "new-policies-edit/sub" + expect(fs.existsSync(correctFilePath)).toBe(false); + + // Verify the content of the created file + const createdContent = await fsp.readFile(incorrectFilePath, 'utf8'); + expect(createdContent).toContain('nested_brand_new_edit_file = true'); + }); + + it('correctPath successfully resolves a path prefixed with @ to its clean counterpart', () => { + const result = correctPath( + '@policies/new-policies.txt', + mockConfigInstance, + ); + + expect(result.success).toBe(true); + if (result.success) { + const expectedPath = path.join( + tempRootDir, + 'policies', + 'new-policies.txt', + ); + expect(result.correctedPath).toBe(expectedPath); + } + }); +}); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index c00ea4c0da..1e3bffa4e5 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -27,7 +27,12 @@ import { import { buildFilePathArgsPattern } from '../policy/utils.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { ToolErrorType } from './tool-error.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { + makeRelative, + shortenPath, + resolveDefensiveToolPath, + resolveToRealPath, +} from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import { correctPath } from '../utils/pathCorrector.js'; import type { Config } from '../config/config.js'; @@ -478,11 +483,12 @@ class EditToolInvocation ); if (this.config.isPlanMode()) { try { - this.resolvedPath = resolveAndValidatePlanPath( + const planPath = resolveAndValidatePlanPath( this.params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during EditTool invocation setup', @@ -495,15 +501,33 @@ class EditToolInvocation } else if (!path.isAbsolute(this.params.file_path)) { const result = correctPath(this.params.file_path, this.config); if (result.success) { - this.resolvedPath = result.correctedPath; + try { + this.resolvedPath = resolveToRealPath(result.correctedPath); + } catch { + this.resolvedPath = result.correctedPath; + } } else { - this.resolvedPath = path.resolve( - this.config.getTargetDir(), + const sanitizedPath = resolveDefensiveToolPath( this.params.file_path, + this.config.getTargetDir(), ); + try { + this.resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch { + this.resolvedPath = path.resolve( + this.config.getTargetDir(), + sanitizedPath, + ); + } } } else { - this.resolvedPath = this.params.file_path; + try { + this.resolvedPath = resolveToRealPath(this.params.file_path); + } catch { + this.resolvedPath = this.params.file_path; + } } } @@ -1094,28 +1118,43 @@ export class EditTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - resolvedPath = resolveAndValidatePlanPath( + const planPath = resolveAndValidatePlanPath( params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + resolvedPath = resolveToRealPath(planPath); } catch (err) { return err instanceof Error ? err.message : String(err); } } else if (!path.isAbsolute(params.file_path)) { const result = correctPath(params.file_path, this.config); if (result.success) { - resolvedPath = result.correctedPath; + try { + resolvedPath = resolveToRealPath(result.correctedPath); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } } else { - resolvedPath = path.resolve( - this.config.getTargetDir(), + const sanitizedPath = resolveDefensiveToolPath( params.file_path, + this.config.getTargetDir(), ); + try { + resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } } } else { - resolvedPath = params.file_path; + try { + resolvedPath = resolveToRealPath(params.file_path); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } } - const newPlaceholders = detectOmissionPlaceholders(params.new_string); if (newPlaceholders.length > 0) { const oldPlaceholders = new Set( @@ -1150,13 +1189,65 @@ export class EditTool } getModifyContext(_: AbortSignal): ModifyContext { + const resolvePath = (params: EditToolParams): string => { + let pathBeforeRealResolve: string; + + try { + if (this.config.isPlanMode()) { + pathBeforeRealResolve = resolveAndValidatePlanPath( + params.file_path, + this.config.storage.getPlansDir(), + this.config.getProjectRoot(), + ); + } else if (!path.isAbsolute(params.file_path)) { + const result = correctPath(params.file_path, this.config); + if (result.success) { + pathBeforeRealResolve = result.correctedPath; + } else { + const sanitizedPath = resolveDefensiveToolPath( + params.file_path, + this.config.getTargetDir(), + ); + pathBeforeRealResolve = path.resolve( + this.config.getTargetDir(), + sanitizedPath, + ); + } + } else { + pathBeforeRealResolve = params.file_path; + } + } catch (err) { + throw new Error( + 'Failed to resolve path: ' + + (err instanceof Error ? err.message : String(err)), + ); + } + + let resolved: string; + try { + resolved = resolveToRealPath(pathBeforeRealResolve); + } catch (err) { + throw new Error( + 'Failed to resolve path: ' + + (err instanceof Error ? err.message : String(err)), + ); + } + + const validationError = this.config.validatePathAccess(resolved); + if (validationError) { + throw new Error(validationError); + } + return resolved; + }; + return { getFilePath: (params: EditToolParams) => params.file_path, getCurrentContent: async (params: EditToolParams): Promise => { try { + const resolvedPath = resolvePath(params); return await this.config .getFileSystemService() - .readTextFile(params.file_path); + .readTextFile(resolvedPath); } catch (err) { if (!isNodeError(err) || err.code !== 'ENOENT') throw err; return ''; @@ -1164,9 +1255,10 @@ export class EditTool }, getProposedContent: async (params: EditToolParams): Promise => { try { + const resolvedPath = resolvePath(params); const currentContent = await this.config .getFileSystemService() - .readTextFile(params.file_path); + .readTextFile(resolvedPath); return applyReplacement( currentContent, params.old_string, diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 778f8967eb..47bdc86de9 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -6,7 +6,12 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import path from 'node:path'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { + makeRelative, + shortenPath, + resolveDefensiveToolPath, + resolveToRealPath, +} from '../utils/paths.js'; import { BaseDeclarativeTool, BaseToolInvocation, @@ -74,10 +79,20 @@ class ReadFileToolInvocation extends BaseToolInvocation< _toolDisplayName?: string, ) { super(params, messageBus, _toolName, _toolDisplayName); - this.resolvedPath = path.resolve( - this.config.getTargetDir(), + const sanitizedPath = resolveDefensiveToolPath( this.params.file_path, + this.config.getTargetDir(), ); + try { + this.resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch { + this.resolvedPath = path.resolve( + this.config.getTargetDir(), + sanitizedPath, + ); + } } getDescription(): string { @@ -242,11 +257,20 @@ export class ReadFileTool extends BaseDeclarativeTool< return "The 'file_path' parameter must be non-empty."; } - const resolvedPath = path.resolve( - this.config.getTargetDir(), + const sanitizedPath = resolveDefensiveToolPath( params.file_path, + this.config.getTargetDir(), ); + let resolvedPath: string; + try { + resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch (err) { + return `Failed to resolve path: ${err instanceof Error ? err.message : String(err)}`; + } + const validationError = this.config.validatePathAccess( resolvedPath, 'read', diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 9ec19b879f..94ec4f3852 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -28,7 +28,12 @@ import { } from './tools.js'; import { buildFilePathArgsPattern } from '../policy/utils.js'; import { ToolErrorType } from './tool-error.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { + makeRelative, + shortenPath, + resolveDefensiveToolPath, + resolveToRealPath, +} from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { ensureCorrectFileContent } from '../utils/editCorrector.js'; import { detectLineEnding } from '../utils/textUtils.js'; @@ -109,10 +114,67 @@ export async function getCorrectedFileContent( let fileExists = false; let correctedContent = proposedContent; + let resolvedPath: string; + if (config.isPlanMode()) { + try { + const planPath = path.isAbsolute(filePath) + ? filePath + : resolveAndValidatePlanPath( + filePath, + config.storage.getPlansDir(), + config.getProjectRoot(), + ); + resolvedPath = resolveToRealPath(planPath); + } catch (err) { + return { + originalContent: '', + correctedContent: proposedContent, + fileExists: false, + error: { + message: + 'Failed to resolve plan path: ' + + (err instanceof Error ? err.message : String(err)), + code: 'EINVAL', + }, + }; + } + } else { + const sanitizedPath = path.isAbsolute(filePath) + ? filePath + : resolveDefensiveToolPath(filePath, config.getTargetDir()); + try { + resolvedPath = resolveToRealPath( + path.resolve(config.getTargetDir(), sanitizedPath), + ); + } catch (err) { + return { + originalContent: '', + correctedContent: proposedContent, + fileExists: false, + error: { + message: + 'Failed to resolve path: ' + + (err instanceof Error ? err.message : String(err)), + code: 'EINVAL', + }, + }; + } + } + + const validationError = config.validatePathAccess(resolvedPath); + if (validationError) { + return { + originalContent: '', + correctedContent: proposedContent, + fileExists: false, + error: { message: validationError, code: 'EACCES' }, + }; + } + try { originalContent = await config .getFileSystemService() - .readTextFile(filePath); + .readTextFile(resolvedPath); fileExists = true; // File exists and was read } catch (err) { if (isNodeError(err) && err.code === 'ENOENT') { @@ -170,11 +232,12 @@ class WriteFileToolInvocation extends BaseToolInvocation< if (this.config.isPlanMode()) { try { - this.resolvedPath = resolveAndValidatePlanPath( + const planPath = resolveAndValidatePlanPath( this.params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during WriteFileTool invocation setup', @@ -184,10 +247,20 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.resolvedPath = this.params.file_path; } } else { - this.resolvedPath = path.resolve( - this.config.getTargetDir(), + const sanitizedPath = resolveDefensiveToolPath( this.params.file_path, + this.config.getTargetDir(), ); + try { + this.resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch { + this.resolvedPath = path.resolve( + this.config.getTargetDir(), + sanitizedPath, + ); + } } } @@ -525,16 +598,27 @@ export class WriteFileTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - resolvedPath = resolveAndValidatePlanPath( + const planPath = resolveAndValidatePlanPath( filePath, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + resolvedPath = resolveToRealPath(planPath); } catch (err) { return err instanceof Error ? err.message : String(err); } } else { - resolvedPath = path.resolve(this.config.getTargetDir(), filePath); + const sanitizedPath = resolveDefensiveToolPath( + filePath, + this.config.getTargetDir(), + ); + try { + resolvedPath = resolveToRealPath( + path.resolve(this.config.getTargetDir(), sanitizedPath), + ); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } } const validationError = this.config.validatePathAccess(resolvedPath); diff --git a/packages/core/src/utils/pathCorrector.ts b/packages/core/src/utils/pathCorrector.ts index d6538ff056..632368ff20 100644 --- a/packages/core/src/utils/pathCorrector.ts +++ b/packages/core/src/utils/pathCorrector.ts @@ -8,6 +8,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { Config } from '../config/config.js'; import { bfsFileSearchSync } from './bfsFileSearch.js'; +import { resolveDefensiveToolPath } from './paths.js'; type SuccessfulPathCorrection = { success: true; @@ -34,8 +35,13 @@ export function correctPath( filePath: string, config: Config, ): PathCorrectionResult { + const sanitizedPath = resolveDefensiveToolPath( + filePath, + config.getTargetDir(), + ); + // Check for direct path relative to the primary target directory. - const directPath = path.join(config.getTargetDir(), filePath); + const directPath = path.join(config.getTargetDir(), sanitizedPath); if (fs.existsSync(directPath)) { return { success: true, correctedPath: directPath }; } @@ -43,8 +49,8 @@ export function correctPath( // If not found directly, search across all workspace directories for ambiguous matches. const workspaceContext = config.getWorkspaceContext(); const searchPaths = workspaceContext.getDirectories(); - const basename = path.basename(filePath); - const normalizedTarget = filePath.replace(/\\/g, '/'); + const basename = path.basename(sanitizedPath); + const normalizedTarget = sanitizedPath.replace(/\\/g, '/'); // Normalize path for matching and check if it ends with the provided relative path const foundFiles = searchPaths diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index d9d543a682..0b9bdd21e1 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -572,3 +572,59 @@ export function isTrustedSystemPath(filePath: string): boolean { ); } } + +/** + * Defensively resolves and sanitizes a file path generated by the LLM, + * stripping user-facing reference prefixes if necessary. + */ +export function resolveDefensiveToolPath( + filePath: string, + targetDir: string, +): string { + if (filePath.includes('\0')) { + return filePath; + } + + try { + const literalPath = path.resolve(targetDir, filePath); + + // If the file literally exists on disk as-is, return the resolved literal path immediately + if (fs.existsSync(literalPath)) { + return filePath; + } + + // If the model supplied a leading @ prefix and the literal path doesn't exist: + if (filePath.startsWith('@') && filePath.length > 1) { + if (filePath.startsWith('@/') || filePath.startsWith('@\\')) { + return filePath.substring(1).replace(/^[\\/]+/, ''); + } + + const strippedPath = filePath.substring(1).replace(/^[\\/]+/, ''); + + // Check if a literal directory/file starting with '@' exists for the first segment. + // If it does, we should preserve the '@' prefix. + const parts = strippedPath.split(/[\\/]/); + const firstSegment = parts[0]; + if (firstSegment) { + const literalFirstSegment = path.resolve(targetDir, '@' + firstSegment); + if (fs.existsSync(literalFirstSegment)) { + return filePath; + } + + // Only strip the '@' prefix if the stripped path's first segment actually exists + const strippedFirstSegment = path.resolve(targetDir, firstSegment); + if (fs.existsSync(strippedFirstSegment)) { + return strippedPath; + } + } + + // Otherwise, preserve the '@' prefix to allow creating new files/directories starting with '@' + return filePath; + } + } catch { + // Fallback to original path if any filesystem or resolution error occurs + } + + // Fallback: return the original path + return filePath; +}