From 93c5aefd9efc6b030781e62a536de43e3a6a99a5 Mon Sep 17 00:00:00 2001 From: gnanam1990 Date: Tue, 28 Apr 2026 11:07:07 +0530 Subject: [PATCH] fix(plugins): sanitize env before spawning git so /plugin marketplace add works (#751) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git 2.30+ refuses to start when any environment value contains a NUL, CR, or LF character ("Unsafe environment: control characters are not allowed in values"). User shells frequently leak such values — a copy-pasted API key with a trailing newline, a terminal-set variable with embedded escape sequences — which made every /plugin marketplace add and /plugin install fail with that error before git even ran. Add a small shared helper that builds the env passed to git child processes and drops keys whose name or value contains a control character. The legacy GIT_NO_PROMPT_ENV overrides (terminal prompt disabled, askpass cleared) move into the same helper. Apply it to every git invocation in marketplaceManager.ts (5 sites: gitPull, gitClone, sparse-checkout, post-sparse checkout, reconcileSparseCheckout) and pluginLoader.ts (8 sites: clone, fetch, checkout in both gitClone and installFromGitSubdir). A debug-level warning is logged once per process listing the dropped key NAMES (not values) so the user can clean them up in their shell. - src/utils/plugins/gitEnv.ts (new): sanitizeEnvForGit + buildGitChildEnv - src/utils/plugins/gitEnv.test.ts (new): 10 unit tests covering CR/LF/NUL in values, control char in key name, undefined values, defaults, extras override - src/utils/plugins/marketplaceManager.ts: replace 5 inline env spreads with buildGitChildEnv() - src/utils/plugins/pluginLoader.ts: pass env: buildGitChildEnv() to 8 git exec sites that previously inherited process.env unfiltered Verified locally on Linux: before fix, git --version with a leaked control-char env value fails with "Unsafe environment"; after fix it runs cleanly. Live marketplaceManager.gitClone against a real GitHub repo with the same leaked env succeeds and the repo is materialized on disk. --- src/utils/plugins/gitEnv.test.ts | 104 ++++++++++++++++++++++++ src/utils/plugins/gitEnv.ts | 70 ++++++++++++++++ src/utils/plugins/marketplaceManager.ts | 16 ++-- src/utils/plugins/pluginLoader.ts | 25 ++++-- 4 files changed, 196 insertions(+), 19 deletions(-) create mode 100644 src/utils/plugins/gitEnv.test.ts create mode 100644 src/utils/plugins/gitEnv.ts diff --git a/src/utils/plugins/gitEnv.test.ts b/src/utils/plugins/gitEnv.test.ts new file mode 100644 index 00000000..5b389c6d --- /dev/null +++ b/src/utils/plugins/gitEnv.test.ts @@ -0,0 +1,104 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' + +import { + __resetGitEnvWarningForTesting, + buildGitChildEnv, + sanitizeEnvForGit, +} from './gitEnv.js' + +describe('sanitizeEnvForGit', () => { + test('drops values containing LF', () => { + const result = sanitizeEnvForGit({ + GOOD: 'value', + BAD_NEWLINE: 'line1\nline2', + }) + expect(result.env).toEqual({ GOOD: 'value' }) + expect(result.dropped).toEqual(['BAD_NEWLINE']) + }) + + test('drops values containing CR', () => { + const result = sanitizeEnvForGit({ + GOOD: 'value', + BAD_CR: 'value\r', + }) + expect(result.dropped).toEqual(['BAD_CR']) + }) + + test('drops values containing NUL', () => { + const result = sanitizeEnvForGit({ + GOOD: 'value', + BAD_NUL: 'a\0b', + }) + expect(result.dropped).toEqual(['BAD_NUL']) + }) + + test('drops keys whose name itself contains a control character', () => { + const result = sanitizeEnvForGit({ + 'BAD\nKEY': 'safe-value', + GOOD: 'value', + }) + expect(result.env).toEqual({ GOOD: 'value' }) + expect(result.dropped).toEqual(['BAD\nKEY']) + }) + + test('skips entries explicitly set to undefined without listing them as dropped', () => { + const result = sanitizeEnvForGit({ + GOOD: 'value', + MAYBE: undefined, + }) + expect(result.env).toEqual({ GOOD: 'value' }) + expect(result.dropped).toEqual([]) + }) + + test('returns input unchanged when nothing is unsafe', () => { + const env = { PATH: '/usr/bin:/bin', HOME: '/home/user', GIT_TERMINAL_PROMPT: '0' } + const result = sanitizeEnvForGit(env) + expect(result.env).toEqual(env) + expect(result.dropped).toEqual([]) + }) +}) + +describe('buildGitChildEnv', () => { + const ORIGINAL_BAD_KEY = 'OPENCLAUDE_TEST_BAD_ENV_FOR_GIT' + let originalValue: string | undefined + + beforeEach(() => { + __resetGitEnvWarningForTesting() + originalValue = process.env[ORIGINAL_BAD_KEY] + }) + + afterEach(() => { + if (originalValue === undefined) { + delete process.env[ORIGINAL_BAD_KEY] + } else { + process.env[ORIGINAL_BAD_KEY] = originalValue + } + }) + + test('always sets the no-prompt overrides', () => { + const env = buildGitChildEnv() + expect(env.GIT_TERMINAL_PROMPT).toBe('0') + expect(env.GIT_ASKPASS).toBe('') + }) + + test('drops process.env values containing control characters (issue #751)', () => { + process.env[ORIGINAL_BAD_KEY] = 'paste-with-newline\n' + const env = buildGitChildEnv() + expect(env[ORIGINAL_BAD_KEY]).toBeUndefined() + expect(env.GIT_TERMINAL_PROMPT).toBe('0') + }) + + test('caller extras override process.env and the no-prompt defaults', () => { + const env = buildGitChildEnv({ + GIT_TERMINAL_PROMPT: '1', + CUSTOM_KEY: 'custom-value', + }) + expect(env.GIT_TERMINAL_PROMPT).toBe('1') + expect(env.CUSTOM_KEY).toBe('custom-value') + }) + + test('caller-provided unsafe extras are also dropped', () => { + const env = buildGitChildEnv({ EXTRA_BAD: 'a\rb' }) + expect(env.EXTRA_BAD).toBeUndefined() + }) +}) diff --git a/src/utils/plugins/gitEnv.ts b/src/utils/plugins/gitEnv.ts new file mode 100644 index 00000000..fa4658bb --- /dev/null +++ b/src/utils/plugins/gitEnv.ts @@ -0,0 +1,70 @@ +import { logForDebugging } from '../debug.js' + +/** + * Git 2.30+ refuses to start when any environment value contains a NUL, + * CR, or LF character ("Unsafe environment: control characters are not + * allowed in values"). User shells frequently leak such values — a + * copy-pasted API key with a trailing newline, or a terminal-set + * variable with embedded escape sequences — which would otherwise break + * every plugin clone or pull. We drop offending entries before forwarding + * the environment to git. + */ +const GIT_UNSAFE_VALUE_RE = /[\0\r\n]/ + +const GIT_NO_PROMPT_ENV = { + GIT_TERMINAL_PROMPT: '0', // Prevent terminal credential prompts + GIT_ASKPASS: '', // Disable askpass GUI programs +} + +let warnedAboutDroppedEnvKeys = false + +/** + * Returns a copy of `env` with any entries whose key OR value contains + * a NUL/CR/LF removed. The list of dropped key names is returned so + * callers can log it without exposing the (possibly secret) values. + */ +export function sanitizeEnvForGit( + env: NodeJS.ProcessEnv, +): { env: NodeJS.ProcessEnv; dropped: string[] } { + const sanitized: NodeJS.ProcessEnv = {} + const dropped: string[] = [] + for (const [key, value] of Object.entries(env)) { + if (value === undefined) continue + if (GIT_UNSAFE_VALUE_RE.test(key) || GIT_UNSAFE_VALUE_RE.test(value)) { + dropped.push(key) + continue + } + sanitized[key] = value + } + return { env: sanitized, dropped } +} + +/** + * Build the environment object passed to a git child process. Merges + * `process.env` with the no-prompt overrides and any caller extras, + * then strips entries that would trigger git's unsafe-value check. The + * first batch of dropped key names is logged once per process so the + * user can clean them up in their shell. + */ +export function buildGitChildEnv( + extras?: NodeJS.ProcessEnv, +): NodeJS.ProcessEnv { + const merged = { ...process.env, ...GIT_NO_PROMPT_ENV, ...(extras ?? {}) } + const { env, dropped } = sanitizeEnvForGit(merged) + if (dropped.length > 0 && !warnedAboutDroppedEnvKeys) { + warnedAboutDroppedEnvKeys = true + logForDebugging( + `git child env: dropped ${dropped.length} key(s) containing control characters: ${dropped.join(', ')}. Git 2.30+ rejects them; clean these up in your shell to forward them to git.`, + { level: 'warn' }, + ) + } + return env +} + +/** + * Test-only escape hatch that resets the once-per-process warning flag + * so unit tests can exercise the warning path repeatedly. + */ +export function __resetGitEnvWarningForTesting(): void { + warnedAboutDroppedEnvKeys = false +} diff --git a/src/utils/plugins/marketplaceManager.ts b/src/utils/plugins/marketplaceManager.ts index 62f5b1e5..7efdf930 100644 --- a/src/utils/plugins/marketplaceManager.ts +++ b/src/utils/plugins/marketplaceManager.ts @@ -53,6 +53,7 @@ import { getAddDirExtraMarketplaces, } from './addDirPluginSettings.js' import { markPluginVersionOrphaned } from './cacheUtils.js' +import { buildGitChildEnv } from './gitEnv.js' import { classifyFetchError, logPluginFetch } from './fetchTelemetry.js' import { removeAllPluginsForMarketplace } from './installedPluginsManager.js' import { @@ -506,11 +507,6 @@ function seedDirFor(installLocation: string): string | undefined { * Provides helpful error messages for common failure scenarios. * If a ref is specified, fetches and checks out that specific branch or tag. */ -// Environment variables to prevent git from prompting for credentials -const GIT_NO_PROMPT_ENV = { - GIT_TERMINAL_PROMPT: '0', // Prevent terminal credential prompts - GIT_ASKPASS: '', // Disable askpass GUI programs -} const DEFAULT_PLUGIN_GIT_TIMEOUT_MS = 120 * 1000 @@ -531,7 +527,7 @@ export async function gitPull( options?: { disableCredentialHelper?: boolean; sparsePaths?: string[] }, ): Promise<{ code: number; stderr: string }> { logForDebugging(`git pull: cwd=${cwd} ref=${ref ?? 'default'}`) - const env = { ...process.env, ...GIT_NO_PROMPT_ENV } + const env = buildGitChildEnv() const baseArgs = ['-c', 'core.hooksPath=/dev/null'] const credentialArgs = options?.disableCredentialHelper ? ['-c', 'credential.helper='] @@ -844,7 +840,7 @@ export async function gitClone( const result = await execFileNoThrowWithCwd(gitExe(), args, { timeout: timeoutMs, stdin: 'ignore', - env: { ...process.env, ...GIT_NO_PROMPT_ENV }, + env: buildGitChildEnv(), }) // Scrub credentials from execa's error/stderr fields before any logging or @@ -870,7 +866,7 @@ export async function gitClone( cwd: targetPath, timeout: timeoutMs, stdin: 'ignore', - env: { ...process.env, ...GIT_NO_PROMPT_ENV }, + env: buildGitChildEnv(), }, ) if (sparseResult.code !== 0) { @@ -889,7 +885,7 @@ export async function gitClone( cwd: targetPath, timeout: timeoutMs, stdin: 'ignore', - env: { ...process.env, ...GIT_NO_PROMPT_ENV }, + env: buildGitChildEnv(), }, ) if (checkoutResult.code !== 0) { @@ -1040,7 +1036,7 @@ export async function reconcileSparseCheckout( cwd: string, sparsePaths: string[] | undefined, ): Promise<{ code: number; stderr: string }> { - const env = { ...process.env, ...GIT_NO_PROMPT_ENV } + const env = buildGitChildEnv() if (sparsePaths && sparsePaths.length > 0) { return execFileNoThrowWithCwd( diff --git a/src/utils/plugins/pluginLoader.ts b/src/utils/plugins/pluginLoader.ts index 196a452a..012c3ea1 100644 --- a/src/utils/plugins/pluginLoader.ts +++ b/src/utils/plugins/pluginLoader.ts @@ -87,6 +87,7 @@ import { getAddDirEnabledPlugins } from './addDirPluginSettings.js' import { verifyAndDemote } from './dependencyResolver.js' import { classifyFetchError, logPluginFetch } from './fetchTelemetry.js' import { checkGitAvailable } from './gitAvailability.js' +import { buildGitChildEnv } from './gitEnv.js' import { getInMemoryInstalledPlugins } from './installedPluginsManager.js' import { getManagedPluginNames } from './managedPlugins.js' import { @@ -560,7 +561,9 @@ export async function gitClone( args.push(gitUrl, targetPath) const cloneStarted = performance.now() - const cloneResult = await execFileNoThrow(gitExe(), args) + const cloneResult = await execFileNoThrow(gitExe(), args, { + env: buildGitChildEnv(), + }) if (cloneResult.code !== 0) { logPluginFetch( @@ -579,7 +582,7 @@ export async function gitClone( const shallowFetchResult = await execFileNoThrowWithCwd( gitExe(), ['fetch', '--depth', '1', 'origin', sha], - { cwd: targetPath }, + { cwd: targetPath, env: buildGitChildEnv() }, ) if (shallowFetchResult.code !== 0) { @@ -591,7 +594,7 @@ export async function gitClone( const unshallowResult = await execFileNoThrowWithCwd( gitExe(), ['fetch', '--unshallow'], - { cwd: targetPath }, + { cwd: targetPath, env: buildGitChildEnv() }, ) if (unshallowResult.code !== 0) { @@ -612,7 +615,7 @@ export async function gitClone( const checkoutResult = await execFileNoThrowWithCwd( gitExe(), ['checkout', sha], - { cwd: targetPath }, + { cwd: targetPath, env: buildGitChildEnv() }, ) if (checkoutResult.code !== 0) { @@ -745,7 +748,9 @@ export async function installFromGitSubdir( } cloneArgs.push(gitUrl, cloneDir) - const cloneResult = await execFileNoThrow(gitExe(), cloneArgs) + const cloneResult = await execFileNoThrow(gitExe(), cloneArgs, { + env: buildGitChildEnv(), + }) if (cloneResult.code !== 0) { throw new Error( `Failed to clone repository for git-subdir source: ${cloneResult.stderr}`, @@ -756,7 +761,7 @@ export async function installFromGitSubdir( const sparseResult = await execFileNoThrowWithCwd( gitExe(), ['sparse-checkout', 'set', '--cone', '--', subdirPath], - { cwd: cloneDir }, + { cwd: cloneDir, env: buildGitChildEnv() }, ) if (sparseResult.code !== 0) { throw new Error( @@ -775,7 +780,7 @@ export async function installFromGitSubdir( const fetchSha = await execFileNoThrowWithCwd( gitExe(), ['fetch', '--depth', '1', 'origin', sha], - { cwd: cloneDir }, + { cwd: cloneDir, env: buildGitChildEnv() }, ) if (fetchSha.code !== 0) { logForDebugging( @@ -784,7 +789,7 @@ export async function installFromGitSubdir( const unshallow = await execFileNoThrowWithCwd( gitExe(), ['fetch', '--unshallow'], - { cwd: cloneDir }, + { cwd: cloneDir, env: buildGitChildEnv() }, ) if (unshallow.code !== 0) { throw new Error(`Failed to fetch commit ${sha}: ${unshallow.stderr}`) @@ -793,7 +798,7 @@ export async function installFromGitSubdir( const checkout = await execFileNoThrowWithCwd( gitExe(), ['checkout', sha], - { cwd: cloneDir }, + { cwd: cloneDir, env: buildGitChildEnv() }, ) if (checkout.code !== 0) { throw new Error(`Failed to checkout commit ${sha}: ${checkout.stderr}`) @@ -808,9 +813,11 @@ export async function installFromGitSubdir( const [checkout, revParse] = await Promise.all([ execFileNoThrowWithCwd(gitExe(), ['checkout', 'HEAD'], { cwd: cloneDir, + env: buildGitChildEnv(), }), execFileNoThrowWithCwd(gitExe(), ['rev-parse', 'HEAD'], { cwd: cloneDir, + env: buildGitChildEnv(), }), ]) if (checkout.code !== 0) {