-
Notifications
You must be signed in to change notification settings - Fork 14.5k
fix(core): strip execution-affecting GIT_* env vars in getSafeGitEnv #29008
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
69c3a35
22edb4e
3369b26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Google LLC | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { getSafeGitEnv } from './gitUtils.js'; | ||
|
|
||
| describe('getSafeGitEnv', () => { | ||
| it('strips execution-affecting GIT_* variables from the base environment', () => { | ||
| const maliciousEnv: Record<string, string | undefined> = { | ||
| PATH: '/usr/bin', | ||
| GIT_EXEC_PATH: '/tmp/evil', | ||
| GIT_SSH_COMMAND: 'calc.exe', | ||
| GIT_PROXY_COMMAND: 'cmd /c calc.exe', | ||
| GIT_SSH_VARIANT: 'ssh', | ||
| GIT_ALTERNATE_OBJECT_DIRECTORIES: '/tmp/evil-objects', | ||
| GIT_TEMPLATE_DIR: '/tmp/evil-template', | ||
| GIT_REPLACE_REF_BASE: 'refs/evil/', | ||
| GIT_CEILING_DIRECTORIES: '/tmp', | ||
| }; | ||
|
|
||
| const safeEnv = getSafeGitEnv(maliciousEnv); | ||
|
|
||
| expect(safeEnv['GIT_EXEC_PATH']).toBeUndefined(); | ||
| expect(safeEnv['GIT_SSH_COMMAND']).toBeUndefined(); | ||
| expect(safeEnv['GIT_PROXY_COMMAND']).toBeUndefined(); | ||
| expect(safeEnv['GIT_SSH_VARIANT']).toBeUndefined(); | ||
| expect(safeEnv['GIT_ALTERNATE_OBJECT_DIRECTORIES']).toBeUndefined(); | ||
| expect(safeEnv['GIT_TEMPLATE_DIR']).toBeUndefined(); | ||
| expect(safeEnv['GIT_REPLACE_REF_BASE']).toBeUndefined(); | ||
| expect(safeEnv['GIT_CEILING_DIRECTORIES']).toBeUndefined(); | ||
| expect(safeEnv['PATH']).toBe('/usr/bin'); | ||
| }); | ||
|
|
||
| it('still strips GIT_CONFIG_* variables from the base environment', () => { | ||
| const env: Record<string, string | undefined> = { | ||
| GIT_CONFIG_COUNT: '1', | ||
| GIT_CONFIG_KEY_0: 'core.pager', | ||
| GIT_CONFIG_VALUE_0: 'evil', | ||
| GIT_CONFIG_PARAMETERS: 'evil', | ||
| }; | ||
|
|
||
| const safeEnv = getSafeGitEnv(env); | ||
|
|
||
| expect(safeEnv['GIT_CONFIG_KEY_0']).not.toBe('core.pager'); | ||
| expect(safeEnv['GIT_CONFIG_PARAMETERS']).toBeUndefined(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,15 +8,35 @@ import * as fs from 'node:fs'; | |||||||||||||||||||||||||||||||||||||||||||
| import * as path from 'node:path'; | ||||||||||||||||||||||||||||||||||||||||||||
| import { spawnAsync } from './shell-utils.js'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // GIT_* variables that control which binaries/helpers git executes. These | ||||||||||||||||||||||||||||||||||||||||||||
| // must never be inherited from a project's .env file (or elsewhere), or a | ||||||||||||||||||||||||||||||||||||||||||||
| // trusted-but-malicious repository could use them to run arbitrary commands | ||||||||||||||||||||||||||||||||||||||||||||
| // via otherwise ordinary, non-model git invocations. | ||||||||||||||||||||||||||||||||||||||||||||
| export const EXECUTION_AFFECTING_GIT_ENV_VARS = new Set([ | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_EXEC_PATH', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_PROXY_COMMAND', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_SSH_COMMAND', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_SSH_VARIANT', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_ALTERNATE_OBJECT_DIRECTORIES', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_TEMPLATE_DIR', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_REPLACE_REF_BASE', | ||||||||||||||||||||||||||||||||||||||||||||
| 'GIT_CEILING_DIRECTORIES', | ||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The In To remediate this, add
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| export function getSafeGitEnv( | ||||||||||||||||||||||||||||||||||||||||||||
| baseEnv: Record<string, string | undefined> = process.env, | ||||||||||||||||||||||||||||||||||||||||||||
| ): Record<string, string | undefined> { | ||||||||||||||||||||||||||||||||||||||||||||
| const devNullPath = process.platform === 'win32' ? 'NUL' : '/dev/null'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Strip pre-existing GIT_CONFIG_* and GIT_CONFIG_PARAMETERS variables to prevent environment pollution | ||||||||||||||||||||||||||||||||||||||||||||
| // Strip pre-existing GIT_CONFIG_*, GIT_CONFIG_PARAMETERS, and | ||||||||||||||||||||||||||||||||||||||||||||
| // execution-affecting GIT_* variables to prevent environment pollution. | ||||||||||||||||||||||||||||||||||||||||||||
| const cleanedEnv: Record<string, string | undefined> = {}; | ||||||||||||||||||||||||||||||||||||||||||||
| for (const [key, value] of Object.entries(baseEnv)) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (!key.startsWith('GIT_CONFIG_') && key !== 'GIT_CONFIG_PARAMETERS') { | ||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||
| !key.startsWith('GIT_CONFIG_') && | ||||||||||||||||||||||||||||||||||||||||||||
| key !== 'GIT_CONFIG_PARAMETERS' && | ||||||||||||||||||||||||||||||||||||||||||||
| !EXECUTION_AFFECTING_GIT_ENV_VARS.has(key) | ||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||
| cleanedEnv[key] = value; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
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.
According to the repository's testing conventions, to "unset" an environment variable when using
vi.stubEnv, you should use an empty string''instead ofundefinedto avoid potential test leakage or unexpected behavior in Vitest.References