From d33170931c3be6384b10f68c7a151767ead055b1 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Thu, 26 Mar 2026 13:04:44 -0700 Subject: [PATCH] fix(core): allow disabling environment variable redaction (#23927) --- .../src/sandbox/macos/MacOsSandboxManager.test.ts | 5 ++++- .../src/services/environmentSanitization.test.ts | 8 ++++---- .../core/src/services/environmentSanitization.ts | 5 ++++- .../src/services/sandboxManager.integration.test.ts | 13 ++++++++++++- packages/core/src/services/sandboxManager.test.ts | 13 ++++++++++--- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index 3f23a22553..d528223b7e 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -112,7 +112,10 @@ describe('MacOsSandboxManager', () => { SAFE_VAR: '1', GITHUB_TOKEN: 'sensitive', }, - policy: mockPolicy, + policy: { + ...mockPolicy, + sanitizationConfig: { enableEnvironmentVariableRedaction: true }, + }, }); expect(result.env['SAFE_VAR']).toBe('1'); diff --git a/packages/core/src/services/environmentSanitization.test.ts b/packages/core/src/services/environmentSanitization.test.ts index a7889ef0c2..e36f879f06 100644 --- a/packages/core/src/services/environmentSanitization.test.ts +++ b/packages/core/src/services/environmentSanitization.test.ts @@ -375,9 +375,9 @@ describe('sanitizeEnvironment', () => { }); describe('getSecureSanitizationConfig', () => { - it('should enable environment variable redaction by default', () => { + it('should default enableEnvironmentVariableRedaction to false', () => { const config = getSecureSanitizationConfig(); - expect(config.enableEnvironmentVariableRedaction).toBe(true); + expect(config.enableEnvironmentVariableRedaction).toBe(false); }); it('should merge allowed and blocked variables from base and requested configs', () => { @@ -440,13 +440,13 @@ describe('getSecureSanitizationConfig', () => { expect(config.blockedEnvironmentVariables).toEqual(['BLOCKED_VAR']); }); - it('should force enableEnvironmentVariableRedaction to true even if requested false', () => { + it('should respect requested enableEnvironmentVariableRedaction value', () => { const requestedConfig = { enableEnvironmentVariableRedaction: false, }; const config = getSecureSanitizationConfig(requestedConfig); - expect(config.enableEnvironmentVariableRedaction).toBe(true); + expect(config.enableEnvironmentVariableRedaction).toBe(false); }); }); diff --git a/packages/core/src/services/environmentSanitization.ts b/packages/core/src/services/environmentSanitization.ts index f3c5628607..eb95a91ca8 100644 --- a/packages/core/src/services/environmentSanitization.ts +++ b/packages/core/src/services/environmentSanitization.ts @@ -230,6 +230,9 @@ export function getSecureSanitizationConfig( allowedEnvironmentVariables: [...new Set(allowed)], blockedEnvironmentVariables: [...new Set(blocked)], // Redaction must be enabled for secure configurations - enableEnvironmentVariableRedaction: true, + enableEnvironmentVariableRedaction: + requestedConfig.enableEnvironmentVariableRedaction ?? + baseConfig?.enableEnvironmentVariableRedaction ?? + false, }; } diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index c4bc2f1cc5..e1954e9a5b 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -108,7 +108,18 @@ function ensureSandboxAvailable(): boolean { if (platform === 'darwin') { if (fs.existsSync('/usr/bin/sandbox-exec')) { - return true; + try { + execSync('sandbox-exec -p "(version 1)(allow default)" echo test', { + stdio: 'ignore', + }); + return true; + } catch { + // eslint-disable-next-line no-console + console.warn( + 'sandbox-exec is present but cannot be used (likely running inside a sandbox already). Skipping sandbox tests.', + ); + return false; + } } throw new Error( 'Sandboxing tests on macOS require /usr/bin/sandbox-exec to be present.', diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 1f3cfa089e..a677c790b1 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -148,6 +148,11 @@ describe('SandboxManager', () => { MY_SECRET: 'super-secret', SAFE_VAR: 'is-safe', }, + policy: { + sanitizationConfig: { + enableEnvironmentVariableRedaction: true, + }, + }, }; const result = await sandboxManager.prepareCommand(req); @@ -158,7 +163,7 @@ describe('SandboxManager', () => { expect(result.env['MY_SECRET']).toBeUndefined(); }); - it('should NOT allow disabling environment variable redaction if requested in config (vulnerability fix)', async () => { + it('should allow disabling environment variable redaction if requested in config', async () => { const req = { command: 'echo', args: ['hello'], @@ -175,8 +180,8 @@ describe('SandboxManager', () => { const result = await sandboxManager.prepareCommand(req); - // API_KEY should be redacted because SandboxManager forces redaction and API_KEY matches NEVER_ALLOWED_NAME_PATTERNS - expect(result.env['API_KEY']).toBeUndefined(); + // API_KEY should be preserved because redaction was explicitly disabled + expect(result.env['API_KEY']).toBe('sensitive-key'); }); it('should respect allowedEnvironmentVariables in config but filter sensitive ones', async () => { @@ -191,6 +196,7 @@ describe('SandboxManager', () => { policy: { sanitizationConfig: { allowedEnvironmentVariables: ['MY_SAFE_VAR', 'MY_TOKEN'], + enableEnvironmentVariableRedaction: true, }, }, }; @@ -214,6 +220,7 @@ describe('SandboxManager', () => { policy: { sanitizationConfig: { blockedEnvironmentVariables: ['BLOCKED_VAR'], + enableEnvironmentVariableRedaction: true, }, }, };