dotenv: expose auto-loaded .env values as non-enumerable on process.env - #35481
dotenv: expose auto-loaded .env values as non-enumerable on process.env#35481robobun wants to merge 14 commits into
Code review found 3 important issues
Found 5 candidates, confirmed 6. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/node/child_process.ts:1023-1032 |
Bun.$ shell drops auto-loaded .env values from subprocess env and $VAR expansion |
| 🟡 Nit | src/jsc/bindings/JSEnvironmentVariableMap.cpp:733-741 |
Founding a SHARE_ENV tree makes auto-loaded .env keys enumerable on the main thread |
| 🟡 Nit | src/js/builtins/ProcessObjectInternals.ts:535-537 |
Stale comment: claims auto-loaded .env keys are absent from envMapList |
| 🟡 Nit | test/cli/run/env.test.ts:333-345 |
child_process env-inheritance test is vacuous: child re-auto-loads .env from cwd |
Annotations
Check failure on line 1032 in src/js/node/child_process.ts
claude / Claude Code Review
Bun.$ shell drops auto-loaded .env values from subprocess env and $VAR expansion
The same compensation applied here for `node:child_process` is missing for `Bun.$`: `shell.ts:252/348` set `BunShell[envSymbol] = process.env`, so line 312 always calls `parsed_shell_script.setEnv(process.env)`; `ParsedShellScript::set_env` iterates via `JSPropertyIterator`, whose every branch (JSPropertyIterator.cpp:65/83/85/88, including the Windows process.env-Proxy special case) uses `DontEnumPropertiesMode::Exclude`; and `interpreter.rs:478-479` uses the resulting `export_env` verbatim with
Check warning on line 741 in src/jsc/bindings/JSEnvironmentVariableMap.cpp
claude / Claude Code Review
Founding a SHARE_ENV tree makes auto-loaded .env keys enumerable on the main thread
Switching the SHARE_ENV seed to `DontEnumPropertiesMode::Include` fixes the direct-read regression, but `JSSharedEnvMap` reports every store entry as enumerable (`getOwnPropertyNames` adds all of `store->keys()` regardless of `mode`, and `getOwnPropertySlot` uses `slot.setValue(object, 0, ...)`). So after the first `new Worker(url, {env: SHARE_ENV})`, `Object.keys(process.env)` / `{...process.env}` on the **main thread** include auto-loaded `.env` keys again — the docs' "not enumerable" contract
Check warning on line 537 in src/js/builtins/ProcessObjectInternals.ts
claude / Claude Code Review
Stale comment: claims auto-loaded .env keys are absent from envMapList
This comment says auto-loaded `.env` keys are "correctly absent from envMapList", but after commit d667f354 `keyArray->push` runs unconditionally in the per-key loop — auto-loaded `.env` keys are now *in* `envMapList` (the comment at line 456-458 says exactly that). Only the "always-present TZ/proxy accessors" half is still accurate. The guard code is correct; just drop the "DontEnum auto-loaded .env keys and" clause so this comment doesn't contradict the sibling comment 80 lines up.
Check warning on line 345 in test/cli/run/env.test.ts
claude / Claude Code Review
child_process env-inheritance test is vacuous: child re-auto-loads .env from cwd
This test can't fail for the property it protects: `execFileSync` inherits the parent's cwd (the temp dir containing `.env`), so the child Bun process re-auto-loads `.env` itself and `process.env.AUTO_FROM_FILE` reads `'secret'` regardless of whether `normalizeSpawnArguments` passed it through the OS env block. Reverting the `child_process.ts` change (`getOwnPropertyNames` → `for..in`) leaves this test green, so the fix is effectively unguarded. Have `child.js` assert `Object.getOwnPropertyDescr