fix(core): preserve system PATH in Git environment to fix ENOENT (#25034)#26587
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 addresses an issue where the Gemini CLI would fail to locate the Git binary on systems with custom installations. By correctly merging the existing process environment into the shadow repository's environment configuration, the CLI can now properly resolve system paths while maintaining the necessary isolation for Git configuration. 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: +297 B (0%) Total Size: 34 MB
ℹ️ View Unchanged
|
gemini-cli
Bot
added
priority/p2
|
/gemini review |
![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 updates the GitService to include the current process environment variables in the shadow repository's environment and adds corresponding unit tests. Feedback identifies a security risk where merging the full process.env could compromise repository isolation, suggesting a sanitized approach instead. Additionally, the new tests violate the style guide by using try...finally blocks for environment stubbing rather than the preferred beforeEach and afterEach hooks.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/services/gitService.ts (61)
The inclusion of ...process.env in getShadowRepoEnv allows environment variables to overwrite GIT_DIR and GIT_WORK_TREE, breaking repository isolation. This can lead to the CLI performing git operations on the user's actual repository instead of the internal history repository. Use a sanitized environment for this configuration to prevent extensions from bypassing environment variable controls or overriding security-sensitive settings, limiting it to necessary variables like PATH.
PATH: process.env.PATH,
References
- Use a sanitized environment for configurations to prevent bypassing environment variable redaction or exfiltration.
- Security-sensitive settings should not use a merge strategy that allows less-trusted configuration scopes to completely override more-trusted scopes.
packages/core/src/services/gitService.test.ts (313-328)
This test uses a try...finally block to manage environment variable stubbing and cleanup. This violates the repository style guide (lines 86-88), which specifies that vi.stubEnv should be used in beforeEach and vi.unstubAllEnvs() in afterEach to ensure consistent cleanup and avoid test leakage.
Please refactor the tests to follow this convention (e.g., by using a nested describe block with its own beforeEach for these specific environment tests) and ensure vi.unstubAllEnvs() is added to the top-level afterEach block.
References
- Environment Variables: When testing code that depends on environment variables, use vi.stubEnv('NAME', 'value') in beforeEach and vi.unstubAllEnvs() in afterEach. (link)
|
addressed all |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the GitService to include the system PATH in the environment for Git operations and adds unit tests to verify environment variable preservation and isolation. The review feedback indicates that only preserving PATH is too restrictive and recommends using a sanitized environment to include other essential variables while preventing the exfiltration of sensitive data. Additionally, it was noted that the shadow repository implementation should be checked for potential environment variable collisions.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/services/gitService.ts (61)
The current implementation only preserves the PATH variable, which is overly restrictive and contradicts the PR description. However, to prevent the exfiltration of sensitive system environment variables, the environment should be sanitized rather than fully spread. Consider using a sanitized environment that includes essential variables like SystemRoot (on Windows) or HOME while redacting sensitive ones, consistent with the repository's security practices for external integrations.
[!IMPORTANT]
If you update the environment, ensure that theshadowGitRepositorygetter (line 129) is updated to avoid collisions where a user'sGIT_DIRorGIT_WORK_TREEmight override the shadow repository's settings.
...getSanitizedEnv(process.env),
References
- Sanitize the environment used for variable expansion to prevent malicious extensions from exfiltrating sensitive system environment variables.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements environment variable sanitization for the shadow Git repository to prevent sensitive data leakage and ensure consistent behavior. It also adds a comprehensive suite of tests to verify the preservation of safe variables (like PATH and HOME) and the isolation of Git-specific configurations. A security review highlighted that while process.env is now sanitized, variables like GIT_DIR and GIT_WORK_TREE could still leak into the shadow repository's environment if not explicitly overridden, potentially breaking isolation. A suggestion was provided to explicitly unset these variables in the base environment configuration.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/services/gitService.ts (64-71)
While merging process.env fixes the PATH issue, it also allows other Git-related environment variables like GIT_DIR and GIT_WORK_TREE to leak into the shadow repository's environment. This can break isolation, especially during setupShadowGitRepository where these variables are not explicitly overridden. To ensure robust isolation and prevent potential security bypasses, these variables should be explicitly unset in the base environment configuration, and the environment should be sanitized to prevent sensitive variable leakage.
return {
...sanitizeEnvironment(
process.env,
getSecureSanitizationConfig({ enableEnvironmentVariableRedaction: true }),
),
// Prevent git from using the user's global git config.
GIT_CONFIG_GLOBAL: gitConfigPath,
GIT_CONFIG_SYSTEM: systemConfigPath,
// Ensure user's GIT_DIR/GIT_WORK_TREE don't interfere with shadow repo operations.
GIT_DIR: undefined,
GIT_WORK_TREE: undefined,
References
- Use a sanitized environment for variable expansion to prevent extensions from bypassing environment variable redaction or exfiltrating sensitive system environment variables.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces environment variable sanitization and improved isolation for the GitService. It integrates sanitizeEnvironment to redact sensitive data and ensures that GIT_DIR and GIT_WORK_TREE are explicitly managed to prevent inheritance from the user's environment. The update also reorders environment application in shadowGitRepository to prioritize internal settings. Extensive unit tests were added to validate these security measures. I have no feedback to provide.
…t on Windows" This reverts commit db0b613.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
This PR fixes an issue where the Gemini CLI fails to start with
spawn git ENOENTon systems with custom Git installations (e.g., in~/bin). TheGitServicewas stripping the systemPATHwhen initializing the shadow repository environment.Details
The
getShadowRepoEnvmethod was creating a clean environment object for Git configuration isolation but forgot to includeprocess.env. By mergingprocess.env(and spreading it before our overrides), we restore access to the user'sPATHwhile maintaining the intended isolation of the shadow repo's configuration and identity.Related Issues
Fixes #25034
How to Validate
PATHthat only includes that directory.npm run startand verify the CLI starts withoutENOENT.npm test -w @google/gemini-cli-core -- src/services/gitService.test.tsto verify regression tests.Pre-Merge Checklist