From 2cae930908f71432c0a4e44c547f094f5f0b11b3 Mon Sep 17 00:00:00 2001 From: Luis Felipe Quevedo Date: Fri, 19 Jun 2026 21:40:45 +0000 Subject: [PATCH] fix(core-tools): resolve defensive path resolution for at-reference files and fix macOS tests --- .../src/test-utils/mockWorkspaceContext.ts | 13 +- .../src/tools/at-reference-resolution.test.ts | 684 ++++++++++++++++++ packages/core/src/tools/edit.test.ts | 29 +- packages/core/src/tools/edit.ts | 131 +++- packages/core/src/tools/read-file.ts | 34 +- packages/core/src/tools/trackerTools.test.ts | 2 +- packages/core/src/tools/write-file.test.ts | 35 +- packages/core/src/tools/write-file.ts | 106 ++- packages/core/src/utils/pathCorrector.ts | 12 +- packages/core/src/utils/paths.test.ts | 17 + packages/core/src/utils/paths.ts | 51 ++ scripts/build.js | 2 +- 12 files changed, 1063 insertions(+), 53 deletions(-) create mode 100644 packages/core/src/tools/at-reference-resolution.test.ts diff --git a/packages/core/src/test-utils/mockWorkspaceContext.ts b/packages/core/src/test-utils/mockWorkspaceContext.ts index 67c614e9f53..604a910670e 100644 --- a/packages/core/src/test-utils/mockWorkspaceContext.ts +++ b/packages/core/src/test-utils/mockWorkspaceContext.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as fs from 'node:fs'; import { vi } from 'vitest'; import type { WorkspaceContext } from '../utils/workspaceContext.js'; @@ -17,7 +18,17 @@ export function createMockWorkspaceContext( rootDir: string, additionalDirs: string[] = [], ): WorkspaceContext { - const allDirs = [rootDir, ...additionalDirs]; + const resolveToRealPathSafe = (p: string) => { + try { + return fs.realpathSync(p); + } catch { + return p; + } + }; + + const resolvedRootDir = resolveToRealPathSafe(rootDir); + const resolvedAdditionalDirs = additionalDirs.map(resolveToRealPathSafe); + const allDirs = [resolvedRootDir, ...resolvedAdditionalDirs]; const mockWorkspaceContext = { addDirectory: vi.fn(), 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 00000000000..e0962d469ee --- /dev/null +++ b/packages/core/src/tools/at-reference-resolution.test.ts @@ -0,0 +1,684 @@ +/** + * @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 NOT have created a literal "@new-policies" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It SHOULD have created the file under "new-policies/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 = 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 NOT have created a literal "@new-policies-edit" directory + expect(fs.existsSync(incorrectFilePath)).toBe(false); + + // It SHOULD have created the file under "new-policies-edit/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_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-alias/sub/brand-new-file.txt', + instruction: 'create new file in nested subdirectory', + old_string: '', + new_string: '[[rule]]\nnested_brand_new_edit_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-edit-alias', + 'sub', + 'brand-new-file.txt', + ); + const correctFilePath = path.join( + tempRootDir, + 'new-policies-edit-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-edit-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_edit_file_alias = 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-alias-win\\sub\\brand-new-file.txt', + instruction: 'create new file in nested subdirectory', + old_string: '', + new_string: '[[rule]]\nnested_brand_new_edit_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-edit-alias-win', + 'sub', + 'brand-new-file.txt', + ) + : path.join( + tempRootDir, + '@\\new-policies-edit-alias-win\\sub\\brand-new-file.txt', + ); + const correctFilePath = isWindows + ? path.join( + tempRootDir, + 'new-policies-edit-alias-win', + 'sub', + 'brand-new-file.txt', + ) + : path.join( + tempRootDir, + 'new-policies-edit-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-edit-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_edit_file_alias_win = 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.test.ts b/packages/core/src/tools/edit.test.ts index 84086bbd696..8af188ba973 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -84,7 +84,10 @@ describe('EditTool', () => { beforeEach(() => { vi.restoreAllMocks(); - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-')); + const rawTempDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'edit-tool-test-'), + ); + tempDir = fs.realpathSync(rawTempDir); rootDir = path.join(tempDir, 'root'); fs.mkdirSync(rootDir); @@ -701,6 +704,30 @@ function doIt() { }; expect(tool.validateToolParams(params)).toBeNull(); }); + + it('should sanitize null bytes in absolute path during validation', () => { + const badPath = path.resolve(rootDir, 'test\0.txt'); + const params: EditToolParams = { + file_path: badPath, + instruction: 'An instruction', + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateToolParams(params)).toBeNull(); + }); + + it('should sanitize null bytes in absolute path during invocation setup', () => { + const badPath = path.resolve(rootDir, 'test\0.txt'); + const invocation = tool.build({ + file_path: badPath, + instruction: 'test', + old_string: 'old', + new_string: 'new', + }); + expect((invocation as any).resolvedPath).toBe( + path.resolve(rootDir, 'test.txt'), + ); + }); }); describe('execute', () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index c00ea4c0da0..19d017f6424 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,13 @@ class EditToolInvocation ); if (this.config.isPlanMode()) { try { - this.resolvedPath = resolveAndValidatePlanPath( - this.params.file_path, + const cleanFilePath = this.params.file_path.replace(/\0/g, ''); + const planPath = resolveAndValidatePlanPath( + cleanFilePath, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during EditTool invocation setup', @@ -490,20 +497,39 @@ class EditToolInvocation ); // Validation fails, set resolvedPath to something that will fail validation downstream or just the raw path. // It's safer to store it so validation in execute() or getConfirmationDetails() catches it. - this.resolvedPath = this.params.file_path; + this.resolvedPath = this.params.file_path.replace(/\0/g, ''); } } 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; + const cleanPath = this.params.file_path.replace(/\0/g, ''); + try { + this.resolvedPath = resolveToRealPath(cleanPath); + } catch { + this.resolvedPath = cleanPath; + } } } @@ -1094,28 +1120,45 @@ export class EditTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - resolvedPath = resolveAndValidatePlanPath( - params.file_path, + const cleanFilePath = params.file_path.replace(/\0/g, ''); + const planPath = resolveAndValidatePlanPath( + cleanFilePath, 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; + const cleanPath = params.file_path.replace(/\0/g, ''); + try { + resolvedPath = resolveToRealPath(cleanPath); + } 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 +1193,66 @@ export class EditTool } getModifyContext(_: AbortSignal): ModifyContext { + const resolvePath = (params: EditToolParams): string => { + let pathBeforeRealResolve: string; + + try { + if (this.config.isPlanMode()) { + const cleanFilePath = params.file_path.replace(/\0/g, ''); + pathBeforeRealResolve = resolveAndValidatePlanPath( + cleanFilePath, + 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.replace(/\0/g, ''); + } + } 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 +1260,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 778f8967eb6..47bdc86de90 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/trackerTools.test.ts b/packages/core/src/tools/trackerTools.test.ts index 5ec59cdf60e..0d168f10aec 100644 --- a/packages/core/src/tools/trackerTools.test.ts +++ b/packages/core/src/tools/trackerTools.test.ts @@ -31,7 +31,7 @@ describe('Tracker Tools Integration', () => { beforeEach(async () => { tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'tracker-tools-test-')); config = new Config({ - sessionId: 'test-session', + sessionId: `test-session-${Math.random().toString(36).substring(7)}`, targetDir: tempDir, cwd: tempDir, model: 'gemini-3-flash', diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index b35d26fe20f..29d636a5522 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -30,7 +30,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 { isSubpath, resolveToRealPath } from '../utils/paths.js'; import fs from 'node:fs'; import os from 'node:os'; import { GeminiClient } from '../core/client.js'; @@ -44,8 +44,8 @@ import { getMockMessageBusInstance, } from '../test-utils/mock-message-bus.js'; -const rootDir = path.resolve(os.tmpdir(), 'gemini-cli-test-root'); -const plansDir = path.resolve(os.tmpdir(), 'gemini-cli-test-plans'); +let rootDir: string; +let plansDir: string; // --- MOCKS --- vi.mock('../core/client.js'); @@ -134,16 +134,20 @@ describe('WriteFileTool', () => { beforeEach(() => { vi.clearAllMocks(); // Create a unique temporary directory for files created outside the root - tempDir = fs.mkdtempSync( + const rawTempDir = fs.mkdtempSync( path.join(os.tmpdir(), 'write-file-test-external-'), ); - // Ensure the rootDir and plansDir for the tool exists - if (!fs.existsSync(rootDir)) { - fs.mkdirSync(rootDir, { recursive: true }); - } - if (!fs.existsSync(plansDir)) { - fs.mkdirSync(plansDir, { recursive: true }); - } + tempDir = fs.realpathSync(rawTempDir); + + const rawRootDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-cli-test-root-'), + ); + rootDir = fs.realpathSync(rawRootDir); + + const rawPlansDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-cli-test-plans-'), + ); + plansDir = fs.realpathSync(rawPlansDir); const workspaceContext = new WorkspaceContext(rootDir, [plansDir]); const mockStorage = { @@ -272,8 +276,9 @@ describe('WriteFileTool', () => { file_path: dirAsFilePath, content: 'hello', }; + const realDirAsFilePath = resolveToRealPath(dirAsFilePath); expect(() => tool.build(params)).toThrow( - `Path is a directory, not a file: ${dirAsFilePath}`, + `Path is a directory, not a file: ${realDirAsFilePath}`, ); }); @@ -441,7 +446,8 @@ describe('WriteFileTool', () => { abortSignal, ); - expect(fsService.readTextFile).toHaveBeenCalledWith(filePath); + const realFilePath = resolveToRealPath(filePath); + expect(fsService.readTextFile).toHaveBeenCalledWith(realFilePath); expect(mockEnsureCorrectFileContent).not.toHaveBeenCalled(); expect(result.correctedContent).toBe(proposedContent); expect(result.originalContent).toBe(''); @@ -1014,8 +1020,9 @@ describe('WriteFileTool', () => { expect(result.error?.type).toBe(errorType); const errorSuffix = errorCode ? ` (${errorCode})` : ''; + const realFilePath = resolveToRealPath(filePath); const expectedMessage = errorCode - ? `${expectedMessagePrefix}: ${filePath}${errorSuffix}` + ? `${expectedMessagePrefix}: ${realFilePath}${errorSuffix}` : `${expectedMessagePrefix}: ${errorMessage}`; expect(result.llmContent).toContain(expectedMessage); expect(result.returnDisplay).toContain(expectedMessage); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 9ec19b879fd..a7d6365c3c9 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 cleanFilePath = filePath.replace(/\0/g, ''); + const planPath = resolveAndValidatePlanPath( + cleanFilePath, + 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 = 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,24 +232,36 @@ class WriteFileToolInvocation extends BaseToolInvocation< if (this.config.isPlanMode()) { try { - this.resolvedPath = resolveAndValidatePlanPath( - this.params.file_path, + const cleanFilePath = this.params.file_path.replace(/\0/g, ''); + const planPath = resolveAndValidatePlanPath( + cleanFilePath, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); + this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during WriteFileTool invocation setup', e, ); // Validation fails, set resolvedPath to something that will fail validation downstream or just the raw path. - this.resolvedPath = this.params.file_path; + this.resolvedPath = this.params.file_path.replace(/\0/g, ''); } } 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 +599,28 @@ export class WriteFileTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - resolvedPath = resolveAndValidatePlanPath( - filePath, + const cleanFilePath = filePath.replace(/\0/g, ''); + const planPath = resolveAndValidatePlanPath( + cleanFilePath, 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 d6538ff056e..632368ff207 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.test.ts b/packages/core/src/utils/paths.test.ts index c76ab7ae6cb..173ec637481 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -20,6 +20,7 @@ import { toAbsolutePath, toPathKey, isTrustedSystemPath, + resolveDefensiveToolPath, } from './paths.js'; vi.mock('node:fs', async (importOriginal) => { @@ -918,4 +919,20 @@ describe('normalizePath', () => { }); }); }); + + describe('resolveDefensiveToolPath', () => { + it('should sanitize paths by stripping null bytes', () => { + const targetDir = '/workspace'; + const filePathWithNull = 'src/index.ts\0.exe'; + const result = resolveDefensiveToolPath(filePathWithNull, targetDir); + expect(result).toBe('src/index.ts.exe'); + }); + + it('should sanitize @ prefixed paths by stripping null bytes', () => { + const targetDir = '/workspace'; + const filePathWithNull = '@/components/Button.tsx\0'; + const result = resolveDefensiveToolPath(filePathWithNull, targetDir); + expect(result).toBe('components/Button.tsx'); + }); + }); }); diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index d9d543a6826..09c2bebb53e 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -572,3 +572,54 @@ 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 { + const cleanPath = filePath.replace(/\0/g, ''); + + try { + const literalPath = path.resolve(targetDir, cleanPath); + + // If the file literally exists on disk as-is, return the resolved literal path immediately + if (fs.existsSync(literalPath)) { + return cleanPath; + } + + // If the model supplied a leading @ prefix and the literal path doesn't exist: + if (cleanPath.startsWith('@') && cleanPath.length > 1) { + if (cleanPath.startsWith('@/') || cleanPath.startsWith('@\\')) { + const stripped = cleanPath.substring(1).replace(/^[\\/]+/, ''); + return stripped.length > 0 ? stripped : cleanPath; + } + + const strippedPath = cleanPath.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 cleanPath; + } + + // Otherwise, strip the '@' prefix to resolve to the standard directory name, + // preventing the accidental creation of literal '@'-prefixed directories (e.g. '@src', '@policies') + // when creating new files or directories. + return strippedPath; + } + } + } catch { + // Fallback to original path if any filesystem or resolution error occurs + } + + // Fallback: return the original path + return cleanPath; +} diff --git a/scripts/build.js b/scripts/build.js index e6aa14faa9b..b5ef6f672b6 100644 --- a/scripts/build.js +++ b/scripts/build.js @@ -54,7 +54,7 @@ if (process.env.CI) { .filter((name) => name !== '@google/gemini-cli-core'); execSync( - `npx npm-run-all --parallel ${parallelWorkspaces.map((w) => `"build -w ${w}"`).join(' ')}`, + `npx --no-install npm-run-all --parallel ${parallelWorkspaces.map((w) => `"build -w ${w}"`).join(' ')}`, { stdio: 'inherit', cwd: root }, ); }