From 49456e4e1534c0ab38249547a14e8657084175b6 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 7 May 2026 14:24:49 -0400 Subject: [PATCH] fix(core): preserve system PATH in Git environment to fix ENOENT (#25034) (#26587) --- packages/core/src/services/gitService.test.ts | 140 ++++++++++++++++++ packages/core/src/services/gitService.ts | 15 +- 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index f5213ac6ea..cc58d39893 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -304,6 +304,146 @@ describe('GitService', () => { ); expect(systemConfigContent).toBe(''); }); + + describe('environment variable preservation', () => { + const customPath = '/custom/bin'; + const safeHome = '/home/user'; + const sensitiveKey = 'sk-123456789'; + + beforeEach(() => { + vi.stubEnv('PATH', customPath); + vi.stubEnv('HOME', safeHome); + vi.stubEnv('API_KEY', sensitiveKey); + vi.stubEnv('UNRELATED_VAR', 'some-value'); + // Explicitly unset strict mode triggers to ensure predictable test behavior + // across local and CI environments. + vi.stubEnv('GITHUB_SHA', ''); + vi.stubEnv('SURFACE', ''); + hoistedMockCheckIsRepo.mockResolvedValue(false); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should preserve system PATH in the Git environment', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + PATH: customPath, + GIT_CONFIG_GLOBAL: expect.any(String), + GIT_AUTHOR_NAME: SHADOW_REPO_AUTHOR_NAME, + }), + ); + }); + + it('should preserve safe environment variables like HOME', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + HOME: safeHome, + }), + ); + }); + + it('should NOT include sensitive environment variables like API_KEY', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.API_KEY).toBeUndefined(); + }); + + it('should preserve unrelated environment variables in non-strict mode', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.UNRELATED_VAR).toBe('some-value'); + }); + + it('should explicitly unset GIT_DIR and GIT_WORK_TREE to maintain isolation', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_DIR).toBeUndefined(); + expect(callArgs.GIT_WORK_TREE).toBeUndefined(); + }); + }); + + describe('GIT_CONFIG isolation', () => { + beforeEach(() => { + vi.stubEnv('GIT_CONFIG_GLOBAL', '/user/global/config'); + vi.stubEnv('GIT_CONFIG_SYSTEM', '/user/system/config'); + hoistedMockCheckIsRepo.mockResolvedValue(false); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should override GIT_CONFIG environment variables from process.env', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const expectedConfigPath = path.join(repoDir, '.gitconfig'); + const expectedSystemPath = path.join( + repoDir, + '.gitconfig_system_empty', + ); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + GIT_CONFIG_GLOBAL: expectedConfigPath, + GIT_CONFIG_SYSTEM: expectedSystemPath, + }), + ); + + // Ensure it's not using the values from stubbed process.env + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_CONFIG_GLOBAL).not.toBe('/user/global/config'); + expect(callArgs.GIT_CONFIG_SYSTEM).not.toBe('/user/system/config'); + }); + }); + + describe('shadowGitRepository prioritization', () => { + beforeEach(() => { + vi.stubEnv('GIT_DIR', '/user/fake/.git'); + vi.stubEnv('GIT_WORK_TREE', '/user/fake/worktree'); + hoistedMockCheckIsRepo.mockResolvedValue(true); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should prioritize internal GIT_DIR and GIT_WORK_TREE over process.env', async () => { + const service = new GitService(projectRoot, storage); + // Trigger a call to shadowGitRepository (e.g., via getCurrentCommitHash) + hoistedMockRaw.mockResolvedValue('hash'); + await service.getCurrentCommitHash(); + + const expectedRepoDir = storage.getHistoryDir(); + const expectedGitDir = path.join(expectedRepoDir, '.git'); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + GIT_DIR: expectedGitDir, + GIT_WORK_TREE: projectRoot, + }), + ); + + // Ensure user env was overridden + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_DIR).not.toBe('/user/fake/.git'); + expect(callArgs.GIT_WORK_TREE).not.toBe('/user/fake/worktree'); + }); + }); }); describe('createFileSnapshot', () => { diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index f923dc6164..c32a06130c 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -11,6 +11,10 @@ import { spawnAsync } from '../utils/shell-utils.js'; import { simpleGit, CheckRepoActions, type SimpleGit } from 'simple-git'; import type { Storage } from '../config/storage.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { + sanitizeEnvironment, + getSecureSanitizationConfig, +} from './environmentSanitization.js'; export const SHADOW_REPO_AUTHOR_NAME = 'Gemini CLI'; export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com'; @@ -58,9 +62,18 @@ export class GitService { const gitConfigPath = path.join(repoDir, '.gitconfig'); const systemConfigPath = path.join(repoDir, '.gitconfig_system_empty'); 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 we don't inherit isolation-breaking variables from the user environment. + GIT_DIR: undefined, + GIT_WORK_TREE: undefined, // Explicitly provide identity to prevent "Author identity unknown" errors // inside sandboxed environments like Docker where the gitconfig might not // be picked up properly. @@ -126,9 +139,9 @@ export class GitService { private get shadowGitRepository(): SimpleGit { const repoDir = this.getHistoryDir(); return simpleGit(this.projectRoot).env({ + ...this.getShadowRepoEnv(repoDir), GIT_DIR: path.join(repoDir, '.git'), GIT_WORK_TREE: this.projectRoot, - ...this.getShadowRepoEnv(repoDir), }); }