fix:reorder env var redaction checks to scan values first (#21059)

Signed-off-by: Kartik Angiras <angiraskartik@gmail.com>

packages/core/src/services/environmentSanitization.test.ts

@@ -206,6 +206,48 @@ 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());

packages/core/src/services/environmentSanitization.ts

@@ -14,11 +14,9 @@ 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 };
   }
@@ -148,7 +146,18 @@ function shouldRedactEnvironmentVariable(
   key = key.toUpperCase();
   value = value?.toUpperCase();
 
-  // User overrides take precedence.
+  if (key.startsWith('GEMINI_CLI_')) {
+    return false;
+  }
+
+  if (value) {
+    for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) {
+      if (pattern.test(value)) {
+        return true;
+      }
+    }
+  }
+
   if (allowedSet?.has(key)) {
     return false;
   }
@@ -156,20 +165,14 @@ function shouldRedactEnvironmentVariable(
     return true;
   }
 
-  // These are never redacted.
-  if (
-    ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key) ||
-    key.startsWith('GEMINI_CLI_')
-  ) {
+  if (ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) {
     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;
   }
@@ -180,14 +183,5 @@ 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;
 }