-
Notifications
You must be signed in to change notification settings - Fork 10.6k
fix: improve path validation in memory discovery and resolve IDE errors #16055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When a GEMINI.md filename matches a directory (e.g., user has .gemini/gemini.md/ as a folder), the code attempted to read it as a file, causing unnecessary warnings. This change adds an lstat check before readFile to silently skip directories with a debug log message. Fixes #4760
- Remove unused validGeminiMdFile (GEMINI.md.md not discovered) - Update assertions to match actual behavior of loadServerHierarchicalMemory - Add clear comments explaining expected behavior
Replace lstat() pre-check with try/catch for EISDIR error based on code review feedback. This approach: - Handles symlinks correctly (fs.stat follows links, lstat does not) - Avoids TOCTOU race conditions - More efficient (no double syscall) - More idiomatic Node.js pattern (EAFP) Fixes #4760 Ref: #15866 (comment)
On non-Debian/Ubuntu Linux distributions, the Antigravity IDE binary is installed as 'antigravity' instead of 'agy'. This commit adds 'antigravity' as a fallback in the editorCommands mapping to enable proper detection on these distributions. Fixes #15553
Summary of ChangesHello @sontoriyama, 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 the security and reliability of the memory management system by introducing robust path traversal prevention for 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 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces important security enhancements and bug fixes, primarily focused on the MemoryTool. The changes effectively add path traversal validation for memory file names and memory imports, which is a significant improvement. The accompanying tests are thorough and cover critical security scenarios, such as preventing imports from outside the project root. The handling of directories named like memory files also adds to the robustness of the memory discovery process.
I have found one critical security vulnerability that remains in the setGeminiMdFilename function, where the validation is incomplete and still allows for path traversal using . or .. as filenames. My review includes a specific suggestion to address this.
Overall, this is a valuable contribution to the security and stability of the tool. Once the identified issue is resolved, this PR will be in great shape.
| if (trimmed.includes('/') || trimmed.includes('\\')) { | ||
| throw new Error( | ||
| `Invalid GEMINI.md filename: ${trimmed}. Filenames cannot contain path separators.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation for newFilename is a good security measure, but it's incomplete. It currently checks for path separators (/ and \\) but fails to prevent the use of . or .. as a filename. This allows for a path traversal vulnerability. For instance, setting newFilename to '..' would cause functions like getGlobalMemoryFilePath to construct a path that traverses up one directory, enabling file writes outside the intended .gemini directory. To mitigate this, the validation should also explicitly disallow . and .. as filenames.
| if (trimmed.includes('/') || trimmed.includes('\\')) { | |
| throw new Error( | |
| `Invalid GEMINI.md filename: ${trimmed}. Filenames cannot contain path separators.`, | |
| ); | |
| } | |
| if (trimmed.includes('/') || trimmed.includes('\\') || trimmed === '.' || trimmed === '..') { | |
| throw new Error( | |
| `Invalid GEMINI.md filename: ${trimmed}. Filenames cannot contain path separators or be '.' or '..'.`, | |
| ); | |
| } |
b9da117 to
7b0bfd3
Compare
When a GEMINI.md filename matches a directory (e.g., user has .gemini/gemini.md/ as a folder), the code attempted to read it as a file, causing unnecessary warnings. This change adds an lstat check before readFile to silently skip directories with a debug log message. Fixes #4760
…ent path traversal
7b0bfd3 to
7158cef
Compare
|
Fixes #16104 |
…orm compatibility
This PR improves the robustness of the memory discovery logic and resolves systemic IDE diagnostic errors.
Key changes: