mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-12 07:59:02 +02:00
fix(evidence): stop bun's dotenv autoload from reaching the spawned command
`bin/gstack-evidence` has a `#!/usr/bin/env bun` shebang, and bun AUTO-LOADS `.env`, `.env.<NODE_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) <noreply@anthropic.com> 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
This commit is contained in:
committed by
Garry Tan
co-authored by
Claude Opus 5
parent
2010b345e6
commit
a0fd8ba529
+99
-2
@@ -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.<NODE_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<string, string> {
|
||||
const out = new Map<string, string>();
|
||||
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<string, string>, scrubbed: string[] } {
|
||||
const env: Record<string, string> = {};
|
||||
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<number> {
|
||||
let label = "default";
|
||||
const li = argv.indexOf("--label");
|
||||
@@ -189,7 +280,13 @@ async function cmdRun(argv: string[]): Promise<number> {
|
||||
let exitCode: number;
|
||||
let proc: ReturnType<typeof Bun.spawn> | 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;
|
||||
|
||||
Reference in New Issue
Block a user