Pure refactor: Consolidate isWithinRoot() function calling. (#4163)

This commit is contained in:
Tommaso Sciortino 2025-07-14 22:55:49 -07:00 committed by GitHub
parent e584241141
commit fefa7ecbea
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 96 additions and 179 deletions

View File

@ -525,7 +525,6 @@ export class Config {
async createToolRegistry(): Promise<ToolRegistry> { async createToolRegistry(): Promise<ToolRegistry> {
const registry = new ToolRegistry(this); const registry = new ToolRegistry(this);
const targetDir = this.getTargetDir();
// helper to create & register core tools that are enabled // helper to create & register core tools that are enabled
// eslint-disable-next-line @typescript-eslint/no-explicit-any // eslint-disable-next-line @typescript-eslint/no-explicit-any
@ -560,14 +559,14 @@ export class Config {
} }
}; };
registerCoreTool(LSTool, targetDir, this); registerCoreTool(LSTool, this);
registerCoreTool(ReadFileTool, targetDir, this); registerCoreTool(ReadFileTool, this);
registerCoreTool(GrepTool, targetDir); registerCoreTool(GrepTool, this);
registerCoreTool(GlobTool, targetDir, this); registerCoreTool(GlobTool, this);
registerCoreTool(EditTool, this); registerCoreTool(EditTool, this);
registerCoreTool(WriteFileTool, this); registerCoreTool(WriteFileTool, this);
registerCoreTool(WebFetchTool, this); registerCoreTool(WebFetchTool, this);
registerCoreTool(ReadManyFilesTool, targetDir, this); registerCoreTool(ReadManyFilesTool, this);
registerCoreTool(ShellTool, this); registerCoreTool(ShellTool, this);
registerCoreTool(MemoryTool); registerCoreTool(MemoryTool);
registerCoreTool(WebSearchTool, this); registerCoreTool(WebSearchTool, this);

View File

@ -24,6 +24,7 @@ import { ensureCorrectEdit } from '../utils/editCorrector.js';
import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js';
import { ReadFileTool } from './read-file.js'; import { ReadFileTool } from './read-file.js';
import { ModifiableTool, ModifyContext } from './modifiable-tool.js'; import { ModifiableTool, ModifyContext } from './modifiable-tool.js';
import { isWithinRoot } from '../utils/fileUtils.js';
/** /**
* Parameters for the Edit tool * Parameters for the Edit tool
@ -72,12 +73,7 @@ export class EditTool
implements ModifiableTool<EditToolParams> implements ModifiableTool<EditToolParams>
{ {
static readonly Name = 'replace'; static readonly Name = 'replace';
private readonly rootDirectory: string;
/**
* Creates a new instance of the EditLogic
* @param rootDirectory Root directory to ground this tool in.
*/
constructor(private readonly config: Config) { constructor(private readonly config: Config) {
super( super(
EditTool.Name, EditTool.Name,
@ -121,24 +117,6 @@ Expectation for required parameters:
type: Type.OBJECT, type: Type.OBJECT,
}, },
); );
this.rootDirectory = path.resolve(this.config.getTargetDir());
}
/**
* Checks if a path is within the root directory.
* @param pathToCheck The absolute path to check.
* @returns True if the path is within the root directory, false otherwise.
*/
private isWithinRoot(pathToCheck: string): boolean {
const normalizedPath = path.normalize(pathToCheck);
const normalizedRoot = this.rootDirectory;
const rootWithSep = normalizedRoot.endsWith(path.sep)
? normalizedRoot
: normalizedRoot + path.sep;
return (
normalizedPath === normalizedRoot ||
normalizedPath.startsWith(rootWithSep)
);
} }
/** /**
@ -156,8 +134,8 @@ Expectation for required parameters:
return `File path must be absolute: ${params.file_path}`; return `File path must be absolute: ${params.file_path}`;
} }
if (!this.isWithinRoot(params.file_path)) { if (!isWithinRoot(params.file_path, this.config.getTargetDir())) {
return `File path must be within the root directory (${this.rootDirectory}): ${params.file_path}`; return `File path must be within the root directory (${this.config.getTargetDir()}): ${params.file_path}`;
} }
return null; return null;
@ -325,7 +303,7 @@ Expectation for required parameters:
); );
const confirmationDetails: ToolEditConfirmationDetails = { const confirmationDetails: ToolEditConfirmationDetails = {
type: 'edit', type: 'edit',
title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`, title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`,
fileName, fileName,
fileDiff, fileDiff,
onConfirm: async (outcome: ToolConfirmationOutcome) => { onConfirm: async (outcome: ToolConfirmationOutcome) => {
@ -341,7 +319,10 @@ Expectation for required parameters:
if (!params.file_path || !params.old_string || !params.new_string) { if (!params.file_path || !params.old_string || !params.new_string) {
return `Model did not provide valid parameters for edit tool`; return `Model did not provide valid parameters for edit tool`;
} }
const relativePath = makeRelative(params.file_path, this.rootDirectory); const relativePath = makeRelative(
params.file_path,
this.config.getTargetDir(),
);
if (params.old_string === '') { if (params.old_string === '') {
return `Create ${shortenPath(relativePath)}`; return `Create ${shortenPath(relativePath)}`;
} }
@ -400,7 +381,7 @@ Expectation for required parameters:
let displayResult: ToolResultDisplay; let displayResult: ToolResultDisplay;
if (editData.isNewFile) { if (editData.isNewFile) {
displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`; displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`;
} else { } else {
// Generate diff for display, even though core logic doesn't technically need it // Generate diff for display, even though core logic doesn't technically need it
// The CLI wrapper will use this part of the ToolResult // The CLI wrapper will use this part of the ToolResult

View File

@ -22,12 +22,13 @@ describe('GlobTool', () => {
const mockConfig = { const mockConfig = {
getFileService: () => new FileDiscoveryService(tempRootDir), getFileService: () => new FileDiscoveryService(tempRootDir),
getFileFilteringRespectGitIgnore: () => true, getFileFilteringRespectGitIgnore: () => true,
} as Partial<Config> as Config; getTargetDir: () => tempRootDir,
} as unknown as Config;
beforeEach(async () => { beforeEach(async () => {
// Create a unique root directory for each test run // Create a unique root directory for each test run
tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-')); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-'));
globTool = new GlobTool(tempRootDir, mockConfig); globTool = new GlobTool(mockConfig);
// Create some test files and directories within this root // Create some test files and directories within this root
// Top-level files // Top-level files
@ -224,8 +225,8 @@ describe('GlobTool', () => {
it("should return error if search path resolves outside the tool's root directory", () => { it("should return error if search path resolves outside the tool's root directory", () => {
// Create a globTool instance specifically for this test, with a deeper root // Create a globTool instance specifically for this test, with a deeper root
const deeperRootDir = path.join(tempRootDir, 'sub'); tempRootDir = path.join(tempRootDir, 'sub');
const specificGlobTool = new GlobTool(deeperRootDir, mockConfig); const specificGlobTool = new GlobTool(mockConfig);
// const params: GlobToolParams = { pattern: '*.txt', path: '..' }; // This line is unused and will be removed. // const params: GlobToolParams = { pattern: '*.txt', path: '..' }; // This line is unused and will be removed.
// This should be fine as tempRootDir is still within the original tempRootDir (the parent of deeperRootDir) // This should be fine as tempRootDir is still within the original tempRootDir (the parent of deeperRootDir)
// Let's try to go further up. // Let's try to go further up.

View File

@ -11,6 +11,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js';
import { BaseTool, ToolResult } from './tools.js'; import { BaseTool, ToolResult } from './tools.js';
import { Type } from '@google/genai'; import { Type } from '@google/genai';
import { shortenPath, makeRelative } from '../utils/paths.js'; import { shortenPath, makeRelative } from '../utils/paths.js';
import { isWithinRoot } from '../utils/fileUtils.js';
import { Config } from '../config/config.js'; import { Config } from '../config/config.js';
// Subset of 'Path' interface provided by 'glob' that we can implement for testing // Subset of 'Path' interface provided by 'glob' that we can implement for testing
@ -79,14 +80,8 @@ export interface GlobToolParams {
*/ */
export class GlobTool extends BaseTool<GlobToolParams, ToolResult> { export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
static readonly Name = 'glob'; static readonly Name = 'glob';
/**
* Creates a new instance of the GlobLogic constructor(private config: Config) {
* @param rootDirectory Root directory to ground this tool in.
*/
constructor(
private rootDirectory: string,
private config: Config,
) {
super( super(
GlobTool.Name, GlobTool.Name,
'FindFiles', 'FindFiles',
@ -118,28 +113,6 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
type: Type.OBJECT, type: Type.OBJECT,
}, },
); );
this.rootDirectory = path.resolve(rootDirectory);
}
/**
* Checks if a given path is within the root directory bounds.
* This security check prevents accessing files outside the designated root directory.
*
* @param pathToCheck The absolute path to validate
* @returns True if the path is within the root directory, false otherwise
*/
private isWithinRoot(pathToCheck: string): boolean {
const absolutePathToCheck = path.resolve(pathToCheck);
const normalizedPath = path.normalize(absolutePathToCheck);
const normalizedRoot = path.normalize(this.rootDirectory);
const rootWithSep = normalizedRoot.endsWith(path.sep)
? normalizedRoot
: normalizedRoot + path.sep;
return (
normalizedPath === normalizedRoot ||
normalizedPath.startsWith(rootWithSep)
);
} }
/** /**
@ -152,15 +125,15 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
} }
const searchDirAbsolute = path.resolve( const searchDirAbsolute = path.resolve(
this.rootDirectory, this.config.getTargetDir(),
params.path || '.', params.path || '.',
); );
if (!this.isWithinRoot(searchDirAbsolute)) { if (!isWithinRoot(searchDirAbsolute, this.config.getTargetDir())) {
return `Search path ("${searchDirAbsolute}") resolves outside the tool's root directory ("${this.rootDirectory}").`; return `Search path ("${searchDirAbsolute}") resolves outside the tool's root directory ("${this.config.getTargetDir()}").`;
} }
const targetDir = searchDirAbsolute || this.rootDirectory; const targetDir = searchDirAbsolute || this.config.getTargetDir();
try { try {
if (!fs.existsSync(targetDir)) { if (!fs.existsSync(targetDir)) {
return `Search path does not exist ${targetDir}`; return `Search path does not exist ${targetDir}`;
@ -189,8 +162,11 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
getDescription(params: GlobToolParams): string { getDescription(params: GlobToolParams): string {
let description = `'${params.pattern}'`; let description = `'${params.pattern}'`;
if (params.path) { if (params.path) {
const searchDir = path.resolve(this.rootDirectory, params.path || '.'); const searchDir = path.resolve(
const relativePath = makeRelative(searchDir, this.rootDirectory); this.config.getTargetDir(),
params.path || '.',
);
const relativePath = makeRelative(searchDir, this.config.getTargetDir());
description += ` within ${shortenPath(relativePath)}`; description += ` within ${shortenPath(relativePath)}`;
} }
return description; return description;
@ -213,7 +189,7 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
try { try {
const searchDirAbsolute = path.resolve( const searchDirAbsolute = path.resolve(
this.rootDirectory, this.config.getTargetDir(),
params.path || '.', params.path || '.',
); );
@ -241,13 +217,15 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
if (respectGitIgnore) { if (respectGitIgnore) {
const relativePaths = entries.map((p) => const relativePaths = entries.map((p) =>
path.relative(this.rootDirectory, p.fullpath()), path.relative(this.config.getTargetDir(), p.fullpath()),
); );
const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, { const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, {
respectGitIgnore, respectGitIgnore,
}); });
const filteredAbsolutePaths = new Set( const filteredAbsolutePaths = new Set(
filteredRelativePaths.map((p) => path.resolve(this.rootDirectory, p)), filteredRelativePaths.map((p) =>
path.resolve(this.config.getTargetDir(), p),
),
); );
filteredEntries = entries.filter((entry) => filteredEntries = entries.filter((entry) =>

View File

@ -9,6 +9,7 @@ import { GrepTool, GrepToolParams } from './grep.js';
import path from 'path'; import path from 'path';
import fs from 'fs/promises'; import fs from 'fs/promises';
import os from 'os'; import os from 'os';
import { Config } from '../config/config.js';
// Mock the child_process module to control grep/git grep behavior // Mock the child_process module to control grep/git grep behavior
vi.mock('child_process', () => ({ vi.mock('child_process', () => ({
@ -30,9 +31,13 @@ describe('GrepTool', () => {
let grepTool: GrepTool; let grepTool: GrepTool;
const abortSignal = new AbortController().signal; const abortSignal = new AbortController().signal;
const mockConfig = {
getTargetDir: () => tempRootDir,
} as unknown as Config;
beforeEach(async () => { beforeEach(async () => {
tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-'));
grepTool = new GrepTool(tempRootDir); grepTool = new GrepTool(mockConfig);
// Create some test files and directories // Create some test files and directories
await fs.writeFile( await fs.writeFile(

View File

@ -16,6 +16,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js';
import { makeRelative, shortenPath } from '../utils/paths.js'; import { makeRelative, shortenPath } from '../utils/paths.js';
import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js';
import { isGitRepository } from '../utils/gitUtils.js'; import { isGitRepository } from '../utils/gitUtils.js';
import { Config } from '../config/config.js';
// --- Interfaces --- // --- Interfaces ---
@ -56,11 +57,7 @@ interface GrepMatch {
export class GrepTool extends BaseTool<GrepToolParams, ToolResult> { export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
static readonly Name = 'search_file_content'; // Keep static name static readonly Name = 'search_file_content'; // Keep static name
/** constructor(private readonly config: Config) {
* Creates a new instance of the GrepLogic
* @param rootDirectory Root directory to ground this tool in. All operations will be restricted to this directory.
*/
constructor(private rootDirectory: string) {
super( super(
GrepTool.Name, GrepTool.Name,
'SearchText', 'SearchText',
@ -87,8 +84,6 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
type: Type.OBJECT, type: Type.OBJECT,
}, },
); );
// Ensure rootDirectory is absolute and normalized
this.rootDirectory = path.resolve(rootDirectory);
} }
// --- Validation Methods --- // --- Validation Methods ---
@ -100,15 +95,18 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
* @throws {Error} If path is outside root, doesn't exist, or isn't a directory. * @throws {Error} If path is outside root, doesn't exist, or isn't a directory.
*/ */
private resolveAndValidatePath(relativePath?: string): string { private resolveAndValidatePath(relativePath?: string): string {
const targetPath = path.resolve(this.rootDirectory, relativePath || '.'); const targetPath = path.resolve(
this.config.getTargetDir(),
relativePath || '.',
);
// Security Check: Ensure the resolved path is still within the root directory. // Security Check: Ensure the resolved path is still within the root directory.
if ( if (
!targetPath.startsWith(this.rootDirectory) && !targetPath.startsWith(this.config.getTargetDir()) &&
targetPath !== this.rootDirectory targetPath !== this.config.getTargetDir()
) { ) {
throw new Error( throw new Error(
`Path validation failed: Attempted path "${relativePath || '.'}" resolves outside the allowed root directory "${this.rootDirectory}".`, `Path validation failed: Attempted path "${relativePath || '.'}" resolves outside the allowed root directory "${this.config.getTargetDir()}".`,
); );
} }
@ -322,11 +320,17 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
description += ` in ${params.include}`; description += ` in ${params.include}`;
} }
if (params.path) { if (params.path) {
const resolvedPath = path.resolve(this.rootDirectory, params.path); const resolvedPath = path.resolve(
if (resolvedPath === this.rootDirectory || params.path === '.') { this.config.getTargetDir(),
params.path,
);
if (resolvedPath === this.config.getTargetDir() || params.path === '.') {
description += ` within ./`; description += ` within ./`;
} else { } else {
const relativePath = makeRelative(resolvedPath, this.rootDirectory); const relativePath = makeRelative(
resolvedPath,
this.config.getTargetDir(),
);
description += ` within ${shortenPath(relativePath)}`; description += ` within ${shortenPath(relativePath)}`;
} }
} }

View File

@ -11,6 +11,7 @@ import { Type } from '@google/genai';
import { SchemaValidator } from '../utils/schemaValidator.js'; import { SchemaValidator } from '../utils/schemaValidator.js';
import { makeRelative, shortenPath } from '../utils/paths.js'; import { makeRelative, shortenPath } from '../utils/paths.js';
import { Config } from '../config/config.js'; import { Config } from '../config/config.js';
import { isWithinRoot } from '../utils/fileUtils.js';
/** /**
* Parameters for the LS tool * Parameters for the LS tool
@ -68,14 +69,7 @@ export interface FileEntry {
export class LSTool extends BaseTool<LSToolParams, ToolResult> { export class LSTool extends BaseTool<LSToolParams, ToolResult> {
static readonly Name = 'list_directory'; static readonly Name = 'list_directory';
/** constructor(private config: Config) {
* Creates a new instance of the LSLogic
* @param rootDirectory Root directory to ground this tool in. All operations will be restricted to this directory.
*/
constructor(
private rootDirectory: string,
private config: Config,
) {
super( super(
LSTool.Name, LSTool.Name,
'ReadFolder', 'ReadFolder',
@ -104,27 +98,6 @@ export class LSTool extends BaseTool<LSToolParams, ToolResult> {
type: Type.OBJECT, type: Type.OBJECT,
}, },
); );
// Set the root directory
this.rootDirectory = path.resolve(rootDirectory);
}
/**
* Checks if a path is within the root directory
* @param dirpath The path to check
* @returns True if the path is within the root directory, false otherwise
*/
private isWithinRoot(dirpath: string): boolean {
const normalizedPath = path.normalize(dirpath);
const normalizedRoot = path.normalize(this.rootDirectory);
// Ensure the normalizedRoot ends with a path separator for proper path comparison
const rootWithSep = normalizedRoot.endsWith(path.sep)
? normalizedRoot
: normalizedRoot + path.sep;
return (
normalizedPath === normalizedRoot ||
normalizedPath.startsWith(rootWithSep)
);
} }
/** /**
@ -140,8 +113,8 @@ export class LSTool extends BaseTool<LSToolParams, ToolResult> {
if (!path.isAbsolute(params.path)) { if (!path.isAbsolute(params.path)) {
return `Path must be absolute: ${params.path}`; return `Path must be absolute: ${params.path}`;
} }
if (!this.isWithinRoot(params.path)) { if (!isWithinRoot(params.path, this.config.getTargetDir())) {
return `Path must be within the root directory (${this.rootDirectory}): ${params.path}`; return `Path must be within the root directory (${this.config.getTargetDir()}): ${params.path}`;
} }
return null; return null;
} }
@ -176,7 +149,7 @@ export class LSTool extends BaseTool<LSToolParams, ToolResult> {
* @returns A string describing the file being read * @returns A string describing the file being read
*/ */
getDescription(params: LSToolParams): string { getDescription(params: LSToolParams): string {
const relativePath = makeRelative(params.path, this.rootDirectory); const relativePath = makeRelative(params.path, this.config.getTargetDir());
return shortenPath(relativePath); return shortenPath(relativePath);
} }
@ -248,7 +221,10 @@ export class LSTool extends BaseTool<LSToolParams, ToolResult> {
} }
const fullPath = path.join(params.path, file); const fullPath = path.join(params.path, file);
const relativePath = path.relative(this.rootDirectory, fullPath); const relativePath = path.relative(
this.config.getTargetDir(),
fullPath,
);
// Check if this file should be git-ignored (only in git repositories) // Check if this file should be git-ignored (only in git repositories)
if ( if (

View File

@ -42,8 +42,9 @@ describe('ReadFileTool', () => {
const fileService = new FileDiscoveryService(tempRootDir); const fileService = new FileDiscoveryService(tempRootDir);
const mockConfigInstance = { const mockConfigInstance = {
getFileService: () => fileService, getFileService: () => fileService,
getTargetDir: () => tempRootDir,
} as unknown as Config; } as unknown as Config;
tool = new ReadFileTool(tempRootDir, mockConfigInstance); tool = new ReadFileTool(mockConfigInstance);
mockProcessSingleFileContent.mockReset(); mockProcessSingleFileContent.mockReset();
}); });

View File

@ -46,10 +46,7 @@ export interface ReadFileToolParams {
export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> { export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
static readonly Name: string = 'read_file'; static readonly Name: string = 'read_file';
constructor( constructor(private config: Config) {
private rootDirectory: string,
private config: Config,
) {
super( super(
ReadFileTool.Name, ReadFileTool.Name,
'ReadFile', 'ReadFile',
@ -76,7 +73,6 @@ export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
type: Type.OBJECT, type: Type.OBJECT,
}, },
); );
this.rootDirectory = path.resolve(rootDirectory);
} }
validateToolParams(params: ReadFileToolParams): string | null { validateToolParams(params: ReadFileToolParams): string | null {
@ -89,8 +85,8 @@ export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
if (!path.isAbsolute(filePath)) { if (!path.isAbsolute(filePath)) {
return `File path must be absolute, but was relative: ${filePath}. You must provide an absolute path.`; return `File path must be absolute, but was relative: ${filePath}. You must provide an absolute path.`;
} }
if (!isWithinRoot(filePath, this.rootDirectory)) { if (!isWithinRoot(filePath, this.config.getTargetDir())) {
return `File path must be within the root directory (${this.rootDirectory}): ${filePath}`; return `File path must be within the root directory (${this.config.getTargetDir()}): ${filePath}`;
} }
if (params.offset !== undefined && params.offset < 0) { if (params.offset !== undefined && params.offset < 0) {
return 'Offset must be a non-negative number'; return 'Offset must be a non-negative number';
@ -115,7 +111,10 @@ export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
) { ) {
return `Path unavailable`; return `Path unavailable`;
} }
const relativePath = makeRelative(params.absolute_path, this.rootDirectory); const relativePath = makeRelative(
params.absolute_path,
this.config.getTargetDir(),
);
return shortenPath(relativePath); return shortenPath(relativePath);
} }
@ -133,7 +132,7 @@ export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
const result = await processSingleFileContent( const result = await processSingleFileContent(
params.absolute_path, params.absolute_path,
this.rootDirectory, this.config.getTargetDir(),
params.offset, params.offset,
params.limit, params.limit,
); );

View File

@ -59,9 +59,10 @@ describe('ReadManyFilesTool', () => {
const mockConfig = { const mockConfig = {
getFileService: () => fileService, getFileService: () => fileService,
getFileFilteringRespectGitIgnore: () => true, getFileFilteringRespectGitIgnore: () => true,
getTargetDir: () => tempRootDir,
} as Partial<Config> as Config; } as Partial<Config> as Config;
tool = new ReadManyFilesTool(tempRootDir, mockConfig); tool = new ReadManyFilesTool(mockConfig);
mockReadFileFn = mockControl.mockReadFile; mockReadFileFn = mockControl.mockReadFile;
mockReadFileFn.mockReset(); mockReadFileFn.mockReset();

View File

@ -124,17 +124,10 @@ export class ReadManyFilesTool extends BaseTool<
ToolResult ToolResult
> { > {
static readonly Name: string = 'read_many_files'; static readonly Name: string = 'read_many_files';
private readonly geminiIgnorePatterns: string[] = []; private readonly geminiIgnorePatterns: string[] = [];
/** constructor(private config: Config) {
* Creates an instance of ReadManyFilesTool.
* @param targetDir The absolute root directory within which this tool is allowed to operate.
* All paths provided in `params` will be resolved relative to this directory.
*/
constructor(
readonly targetDir: string,
private config: Config,
) {
const parameterSchema: Schema = { const parameterSchema: Schema = {
type: Type.OBJECT, type: Type.OBJECT,
properties: { properties: {
@ -205,7 +198,6 @@ This tool is useful when you need to understand or analyze a collection of files
Use this tool when the user's query implies needing the content of several files simultaneously for context, analysis, or summarization. For text files, it uses default UTF-8 encoding and a '--- {filePath} ---' separator between file contents. Ensure paths are relative to the target directory. Glob patterns like 'src/**/*.js' are supported. Avoid using for single files if a more specific single-file reading tool is available, unless the user specifically requests to process a list containing just one file via this tool. Other binary files (not explicitly requested as image/PDF) are generally skipped. Default excludes apply to common non-text files (except for explicitly requested images/PDFs) and large dependency directories unless 'useDefaultExcludes' is false.`, Use this tool when the user's query implies needing the content of several files simultaneously for context, analysis, or summarization. For text files, it uses default UTF-8 encoding and a '--- {filePath} ---' separator between file contents. Ensure paths are relative to the target directory. Glob patterns like 'src/**/*.js' are supported. Avoid using for single files if a more specific single-file reading tool is available, unless the user specifically requests to process a list containing just one file via this tool. Other binary files (not explicitly requested as image/PDF) are generally skipped. Default excludes apply to common non-text files (except for explicitly requested images/PDFs) and large dependency directories unless 'useDefaultExcludes' is false.`,
parameterSchema, parameterSchema,
); );
this.targetDir = path.resolve(targetDir);
this.geminiIgnorePatterns = config this.geminiIgnorePatterns = config
.getFileService() .getFileService()
.getGeminiIgnorePatterns(); .getGeminiIgnorePatterns();
@ -221,7 +213,7 @@ Use this tool when the user's query implies needing the content of several files
getDescription(params: ReadManyFilesParams): string { getDescription(params: ReadManyFilesParams): string {
const allPatterns = [...params.paths, ...(params.include || [])]; const allPatterns = [...params.paths, ...(params.include || [])];
const pathDesc = `using patterns: \`${allPatterns.join('`, `')}\` (within target directory: \`${this.targetDir}\`)`; const pathDesc = `using patterns: \`${allPatterns.join('`, `')}\` (within target directory: \`${this.config.getTargetDir()}\`)`;
// Determine the final list of exclusion patterns exactly as in execute method // Determine the final list of exclusion patterns exactly as in execute method
const paramExcludes = params.exclude || []; const paramExcludes = params.exclude || [];
@ -273,7 +265,6 @@ Use this tool when the user's query implies needing the content of several files
// Get centralized file discovery service // Get centralized file discovery service
const fileDiscovery = this.config.getFileService(); const fileDiscovery = this.config.getFileService();
const toolBaseDir = this.targetDir;
const filesToConsider = new Set<string>(); const filesToConsider = new Set<string>();
const skippedFiles: Array<{ path: string; reason: string }> = []; const skippedFiles: Array<{ path: string; reason: string }> = [];
const processedFilesRelativePaths: string[] = []; const processedFilesRelativePaths: string[] = [];
@ -293,7 +284,7 @@ Use this tool when the user's query implies needing the content of several files
try { try {
const entries = await glob(searchPatterns, { const entries = await glob(searchPatterns, {
cwd: toolBaseDir, cwd: this.config.getTargetDir(),
ignore: effectiveExcludes, ignore: effectiveExcludes,
nodir: true, nodir: true,
dot: true, dot: true,
@ -305,21 +296,21 @@ Use this tool when the user's query implies needing the content of several files
const filteredEntries = respectGitIgnore const filteredEntries = respectGitIgnore
? fileDiscovery ? fileDiscovery
.filterFiles( .filterFiles(
entries.map((p) => path.relative(toolBaseDir, p)), entries.map((p) => path.relative(this.config.getTargetDir(), p)),
{ {
respectGitIgnore, respectGitIgnore,
}, },
) )
.map((p) => path.resolve(toolBaseDir, p)) .map((p) => path.resolve(this.config.getTargetDir(), p))
: entries; : entries;
let gitIgnoredCount = 0; let gitIgnoredCount = 0;
for (const absoluteFilePath of entries) { for (const absoluteFilePath of entries) {
// Security check: ensure the glob library didn't return something outside targetDir. // Security check: ensure the glob library didn't return something outside targetDir.
if (!absoluteFilePath.startsWith(toolBaseDir)) { if (!absoluteFilePath.startsWith(this.config.getTargetDir())) {
skippedFiles.push({ skippedFiles.push({
path: absoluteFilePath, path: absoluteFilePath,
reason: `Security: Glob library returned path outside target directory. Base: ${toolBaseDir}, Path: ${absoluteFilePath}`, reason: `Security: Glob library returned path outside target directory. Base: ${this.config.getTargetDir()}, Path: ${absoluteFilePath}`,
}); });
continue; continue;
} }
@ -351,7 +342,7 @@ Use this tool when the user's query implies needing the content of several files
for (const filePath of sortedFiles) { for (const filePath of sortedFiles) {
const relativePathForDisplay = path const relativePathForDisplay = path
.relative(toolBaseDir, filePath) .relative(this.config.getTargetDir(), filePath)
.replace(/\\/g, '/'); .replace(/\\/g, '/');
const fileType = detectFileType(filePath); const fileType = detectFileType(filePath);
@ -378,7 +369,7 @@ Use this tool when the user's query implies needing the content of several files
// Use processSingleFileContent for all file types now // Use processSingleFileContent for all file types now
const fileReadResult = await processSingleFileContent( const fileReadResult = await processSingleFileContent(
filePath, filePath,
toolBaseDir, this.config.getTargetDir(),
); );
if (fileReadResult.error) { if (fileReadResult.error) {
@ -412,7 +403,7 @@ Use this tool when the user's query implies needing the content of several files
} }
} }
let displayMessage = `### ReadManyFiles Result (Target Dir: \`${this.targetDir}\`)\n\n`; let displayMessage = `### ReadManyFiles Result (Target Dir: \`${this.config.getTargetDir()}\`)\n\n`;
if (processedFilesRelativePaths.length > 0) { if (processedFilesRelativePaths.length > 0) {
displayMessage += `Successfully read and concatenated content from **${processedFilesRelativePaths.length} file(s)**.\n`; displayMessage += `Successfully read and concatenated content from **${processedFilesRelativePaths.length} file(s)**.\n`;
if (processedFilesRelativePaths.length <= 10) { if (processedFilesRelativePaths.length <= 10) {

View File

@ -26,7 +26,7 @@ import {
} from '../utils/editCorrector.js'; } from '../utils/editCorrector.js';
import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js';
import { ModifiableTool, ModifyContext } from './modifiable-tool.js'; import { ModifiableTool, ModifyContext } from './modifiable-tool.js';
import { getSpecificMimeType } from '../utils/fileUtils.js'; import { getSpecificMimeType, isWithinRoot } from '../utils/fileUtils.js';
import { import {
recordFileOperationMetric, recordFileOperationMetric,
FileOperation, FileOperation,
@ -93,25 +93,6 @@ export class WriteFileTool
); );
} }
/**
* Checks if a given path is within the root directory bounds.
* This security check prevents writing files outside the designated root directory.
*
* @param pathToCheck The absolute path to validate
* @returns True if the path is within the root directory, false otherwise
*/
private isWithinRoot(pathToCheck: string): boolean {
const normalizedPath = path.normalize(pathToCheck);
const normalizedRoot = path.normalize(this.config.getTargetDir());
const rootWithSep = normalizedRoot.endsWith(path.sep)
? normalizedRoot
: normalizedRoot + path.sep;
return (
normalizedPath === normalizedRoot ||
normalizedPath.startsWith(rootWithSep)
);
}
validateToolParams(params: WriteFileToolParams): string | null { validateToolParams(params: WriteFileToolParams): string | null {
const errors = SchemaValidator.validate(this.schema.parameters, params); const errors = SchemaValidator.validate(this.schema.parameters, params);
if (errors) { if (errors) {
@ -122,7 +103,7 @@ export class WriteFileTool
if (!path.isAbsolute(filePath)) { if (!path.isAbsolute(filePath)) {
return `File path must be absolute: ${filePath}`; return `File path must be absolute: ${filePath}`;
} }
if (!this.isWithinRoot(filePath)) { if (!isWithinRoot(filePath, this.config.getTargetDir())) {
return `File path must be within the root directory (${this.config.getTargetDir()}): ${filePath}`; return `File path must be within the root directory (${this.config.getTargetDir()}): ${filePath}`;
} }

View File

@ -36,8 +36,8 @@ export function isWithinRoot(
pathToCheck: string, pathToCheck: string,
rootDirectory: string, rootDirectory: string,
): boolean { ): boolean {
const normalizedPathToCheck = path.normalize(pathToCheck); const normalizedPathToCheck = path.resolve(pathToCheck);
const normalizedRootDirectory = path.normalize(rootDirectory); const normalizedRootDirectory = path.resolve(rootDirectory);
// Ensure the rootDirectory path ends with a separator for correct startsWith comparison, // Ensure the rootDirectory path ends with a separator for correct startsWith comparison,
// unless it's the root path itself (e.g., '/' or 'C:\'). // unless it's the root path itself (e.g., '/' or 'C:\').