fix(core): prevent path traversal in custome command file injection#27234
Conversation
Add a workspace boundary check for relative paths in readPathFromWorkspace to prevent unauthorized reading of files outside the workspace. Fixes #1489
…xpansion Re-validate every file found during directory expansion to prevent traversal via symlinks or other methods during recursion. Related to #1489
Summary of ChangesHello, 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 addresses a security vulnerability related to path traversal in the custom command processor. By enforcing workspace boundary checks during path resolution and file discovery, the changes prevent unauthorized access to local files outside the intended workspace, significantly hardening the file injection mechanism. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
Size Change: +375 B (0%) Total Size: 33.8 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request enhances security in readPathFromWorkspace by implementing checks to prevent path traversal outside the workspace, supported by a new test case for relative paths. While the security improvements are valuable, the reviewer pointed out a significant performance risk in the "defense in depth" check within the file processing loop. Specifically, calling isPathWithinWorkspace for every file entry triggers repeated I/O-bound realpath resolutions, which could degrade performance in large directories; optimizing this by checking for symbolic links or using more efficient validation is recommended.
| if (!workspace.isPathWithinWorkspace(filePath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
While this check is important for security (defense in depth), calling isPathWithinWorkspace for every file in a directory expansion can lead to significant performance degradation. isPathWithinWorkspace performs a realpath resolution which involves I/O. For directories with a large number of files, this will result in thousands of synchronous-like I/O operations. Consider optimizing this by checking if the file is a symbolic link first, or by using a more efficient path validation that doesn't require full resolution for every entry if the parent directory has already been validated and the entry is not a symlink.
…aversal Ensure that symlink escapes and traversal through nested directories are explicitly verified within the pathReader test suite. Related to #1489
Provide transparency when files are skipped during directory expansion by adding a placeholder message indicating the path traversal was blocked. Related to #1489
Verify that multiple out-of-workspace files discovered during directory expansion each generate a corresponding transparency message. Related to #1489
260d431 to
b92af70
Compare
Summary
Fixes a path traversal vulnerability in the custom command processor. It ensures that file injection via the @{...} syntax is strictly confined to the defined workspace boundaries, preventing unauthorized access to other local files.
Details
Related Issues
N/A
How to Validate
npm test -w @google/gemini-cli-core -- src/utils/pathReader.test.ts
Pre-Merge Checklist