Feature - file delete tool#635
Conversation
Summary of ChangesHello @jutaz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances agent capabilities by introducing a dedicated and highly configurable Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new file_delete power tool, a significant and well-implemented feature. However, a critical security vulnerability has been identified: the primary path traversal protection, specifically within the fileDeleteTool, relies on a flawed validation function isValidProjectFile. This function can be bypassed using prefix-matching paths, potentially allowing an agent to delete or inspect files outside the designated task directory if they share a common path prefix. While the tool also includes configurable pattern-based restrictions, size limits, a dry-run mode, and an extensive test suite, addressing this vulnerability is paramount. Additionally, there is a suggestion for a minor refactoring to improve code maintainability.
| }); | ||
|
|
||
| // Path traversal protection - validate boundary | ||
| if (!isValidProjectFile(absolutePath, task.getTaskDir())) { |
There was a problem hiding this comment.
The fileDeleteTool relies on isValidProjectFile to prevent path traversal, but this check is insufficient. The isValidProjectFile function (defined in src/main/utils/file-system.ts) uses startsWith to verify if the resolved path is within the task directory. This can be bypassed if the target path shares a prefix with the task directory.
For example, if the task directory is /path/to/task, a target path of /path/to/task-secret would be incorrectly validated as safe because /path/to/task-secret starts with /path/to/task. This allows an attacker (or a compromised LLM) to delete files or directories outside the intended scope, or list their contents using the dryRun mode.
To fix this, update isValidProjectFile to use a more robust containment check, such as ensuring the base directory ends with a path separator before comparison, or using path.relative and checking for .. segments:
const relative = path.relative(taskDir, absolutePath);
const isInside = !relative.startsWith('..') && !path.isAbsolute(relative);| if (deleteSettings.allowedDirectoryPatterns && deleteSettings.allowedDirectoryPatterns.length > 0) { | ||
| const matchesAllowed = deleteSettings.allowedDirectoryPatterns.some((pattern) => minimatch(relativePath, pattern, { dot: true })); | ||
| if (!matchesAllowed) { | ||
| return `Error: Directory '${relativePath}' does not match any allowed directory patterns.`; | ||
| } | ||
| } | ||
|
|
||
| if (deleteSettings.deniedDirectoryPatterns && deleteSettings.deniedDirectoryPatterns.length > 0) { | ||
| const matchesDenied = deleteSettings.deniedDirectoryPatterns.some((pattern) => minimatch(relativePath, pattern, { dot: true })); | ||
| if (matchesDenied) { | ||
| return `Error: Directory '${relativePath}' matches a protected directory pattern.`; | ||
| } |
There was a problem hiding this comment.
The pattern checking logic for allowed and denied patterns is duplicated for both directories (here) and files (lines 1050-1061). This could be extracted into a reusable helper function to improve maintainability and reduce code duplication.
A helper function could look like this:
const checkPatterns = (
relativePath: string,
itemType: 'File' | 'Directory',
allowedPatterns?: string[],
deniedPatterns?: string[],
): string | null => {
if (allowedPatterns && allowedPatterns.length > 0) {
const matchesAllowed = allowedPatterns.some((pattern) => minimatch(relativePath, pattern, { dot: true }));
if (!matchesAllowed) {
return `Error: ${itemType} '${relativePath}' does not match any allowed ${itemType.toLowerCase()} patterns.`;
}
}
if (deniedPatterns && deniedPatterns.length > 0) {
const matchesDenied = deniedPatterns.some((pattern) => minimatch(relativePath, pattern, { dot: true }));
if (matchesDenied) {
return `Error: ${itemType} '${relativePath}' matches a protected ${itemType.toLowerCase()} pattern.`;
}
}
return null;
};You could then use it for both directories and files, simplifying the logic in the execute function.
1 participant
Adds a file delete tool with various configurable safeguards. This is likely better than trying to configure and safeguard
rmtool which gets very complex very quickly.TODO: