From d17a813cc35842afede7ee3ac7c286299b4f3840 Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Wed, 13 May 2026 15:29:05 -0700 Subject: [PATCH] Revert "fix:reorder env var redaction checks to scan values first (#21059)" This reverts commit 9773a084c99719ccc1e357bf16034f2e3fce57fd. --- .../services/environmentSanitization.test.ts | 42 ------------------- .../src/services/environmentSanitization.ts | 36 ++++++++-------- 2 files changed, 19 insertions(+), 59 deletions(-) diff --git a/packages/core/src/services/environmentSanitization.test.ts b/packages/core/src/services/environmentSanitization.test.ts index e36f879f06..dcf665f88e 100644 --- a/packages/core/src/services/environmentSanitization.test.ts +++ b/packages/core/src/services/environmentSanitization.test.ts @@ -230,48 +230,6 @@ describe('sanitizeEnvironment', () => { }); }); - describe('value-first security: secret values must be caught even for allowed variable names', () => { - it('should redact ALWAYS_ALLOWED variables whose values contain a GitHub token', () => { - const env = { - HOME: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', - PATH: '/usr/bin', - }; - const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS); - expect(sanitized).toEqual({ PATH: '/usr/bin' }); - }); - - it('should redact ALWAYS_ALLOWED variables whose values contain a certificate', () => { - const env = { - SHELL: - '-----BEGIN RSA PRIVATE KEY-----\nMIIE...\n-----END RSA PRIVATE KEY-----', - USER: 'alice', - }; - const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS); - expect(sanitized).toEqual({ USER: 'alice' }); - }); - - it('should redact user-allowlisted variables whose values contain a secret', () => { - const env = { - MY_SAFE_VAR: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', - OTHER: 'fine', - }; - const sanitized = sanitizeEnvironment(env, { - allowedEnvironmentVariables: ['MY_SAFE_VAR'], - blockedEnvironmentVariables: [], - enableEnvironmentVariableRedaction: true, - }); - expect(sanitized).toEqual({ OTHER: 'fine' }); - }); - - it('should NOT redact GEMINI_CLI_ variables even if their value looks like a secret (fully trusted)', () => { - const env = { - GEMINI_CLI_INTERNAL: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', - }; - const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS); - expect(sanitized).toEqual(env); - }); - }); - it('should ensure all names in the sets are capitalized', () => { for (const name of ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES) { expect(name).toBe(name.toUpperCase()); diff --git a/packages/core/src/services/environmentSanitization.ts b/packages/core/src/services/environmentSanitization.ts index eb95a91ca8..909a3518b1 100644 --- a/packages/core/src/services/environmentSanitization.ts +++ b/packages/core/src/services/environmentSanitization.ts @@ -14,9 +14,11 @@ export function sanitizeEnvironment( processEnv: NodeJS.ProcessEnv, config: EnvironmentSanitizationConfig, ): NodeJS.ProcessEnv { + // Enable strict sanitization in GitHub actions. const isStrictSanitization = !!processEnv['GITHUB_SHA'] || processEnv['SURFACE'] === 'Github'; + // Always sanitize when in GitHub actions. if (!config.enableEnvironmentVariableRedaction && !isStrictSanitization) { return { ...processEnv }; } @@ -150,22 +152,7 @@ function shouldRedactEnvironmentVariable( key = key.toUpperCase(); value = value?.toUpperCase(); - if (key.startsWith('GEMINI_CLI_')) { - return false; - } - - if (value) { - for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) { - if (pattern.test(value)) { - return true; - } - } - } - - if (key.startsWith('GIT_CONFIG_')) { - return false; - } - + // User overrides take precedence. if (allowedSet?.has(key)) { return false; } @@ -173,14 +160,20 @@ function shouldRedactEnvironmentVariable( return true; } - if (ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) { + // These are never redacted. + if ( + ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key) || + key.startsWith('GEMINI_CLI_') + ) { return false; } + // These are always redacted. if (NEVER_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) { return true; } + // If in strict mode (e.g. GitHub Action), and not explicitly allowed, redact it. if (isStrictSanitization) { return true; } @@ -191,6 +184,15 @@ function shouldRedactEnvironmentVariable( } } + // Redact if the value looks like a key/cert. + if (value) { + for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) { + if (pattern.test(value)) { + return true; + } + } + } + return false; }