From a0fd8ba529a5e589bf9aad251df81592b4263b01 Mon Sep 17 00:00:00 2001 From: Connex Client Access Date: Thu, 20 Aug 2026 10:55:48 -0400 Subject: [PATCH] fix(evidence): stop bun's dotenv autoload from reaching the spawned command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bin/gstack-evidence` has a `#!/usr/bin/env bun` shebang, and bun AUTO-LOADS `.env`, `.env.` and `.env.local` from the cwd into `process.env`. The wrapper then spawned the command with no `env` override, so every command run through it inherited those variables — and a repo `.env.local` routinely holds production credentials. Two things go wrong, and the second is worse than the leak: 1. Secrets reach a child that would not otherwise have them. `npm test` run by hand in the same shell sees none of them; the same command through the wrapper sees all of them. 2. THE COMMAND UNDER TEST BEHAVES DIFFERENTLY, so the ledger certifies a run that is not the run CI performs. Observed in a Next.js repo on 2026-08-20: four tests failed 4/4 through the wrapper and passed 5/5 without it, because app code branched on env vars only the wrapper supplied. Nearly an hour went into chasing a "flake" that was the measuring instrument. The wrapper exists to record trustworthy evidence, so silently altering the environment defeats its purpose. The fix builds the child env from `process.env` minus the keys bun injected, and detection is exact rather than heuristic: verified on bun 1.3.11, a dotenv file does NOT override a variable the shell already exported (the shell's value wins). So a key whose live value equals the dotenv file's value was injected by bun, and dropping it restores the environment the user's own shell would have given the command. A key whose live value differs is genuinely the caller's and survives. `BUN_DOTENV_FILES()` mirrors bun's precedence, including that `.env.local` is skipped when NODE_ENV is "test" — scrubbing a key bun never loaded would strip a variable the caller legitimately provided. Escape hatch: GSTACK_EVIDENCE_KEEP_DOTENV=1 keeps the old behaviour. When keys are scrubbed the wrapper warns with the KEY NAMES ONLY, so the diagnostic cannot become the leak it prevents. Tests: 6 cases, mutation-verified — removing `env: spawnEnv` reddens exactly the two leak tests and restoring it gives 30/30. Every leak test asserts the scrub warning fired, because `bun test` sets NODE_ENV=test and the first version of these tests passed vacuously against a `.env.local` bun had never loaded. Co-Authored-By: Claude Opus 5 (1M context) Absorbed from PR #2652 with authorship preserved. Wave additions: a doc-comment on the ${VAR}-expansion limitation (bun expands refs, the reader compares raw text — those keys are left in the child env, failing open) and a regression pin for the unreadable-.env fail-open path with a functional DAC-override skip guard. Fixes #2624 --- bin/gstack-evidence | 101 +++++++++++++++++++++++++++++++++++++++++- test/evidence.test.ts | 93 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 2 deletions(-) diff --git a/bin/gstack-evidence b/bin/gstack-evidence index 9d72bf45b..ca25df373 100755 --- a/bin/gstack-evidence +++ b/bin/gstack-evidence @@ -34,7 +34,7 @@ * in the ledger; it cannot prove that an expected lane ever ran. */ -import { mkdirSync, openSync, writeSync, closeSync, readdirSync, statSync, unlinkSync, chmodSync } from "fs"; +import { mkdirSync, openSync, writeSync, closeSync, readdirSync, statSync, unlinkSync, chmodSync, readFileSync } from "fs"; import { join, dirname } from "path"; import { spawnSync } from "child_process"; import { appendJsonl, readJsonl } from "../lib/jsonl-store"; @@ -154,6 +154,97 @@ function openLog(logsDir: string, label: string, cmdSha: string): { fd: number; return undefined; } +/** + * Bun AUTO-LOADS `.env`, `.env.` and `.env.local` from the working + * directory into `process.env`. This file has a `#!/usr/bin/env bun` shebang, so + * every command it spawns inherits those variables — and a repo `.env.local` + * routinely holds PRODUCTION credentials. + * + * Two things go wrong, and the second one is worse than the leak: + * + * 1. Secrets reach a child process that would not have had them. `npm test` run + * by hand in the same shell sees none of this; run through the wrapper it sees + * all of it. + * 2. THE COMMAND UNDER TEST BEHAVES DIFFERENTLY, so the evidence ledger + * certifies a run that is not the run CI performs. Observed in a Next.js repo + * 2026-08-20: four tests failed 4/4 through the wrapper and passed 5/5 without + * it, because app code branched on env vars only the wrapper supplied. The + * wrapper exists to record trustworthy evidence, so silently changing the + * environment defeats its whole purpose. + * + * Verified against bun 1.3.11: a dotenv file does NOT override a variable the + * shell already exported (the shell's value wins). So a key whose live value is + * exactly the dotenv file's value was injected by bun, and dropping it restores + * the environment the user's own shell would have given the command. + * + * Escape hatch: GSTACK_EVIDENCE_KEEP_DOTENV=1 keeps the old behaviour for anyone + * who really does want the wrapper to supply .env values. + */ +const BUN_DOTENV_FILES = (): string[] => { + const nodeEnv = process.env.NODE_ENV; + // bun's documented precedence, lowest first. `.env.local` is skipped by bun + // when NODE_ENV is "test"; mirror that rather than guessing. + const files = [".env"]; + if (nodeEnv) files.push(`.env.${nodeEnv}`); + if (nodeEnv !== "test") files.push(".env.local"); + return files; +}; + +/** Minimal dotenv reader: KEY=VALUE, one per line. Quotes stripped, comments and + * `export ` prefixes tolerated. Multi-line values are not parsed — a key we fail + * to parse is simply left in the child env, which is the safe direction. + * Known limitation: bun EXPANDS ${VAR} references inside dotenv values, but this + * reader compares the raw file text, so an expanded live value never matches and + * that key is left in the child env — the pre-scrub behavior persists for those + * keys (fails open, same safe direction as above). */ +function parseDotenv(text: string): Map { + const out = new Map(); + for (const raw of text.split(/\r?\n/)) { + const line = raw.trim(); + if (!line || line.startsWith("#")) continue; + const eq = line.indexOf("="); + if (eq <= 0) continue; + let key = line.slice(0, eq).trim(); + if (key.startsWith("export ")) key = key.slice(7).trim(); + if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key)) continue; + let val = line.slice(eq + 1).trim(); + if ((val.startsWith('"') && val.endsWith('"') && val.length > 1) || + (val.startsWith("'") && val.endsWith("'") && val.length > 1)) { + val = val.slice(1, -1); + } + out.set(key, val); + } + return out; +} + +/** process.env minus the variables bun injected from the repo's dotenv files. + * Returns the scrubbed env and the KEY NAMES removed (never the values). */ +function childEnv(cwd: string): { env: Record, scrubbed: string[] } { + const env: Record = {}; + for (const [k, v] of Object.entries(process.env)) if (v !== undefined) env[k] = v; + if (process.env.GSTACK_EVIDENCE_KEEP_DOTENV === "1") return { env, scrubbed: [] }; + + const scrubbed: string[] = []; + for (const file of BUN_DOTENV_FILES()) { + let text: string; + try { + text = readFileSync(join(cwd, file), "utf-8"); + } catch { + continue; // absent or unreadable — nothing to scrub from it + } + for (const [k, v] of parseDotenv(text)) { + // Only when the live value IS the file's value. A different live value means + // the shell exported its own and bun left it alone, so it is genuinely the + // user's environment and must survive. + if (env[k] !== undefined && env[k] === v) { + delete env[k]; + if (!scrubbed.includes(k)) scrubbed.push(k); + } + } + } + return { env, scrubbed }; +} + async function cmdRun(argv: string[]): Promise { let label = "default"; const li = argv.indexOf("--label"); @@ -189,7 +280,13 @@ async function cmdRun(argv: string[]): Promise { let exitCode: number; let proc: ReturnType | undefined; try { - proc = Bun.spawn(spawnArgv, { stdin: "inherit", stdout: "pipe", stderr: "pipe" }); + const { env: spawnEnv, scrubbed } = childEnv(process.cwd()); + if (scrubbed.length > 0) { + // Names only. Printing values here would defeat the point. + warn(`scrubbed ${scrubbed.length} bun-injected dotenv var(s) from the child env: ${scrubbed.join(", ")} ` + + `(GSTACK_EVIDENCE_KEEP_DOTENV=1 to keep them)`); + } + proc = Bun.spawn(spawnArgv, { stdin: "inherit", stdout: "pipe", stderr: "pipe", env: spawnEnv }); } catch (e: any) { // Spawn failure (ENOENT on argv-direct form): record exit 127, propagate 127. exitCode = 127; diff --git a/test/evidence.test.ts b/test/evidence.test.ts index 77652806f..978866dce 100644 --- a/test/evidence.test.ts +++ b/test/evidence.test.ts @@ -314,3 +314,96 @@ describe('gstack-evidence check', () => { } }); }); + +describe('gstack-evidence run — bun dotenv autoload must not reach the child', () => { + // bun auto-loads .env / .env. / .env.local from the cwd into + // process.env, and this binary has a bun shebang, so without scrubbing every + // spawned command inherits them. That leaks production credentials into a child + // that would not otherwise have them AND changes the behaviour of the command + // being certified, which is the worse half: the ledger would vouch for a run + // that differs from the one CI performs. + // + // ⚠️ `bun test` runs with NODE_ENV=test, and bun SKIPS .env.local in test mode + // (verified on bun 1.3.11: NODE_ENV=test loads .env but not .env.local). A + // .env.local fixture here therefore proves nothing unless NODE_ENV is cleared + // for the spawn — the first version of these tests passed for exactly that + // wrong reason. Every leak test below asserts the scrub WARNING fired, so a + // fixture bun never loaded fails instead of passing silently. + + function runWith(env: Record, cmd: string) { + return spawnSync(EVIDENCE, ['run', '--label', 'envprobe', '--', cmd], { + cwd: repoDir, + env: { ...process.env, GSTACK_HOME: gstackHome, ...env }, + encoding: 'utf-8', + timeout: 60000, + }); + } + + test('a .env value is scrubbed, and the warning names the key but never the value', () => { + fs.writeFileSync(path.join(repoDir, '.env'), 'ZZ_TOKEN_PROBE="s3cret-value"\n'); + const r = run(['run', '--label', 'envprobe', '--', 'echo "saw=[${ZZ_TOKEN_PROBE:-absent}]"']); + expect(r.status).toBe(0); + expect(r.stderr).toContain('ZZ_TOKEN_PROBE'); // positive control: the scrub ran + expect(r.stdout).toContain('saw=[absent]'); + // The diagnostic must not become the leak it prevents. + expect(r.stderr).not.toContain('s3cret-value'); + expect(r.stdout).not.toContain('s3cret-value'); + }); + + test('a .env.local value is scrubbed when bun actually loads it (NODE_ENV cleared)', () => { + fs.writeFileSync(path.join(repoDir, '.env.local'), 'ZZ_LOCAL_PROBE=leaked\n'); + const r = runWith({ NODE_ENV: undefined }, 'echo "saw=[${ZZ_LOCAL_PROBE:-absent}]"'); + expect(r.stderr ?? '').toContain('ZZ_LOCAL_PROBE'); // positive control + expect(r.stdout ?? '').toContain('saw=[absent]'); + expect(r.stdout ?? '').not.toContain('leaked'); + }); + + test('.env.local is left alone under NODE_ENV=test, because bun never loaded it', () => { + // Mirrors bun's own precedence. Scrubbing a key bun did not inject would strip + // a variable the caller's shell legitimately provided. + fs.writeFileSync(path.join(repoDir, '.env.local'), 'ZZ_TESTMODE_PROBE=from_file\n'); + const r = runWith({ NODE_ENV: 'test', ZZ_TESTMODE_PROBE: 'from_shell' }, + 'echo "saw=[${ZZ_TESTMODE_PROBE:-absent}]"'); + expect(r.stdout ?? '').toContain('saw=[from_shell]'); + }); + + test('CONTROL — a var the shell exported with a different value SURVIVES', () => { + // bun does not override an already-exported var (verified on bun 1.3.11), so a + // live value that differs from the file is genuinely the user's environment. + fs.writeFileSync(path.join(repoDir, '.env'), 'ZZ_KEEP_PROBE=from_file\n'); + const r = runWith({ ZZ_KEEP_PROBE: 'from_shell' }, 'echo "saw=[${ZZ_KEEP_PROBE:-absent}]"'); + expect(r.stdout ?? '').toContain('saw=[from_shell]'); + expect(r.stderr ?? '').not.toContain('ZZ_KEEP_PROBE'); + }); + + test('GSTACK_EVIDENCE_KEEP_DOTENV=1 restores the old pass-through behaviour', () => { + fs.writeFileSync(path.join(repoDir, '.env'), 'ZZ_OPTOUT_PROBE=kept\n'); + const r = runWith({ GSTACK_EVIDENCE_KEEP_DOTENV: '1' }, 'echo "saw=[${ZZ_OPTOUT_PROBE:-absent}]"'); + expect(r.stdout ?? '').toContain('saw=[kept]'); + expect(r.stderr ?? '').not.toContain('scrubbed'); + }); + + test('no dotenv file means no scrub warning at all', () => { + const r = run(['run', '--label', 'envprobe', '--', 'echo hi']); + expect(r.status).toBe(0); + expect(r.stderr).not.toContain('scrubbed'); + }); + + test('an UNREADABLE .env fails open: evidence still runs, nothing scrubbed', () => { + const envPath = path.join(repoDir, '.env'); + fs.writeFileSync(envPath, 'ZZ_DENIED_PROBE=hidden\n'); + fs.chmodSync(envPath, 0o000); + // chmod 000 cannot create unreadability for root or CAP_DAC_OVERRIDE + // environments (reads succeed regardless) — probe functionally and skip + // rather than assert a condition the fixture couldn't create. + try { fs.readFileSync(envPath); fs.chmodSync(envPath, 0o644); return; } catch {} + try { + const r = run(['run', '--label', 'envprobe', '--', 'echo hi']); + // The scrub must skip the unreadable file and the run must still be recorded. + expect(r.status).toBe(0); + expect(r.stderr ?? '').not.toContain('ZZ_DENIED_PROBE'); + } finally { + fs.chmodSync(envPath, 0o644); // let afterEach rmSync succeed + } + }); +});