fix: Prevent WriteFileTool from writing to directory paths
- Enhances WriteFileTool validation to check if the target file_path is an existing directory. - If it is, the tool now returns a validation error "Path is a directory, not a file: <filePath>", preventing the attempt to write. - This proactive check avoids underlying file system errors that would occur if fs.writeFileSync were called on a directory path, which could lead to console errors. - Test cases have been updated to reflect this stricter validation. Fixes https://b.corp.google.com/issues/418348176
This commit is contained in:
parent
5dcdbe64ab
commit
feb9dee4b1
|
@ -101,14 +101,15 @@ describe('WriteFileTool', () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should return null for path that is the root itself', () => {
|
it('should return error for path that is the root itself', () => {
|
||||||
const params = {
|
const params = {
|
||||||
file_path: rootDir, // Attempting to write to the root directory itself (as a file)
|
file_path: rootDir, // Attempting to write to the root directory itself
|
||||||
content: 'hello',
|
content: 'hello',
|
||||||
};
|
};
|
||||||
// This is a tricky case. The validation should allow it if it's treated as a file path.
|
// With the new validation, this should now return an error as rootDir is a directory.
|
||||||
// The actual write operation might fail if it's a directory, but validation should pass.
|
expect(tool.validateToolParams(params)).toMatch(
|
||||||
expect(tool.validateToolParams(params)).toBeNull();
|
`Path is a directory, not a file: ${rootDir}`,
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should return error for path that is just / and root is not /', () => {
|
it('should return error for path that is just / and root is not /', () => {
|
||||||
|
|
|
@ -86,12 +86,29 @@ export class WriteFileTool extends BaseTool<WriteFileToolParams, ToolResult> {
|
||||||
) {
|
) {
|
||||||
return 'Parameters failed schema validation.';
|
return 'Parameters failed schema validation.';
|
||||||
}
|
}
|
||||||
if (!path.isAbsolute(params.file_path)) {
|
const filePath = params.file_path;
|
||||||
return `File path must be absolute: ${params.file_path}`;
|
if (!path.isAbsolute(filePath)) {
|
||||||
|
return `File path must be absolute: ${filePath}`;
|
||||||
}
|
}
|
||||||
if (!this.isWithinRoot(params.file_path)) {
|
if (!this.isWithinRoot(filePath)) {
|
||||||
return `File path must be within the root directory (${this.config.getTargetDir()}): ${params.file_path}`;
|
return `File path must be within the root directory (${this.config.getTargetDir()}): ${filePath}`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
// This check should be performed only if the path exists.
|
||||||
|
// If it doesn't exist, it's a new file, which is valid for writing.
|
||||||
|
if (fs.existsSync(filePath)) {
|
||||||
|
const stats = fs.lstatSync(filePath);
|
||||||
|
if (stats.isDirectory()) {
|
||||||
|
return `Path is a directory, not a file: ${filePath}`;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
} catch (statError: unknown) {
|
||||||
|
// If fs.existsSync is true but lstatSync fails (e.g., permissions, race condition where file is deleted)
|
||||||
|
// this indicates an issue with accessing the path that should be reported.
|
||||||
|
return `Error accessing path properties for validation: ${filePath}. Reason: ${statError instanceof Error ? statError.message : String(statError)}`;
|
||||||
|
}
|
||||||
|
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue