From fefa7ecbea2ab985e48679ceb3010a174777a188 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Mon, 14 Jul 2025 22:55:49 -0700 Subject: [PATCH] Pure refactor: Consolidate isWithinRoot() function calling. (#4163) --- packages/core/src/config/config.ts | 11 ++-- packages/core/src/tools/edit.ts | 37 +++--------- packages/core/src/tools/glob.test.ts | 9 +-- packages/core/src/tools/glob.ts | 56 ++++++------------- packages/core/src/tools/grep.test.ts | 7 ++- packages/core/src/tools/grep.ts | 32 ++++++----- packages/core/src/tools/ls.ts | 42 +++----------- packages/core/src/tools/read-file.test.ts | 3 +- packages/core/src/tools/read-file.ts | 17 +++--- .../core/src/tools/read-many-files.test.ts | 3 +- packages/core/src/tools/read-many-files.ts | 31 ++++------ packages/core/src/tools/write-file.ts | 23 +------- packages/core/src/utils/fileUtils.ts | 4 +- 13 files changed, 96 insertions(+), 179 deletions(-) diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 1b101815..fead0a9c 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -525,7 +525,6 @@ export class Config { async createToolRegistry(): Promise { const registry = new ToolRegistry(this); - const targetDir = this.getTargetDir(); // helper to create & register core tools that are enabled // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -560,14 +559,14 @@ export class Config { } }; - registerCoreTool(LSTool, targetDir, this); - registerCoreTool(ReadFileTool, targetDir, this); - registerCoreTool(GrepTool, targetDir); - registerCoreTool(GlobTool, targetDir, this); + registerCoreTool(LSTool, this); + registerCoreTool(ReadFileTool, this); + registerCoreTool(GrepTool, this); + registerCoreTool(GlobTool, this); registerCoreTool(EditTool, this); registerCoreTool(WriteFileTool, this); registerCoreTool(WebFetchTool, this); - registerCoreTool(ReadManyFilesTool, targetDir, this); + registerCoreTool(ReadManyFilesTool, this); registerCoreTool(ShellTool, this); registerCoreTool(MemoryTool); registerCoreTool(WebSearchTool, this); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 45e74e93..8d8753d4 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -24,6 +24,7 @@ import { ensureCorrectEdit } from '../utils/editCorrector.js'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { ReadFileTool } from './read-file.js'; import { ModifiableTool, ModifyContext } from './modifiable-tool.js'; +import { isWithinRoot } from '../utils/fileUtils.js'; /** * Parameters for the Edit tool @@ -72,12 +73,7 @@ export class EditTool implements ModifiableTool { 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) { super( EditTool.Name, @@ -121,24 +117,6 @@ Expectation for required parameters: 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}`; } - if (!this.isWithinRoot(params.file_path)) { - return `File path must be within the root directory (${this.rootDirectory}): ${params.file_path}`; + if (!isWithinRoot(params.file_path, this.config.getTargetDir())) { + return `File path must be within the root directory (${this.config.getTargetDir()}): ${params.file_path}`; } return null; @@ -325,7 +303,7 @@ Expectation for required parameters: ); const confirmationDetails: ToolEditConfirmationDetails = { type: 'edit', - title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`, + title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`, fileName, fileDiff, onConfirm: async (outcome: ToolConfirmationOutcome) => { @@ -341,7 +319,10 @@ Expectation for required parameters: if (!params.file_path || !params.old_string || !params.new_string) { 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 === '') { return `Create ${shortenPath(relativePath)}`; } @@ -400,7 +381,7 @@ Expectation for required parameters: let displayResult: ToolResultDisplay; if (editData.isNewFile) { - displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`; + displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`; } else { // Generate diff for display, even though core logic doesn't technically need it // The CLI wrapper will use this part of the ToolResult diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index acda3729..c63b41cc 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -22,12 +22,13 @@ describe('GlobTool', () => { const mockConfig = { getFileService: () => new FileDiscoveryService(tempRootDir), getFileFilteringRespectGitIgnore: () => true, - } as Partial as Config; + getTargetDir: () => tempRootDir, + } as unknown as Config; beforeEach(async () => { // Create a unique root directory for each test run 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 // Top-level files @@ -224,8 +225,8 @@ describe('GlobTool', () => { 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 - const deeperRootDir = path.join(tempRootDir, 'sub'); - const specificGlobTool = new GlobTool(deeperRootDir, mockConfig); + tempRootDir = path.join(tempRootDir, 'sub'); + const specificGlobTool = new GlobTool(mockConfig); // 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) // Let's try to go further up. diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 23f05c2e..9381894e 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -11,6 +11,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js'; import { BaseTool, ToolResult } from './tools.js'; import { Type } from '@google/genai'; import { shortenPath, makeRelative } from '../utils/paths.js'; +import { isWithinRoot } from '../utils/fileUtils.js'; import { Config } from '../config/config.js'; // 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 { static readonly Name = 'glob'; - /** - * Creates a new instance of the GlobLogic - * @param rootDirectory Root directory to ground this tool in. - */ - constructor( - private rootDirectory: string, - private config: Config, - ) { + + constructor(private config: Config) { super( GlobTool.Name, 'FindFiles', @@ -118,28 +113,6 @@ export class GlobTool extends BaseTool { 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 { } const searchDirAbsolute = path.resolve( - this.rootDirectory, + this.config.getTargetDir(), params.path || '.', ); - if (!this.isWithinRoot(searchDirAbsolute)) { - return `Search path ("${searchDirAbsolute}") resolves outside the tool's root directory ("${this.rootDirectory}").`; + if (!isWithinRoot(searchDirAbsolute, this.config.getTargetDir())) { + 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 { if (!fs.existsSync(targetDir)) { return `Search path does not exist ${targetDir}`; @@ -189,8 +162,11 @@ export class GlobTool extends BaseTool { getDescription(params: GlobToolParams): string { let description = `'${params.pattern}'`; if (params.path) { - const searchDir = path.resolve(this.rootDirectory, params.path || '.'); - const relativePath = makeRelative(searchDir, this.rootDirectory); + const searchDir = path.resolve( + this.config.getTargetDir(), + params.path || '.', + ); + const relativePath = makeRelative(searchDir, this.config.getTargetDir()); description += ` within ${shortenPath(relativePath)}`; } return description; @@ -213,7 +189,7 @@ export class GlobTool extends BaseTool { try { const searchDirAbsolute = path.resolve( - this.rootDirectory, + this.config.getTargetDir(), params.path || '.', ); @@ -241,13 +217,15 @@ export class GlobTool extends BaseTool { if (respectGitIgnore) { const relativePaths = entries.map((p) => - path.relative(this.rootDirectory, p.fullpath()), + path.relative(this.config.getTargetDir(), p.fullpath()), ); const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, { respectGitIgnore, }); 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) => diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 62346bcd..2e018cce 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -9,6 +9,7 @@ import { GrepTool, GrepToolParams } from './grep.js'; import path from 'path'; import fs from 'fs/promises'; import os from 'os'; +import { Config } from '../config/config.js'; // Mock the child_process module to control grep/git grep behavior vi.mock('child_process', () => ({ @@ -30,9 +31,13 @@ describe('GrepTool', () => { let grepTool: GrepTool; const abortSignal = new AbortController().signal; + const mockConfig = { + getTargetDir: () => tempRootDir, + } as unknown as Config; + beforeEach(async () => { 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 await fs.writeFile( diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index b3c7a17b..afe83050 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -16,6 +16,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { isGitRepository } from '../utils/gitUtils.js'; +import { Config } from '../config/config.js'; // --- Interfaces --- @@ -56,11 +57,7 @@ interface GrepMatch { export class GrepTool extends BaseTool { static readonly Name = 'search_file_content'; // Keep static name - /** - * 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) { + constructor(private readonly config: Config) { super( GrepTool.Name, 'SearchText', @@ -87,8 +84,6 @@ export class GrepTool extends BaseTool { type: Type.OBJECT, }, ); - // Ensure rootDirectory is absolute and normalized - this.rootDirectory = path.resolve(rootDirectory); } // --- Validation Methods --- @@ -100,15 +95,18 @@ export class GrepTool extends BaseTool { * @throws {Error} If path is outside root, doesn't exist, or isn't a directory. */ 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. if ( - !targetPath.startsWith(this.rootDirectory) && - targetPath !== this.rootDirectory + !targetPath.startsWith(this.config.getTargetDir()) && + targetPath !== this.config.getTargetDir() ) { 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 { description += ` in ${params.include}`; } if (params.path) { - const resolvedPath = path.resolve(this.rootDirectory, params.path); - if (resolvedPath === this.rootDirectory || params.path === '.') { + const resolvedPath = path.resolve( + this.config.getTargetDir(), + params.path, + ); + if (resolvedPath === this.config.getTargetDir() || params.path === '.') { description += ` within ./`; } else { - const relativePath = makeRelative(resolvedPath, this.rootDirectory); + const relativePath = makeRelative( + resolvedPath, + this.config.getTargetDir(), + ); description += ` within ${shortenPath(relativePath)}`; } } diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index df09cc50..9fb60072 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -11,6 +11,7 @@ import { Type } from '@google/genai'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { Config } from '../config/config.js'; +import { isWithinRoot } from '../utils/fileUtils.js'; /** * Parameters for the LS tool @@ -68,14 +69,7 @@ export interface FileEntry { export class LSTool extends BaseTool { static readonly Name = 'list_directory'; - /** - * 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, - ) { + constructor(private config: Config) { super( LSTool.Name, 'ReadFolder', @@ -104,27 +98,6 @@ export class LSTool extends BaseTool { 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 { if (!path.isAbsolute(params.path)) { return `Path must be absolute: ${params.path}`; } - if (!this.isWithinRoot(params.path)) { - return `Path must be within the root directory (${this.rootDirectory}): ${params.path}`; + if (!isWithinRoot(params.path, this.config.getTargetDir())) { + return `Path must be within the root directory (${this.config.getTargetDir()}): ${params.path}`; } return null; } @@ -176,7 +149,7 @@ export class LSTool extends BaseTool { * @returns A string describing the file being read */ getDescription(params: LSToolParams): string { - const relativePath = makeRelative(params.path, this.rootDirectory); + const relativePath = makeRelative(params.path, this.config.getTargetDir()); return shortenPath(relativePath); } @@ -248,7 +221,10 @@ export class LSTool extends BaseTool { } 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) if ( diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 96bb7680..3c67b9dd 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -42,8 +42,9 @@ describe('ReadFileTool', () => { const fileService = new FileDiscoveryService(tempRootDir); const mockConfigInstance = { getFileService: () => fileService, + getTargetDir: () => tempRootDir, } as unknown as Config; - tool = new ReadFileTool(tempRootDir, mockConfigInstance); + tool = new ReadFileTool(mockConfigInstance); mockProcessSingleFileContent.mockReset(); }); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 5c37f45b..a2ff89c1 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -46,10 +46,7 @@ export interface ReadFileToolParams { export class ReadFileTool extends BaseTool { static readonly Name: string = 'read_file'; - constructor( - private rootDirectory: string, - private config: Config, - ) { + constructor(private config: Config) { super( ReadFileTool.Name, 'ReadFile', @@ -76,7 +73,6 @@ export class ReadFileTool extends BaseTool { type: Type.OBJECT, }, ); - this.rootDirectory = path.resolve(rootDirectory); } validateToolParams(params: ReadFileToolParams): string | null { @@ -89,8 +85,8 @@ export class ReadFileTool extends BaseTool { if (!path.isAbsolute(filePath)) { return `File path must be absolute, but was relative: ${filePath}. You must provide an absolute path.`; } - if (!isWithinRoot(filePath, this.rootDirectory)) { - return `File path must be within the root directory (${this.rootDirectory}): ${filePath}`; + if (!isWithinRoot(filePath, this.config.getTargetDir())) { + return `File path must be within the root directory (${this.config.getTargetDir()}): ${filePath}`; } if (params.offset !== undefined && params.offset < 0) { return 'Offset must be a non-negative number'; @@ -115,7 +111,10 @@ export class ReadFileTool extends BaseTool { ) { return `Path unavailable`; } - const relativePath = makeRelative(params.absolute_path, this.rootDirectory); + const relativePath = makeRelative( + params.absolute_path, + this.config.getTargetDir(), + ); return shortenPath(relativePath); } @@ -133,7 +132,7 @@ export class ReadFileTool extends BaseTool { const result = await processSingleFileContent( params.absolute_path, - this.rootDirectory, + this.config.getTargetDir(), params.offset, params.limit, ); diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index d2591a8b..3bb824cd 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -59,9 +59,10 @@ describe('ReadManyFilesTool', () => { const mockConfig = { getFileService: () => fileService, getFileFilteringRespectGitIgnore: () => true, + getTargetDir: () => tempRootDir, } as Partial as Config; - tool = new ReadManyFilesTool(tempRootDir, mockConfig); + tool = new ReadManyFilesTool(mockConfig); mockReadFileFn = mockControl.mockReadFile; mockReadFileFn.mockReset(); diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 5557dc43..c43841b5 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -124,17 +124,10 @@ export class ReadManyFilesTool extends BaseTool< ToolResult > { static readonly Name: string = 'read_many_files'; + private readonly geminiIgnorePatterns: string[] = []; - /** - * 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, - ) { + constructor(private config: Config) { const parameterSchema: Schema = { type: Type.OBJECT, 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.`, parameterSchema, ); - this.targetDir = path.resolve(targetDir); this.geminiIgnorePatterns = config .getFileService() .getGeminiIgnorePatterns(); @@ -221,7 +213,7 @@ Use this tool when the user's query implies needing the content of several files getDescription(params: ReadManyFilesParams): string { 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 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 const fileDiscovery = this.config.getFileService(); - const toolBaseDir = this.targetDir; const filesToConsider = new Set(); const skippedFiles: Array<{ path: string; reason: string }> = []; const processedFilesRelativePaths: string[] = []; @@ -293,7 +284,7 @@ Use this tool when the user's query implies needing the content of several files try { const entries = await glob(searchPatterns, { - cwd: toolBaseDir, + cwd: this.config.getTargetDir(), ignore: effectiveExcludes, nodir: 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 ? fileDiscovery .filterFiles( - entries.map((p) => path.relative(toolBaseDir, p)), + entries.map((p) => path.relative(this.config.getTargetDir(), p)), { respectGitIgnore, }, ) - .map((p) => path.resolve(toolBaseDir, p)) + .map((p) => path.resolve(this.config.getTargetDir(), p)) : entries; let gitIgnoredCount = 0; for (const absoluteFilePath of entries) { // Security check: ensure the glob library didn't return something outside targetDir. - if (!absoluteFilePath.startsWith(toolBaseDir)) { + if (!absoluteFilePath.startsWith(this.config.getTargetDir())) { skippedFiles.push({ 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; } @@ -351,7 +342,7 @@ Use this tool when the user's query implies needing the content of several files for (const filePath of sortedFiles) { const relativePathForDisplay = path - .relative(toolBaseDir, filePath) + .relative(this.config.getTargetDir(), filePath) .replace(/\\/g, '/'); 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 const fileReadResult = await processSingleFileContent( filePath, - toolBaseDir, + this.config.getTargetDir(), ); 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) { displayMessage += `Successfully read and concatenated content from **${processedFilesRelativePaths.length} file(s)**.\n`; if (processedFilesRelativePaths.length <= 10) { diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index e936ce0b..a3756c69 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -26,7 +26,7 @@ import { } from '../utils/editCorrector.js'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { ModifiableTool, ModifyContext } from './modifiable-tool.js'; -import { getSpecificMimeType } from '../utils/fileUtils.js'; +import { getSpecificMimeType, isWithinRoot } from '../utils/fileUtils.js'; import { recordFileOperationMetric, 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 { const errors = SchemaValidator.validate(this.schema.parameters, params); if (errors) { @@ -122,7 +103,7 @@ export class WriteFileTool if (!path.isAbsolute(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}`; } diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index a6b2e456..33eda6ab 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -36,8 +36,8 @@ export function isWithinRoot( pathToCheck: string, rootDirectory: string, ): boolean { - const normalizedPathToCheck = path.normalize(pathToCheck); - const normalizedRootDirectory = path.normalize(rootDirectory); + const normalizedPathToCheck = path.resolve(pathToCheck); + const normalizedRootDirectory = path.resolve(rootDirectory); // Ensure the rootDirectory path ends with a separator for correct startsWith comparison, // unless it's the root path itself (e.g., '/' or 'C:\').