mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-07-11 02:20:48 -07:00
fix(core-tools): resolve defensive path resolution for at-reference files and fix macOS tests (#28053)
Co-authored-by: David Pierce <davidapierce@google.com>
This commit is contained in:
@@ -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(),
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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', () => {
|
||||
|
||||
+114
-17
@@ -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<EditToolParams> {
|
||||
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<string> => {
|
||||
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<string> => {
|
||||
try {
|
||||
const resolvedPath = resolvePath(params);
|
||||
const currentContent = await this.config
|
||||
.getFileSystemService()
|
||||
.readTextFile(params.file_path);
|
||||
.readTextFile(resolvedPath);
|
||||
return applyReplacement(
|
||||
currentContent,
|
||||
params.old_string,
|
||||
|
||||
@@ -37,7 +37,10 @@ describe('GlobTool', () => {
|
||||
|
||||
beforeEach(async () => {
|
||||
// Create a unique root directory for each test run
|
||||
tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-'));
|
||||
const rawTempRootDir = await fs.mkdtemp(
|
||||
path.join(os.tmpdir(), 'glob-tool-root-'),
|
||||
);
|
||||
tempRootDir = await fs.realpath(rawTempRootDir);
|
||||
await fs.writeFile(path.join(tempRootDir, '.git'), ''); // Fake git repo
|
||||
|
||||
const rootDir = tempRootDir;
|
||||
|
||||
@@ -18,7 +18,11 @@ import {
|
||||
type ToolConfirmationOutcome,
|
||||
type ExecuteOptions,
|
||||
} from './tools.js';
|
||||
import { shortenPath, makeRelative } from '../utils/paths.js';
|
||||
import {
|
||||
shortenPath,
|
||||
makeRelative,
|
||||
resolveToRealPath,
|
||||
} from '../utils/paths.js';
|
||||
import { type Config } from '../config/config.js';
|
||||
import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js';
|
||||
import { ToolErrorType } from './tool-error.js';
|
||||
@@ -138,10 +142,22 @@ class GlobToolInvocation extends BaseToolInvocation<
|
||||
// If a specific path is provided, resolve it and check if it's within workspace
|
||||
let searchDirectories: readonly string[];
|
||||
if (this.params.dir_path) {
|
||||
const searchDirAbsolute = path.resolve(
|
||||
this.config.getTargetDir(),
|
||||
this.params.dir_path,
|
||||
);
|
||||
let searchDirAbsolute: string;
|
||||
try {
|
||||
searchDirAbsolute = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), this.params.dir_path),
|
||||
);
|
||||
} catch (err) {
|
||||
const errMsg = err instanceof Error ? err.message : String(err);
|
||||
return {
|
||||
llmContent: errMsg,
|
||||
returnDisplay: 'Path resolution failed.',
|
||||
error: {
|
||||
message: errMsg,
|
||||
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
|
||||
},
|
||||
};
|
||||
}
|
||||
const validationError = this.config.validatePathAccess(
|
||||
searchDirAbsolute,
|
||||
'read',
|
||||
@@ -189,9 +205,22 @@ class GlobToolInvocation extends BaseToolInvocation<
|
||||
allEntries.push(...entries);
|
||||
}
|
||||
|
||||
const relativePaths = allEntries.map((p) =>
|
||||
path.relative(this.config.getTargetDir(), p.fullpath()),
|
||||
);
|
||||
let realTargetDir = this.config.getTargetDir();
|
||||
try {
|
||||
realTargetDir = resolveToRealPath(realTargetDir);
|
||||
} catch {
|
||||
// Ignore and use raw targetDir
|
||||
}
|
||||
|
||||
const relativePaths = allEntries.map((p) => {
|
||||
let realFullPath = p.fullpath();
|
||||
try {
|
||||
realFullPath = resolveToRealPath(realFullPath);
|
||||
} catch {
|
||||
// Ignore and use raw fullpath
|
||||
}
|
||||
return path.relative(realTargetDir, realFullPath);
|
||||
});
|
||||
|
||||
const { filteredPaths, ignoredCount } =
|
||||
fileDiscovery.filterFilesWithReport(relativePaths, {
|
||||
@@ -304,10 +333,14 @@ export class GlobTool extends BaseDeclarativeTool<GlobToolParams, ToolResult> {
|
||||
protected override validateToolParamValues(
|
||||
params: GlobToolParams,
|
||||
): string | null {
|
||||
const searchDirAbsolute = path.resolve(
|
||||
this.config.getTargetDir(),
|
||||
params.dir_path || '.',
|
||||
);
|
||||
let searchDirAbsolute: string;
|
||||
try {
|
||||
searchDirAbsolute = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), params.dir_path || '.'),
|
||||
);
|
||||
} catch (err) {
|
||||
return err instanceof Error ? err.message : String(err);
|
||||
}
|
||||
|
||||
const validationError = this.config.validatePathAccess(
|
||||
searchDirAbsolute,
|
||||
|
||||
@@ -8,7 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import { GrepTool, type GrepToolParams } from './grep.js';
|
||||
import type { ToolResult, GrepResult, ExecuteOptions } from './tools.js';
|
||||
import path from 'node:path';
|
||||
import { isSubpath } from '../utils/paths.js';
|
||||
import { isSubpath, resolveToRealPath } from '../utils/paths.js';
|
||||
import fs from 'node:fs/promises';
|
||||
import os from 'node:os';
|
||||
import type { Config } from '../config/config.js';
|
||||
@@ -156,7 +156,7 @@ describe('GrepTool', () => {
|
||||
});
|
||||
|
||||
it('should return error if path is a file, not a directory', async () => {
|
||||
const filePath = path.join(tempRootDir, 'fileA.txt');
|
||||
const filePath = resolveToRealPath(path.join(tempRootDir, 'fileA.txt'));
|
||||
const params: GrepToolParams = { pattern: 'hello', dir_path: filePath };
|
||||
expect(grepTool.validateToolParams(params)).toContain(
|
||||
`Path is not a directory: ${filePath}`,
|
||||
|
||||
@@ -25,7 +25,11 @@ import {
|
||||
type ToolConfirmationOutcome,
|
||||
type ExecuteOptions,
|
||||
} from './tools.js';
|
||||
import { makeRelative, shortenPath } from '../utils/paths.js';
|
||||
import {
|
||||
makeRelative,
|
||||
shortenPath,
|
||||
resolveToRealPath,
|
||||
} from '../utils/paths.js';
|
||||
import { getErrorMessage, isNodeError } from '../utils/errors.js';
|
||||
import { isGitRepository } from '../utils/gitUtils.js';
|
||||
import type { Config } from '../config/config.js';
|
||||
@@ -146,7 +150,21 @@ class GrepToolInvocation extends BaseToolInvocation<
|
||||
|
||||
let searchDirAbs: string | null = null;
|
||||
if (pathParam) {
|
||||
searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam);
|
||||
try {
|
||||
searchDirAbs = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), pathParam),
|
||||
);
|
||||
} catch (err) {
|
||||
const errMsg = err instanceof Error ? err.message : String(err);
|
||||
return {
|
||||
llmContent: errMsg,
|
||||
returnDisplay: 'Error: Path resolution failed.',
|
||||
error: {
|
||||
message: errMsg,
|
||||
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
|
||||
},
|
||||
};
|
||||
}
|
||||
const validationError = this.config.validatePathAccess(
|
||||
searchDirAbs,
|
||||
'read',
|
||||
@@ -722,10 +740,14 @@ export class GrepTool extends BaseDeclarativeTool<GrepToolParams, ToolResult> {
|
||||
|
||||
// Only validate dir_path if one is provided
|
||||
if (params.dir_path) {
|
||||
const resolvedPath = path.resolve(
|
||||
this.config.getTargetDir(),
|
||||
params.dir_path,
|
||||
);
|
||||
let resolvedPath: string;
|
||||
try {
|
||||
resolvedPath = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), params.dir_path),
|
||||
);
|
||||
} catch (err) {
|
||||
return err instanceof Error ? err.message : String(err);
|
||||
}
|
||||
const validationError = this.config.validatePathAccess(
|
||||
resolvedPath,
|
||||
'read',
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -46,7 +46,7 @@ vi.mock('../utils/paths.js', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../utils/paths.js')>();
|
||||
return {
|
||||
...actual,
|
||||
resolveToRealPath: vi.fn((p) => p),
|
||||
resolveToRealPath: vi.fn((p) => actual.resolveToRealPath(p)),
|
||||
normalizePath: vi.fn((p) =>
|
||||
typeof p === 'string' ? p.replace(/\\/g, '/') : p,
|
||||
),
|
||||
@@ -1351,7 +1351,9 @@ describe('RipGrepTool', () => {
|
||||
});
|
||||
|
||||
it('should add .geminiignore when enabled and patterns exist', async () => {
|
||||
const geminiIgnorePath = path.join(tempRootDir, GEMINI_IGNORE_FILE_NAME);
|
||||
const geminiIgnorePath = resolveToRealPath(
|
||||
path.join(tempRootDir, GEMINI_IGNORE_FILE_NAME),
|
||||
);
|
||||
await fs.writeFile(geminiIgnorePath, 'ignored.log');
|
||||
|
||||
const configWithGeminiIgnore = createMockConfig(tempRootDir);
|
||||
@@ -1395,7 +1397,9 @@ describe('RipGrepTool', () => {
|
||||
});
|
||||
|
||||
it('should skip .geminiignore when disabled', async () => {
|
||||
const geminiIgnorePath = path.join(tempRootDir, GEMINI_IGNORE_FILE_NAME);
|
||||
const geminiIgnorePath = resolveToRealPath(
|
||||
path.join(tempRootDir, GEMINI_IGNORE_FILE_NAME),
|
||||
);
|
||||
await fs.writeFile(geminiIgnorePath, 'ignored.log');
|
||||
const configWithoutGeminiIgnore = createMockConfig(tempRootDir);
|
||||
vi.spyOn(
|
||||
|
||||
@@ -185,7 +185,22 @@ class GrepToolInvocation extends BaseToolInvocation<
|
||||
// This forces CWD search instead of 'all workspaces' search by default.
|
||||
const pathParam = this.params.dir_path || '.';
|
||||
|
||||
const searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam);
|
||||
let searchDirAbs: string;
|
||||
try {
|
||||
searchDirAbs = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), pathParam),
|
||||
);
|
||||
} catch (err) {
|
||||
const errMsg = err instanceof Error ? err.message : String(err);
|
||||
return {
|
||||
llmContent: errMsg,
|
||||
returnDisplay: 'Error: Path resolution failed.',
|
||||
error: {
|
||||
message: errMsg,
|
||||
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
|
||||
},
|
||||
};
|
||||
}
|
||||
const validationError = this.config.validatePathAccess(
|
||||
searchDirAbs,
|
||||
'read',
|
||||
@@ -624,8 +639,14 @@ export class RipGrepTool extends BaseDeclarativeTool<
|
||||
true, // isOutputMarkdown
|
||||
false, // canUpdateOutput
|
||||
);
|
||||
let targetDir = config.getTargetDir();
|
||||
try {
|
||||
targetDir = resolveToRealPath(targetDir);
|
||||
} catch {
|
||||
// Ignore and use raw targetDir
|
||||
}
|
||||
this.fileDiscoveryService = new FileDiscoveryService(
|
||||
config.getTargetDir(),
|
||||
targetDir,
|
||||
config.getFileFilteringOptions(),
|
||||
);
|
||||
}
|
||||
@@ -670,10 +691,14 @@ export class RipGrepTool extends BaseDeclarativeTool<
|
||||
|
||||
// Only validate path if one is provided
|
||||
if (params.dir_path) {
|
||||
const resolvedPath = path.resolve(
|
||||
this.config.getTargetDir(),
|
||||
params.dir_path,
|
||||
);
|
||||
let resolvedPath: string;
|
||||
try {
|
||||
resolvedPath = resolveToRealPath(
|
||||
path.resolve(this.config.getTargetDir(), params.dir_path),
|
||||
);
|
||||
} catch (err) {
|
||||
return err instanceof Error ? err.message : String(err);
|
||||
}
|
||||
const validationError = this.config.validatePathAccess(
|
||||
resolvedPath,
|
||||
'read',
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -20,7 +20,10 @@ describe('pathCorrector', () => {
|
||||
let mockConfig: Config;
|
||||
|
||||
beforeEach(() => {
|
||||
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-corrector-test-'));
|
||||
const rawTempDir = fs.mkdtempSync(
|
||||
path.join(os.tmpdir(), 'path-corrector-test-'),
|
||||
);
|
||||
tempDir = fs.realpathSync(rawTempDir);
|
||||
rootDir = path.join(tempDir, 'root');
|
||||
otherWorkspaceDir = path.join(tempDir, 'other');
|
||||
fs.mkdirSync(rootDir, { recursive: true });
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user