Repository navigation
fix(core): improve editor detection on Windows - #21576
dogukanozen wants to merge 6 commits into
Conversation
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 significantly enhances the editor detection mechanism on Windows by introducing dynamic command resolution and explicit checks for common installation directories. It also refines how backspace input is interpreted in Windows Terminal, leading to more accurate and reliable editor identification and input handling, particularly for users whose editors are not in their system's PATH. Highlights
Changelog
Activity
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 effectively improves editor detection on Windows by adding checks for common installation paths and refactoring the configuration to be dynamic. However, it introduces potential command injection vulnerabilities due to insufficient sanitization of environment variables and command strings. It is recommended to sanitize the alias or use safer process execution methods. Additionally, there is significant duplication in test configurations that should be refactored for better maintainability.
| case 'emacs': | ||
| return { win32: ['emacs.exe', 'emacs'], default: ['emacs'] }; | ||
| case 'antigravity': { | ||
| const alias = process.env['ANTIGRAVITY_CLI_ALIAS']; |
There was a problem hiding this comment.
The ANTIGRAVITY_CLI_ALIAS environment variable is used to construct a command string that is passed to execSync without proper sanitization. An attacker who can control this environment variable can execute arbitrary commands. For example, setting ANTIGRAVITY_CLI_ALIAS to agy & calc.exe would cause calc.exe to be executed when the application checks if the editor exists.
To remediate this, sanitize the alias variable to ensure it only contains alphanumeric characters, or use spawnSync with an array of arguments to avoid shell interpretation.
References
- Environment variables used in security-sensitive contexts, such as constructing commands, should be sanitized to prevent malicious input from leading to command injection. This aligns with the principle of sanitizing environment variables to prevent extensions from bypassing security measures, as seen in rules for stdio-based MCP server configurations and HTTP headers.
| if (cmd.includes('\\') || cmd.includes('/')) { | ||
| return existsSync(cmd); | ||
| } |
There was a problem hiding this comment.
The check if (cmd.includes('\\') || cmd.includes('/')) is insufficient to prevent command injection when cmd is passed to execSync. Shell metacharacters like &, ;, |, or backticks can still be used to inject commands.
Consider using spawnSync with an array of arguments instead of execSync with a command string to safely check for command existence.
References
- While the rule states not to sanitize command names for ANSI escape sequences or other C0 control characters, this comment addresses the need to sanitize for shell metacharacters (e.g.,
&,;,|) to prevent command injection. These are distinct concerns, and the current behavior of not sanitizing for shell metacharacters is not desired in this context due to security implications.
| }> = [ | ||
| { editor: 'vscode', commands: ['code'], win32Commands: ['code.cmd'] }, | ||
| { | ||
| editor: 'vscode', | ||
| commands: ['code'], | ||
| win32Commands: [ | ||
| 'code.cmd', | ||
| 'code', | ||
| 'C:\\LOCALAPPDATA\\Programs\\Microsoft VS Code\\bin\\code.cmd', | ||
| 'C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd', | ||
| ], | ||
| }, | ||
| { | ||
| editor: 'vscodium', | ||
| commands: ['codium'], | ||
| win32Commands: ['codium.cmd'], | ||
| win32Commands: [ | ||
| 'codium.cmd', | ||
| 'codium', | ||
| 'C:\\LOCALAPPDATA\\Programs\\VSCodium\\bin\\codium.cmd', | ||
| 'C:\\Program Files\\VSCodium\\bin\\codium.cmd', | ||
| ], | ||
| }, | ||
| { | ||
| editor: 'windsurf', | ||
| commands: ['windsurf'], | ||
| win32Commands: ['windsurf'], | ||
| }, | ||
| { editor: 'cursor', commands: ['cursor'], win32Commands: ['cursor'] }, | ||
| { | ||
| editor: 'cursor', | ||
| commands: ['cursor'], | ||
| win32Commands: [ | ||
| 'cursor', | ||
| 'C:\\LOCALAPPDATA\\Programs\\cursor\\resources\\app\\bin\\cursor.cmd', | ||
| 'C:\\Program Files\\cursor\\resources\\app\\bin\\cursor.cmd', | ||
| ], | ||
| }, | ||
| { editor: 'zed', commands: ['zed', 'zeditor'], win32Commands: ['zed'] }, | ||
| { | ||
| editor: 'antigravity', | ||
| commands: ['agy', 'antigravity'], | ||
| win32Commands: ['agy.cmd', 'antigravity.cmd', 'antigravity'], | ||
| commands: ['gemini', 'agy', 'antigravity'], | ||
| win32Commands: [ | ||
| 'gemini.cmd', | ||
| 'gemini', | ||
| 'agy.cmd', | ||
| 'agy', | ||
| 'antigravity.cmd', | ||
| 'antigravity', | ||
| 'C:\\APPDATA\\npm\\agy.cmd', | ||
| ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
The editor configurations in this array are largely duplicated from the testCases array defined earlier in this file (lines 70-131). This duplication makes the tests harder to maintain, as changes to editor configurations must be updated in multiple places, increasing the risk of inconsistencies. To improve maintainability, I suggest refactoring to define the editor test configurations in a single, shared data structure that can be reused across both the hasValidEditorCommand and getDiffCommand test suites. You could then filter this structure as needed for each suite.
References
- While the rule suggests preferring hardcoded literal values in tests to ensure they are self-contained and less brittle, this comment addresses the duplication of complex test configurations within the same test file. Refactoring such configurations into a shared data structure within the test file improves maintainability without necessarily violating the principle of self-contained tests or introducing brittleness from external dependencies.
2 participants
Summary
Improves editor detection on Windows by resolving common installation paths and refactoring command resolution to be dynamic. This ensures that VS Code and Antigravity (detected as
geminioragy) are correctly identified even when they are not in the systemPATH.Details
editorCommandsto a function-based resolution to ensure environment variables likeLOCALAPPDATAandProgramFilesare resolved at runtime.geminiandgemini.cmdto the detection list for the Antigravity editor, ensuring it's found across different install methods.fs.existsSyncchecks for absolute paths, avoiding unnecessary and potentially slowwhere.execalls.Related Issues
Fixes #21571
How to Validate
Run the updated unit tests to ensure no regressions:
npm test -w @google/gemini-cli-core -- src/utils/editor.test.ts(All 147 tests should pass).
Manual verification:
/editorcommand.Pre-Merge Checklist