diff --git a/src/env-utils.test.ts b/src/env-utils.test.ts index 28fd74881..77d634ae3 100644 --- a/src/env-utils.test.ts +++ b/src/env-utils.test.ts @@ -2,7 +2,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { WrapperConfig } from './types'; -import { getConfigEnvValue, getLowerCaseProcessEnvValue, pickEnvVars } from './env-utils'; +import { copyEnvEntries, getConfigEnvValue, getLowerCaseProcessEnvValue, pickEnvVars } from './env-utils'; function makeWrapperConfig(overrides: Partial = {}): WrapperConfig { return { @@ -149,3 +149,109 @@ describe('getLowerCaseProcessEnvValue', () => { expect(getLowerCaseProcessEnvValue('TEST_LOWERCASE_ENV_VALUE')).toBeUndefined(); }); }); + +describe('copyEnvEntries', () => { + it('copies all defined entries from source to target', () => { + const source: Record = { A: 'a', B: 'b' }; + const target: Record = {}; + copyEnvEntries(source, target); + expect(target).toEqual({ A: 'a', B: 'b' }); + }); + + it('skips entries with undefined values', () => { + const source: Record = { A: 'a', B: undefined }; + const target: Record = {}; + copyEnvEntries(source, target); + expect(target).toEqual({ A: 'a' }); + }); + + it('skips keys in excludedKeys', () => { + const source: Record = { A: 'a', B: 'b', C: 'c' }; + const target: Record = {}; + copyEnvEntries(source, target, { excludedKeys: new Set(['B']) }); + expect(target).toEqual({ A: 'a', C: 'c' }); + }); + + it('allows keys in allowKeys even when they are also in excludedKeys', () => { + const source: Record = { A: 'a', B: 'b' }; + const target: Record = {}; + copyEnvEntries(source, target, { + excludedKeys: new Set(['A', 'B']), + allowKeys: new Set(['B']), + }); + expect(target).toEqual({ B: 'b' }); + }); + + it('does not overwrite existing keys when noOverwrite is true', () => { + const source: Record = { A: 'new', B: 'new' }; + const target: Record = { A: 'original' }; + copyEnvEntries(source, target, { noOverwrite: true }); + expect(target).toEqual({ A: 'original', B: 'new' }); + }); + + it('overwrites existing keys when noOverwrite is false (default)', () => { + const source: Record = { A: 'new' }; + const target: Record = { A: 'original' }; + copyEnvEntries(source, target); + expect(target).toEqual({ A: 'new' }); + }); + + it('only copies keys matching keyPredicate', () => { + const source: Record = { OTEL_FOO: 'x', OTHER: 'y' }; + const target: Record = {}; + copyEnvEntries(source, target, { keyPredicate: (k) => k.startsWith('OTEL_') }); + expect(target).toEqual({ OTEL_FOO: 'x' }); + }); + + it('skips entries exceeding maxValueSizeBytes and calls onSkippedOversized', () => { + const bigValue = 'x'.repeat(200); + const source: Record = { SMALL: 'hi', BIG: bigValue }; + const target: Record = {}; + const skipped: Array<{ key: string; sizeBytes: number }> = []; + copyEnvEntries(source, target, { + maxValueSizeBytes: 10, + onSkippedOversized: (key, sizeBytes) => skipped.push({ key, sizeBytes }), + }); + expect(target).toEqual({ SMALL: 'hi' }); + expect(skipped).toHaveLength(1); + expect(skipped[0].key).toBe('BIG'); + expect(skipped[0].sizeBytes).toBe(200); + }); + + it('copies entries at exactly maxValueSizeBytes (boundary is exclusive)', () => { + const value = 'x'.repeat(10); + const source: Record = { V: value }; + const target: Record = {}; + copyEnvEntries(source, target, { maxValueSizeBytes: 10 }); + expect(target).toEqual({ V: value }); + }); + + it('skips entries one byte over maxValueSizeBytes', () => { + const value = 'x'.repeat(11); + const source: Record = { V: value }; + const target: Record = {}; + copyEnvEntries(source, target, { maxValueSizeBytes: 10 }); + expect(target).toEqual({}); + }); + + it('applies all filters together', () => { + const source: Record = { + OTEL_KEEP: 'ok', + OTEL_EXCLUDED: 'no', + OTEL_BIG: 'x'.repeat(200), + OTEL_EXISTING: 'old', + OTHER: 'ignored', + }; + const target: Record = { OTEL_EXISTING: 'original' }; + const skipped: string[] = []; + copyEnvEntries(source, target, { + excludedKeys: new Set(['OTEL_EXCLUDED']), + noOverwrite: true, + keyPredicate: (k) => k.startsWith('OTEL_'), + maxValueSizeBytes: 10, + onSkippedOversized: (key) => skipped.push(key), + }); + expect(target).toEqual({ OTEL_EXISTING: 'original', OTEL_KEEP: 'ok' }); + expect(skipped).toEqual(['OTEL_BIG']); + }); +}); diff --git a/src/env-utils.ts b/src/env-utils.ts index 2a3ab2785..a7e2e83a6 100644 --- a/src/env-utils.ts +++ b/src/env-utils.ts @@ -1,6 +1,76 @@ import { readEnvFile } from './github-env'; import { WrapperConfig } from './types'; +/** + * Options for {@link copyEnvEntries}. + */ +export interface CopyEnvEntriesOptions { + /** + * Keys to skip. An entry whose key is in this set is omitted unless it + * also appears in `allowKeys`. + */ + excludedKeys?: Set; + /** + * Keys that bypass `excludedKeys`. An entry whose key is in both + * `excludedKeys` and `allowKeys` is still copied. + */ + allowKeys?: Set; + /** + * When `true`, entries whose key is already present in `target` are not + * overwritten. Default: `false`. + */ + noOverwrite?: boolean; + /** + * Additional predicate applied to each key before copying. Only entries + * for which the predicate returns `true` are copied. + */ + keyPredicate?: (key: string) => boolean; + /** + * Maximum allowed value size in bytes (UTF-8). Entries whose value + * exceeds this limit are skipped; `onSkippedOversized` is called for each + * skipped entry when provided. + */ + maxValueSizeBytes?: number; + /** + * Called for each entry skipped because it exceeded `maxValueSizeBytes`. + * @param key - The environment variable name. + * @param sizeBytes - The actual UTF-8 byte length of the value. + */ + onSkippedOversized?: (key: string, sizeBytes: number) => void; +} + +/** + * Copies entries from `source` into `target` according to the given options. + * + * Entries with `undefined` values are always skipped, since they cannot be + * represented in a `Record`. + * + * This helper centralises the repeated env-filtering loop that would otherwise + * be duplicated across sbx sanitization, host passthrough, OTEL forwarding, + * and GitHub Actions env-file / additionalEnv merging. + */ +export function copyEnvEntries( + source: Record, + target: Record, + options: CopyEnvEntriesOptions = {}, +): void { + const { excludedKeys, allowKeys, noOverwrite = false, keyPredicate, maxValueSizeBytes, onSkippedOversized } = options; + for (const [key, value] of Object.entries(source)) { + if (value === undefined) continue; + if (excludedKeys?.has(key) && !(allowKeys?.has(key))) continue; + if (keyPredicate !== undefined && !keyPredicate(key)) continue; + if (noOverwrite && Object.prototype.hasOwnProperty.call(target, key)) continue; + if (maxValueSizeBytes !== undefined) { + const sizeBytes = Buffer.byteLength(value, 'utf8'); + if (sizeBytes > maxValueSizeBytes) { + onSkippedOversized?.(key, sizeBytes); + continue; + } + } + target[key] = value; + } +} + function normalizeEnvValue(value: string | undefined): string | undefined { const normalizedValue = value?.trim(); return normalizedValue || undefined; diff --git a/src/sbx-manager.ts b/src/sbx-manager.ts index 17f4dc4af..62f82f22a 100644 --- a/src/sbx-manager.ts +++ b/src/sbx-manager.ts @@ -25,6 +25,7 @@ import execa from 'execa'; import * as fs from 'fs'; import * as path from 'path'; +import { copyEnvEntries } from './env-utils'; import { logger } from './logger'; import { HOME_TOOL_SUBDIRS } from './services/agent-volumes/home-whitelist'; import { credentialEntriesUnderMountedParents } from './config/mount-policy'; @@ -79,11 +80,9 @@ export function sanitizeEnvForSbx( overrides: Record = {}, ): Record { const clean: Record = {}; - for (const [key, value] of Object.entries(process.env)) { - if (!SECRET_ENV_PATTERNS.some((p) => p.test(key))) { - clean[key] = value; - } - } + copyEnvEntries(process.env, clean, { + keyPredicate: (key) => !SECRET_ENV_PATTERNS.some((p) => p.test(key)), + }); return { ...clean, ...overrides }; } diff --git a/src/services/agent-environment/env-passthrough.ts b/src/services/agent-environment/env-passthrough.ts index f23106eca..3773c9730 100644 --- a/src/services/agent-environment/env-passthrough.ts +++ b/src/services/agent-environment/env-passthrough.ts @@ -1,4 +1,5 @@ import { MAX_ENV_VALUE_SIZE } from '../../constants'; +import { copyEnvEntries } from '../../env-utils'; import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; @@ -13,16 +14,14 @@ export function passthroughHostEnvironment(params: EnvPassthroughParams): void { if (config.envAll) { const skippedLargeVars: string[] = []; - for (const [key, value] of Object.entries(process.env)) { - if (value !== undefined && !excludedEnvVars.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) { - const valueSizeBytes = Buffer.byteLength(value, 'utf8'); - if (valueSizeBytes > MAX_ENV_VALUE_SIZE) { - skippedLargeVars.push(`${key} (${(valueSizeBytes / 1024).toFixed(0)} KB)`); - continue; - } - environment[key] = value; - } - } + copyEnvEntries(process.env, environment, { + excludedKeys: excludedEnvVars, + noOverwrite: true, + maxValueSizeBytes: MAX_ENV_VALUE_SIZE, + onSkippedOversized: (key, sizeBytes) => { + skippedLargeVars.push(`${key} (${(sizeBytes / 1024).toFixed(0)} KB)`); + }, + }); if (skippedLargeVars.length > 0) { logger.warn(`Skipped ${skippedLargeVars.length} oversized env var(s) from --env-all passthrough (>${(MAX_ENV_VALUE_SIZE / 1024).toFixed(0)} KB each):`); diff --git a/src/services/agent-environment/github-actions-environment.ts b/src/services/agent-environment/github-actions-environment.ts index 771a97d22..d1eaebd8f 100644 --- a/src/services/agent-environment/github-actions-environment.ts +++ b/src/services/agent-environment/github-actions-environment.ts @@ -5,6 +5,7 @@ import { import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; import { PROXY_ENV_VARS } from '../../upstream-proxy'; +import { copyEnvEntries } from '../../env-utils'; interface GitHubActionsEnvironmentParams { config: WrapperConfig; @@ -30,11 +31,10 @@ export function buildGitHubActionsEnvironment(params: GitHubActionsEnvironmentPa if (config.envFile) { const fileEnv = readEnvFile(config.envFile); - for (const [key, value] of Object.entries(fileEnv)) { - if (!excludedEnvVars.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) { - environment[key] = value; - } - } + copyEnvEntries(fileEnv, environment, { + excludedKeys: excludedEnvVars, + noOverwrite: true, + }); } if (config.additionalEnv) { @@ -42,11 +42,10 @@ export function buildGitHubActionsEnvironment(params: GitHubActionsEnvironmentPa // envAll, but explicit --env overrides (additionalEnv) should still be // able to set them (e.g. NO_PROXY customization). const proxyVarSet = new Set(PROXY_ENV_VARS); - for (const [key, value] of Object.entries(config.additionalEnv)) { - if (!excludedEnvVars.has(key) || proxyVarSet.has(key)) { - environment[key] = value; - } - } + copyEnvEntries(config.additionalEnv, environment, { + excludedKeys: excludedEnvVars, + allowKeys: proxyVarSet, + }); } if (environment.NO_PROXY !== environment.no_proxy) { diff --git a/src/services/agent-environment/observability-environment.ts b/src/services/agent-environment/observability-environment.ts index 9ff5e5054..d44be9f4b 100644 --- a/src/services/agent-environment/observability-environment.ts +++ b/src/services/agent-environment/observability-environment.ts @@ -1,3 +1,4 @@ +import { copyEnvEntries } from '../../env-utils'; import { SslConfig } from '../../host-env'; import { WrapperConfig } from '../../types'; @@ -13,13 +14,11 @@ export function buildOtelEnvironment(params: OtelEnvironmentParams): void { return; } - for (const [key, value] of Object.entries(process.env)) { - if (key.startsWith('OTEL_') && value !== undefined - && !excludedEnvVars.has(key) - && !Object.prototype.hasOwnProperty.call(environment, key)) { - environment[key] = value; - } - } + copyEnvEntries(process.env, environment, { + excludedKeys: excludedEnvVars, + noOverwrite: true, + keyPredicate: (key) => key.startsWith('OTEL_'), + }); } export function buildSslEnvironment(environment: Record, sslConfig?: SslConfig): void {