Skip to content

Commit 3c135af

Browse files
scidominogalz10
scidomino
authored and
galz10
committed
Pure refactor: Consolidate isWithinRoot() function calling. (#4163)
1 parent 071f8de commit 3c135af

File tree

13 files changed

+96
-179
lines changed

13 files changed

+96
-179
lines changed

‎packages/core/src/config/config.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,6 @@ export class Config {
525525

526526
async createToolRegistry(): Promise<ToolRegistry> {
527527
const registry = new ToolRegistry(this);
528-
const targetDir = this.getTargetDir();
529528

530529
// helper to create & register core tools that are enabled
531530
// eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -560,14 +559,14 @@ export class Config {
560559
}
561560
};
562561

563-
registerCoreTool(LSTool, targetDir, this);
564-
registerCoreTool(ReadFileTool, targetDir, this);
565-
registerCoreTool(GrepTool, targetDir);
566-
registerCoreTool(GlobTool, targetDir, this);
562+
registerCoreTool(LSTool, this);
563+
registerCoreTool(ReadFileTool, this);
564+
registerCoreTool(GrepTool, this);
565+
registerCoreTool(GlobTool, this);
567566
registerCoreTool(EditTool, this);
568567
registerCoreTool(WriteFileTool, this);
569568
registerCoreTool(WebFetchTool, this);
570-
registerCoreTool(ReadManyFilesTool, targetDir, this);
569+
registerCoreTool(ReadManyFilesTool, this);
571570
registerCoreTool(ShellTool, this);
572571
registerCoreTool(MemoryTool);
573572
registerCoreTool(WebSearchTool, this);

‎packages/core/src/tools/edit.ts

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { ensureCorrectEdit } from '../utils/editCorrector.js';
2424
import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js';
2525
import { ReadFileTool } from './read-file.js';
2626
import { ModifiableTool, ModifyContext } from './modifiable-tool.js';
27+
import { isWithinRoot } from '../utils/fileUtils.js';
2728

2829
/**
2930
* Parameters for the Edit tool
@@ -72,12 +73,7 @@ export class EditTool
7273
implements ModifiableTool<EditToolParams>
7374
{
7475
static readonly Name = 'replace';
75-
private readonly rootDirectory: string;
7676

77-
/**
78-
* Creates a new instance of the EditLogic
79-
* @param rootDirectory Root directory to ground this tool in.
80-
*/
8177
constructor(private readonly config: Config) {
8278
super(
8379
EditTool.Name,
@@ -121,24 +117,6 @@ Expectation for required parameters:
121117
type: Type.OBJECT,
122118
},
123119
);
124-
this.rootDirectory = path.resolve(this.config.getTargetDir());
125-
}
126-
127-
/**
128-
* Checks if a path is within the root directory.
129-
* @param pathToCheck The absolute path to check.
130-
* @returns True if the path is within the root directory, false otherwise.
131-
*/
132-
private isWithinRoot(pathToCheck: string): boolean {
133-
const normalizedPath = path.normalize(pathToCheck);
134-
const normalizedRoot = this.rootDirectory;
135-
const rootWithSep = normalizedRoot.endsWith(path.sep)
136-
? normalizedRoot
137-
: normalizedRoot + path.sep;
138-
return (
139-
normalizedPath === normalizedRoot ||
140-
normalizedPath.startsWith(rootWithSep)
141-
);
142120
}
143121

144122
/**
@@ -156,8 +134,8 @@ Expectation for required parameters:
156134
return `File path must be absolute: ${params.file_path}`;
157135
}
158136

159-
if (!this.isWithinRoot(params.file_path)) {
160-
return `File path must be within the root directory (${this.rootDirectory}): ${params.file_path}`;
137+
if (!isWithinRoot(params.file_path, this.config.getTargetDir())) {
138+
return `File path must be within the root directory (${this.config.getTargetDir()}): ${params.file_path}`;
161139
}
162140

163141
return null;
@@ -325,7 +303,7 @@ Expectation for required parameters:
325303
);
326304
const confirmationDetails: ToolEditConfirmationDetails = {
327305
type: 'edit',
328-
title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`,
306+
title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`,
329307
fileName,
330308
fileDiff,
331309
onConfirm: async (outcome: ToolConfirmationOutcome) => {
@@ -341,7 +319,10 @@ Expectation for required parameters:
341319
if (!params.file_path || !params.old_string || !params.new_string) {
342320
return `Model did not provide valid parameters for edit tool`;
343321
}
344-
const relativePath = makeRelative(params.file_path, this.rootDirectory);
322+
const relativePath = makeRelative(
323+
params.file_path,
324+
this.config.getTargetDir(),
325+
);
345326
if (params.old_string === '') {
346327
return `Create ${shortenPath(relativePath)}`;
347328
}
@@ -400,7 +381,7 @@ Expectation for required parameters:
400381

401382
let displayResult: ToolResultDisplay;
402383
if (editData.isNewFile) {
403-
displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`;
384+
displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`;
404385
} else {
405386
// Generate diff for display, even though core logic doesn't technically need it
406387
// The CLI wrapper will use this part of the ToolResult

‎packages/core/src/tools/glob.test.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,13 @@ describe('GlobTool', () => {
2222
const mockConfig = {
2323
getFileService: () => new FileDiscoveryService(tempRootDir),
2424
getFileFilteringRespectGitIgnore: () => true,
25-
} as Partial<Config> as Config;
25+
getTargetDir: () => tempRootDir,
26+
} as unknown as Config;
2627

2728
beforeEach(async () => {
2829
// Create a unique root directory for each test run
2930
tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-'));
30-
globTool = new GlobTool(tempRootDir, mockConfig);
31+
globTool = new GlobTool(mockConfig);
3132

3233
// Create some test files and directories within this root
3334
// Top-level files
@@ -224,8 +225,8 @@ describe('GlobTool', () => {
224225

225226
it("should return error if search path resolves outside the tool's root directory", () => {
226227
// Create a globTool instance specifically for this test, with a deeper root
227-
const deeperRootDir = path.join(tempRootDir, 'sub');
228-
const specificGlobTool = new GlobTool(deeperRootDir, mockConfig);
228+
tempRootDir = path.join(tempRootDir, 'sub');
229+
const specificGlobTool = new GlobTool(mockConfig);
229230
// const params: GlobToolParams = { pattern: '*.txt', path: '..' }; // This line is unused and will be removed.
230231
// This should be fine as tempRootDir is still within the original tempRootDir (the parent of deeperRootDir)
231232
// Let's try to go further up.

‎packages/core/src/tools/glob.ts

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js';
1111
import { BaseTool, ToolResult } from './tools.js';
1212
import { Type } from '@google/genai';
1313
import { shortenPath, makeRelative } from '../utils/paths.js';
14+
import { isWithinRoot } from '../utils/fileUtils.js';
1415
import { Config } from '../config/config.js';
1516

1617
// Subset of 'Path' interface provided by 'glob' that we can implement for testing
@@ -79,14 +80,8 @@ export interface GlobToolParams {
7980
*/
8081
export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
8182
static readonly Name = 'glob';
82-
/**
83-
* Creates a new instance of the GlobLogic
84-
* @param rootDirectory Root directory to ground this tool in.
85-
*/
86-
constructor(
87-
private rootDirectory: string,
88-
private config: Config,
89-
) {
83+
84+
constructor(private config: Config) {
9085
super(
9186
GlobTool.Name,
9287
'FindFiles',
@@ -118,28 +113,6 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
118113
type: Type.OBJECT,
119114
},
120115
);
121-
122-
this.rootDirectory = path.resolve(rootDirectory);
123-
}
124-
125-
/**
126-
* Checks if a given path is within the root directory bounds.
127-
* This security check prevents accessing files outside the designated root directory.
128-
*
129-
* @param pathToCheck The absolute path to validate
130-
* @returns True if the path is within the root directory, false otherwise
131-
*/
132-
private isWithinRoot(pathToCheck: string): boolean {
133-
const absolutePathToCheck = path.resolve(pathToCheck);
134-
const normalizedPath = path.normalize(absolutePathToCheck);
135-
const normalizedRoot = path.normalize(this.rootDirectory);
136-
const rootWithSep = normalizedRoot.endsWith(path.sep)
137-
? normalizedRoot
138-
: normalizedRoot + path.sep;
139-
return (
140-
normalizedPath === normalizedRoot ||
141-
normalizedPath.startsWith(rootWithSep)
142-
);
143116
}
144117

145118
/**
@@ -152,15 +125,15 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
152125
}
153126

154127
const searchDirAbsolute = path.resolve(
155-
this.rootDirectory,
128+
this.config.getTargetDir(),
156129
params.path || '.',
157130
);
158131

159-
if (!this.isWithinRoot(searchDirAbsolute)) {
160-
return `Search path ("${searchDirAbsolute}") resolves outside the tool's root directory ("${this.rootDirectory}").`;
132+
if (!isWithinRoot(searchDirAbsolute, this.config.getTargetDir())) {
133+
return `Search path ("${searchDirAbsolute}") resolves outside the tool's root directory ("${this.config.getTargetDir()}").`;
161134
}
162135

163-
const targetDir = searchDirAbsolute || this.rootDirectory;
136+
const targetDir = searchDirAbsolute || this.config.getTargetDir();
164137
try {
165138
if (!fs.existsSync(targetDir)) {
166139
return `Search path does not exist ${targetDir}`;
@@ -189,8 +162,11 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
189162
getDescription(params: GlobToolParams): string {
190163
let description = `'${params.pattern}'`;
191164
if (params.path) {
192-
const searchDir = path.resolve(this.rootDirectory, params.path || '.');
193-
const relativePath = makeRelative(searchDir, this.rootDirectory);
165+
const searchDir = path.resolve(
166+
this.config.getTargetDir(),
167+
params.path || '.',
168+
);
169+
const relativePath = makeRelative(searchDir, this.config.getTargetDir());
194170
description += ` within ${shortenPath(relativePath)}`;
195171
}
196172
return description;
@@ -213,7 +189,7 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
213189

214190
try {
215191
const searchDirAbsolute = path.resolve(
216-
this.rootDirectory,
192+
this.config.getTargetDir(),
217193
params.path || '.',
218194
);
219195

@@ -241,13 +217,15 @@ export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
241217

242218
if (respectGitIgnore) {
243219
const relativePaths = entries.map((p) =>
244-
path.relative(this.rootDirectory, p.fullpath()),
220+
path.relative(this.config.getTargetDir(), p.fullpath()),
245221
);
246222
const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, {
247223
respectGitIgnore,
248224
});
249225
const filteredAbsolutePaths = new Set(
250-
filteredRelativePaths.map((p) => path.resolve(this.rootDirectory, p)),
226+
filteredRelativePaths.map((p) =>
227+
path.resolve(this.config.getTargetDir(), p),
228+
),
251229
);
252230

253231
filteredEntries = entries.filter((entry) =>

‎packages/core/src/tools/grep.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { GrepTool, GrepToolParams } from './grep.js';
99
import path from 'path';
1010
import fs from 'fs/promises';
1111
import os from 'os';
12+
import { Config } from '../config/config.js';
1213

1314
// Mock the child_process module to control grep/git grep behavior
1415
vi.mock('child_process', () => ({
@@ -30,9 +31,13 @@ describe('GrepTool', () => {
3031
let grepTool: GrepTool;
3132
const abortSignal = new AbortController().signal;
3233

34+
const mockConfig = {
35+
getTargetDir: () => tempRootDir,
36+
} as unknown as Config;
37+
3338
beforeEach(async () => {
3439
tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-'));
35-
grepTool = new GrepTool(tempRootDir);
40+
grepTool = new GrepTool(mockConfig);
3641

3742
// Create some test files and directories
3843
await fs.writeFile(

‎packages/core/src/tools/grep.ts

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js';
1616
import { makeRelative, shortenPath } from '../utils/paths.js';
1717
import { getErrorMessage, isNodeError } from '../utils/errors.js';
1818
import { isGitRepository } from '../utils/gitUtils.js';
19+
import { Config } from '../config/config.js';
1920

2021
// --- Interfaces ---
2122

@@ -56,11 +57,7 @@ interface GrepMatch {
5657
export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
5758
static readonly Name = 'search_file_content'; // Keep static name
5859

59-
/**
60-
* Creates a new instance of the GrepLogic
61-
* @param rootDirectory Root directory to ground this tool in. All operations will be restricted to this directory.
62-
*/
63-
constructor(private rootDirectory: string) {
60+
constructor(private readonly config: Config) {
6461
super(
6562
GrepTool.Name,
6663
'SearchText',
@@ -87,8 +84,6 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
8784
type: Type.OBJECT,
8885
},
8986
);
90-
// Ensure rootDirectory is absolute and normalized
91-
this.rootDirectory = path.resolve(rootDirectory);
9287
}
9388

9489
// --- Validation Methods ---
@@ -100,15 +95,18 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
10095
* @throws {Error} If path is outside root, doesn't exist, or isn't a directory.
10196
*/
10297
private resolveAndValidatePath(relativePath?: string): string {
103-
const targetPath = path.resolve(this.rootDirectory, relativePath || '.');
98+
const targetPath = path.resolve(
99+
this.config.getTargetDir(),
100+
relativePath || '.',
101+
);
104102

105103
// Security Check: Ensure the resolved path is still within the root directory.
106104
if (
107-
!targetPath.startsWith(this.rootDirectory) &&
108-
targetPath !== this.rootDirectory
105+
!targetPath.startsWith(this.config.getTargetDir()) &&
106+
targetPath !== this.config.getTargetDir()
109107
) {
110108
throw new Error(
111-
`Path validation failed: Attempted path "${relativePath || '.'}" resolves outside the allowed root directory "${this.rootDirectory}".`,
109+
`Path validation failed: Attempted path "${relativePath || '.'}" resolves outside the allowed root directory "${this.config.getTargetDir()}".`,
112110
);
113111
}
114112

@@ -322,11 +320,17 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
322320
description += ` in ${params.include}`;
323321
}
324322
if (params.path) {
325-
const resolvedPath = path.resolve(this.rootDirectory, params.path);
326-
if (resolvedPath === this.rootDirectory || params.path === '.') {
323+
const resolvedPath = path.resolve(
324+
this.config.getTargetDir(),
325+
params.path,
326+
);
327+
if (resolvedPath === this.config.getTargetDir() || params.path === '.') {
327328
description += ` within ./`;
328329
} else {
329-
const relativePath = makeRelative(resolvedPath, this.rootDirectory);
330+
const relativePath = makeRelative(
331+
resolvedPath,
332+
this.config.getTargetDir(),
333+
);
330334
description += ` within ${shortenPath(relativePath)}`;
331335
}
332336
}

0 commit comments

Comments
 (0)