From 369e23b084f12a073bbbe5734bdbcc79da88eda2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:04:25 -0800 Subject: [PATCH] fix(git): keep the agent-terminal credential guard scalar-only so env scrubs can't break git (#38716) * fix(git): keep the agent-terminal credential guard scalar-only so env scrubs can't break git Agents that scrub env names matching KEY/TOKEN/... (DeepSeek Harness) dropped GIT_CONFIG_KEY_n but kept GIT_CONFIG_COUNT, so every git command in those terminals failed with "missing config key GIT_CONFIG_KEY_0". GCM_INTERACTIVE already carries credential.interactive and suppresses GCM's GUI, so the guard now emits only scalar variables. Tracked in #26594. Co-authored-by: yu.xia * chore(git): drop stale indexed-config comments and a dead WSL forward branch --------- Co-authored-by: yu.xia --- .../pty-subprocess/spawn-environment.ts | 2 +- .../git/command-runner/git-process-env.ts | 2 - src/main/git/runner.test.ts | 82 ++++--------------- src/main/ipc/pty/host-env/types.ts | 2 +- .../pty-daemon-spawn-session-identity.test.ts | 6 +- .../pty/pty-spawn-env-terminal-basics.test.ts | 6 +- src/main/workspace-hooks/hooks.ts | 9 +- .../git/commands/git-credential-prompt-env.ts | 47 +++-------- .../git-binary-compatibility.test.ts | 24 +++--- .../launch/terminal-git-credential-guard.ts | 8 +- src/wsl-guest/relay-command-env.test.ts | 6 +- 11 files changed, 51 insertions(+), 143 deletions(-) diff --git a/src/main/daemon/pty-subprocess/spawn-environment.ts b/src/main/daemon/pty-subprocess/spawn-environment.ts index 29e0e59566c..324e96ea758 100644 --- a/src/main/daemon/pty-subprocess/spawn-environment.ts +++ b/src/main/daemon/pty-subprocess/spawn-environment.ts @@ -56,7 +56,7 @@ function composeGuardedDaemonGitConfigEnv( if (policy !== 'guard' && launchAgent === undefined) { return } - // Preserve the long-lived daemon’s inherited Git config while appending the guard. + // Apply the guard after the inherited-env merge so it reflects the pane's final askpass values. Object.assign(env, gitCredentialPromptGuardEnv(env, process.platform)) } diff --git a/src/main/git/command-runner/git-process-env.ts b/src/main/git/command-runner/git-process-env.ts index 963feb21943..9a72134400a 100644 --- a/src/main/git/command-runner/git-process-env.ts +++ b/src/main/git/command-runner/git-process-env.ts @@ -11,8 +11,6 @@ export function gitOptionalLocksDisabledEnv( } } -/** Append Git >= 2.31 config-env entries without overwriting the caller's existing entries. */ - /** Pin Git diagnostics to English so parsers work under any user locale. */ export function untranslatedGitOutputEnv(env: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { return { ...env, ...UNTRANSLATED_GIT_OUTPUT_ENV } diff --git a/src/main/git/runner.test.ts b/src/main/git/runner.test.ts index a46c8035ab6..c4e1cf7547c 100644 --- a/src/main/git/runner.test.ts +++ b/src/main/git/runner.test.ts @@ -1,8 +1,5 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -import { - appendGitConfigEnv, - mergeGitConfigEnvProtocol -} from '../../shared/git/commands/git-credential-prompt-env' +import { mergeGitConfigEnvProtocol } from '../../shared/git/commands/git-credential-prompt-env' import { extractExecError, isTransientGhError, @@ -14,19 +11,6 @@ import { untranslatedGitOutputEnv } from './runner' -function readGitConfigEnv(env: NodeJS.ProcessEnv): Record { - const count = Number.parseInt(env.GIT_CONFIG_COUNT ?? '0', 10) - const config: Record = {} - for (let i = 0; i < count; i++) { - const key = env[`GIT_CONFIG_KEY_${i}`] - const value = env[`GIT_CONFIG_VALUE_${i}`] - if (key !== undefined && value !== undefined) { - config[key] = value - } - } - return config -} - describe('redirectPortedHostnameToEnv', () => { it('moves a ported --hostname into GITLAB_HOST and strips the flag', () => { const { args, options } = redirectPortedHostnameToEnv( @@ -163,45 +147,6 @@ describe('extractExecError', () => { }) }) -describe('appendGitConfigEnv', () => { - it('injects entries starting at count 0 when none exist', () => { - const env = appendGitConfigEnv({ PATH: '/usr/bin' }, [['credential.interactive', 'false']]) - expect(env.GIT_CONFIG_COUNT).toBe('1') - expect(env.GIT_CONFIG_KEY_0).toBe('credential.interactive') - expect(env.GIT_CONFIG_VALUE_0).toBe('false') - expect(env.PATH).toBe('/usr/bin') - }) - - it('composes with an existing count instead of clobbering caller config', () => { - const env = appendGitConfigEnv( - { GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'core.quotePath', GIT_CONFIG_VALUE_0: 'false' }, - [['credential.guiPrompt', 'false']] - ) - expect(env.GIT_CONFIG_COUNT).toBe('2') - expect(env.GIT_CONFIG_KEY_0).toBe('core.quotePath') - expect(env.GIT_CONFIG_VALUE_0).toBe('false') - expect(env.GIT_CONFIG_KEY_1).toBe('credential.guiPrompt') - expect(env.GIT_CONFIG_VALUE_1).toBe('false') - }) - - it.each(['bogus', '-1', '0', String(Number.MAX_SAFE_INTEGER)])( - 'does not overwrite dangling caller config when count is %s', - (count) => { - const original = { - GIT_CONFIG_COUNT: count, - GIT_CONFIG_KEY_0: 'user.key', - GIT_CONFIG_VALUE_0: 'caller-value' - } - expect(appendGitConfigEnv(original, [['credential.interactive', 'false']])).toEqual(original) - } - ) - - it('does not append to an incomplete indexed-config protocol', () => { - const original = { GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'user.key' } - expect(appendGitConfigEnv(original, [['credential.interactive', 'false']])).toEqual(original) - }) -}) - describe('mergeGitConfigEnvProtocol', () => { it('replaces inherited indexed config atomically when an override has a smaller count', () => { const env = mergeGitConfigEnvProtocol( @@ -230,11 +175,8 @@ describe('promptGuardGitEnv credential-interactivity disable (STA-1292)', () => it('disables the GCM GUI prompt without nuking the credential helper', () => { const env = promptGuardGitEnv({ PATH: '/usr/bin' }) expect(env.GCM_INTERACTIVE).toBe('never') - const config = readGitConfigEnv(env) - expect(config['credential.interactive']).toBe('false') - expect(config['credential.guiPrompt']).toBe('false') - // Preserve credential helpers so private repos can still use cached authentication. - expect(config['credential.helper']).toBeUndefined() + // Scalar guards only: env scrubs can split an indexed config protocol (#26594). + expect(env.GIT_CONFIG_COUNT).toBeUndefined() expect(env.GIT_TERMINAL_PROMPT).toBe('0') }) }) @@ -243,25 +185,29 @@ describe('nonInteractiveGitEnv credential-interactivity disable (STA-1292)', () it('carries the credential-interactivity disable through from promptGuardGitEnv', () => { const env = nonInteractiveGitEnv({ PATH: '/usr/bin' }) expect(env.GCM_INTERACTIVE).toBe('never') - const config = readGitConfigEnv(env) - expect(config['credential.interactive']).toBe('false') - expect(config['credential.guiPrompt']).toBe('false') - expect(config['credential.helper']).toBeUndefined() + expect(env.GIT_CONFIG_COUNT).toBeUndefined() expect(env.GIT_SSH_COMMAND).toBe('ssh -o BatchMode=yes') }) }) describe('guard-env WSLENV forwarding (#7652)', () => { it('registers the guard vars in WSLENV on Windows so WSL-routed git imports them', () => { - const env = promptGuardGitEnv({ PATH: '/usr/bin' }, 'win32') + const env = promptGuardGitEnv( + { + PATH: '/usr/bin', + GIT_CONFIG_COUNT: '1', + GIT_CONFIG_KEY_0: 'http.proxy', + GIT_CONFIG_VALUE_0: 'http://proxy.invalid' + }, + 'win32' + ) const keys = (env.WSLENV ?? '').split(':') expect(keys).toContain('GIT_TERMINAL_PROMPT') expect(keys).toContain('GCM_INTERACTIVE') + // A caller's indexed config is forwarded as a complete set. expect(keys).toContain('GIT_CONFIG_COUNT') expect(keys).toContain('GIT_CONFIG_KEY_0') expect(keys).toContain('GIT_CONFIG_VALUE_0') - expect(keys).toContain('GIT_CONFIG_KEY_1') - expect(keys).toContain('GIT_CONFIG_VALUE_1') // Windows askpass paths are meaningless inside a distro. expect(keys).not.toContain('GIT_ASKPASS') expect(keys).not.toContain('SSH_ASKPASS') diff --git a/src/main/ipc/pty/host-env/types.ts b/src/main/ipc/pty/host-env/types.ts index 586fe03e34f..bef62088cd7 100644 --- a/src/main/ipc/pty/host-env/types.ts +++ b/src/main/ipc/pty/host-env/types.ts @@ -32,7 +32,7 @@ export type BuildPtyHostEnvOptions = { networkProxySettings?: NetworkProxySettings /** Headless paired runtimes hand browser launches to the client-hosted Orca browser. */ routeBrowserOpensToClient?: boolean - /** Keep indexed Git config off the sparse daemon wire; the daemon appends guard entries after merging its inherited env. */ + /** The daemon reapplies the Git credential guard after merging its inherited env, so askpass reflects the final pane env. */ deferGitConfigGuardToDaemon?: boolean } diff --git a/src/main/ipc/pty/pty-daemon-spawn-session-identity.test.ts b/src/main/ipc/pty/pty-daemon-spawn-session-identity.test.ts index 52c2ae9eaa4..898f7f1d0ca 100644 --- a/src/main/ipc/pty/pty-daemon-spawn-session-identity.test.ts +++ b/src/main/ipc/pty/pty-daemon-spawn-session-identity.test.ts @@ -244,9 +244,9 @@ describe('registerPtyHandlers', () => { expect(env.GIT_TERMINAL_PROMPT).toBe('0') expect(env.GCM_INTERACTIVE).toBe('never') - expect(env.GIT_CONFIG_COUNT).toBe('3') - expect(env.GIT_CONFIG_KEY_1).toBe('credential.interactive') - expect(env.GIT_CONFIG_KEY_2).toBe('credential.guiPrompt') + expect(env.GIT_CONFIG_COUNT).toBe('1') + expect(env.GIT_CONFIG_KEY_0).toBe('http.proxy') + expect(env.GIT_CONFIG_KEY_1).toBeUndefined() }) it('passes the minted sessionId through to provider.spawn and host env setup', async () => { const daemonSpawn = setupDaemonAdapter() diff --git a/src/main/ipc/pty/pty-spawn-env-terminal-basics.test.ts b/src/main/ipc/pty/pty-spawn-env-terminal-basics.test.ts index 1cac45330e0..d4ce8fefeee 100644 --- a/src/main/ipc/pty/pty-spawn-env-terminal-basics.test.ts +++ b/src/main/ipc/pty/pty-spawn-env-terminal-basics.test.ts @@ -550,12 +550,12 @@ describe('registerPtyHandlers', () => { const env = await spawnAndGetEnv() expect(env.ORCA_IMAGE_PROTOCOL).toBe('kitty') }) - it('keeps indexed Git prompt guards in a local agent terminal env', async () => { + it('guards a local agent terminal with scalar Git prompt variables only', async () => { const env = await spawnAndGetEnv(undefined, undefined, undefined, undefined, 'claude') expect(env.GIT_TERMINAL_PROMPT).toBe('0') expect(env.GCM_INTERACTIVE).toBe('never') - expect(Object.values(env)).toContain('credential.interactive') - expect(Object.values(env)).toContain('credential.guiPrompt') + // Agents that scrub KEY-named env would split an indexed protocol and break git (#26594). + expect(Object.keys(env).filter((key) => key.startsWith('GIT_CONFIG_'))).toEqual([]) }) it('guards a trusted local agent when its command uses a custom wrapper', async () => { const env = await spawnAndGetEnv( diff --git a/src/main/workspace-hooks/hooks.ts b/src/main/workspace-hooks/hooks.ts index fb451bf5c4f..edb8b9d190b 100644 --- a/src/main/workspace-hooks/hooks.ts +++ b/src/main/workspace-hooks/hooks.ts @@ -205,16 +205,11 @@ export function runHook( for (const [key, value] of Object.entries(envVars)) { wslEnv[key] = toLinuxPath(value) } - // Forward unattended Git flags and indexed config to WSL; keep host askpass out of the guest. + // Forward unattended Git flags to WSL; keep host askpass out of the guest. const guardedEnv = promptGuardShellEnv(wslEnv) const guestEnv: Record = { ...wslEnv } for (const [key, value] of Object.entries(guardedEnv)) { - if ( - value !== undefined && - (key === 'GIT_TERMINAL_PROMPT' || - key === 'GCM_INTERACTIVE' || - key.startsWith('GIT_CONFIG_')) - ) { + if (value !== undefined && (key === 'GIT_TERMINAL_PROMPT' || key === 'GCM_INTERACTIVE')) { guestEnv[key] = value } } diff --git a/src/shared/git/commands/git-credential-prompt-env.ts b/src/shared/git/commands/git-credential-prompt-env.ts index 7d8c38d906e..9276f26e123 100644 --- a/src/shared/git/commands/git-credential-prompt-env.ts +++ b/src/shared/git/commands/git-credential-prompt-env.ts @@ -57,25 +57,6 @@ export function readValidGitConfigEnvCount(env: NodeJS.ProcessEnv): number | nul return hasDanglingIndex ? null : count } -/** Compose indexed Git config without clobbering caller-provided entries. */ -export function appendGitConfigEnv( - env: NodeJS.ProcessEnv, - entries: readonly (readonly [key: string, value: string])[] -): NodeJS.ProcessEnv { - const next = { ...env } - const base = readValidGitConfigEnvCount(env) - if (base === null) { - // Ambiguous indices may hold caller data; use scalar guards instead of overwriting them. - return next - } - entries.forEach(([key, value], index) => { - next[`GIT_CONFIG_KEY_${base + index}`] = key - next[`GIT_CONFIG_VALUE_${base + index}`] = value - }) - next.GIT_CONFIG_COUNT = String(base + entries.length) - return next -} - /** * Disable interactive Git credential UI while preserving cached credentials * and caller-provided askpass programs. @@ -84,23 +65,19 @@ export function gitCredentialPromptGuardEnv( env: NodeJS.ProcessEnv, platform: NodeJS.Platform = process.platform ): NodeJS.ProcessEnv { - const next = appendGitConfigEnv( - { - ...env, - GIT_TERMINAL_PROMPT: '0', - GIT_ASKPASS: env.GIT_ASKPASS ?? '', - SSH_ASKPASS: env.SSH_ASKPASS ?? '', - // Why: GCM can ignore terminal/askpass guards and open its own GUI. - GCM_INTERACTIVE: 'never' - }, - // Retain cached credentials while disabling only interactive helper fallback. - [ - ['credential.interactive', 'false'], - ['credential.guiPrompt', 'false'] - ] - ) + // Why: scalar guards only. Agents scrub env names matching KEY/TOKEN/..., which drops + // GIT_CONFIG_KEY_n but keeps GIT_CONFIG_COUNT, and Git then refuses every command (#26594). + // GCM_INTERACTIVE is GCM's env form of credential.interactive and also suppresses its GUI. + const next: NodeJS.ProcessEnv = { + ...env, + GIT_TERMINAL_PROMPT: '0', + GIT_ASKPASS: env.GIT_ASKPASS ?? '', + SSH_ASKPASS: env.SSH_ASKPASS ?? '', + // Why: GCM can ignore terminal/askpass guards and open its own GUI. + GCM_INTERACTIVE: 'never' + } if (platform === 'win32') { - // WSLENV must register the complete indexed config protocol or Git rejects its count. + // WSLENV must forward a caller's indexed config as a complete set or Git rejects its count. const configKeys = readValidGitConfigEnvCount(next) === null ? [] diff --git a/src/shared/git/compatibility/git-binary-compatibility.test.ts b/src/shared/git/compatibility/git-binary-compatibility.test.ts index a1f9b76509f..1cf876317d7 100644 --- a/src/shared/git/compatibility/git-binary-compatibility.test.ts +++ b/src/shared/git/compatibility/git-binary-compatibility.test.ts @@ -747,19 +747,17 @@ describeBinaryCompatibility('real Git binary compatibility', () => { await runGit(['-C', worktree, 'update-ref', '-d', backupRef, backupOid]) }) - it('degrades indexed credential config safely at the Git 2.31 boundary', async () => { - const guardEnv = gitCredentialPromptGuardEnv({}, 'linux') - await expect(runGit(['status', '--short'], guardEnv)).resolves.toBeDefined() - - try { - const result = await runGit(['config', '--get', 'credential.interactive'], guardEnv) - expect(supports(2, 31)).toBe(true) - expect(result.stdout.trim()).toBe('false') - } catch { - // Git 2.25 ignores the indexed variables rather than rejecting commands; - // the scalar prompt guards still provide the baseline fail-fast behavior. - expect(supports(2, 31)).toBe(false) - } + it('keeps git usable when an agent scrubs credential-shaped env names (#26594)', async () => { + // The DeepSeek Harness subprocess filter, recorded in #26594. + const sensitiveEnvPattern = /KEY|PASSWORD|SECRET|TOKEN/i + const scrubbed = Object.fromEntries( + Object.entries(gitCredentialPromptGuardEnv({}, 'linux')).filter( + ([key]) => !sensitiveEnvPattern.test(key) + ) + ) + expect(scrubbed.GIT_TERMINAL_PROMPT).toBe('0') + expect(scrubbed.GCM_INTERACTIVE).toBe('never') + await expect(runGit(['status', '--short'], scrubbed)).resolves.toBeDefined() }) // Git 2.25–2.49 echoes --end-of-options with --symbolic-full-name but consumes it with --verify; git-history skips the echoed line. diff --git a/src/shared/shell/launch/terminal-git-credential-guard.ts b/src/shared/shell/launch/terminal-git-credential-guard.ts index 4856737305d..a981d3f2dc8 100644 --- a/src/shared/shell/launch/terminal-git-credential-guard.ts +++ b/src/shared/shell/launch/terminal-git-credential-guard.ts @@ -1,8 +1,6 @@ import { recognizeAgentLaunchCommand } from '../../agent-launch/startup/agent-command-line-intent' import { gitCredentialPromptGuardEnv } from '../../git/commands/git-credential-prompt-env' -const GIT_CONFIG_PROTOCOL_KEY_RE = /^GIT_CONFIG_(?:COUNT|KEY_\d+|VALUE_\d+)$/ - export const TERMINAL_GIT_CREDENTIAL_GUARD_POLICY_ENV = 'ORCA_INTERNAL_TERMINAL_GIT_CREDENTIAL_GUARD_POLICY' @@ -13,7 +11,7 @@ export function applyTerminalGitCredentialPromptGuard( launchCommand?: string | null isUnattended?: boolean platform?: NodeJS.Platform - /** A detached host appends indexed config after its authoritative env merge. */ + /** A detached host reapplies the guard after its authoritative env merge. */ deferGitConfigGuardToHost?: boolean } ): boolean { @@ -37,10 +35,10 @@ export function applyTerminalGitCredentialPromptGuard( return true } - // The daemon must append indexed config after merging inherited env; send only the policy and scalar guards. + // The daemon reapplies the guard after merging inherited env; send the policy and scalar guards. env[TERMINAL_GIT_CREDENTIAL_GUARD_POLICY_ENV] = 'guard' for (const [key, value] of Object.entries(guarded)) { - if (typeof value !== 'string' || GIT_CONFIG_PROTOCOL_KEY_RE.test(key)) { + if (typeof value !== 'string') { continue } if (key === 'WSLENV') { diff --git a/src/wsl-guest/relay-command-env.test.ts b/src/wsl-guest/relay-command-env.test.ts index 06640aa1889..dbd204fbca8 100644 --- a/src/wsl-guest/relay-command-env.test.ts +++ b/src/wsl-guest/relay-command-env.test.ts @@ -68,11 +68,7 @@ describe('buildRelayUnattendedGitEnv', () => { expect(env.GIT_TERMINAL_PROMPT).toBe('0') expect(env.GCM_INTERACTIVE).toBe('never') expect(env.GIT_ASKPASS).toBe('/opt/noninteractive-credential-feeder') - expect(env.GIT_CONFIG_COUNT).toBe('2') - expect(env.GIT_CONFIG_KEY_0).toBe('credential.interactive') - expect(env.GIT_CONFIG_VALUE_0).toBe('false') - expect(env.GIT_CONFIG_KEY_1).toBe('credential.guiPrompt') - expect(env.GIT_CONFIG_VALUE_1).toBe('false') + expect(env.GIT_CONFIG_COUNT).toBeUndefined() expect(env.GIT_SSH_COMMAND).toBeUndefined() expect(env.LC_ALL).toBe('en_US.UTF-8') expect(env.PATH?.split(':')).toEqual(expect.arrayContaining(['/custom/bin', '/usr/bin']))