From 0a18382acd50090b96a782aed8fb13af97b814d8 Mon Sep 17 00:00:00 2001 From: galz10 Date: Thu, 26 Mar 2026 14:48:18 -0700 Subject: [PATCH] fix(core): allow disabling environment variable redaction (#23927) --- .../services/environmentSanitization.test.ts | 8 +- .../src/services/environmentSanitization.ts | 5 +- .../core/src/services/sandboxManager.test.ts | 235 +++++++++++++++++- 3 files changed, 232 insertions(+), 16 deletions(-) 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.test.ts b/packages/core/src/services/sandboxManager.test.ts index 44d52aa83c..54e9a0b5d2 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -30,18 +30,231 @@ describe('NoopSandboxManager', () => { expect(result.args).toEqual(['-la']); }); - it('should sanitize the environment variables', async () => { - const req = { - command: 'echo', - args: ['hello'], - cwd: '/tmp', - env: { - PATH: '/usr/bin', - GITHUB_TOKEN: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', - MY_SECRET: 'super-secret', - SAFE_VAR: 'is-safe', + describe('tryRealpath', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return the realpath if the file exists', async () => { + vi.spyOn(fs, 'realpath').mockResolvedValue('/real/path/to/file.txt'); + const result = await tryRealpath('/some/symlink/to/file.txt'); + expect(result).toBe('/real/path/to/file.txt'); + expect(fs.realpath).toHaveBeenCalledWith('/some/symlink/to/file.txt'); + }); + + it('should fallback to parent directory if file does not exist (ENOENT)', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { + if (p === '/workspace/nonexistent.txt') { + throw Object.assign(new Error('ENOENT: no such file or directory'), { + code: 'ENOENT', + }); + } + if (p === '/workspace') { + return '/real/workspace'; + } + throw new Error(`Unexpected path: ${p}`); + }); + + const result = await tryRealpath('/workspace/nonexistent.txt'); + + // It should combine the real path of the parent with the original basename + expect(result).toBe(path.join('/real/workspace', 'nonexistent.txt')); + }); + + it('should recursively fallback up the directory tree on multiple ENOENT errors', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async (p) => { + if (p === '/workspace/missing_dir/missing_file.txt') { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + } + if (p === '/workspace/missing_dir') { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + } + if (p === '/workspace') { + return '/real/workspace'; + } + throw new Error(`Unexpected path: ${p}`); + }); + + const result = await tryRealpath( + '/workspace/missing_dir/missing_file.txt', + ); + + // It should resolve '/workspace' to '/real/workspace' and append the missing parts + expect(result).toBe( + path.join('/real/workspace', 'missing_dir', 'missing_file.txt'), + ); + }); + + it('should return the path unchanged if it reaches the root directory and it still does not exist', async () => { + const rootPath = path.resolve('/'); + vi.spyOn(fs, 'realpath').mockImplementation(async () => { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + + const result = await tryRealpath(rootPath); + expect(result).toBe(rootPath); + }); + + it('should throw an error if realpath fails with a non-ENOENT error (e.g. EACCES)', async () => { + vi.spyOn(fs, 'realpath').mockImplementation(async () => { + throw Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }); + }); + + await expect(tryRealpath('/secret/file.txt')).rejects.toThrow( + 'EACCES: permission denied', + ); + }); + }); + + describe('NoopSandboxManager', () => { + const sandboxManager = new NoopSandboxManager(); + + it('should pass through the command and arguments unchanged', async () => { + const req = { + command: 'ls', + args: ['-la'], + cwd: '/tmp', + env: { PATH: '/usr/bin' }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.program).toBe('ls'); + expect(result.args).toEqual(['-la']); + }); + + it('should sanitize the environment variables', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + PATH: '/usr/bin', + GITHUB_TOKEN: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', + MY_SECRET: 'super-secret', + SAFE_VAR: 'is-safe', + }, + policy: { + sanitizationConfig: { + enableEnvironmentVariableRedaction: true, + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.env['PATH']).toBe('/usr/bin'); + expect(result.env['SAFE_VAR']).toBe('is-safe'); + expect(result.env['GITHUB_TOKEN']).toBeUndefined(); + expect(result.env['MY_SECRET']).toBeUndefined(); + }); + + it('should allow disabling environment variable redaction if requested in config', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + API_KEY: 'sensitive-key', + }, + policy: { + sanitizationConfig: { + enableEnvironmentVariableRedaction: false, + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + // 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 () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + MY_SAFE_VAR: 'safe-value', + MY_TOKEN: 'secret-token', + }, + policy: { + sanitizationConfig: { + allowedEnvironmentVariables: ['MY_SAFE_VAR', 'MY_TOKEN'], + enableEnvironmentVariableRedaction: true, + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.env['MY_SAFE_VAR']).toBe('safe-value'); + // MY_TOKEN matches /TOKEN/i so it should be redacted despite being allowed in config + expect(result.env['MY_TOKEN']).toBeUndefined(); + }); + + it('should respect blockedEnvironmentVariables in config', async () => { + const req = { + command: 'echo', + args: ['hello'], + cwd: '/tmp', + env: { + SAFE_VAR: 'safe-value', + BLOCKED_VAR: 'blocked-value', + }, + policy: { + sanitizationConfig: { + blockedEnvironmentVariables: ['BLOCKED_VAR'], + enableEnvironmentVariableRedaction: true, + }, + }, + }; + + const result = await sandboxManager.prepareCommand(req); + + expect(result.env['SAFE_VAR']).toBe('safe-value'); + expect(result.env['BLOCKED_VAR']).toBeUndefined(); + }); + + it('should delegate isKnownSafeCommand to platform specific checkers', () => { + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + expect(sandboxManager.isKnownSafeCommand(['ls'])).toBe(true); + expect(sandboxManager.isKnownSafeCommand(['dir'])).toBe(false); + + vi.spyOn(os, 'platform').mockReturnValue('win32'); + expect(sandboxManager.isKnownSafeCommand(['dir'])).toBe(true); + }); + + it('should delegate isDangerousCommand to platform specific checkers', () => { + vi.spyOn(os, 'platform').mockReturnValue('darwin'); + expect(sandboxManager.isDangerousCommand(['rm', '-rf', '.'])).toBe(true); + expect(sandboxManager.isDangerousCommand(['del'])).toBe(false); + + vi.spyOn(os, 'platform').mockReturnValue('win32'); + expect(sandboxManager.isDangerousCommand(['del'])).toBe(true); + }); + }); + + describe('createSandboxManager', () => { + it('should return NoopSandboxManager if sandboxing is disabled', () => { + const manager = createSandboxManager({ enabled: false }, '/workspace'); + expect(manager).toBeInstanceOf(NoopSandboxManager); + }); + + it.each([ + { platform: 'linux', expected: LinuxSandboxManager }, + { platform: 'darwin', expected: MacOsSandboxManager }, + ] as const)( + 'should return $expected.name if sandboxing is enabled and platform is $platform', + ({ platform, expected }) => { + vi.spyOn(os, 'platform').mockReturnValue(platform); + const manager = createSandboxManager({ enabled: true }, '/workspace'); + expect(manager).toBeInstanceOf(expected); }, - }; + ); const result = await sandboxManager.prepareCommand(req);