Stop inlining process.env dot-reads in Worker-thread transpiles - #34211
Stop inlining process.env dot-reads in Worker-thread transpiles#34211robobun wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
LGTM — one-line config assignment matching the established pattern, with a regression test.
What was reviewed:
- Verified the new
env.behavior = LoadAllWithoutInliningassignment matches the identical pre-configure_defines()setup in run_command.rs, test_command.rs, repl_command.rs, and bake/production.rs. - Checked ordering: set before
configure_defines()and beforeapply_standalone_runtime_flags(which already set it for the standalone-graph branch), so non-standalone workers now get the same behavior. - Test mirrors the existing main-thread "does not inline process.env" test;
bunRunthrows on non-zero exit so a worker crash would fail the test; onlybig-env.jsexceeds the 4 KiB cache threshold sonewCacheCount() == 1holds.
Extended reasoning...
Overview
Five-line addition to src/jsc/web_worker.rs (WebWorker::start_vm) setting transpiler.options.env.behavior = DotEnvBehavior::LoadAllWithoutInlining on the worker VM before configure_defines(), plus an 18-line regression test in test/cli/run/transpiler-cache.test.ts.
Security risks
None. This narrows behavior (stops inlining env values into transpiled output). No new inputs are parsed and no untrusted data is touched; it's a plain enum field assignment.
Level of scrutiny
Low-to-medium. The runtime change is a single field write in the worker VM setup path, following a pattern already used verbatim at five other VM-startup call sites (run_command.rs:1052, test_command.rs:2205, repl_command.rs:124, bake/production.rs:173, bun.js.rs:32). Placement immediately before configure_defines() matches run_command.rs exactly. The standalone-graph branch just below already reached the same state via apply_standalone_runtime_flags, so this only changes the non-standalone worker case — which is precisely the reported bug.
Other factors
- The regression test is placed directly after the existing main-thread "does not inline process.env" test and uses the same
dummyFile/bunRun/newCacheCountmachinery, so it inherits proven test infrastructure. bunRunthrows on non-zero exit, and workers keep the parent event loop ref'd by default, so the two-process assertion (firstthensecond) is deterministic.- The fully-qualified path
bun_options_types::schema::api::DotEnvBehavioris used elsewhere in the tree (e.g.jsc_hooks.rs), so it compiles without new imports. - PR description states the full transpiler-cache suite and
worker.test.tspass, and that the test fails on the unfixed build. - The bug hunting system found no issues.
|
Thanks for the review. No action items raised; CI is running on the current commit. |
|
CI triage for build 73179: the only hard failure is test-net-connect-memleak.js on alpine x64-baseline, which is pre-existing on main and unrelated to this change. The remaining failures (spawn.test.ts timeout on Windows, 30205 napi finalizer leak on asan, repl-close EPIPE on Windows, serve.test.ts on macOS) are known flaky and passed on retry. The new transpiler-cache worker test passed on all lanes. Diff is ready for review. |
|
@robobun Was this issue introduced in the Rust re-write or did it pre-exist in v1.3.14? |
|
It pre-existed. The issue was reported and reproduced on v1.3.14, and the Zig code before the rewrite had the same gap: the main-thread setup (src/bun.js.zig) set |
The main thread sets DotEnvBehavior::LoadAllWithoutInlining before configure_defines(), but the Worker-thread VM startup path never did, so workers inlined process.env.X dot-reads as string literals. Those literals were then stored in the shared on-disk runtime transpiler cache (keyed by content only), so later processes with different env values executed the first process's values. Fixes #34210
The worker transpiler inlined every launch-environ var as a process.env.X
define, so a worker spawned with env: {} still returned the real value
from a literal dot read even though enumeration/has reported the key
absent. Covered by the same LoadAllWithoutInlining fix as #34210; this
test exercises the node worker_threads env-option side of it.
39c8302 to
2f1fe79
Compare
|
Rebased onto main and added a second test in |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughWorker VM startup now disables ChangesWorker environment handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/transpiler-cache.test.ts`:
- Around line 239-240: Remove the explanatory comment at
test/cli/run/transpiler-cache.test.ts lines 239-240, leaving the existing issue
URL comment as the sole regression-test comment. In
test/js/node/worker_threads/worker_threads.test.ts lines 1423-1426, replace the
prose comment with only the issue `#34210` URL comment.
- Around line 241-242: Strengthen the second-run assertions in the transpiler
cache test by capturing newCacheCount() after invoking bunRun for the “second”
worker and asserting it remains unchanged from the first run. Keep the existing
stdout assertion, and ensure the test verifies reuse of the shared cached module
rather than merely successful execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95122bac-ab6d-4e61-a6d8-74de064b5e37
📒 Files selected for processing (3)
src/jsc/web_worker.rstest/cli/run/transpiler-cache.test.tstest/js/node/worker_threads/worker_threads.test.ts
Entries written by a pre-fix worker carry the writing process's env values as string literals, and neither input_hash nor features_hash changes with env.behavior, so a cache hit on an old entry reinstates the bug. Also assert the second worker run reuses the cache entry, and trim test comments.
There was a problem hiding this comment.
LGTM — the EXPECTED_VERSION bump to 24 addresses the stale-cache concern from my earlier review; poisoned pre-fix .pile entries are now rejected on load. The comment-cop flag on RuntimeTranspilerCache.rs:48 is a false positive: the Version 24 history entry is the same 3-line shape as Version 22 (which is what I asked for), not a workaround justification.
Extended reasoning...
Overview
Two production lines and a constant bump: WebWorker::start_vm now sets b.options.env.behavior = LoadAllWithoutInlining before configure_defines(), matching run_command.rs:1044, test_command.rs:2323, repl_command.rs:124, and bake/production.rs:172. EXPECTED_VERSION in RuntimeTranspilerCache.rs is bumped 23→24 with a history entry so pre-fix cache entries (which baked env values as string literals) are rejected. Two regression tests cover both user-visible symptoms of #34210: cross-process cache poisoning and env: {} failing to scrub literal process.env.X reads.
Security risks
None introduced. The change removes a data leak: a worker with env: {} previously exposed the parent's launch environment via inlined dot-reads even though enumeration/in reported the key absent. No new attack surface.
Level of scrutiny
Low-to-medium. The fix is a one-line behavior flag matching five sibling call sites, placed at the same point in the setup sequence (after resolver.env_loader is wired, before configure_defines()). The standalone-graph branch immediately below already set the same value via apply_standalone_runtime_flags (bun.js.rs:32), so ordering is correct. The version bump is the standard mechanism for invalidating semantically-stale cache entries.
Other factors
- My prior review's only finding (cache version not bumped → poisoned entries survive upgrade) is fully addressed in 7bca36c.
- CodeRabbit's asks (issue-URL-only regression comments; second
newCacheCount()assertion proving cache reuse) are both applied. - The outstanding comment-cop bot flag on the Version 24 history comment is a heuristic false positive — it matches the Version 22 entry's shape exactly and is a version log, not a workaround justification.
- CI failures on build #80224 (mysql tests, macOS/musl build-bun infra) are unrelated to this diff.
- Tests follow harness conventions:
bunEnvspread, concurrent pipe drain, stdout asserted before exitCode,await usingfor the spawn.
|
Thanks. All review findings are addressed: env inlining disabled for worker transpiles, cache version bumped to 24 so pre-fix entries are rejected, and the test assertions tightened. No open threads remain; ready for a maintainer. |
There was a problem hiding this comment.
LGTM — the cache-version bump addresses my earlier finding.
What was reviewed:
- Confirmed the new
LoadAllWithoutInliningassignment mirrors run_command.rs / test_command.rs / repl_command.rs / bake, and that the standalone-graph branch below may override it todisable(unchanged behavior). EXPECTED_VERSION23→24 with a history entry matching the Version 22 format; rejects pre-fix.pileentries as intended.- Both new tests follow the existing harness patterns; the transpiler-cache test now asserts
newCacheCount() === 0on the second run so cache reuse is proven.
Extended reasoning...
Overview
Four files: a one-line behavior flag in src/jsc/web_worker.rs (set env.behavior = LoadAllWithoutInlining on the worker VM's transpiler before configure_defines()), a cache-format version bump 23→24 in src/jsc/RuntimeTranspilerCache.rs, and two regression tests in test/cli/run/transpiler-cache.test.ts and test/js/node/worker_threads/worker_threads.test.ts.
Prior review
I previously flagged that without a version bump, poisoned .pile entries written by pre-fix builds would still be accepted (the cache key is (input_hash, features_hash, EXPECTED_VERSION) and env.behavior participates in none of those). That was addressed in commit 7bca36c by bumping EXPECTED_VERSION to 24 with a changelog entry in the same shape as Version 22. CodeRabbit's two nits (regression-test comment style, second newCacheCount() assertion) and the comment-cop bot's length complaint are also addressed in the current diff.
Security risks
None. The change removes an env-value leak vector (a worker with env: {} could previously read the parent's launch environment via literal process.env.X because the value was baked in at transpile time). No new attack surface.
Level of scrutiny
Low-to-moderate. The runtime change is a single enum assignment that brings the worker VM startup path in line with every other runtime entry point (run_command.rs:1044, test_command.rs:2323, repl_command.rs:124, bake/production.rs:172) — grepped and confirmed. The standalone-graph branch immediately below may override it to disable (via apply_standalone_runtime_flags), which is unchanged and correct. The cache version bump is the established mechanism for invalidating semantically-stale entries.
Other factors
Tests follow file-local conventions exactly: the transpiler-cache test mirrors the adjacent "does not inline process.env" test but routes through a Worker, and asserts newCacheCount() on both runs so a hypothetical "workers don't cache at all" regression would fail it. The worker_threads test uses bunExe/bunEnv, await using proc, concurrent stdout/stderr/exited drain, and asserts stderr/stdout before exitCode. CI failures on the latest build (mysql integration tests, macOS/musl/freebsd "step failed outside runner") are unrelated infrastructure failures.
|
Build 80594 failed on CI infrastructure: every build-bun job timed out or expired before compiling, so no tests ran (no failure annotations). The change itself is unaffected; the previous run's only test failures were known-flaky or pre-existing on main, and both test files pass locally. Needs a CI retrigger or a maintainer review. |
Fixes #34210
Repro
A module over the runtime transpiler cache minimum size, dynamically imported inside a Worker, run twice with different env values and a shared cache:
where
main.tsdoesnew Worker(...)and the worker dynamically imports a large module that readsprocess.env.MY_TEST_IDENTITY. The cached.pileentry contains the first run's value as a string literal.The same inlining also defeats
new Worker(file, { env: {} }): the worker'sprocess.envobject is the requested empty map (enumeration,in,hasOwnPropertyall agree), but a literalprocess.env.HOMEread still returns the launch value because the transpiler substituted it at parse time.Cause
The main thread sets
env.behavior = DotEnvBehavior::LoadAllWithoutInliningbeforeconfigure_defines()(src/runtime/cli/run_command.rs), and so do the test runner, repl, bake, and standalone paths. The Worker-thread VM startup path (WebWorker::start_vmin src/jsc/web_worker.rs) never did, so the worker's transpiler ran with theTarget::Bundefault ofLoadAlland baked everyprocess.env.Xdot-read into the transpiled output as aDotDefine. That output lands in the shared on-disk runtime transpiler cache, which is keyed by content and features only, so every later process with different env values got the first process's values as compile-time constants. And because the define table is built from the worker'senv_loader(cloned from the parent's launch environ) rather than theoptions.envmap installed onm_processEnvObject, an explicitenv: {}cannot scrub the literal reads.Fix
Set
LoadAllWithoutInliningon the worker VM's transpiler beforeconfigure_defines(), matching the main thread. This stops the inlining, stops poisoned cache entries from being written, and lets the plainprocess.envobject installed for an explicitenv:option be the sole source of truth for dot reads. The standalone-graph branch right below already set it viaapply_standalone_runtime_flags.Verification
test/cli/run/transpiler-cache.test.ts("does not inline process.env in Worker threads") runs the two-process repro with a shared cache dir; fails on the unfixed build (prints the first run's value), passes with the fix.test/js/node/worker_threads/worker_threads.test.ts("env: {} scrubs the launch environment from the worker's process.env") spawns a worker withenv: { ONLY: "1" }under a process that hasLAUNCH_SECRET/NODE_ENVset, and asserts literalprocess.env.LAUNCH_SECRET/process.env.NODE_ENVreads come backnull; fails before ("s3cr3t"/"production"), passes after.test/js/node/worker_threads/worker_threads.test.ts(92 tests) andtest/cli/run/transpiler-cache.test.tspass on the debug build.