Repository navigation
worker: resolve fetch() proxy env (and the rest of the env loader) from the worker's own environment - #41932
worker: resolve fetch() proxy env (and the rest of the env loader) from the worker's own environment#41932robobun wants to merge 7 commits into
Conversation
A Worker given `env` (or started after the parent touched process.env) only got a process.env JS object built from that map. The worker VM's env loader, which fetch() reads for HTTP_PROXY / HTTPS_PROXY / NO_PROXY, NODE_TLS_REJECT_UNAUTHORIZED and BUN_CONFIG_VERBOSE_FETCH and which Bun.spawn passes to children, stayed a clone of the parent's loader and was then overlaid with the launch environment again. WebWorker__create now receives the env pairs and builds the worker's loader from exactly those entries, marked as fully loaded so configure_defines overlays nothing. The worker's process.env is then created lazily by createEnvironmentVariablesMap from that loader, the same path as the main thread, so the proxy accessors write back to the worker's own map. On Windows a worker's process.env writes no longer reach the OS environment block.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughWorker creation now passes explicit environment entries as ordered pairs or derives the worker environment from parent state. Worker initialization changes how loaders and inherited environment variables are handled. New tests cover proxy, TLS, shared, nested, and child-process environment behavior. ChangesWorker environment handling
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and specifically summarizes the main change: workers now resolve fetch() proxy settings and other environment-loader behavior from their own environment. |
| Description check | |
The description clearly explains the problem, implementation, behavior changes, trade-offs, and verification results. It does not use the exact template headings, but it provides the required content … |
Full details: Description check
-
Explanation
The description clearly explains the problem, implementation, behavior changes, trade-offs, and verification results. It does not use the exact template headings, but it provides the required content in equivalent sections.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 8:00 AM PT - Oct 16th, 2026
✅ @robobun, your commit 330c47e9a498aec4046debe04f45e5e427521d8b passed in 🧪 To try this PR locally: bunx bun-pr 41932That installs a local version of the PR into your bun-41932 --bun |
|
Status Reproduced on main (1.4.3-canary, 367d939, Linux x64) with a loopback origin and a loopback proxy. A worker gets
With this branch (330c47e, merged with main at 620b50f) the worker's own environment decides in all four cases, as in Node. Open decisions for a maintainer are in the PR body notes: the |
…ned loader keeps its loaded state A worker's process.env now comes from its env loader, and node:worker_threads reads process.env.NODE_CHANNEL_FD there to decide whether process.send is a disabled stub (test-worker-unsupported-things.js). So a worker keeps the variable it was given instead of having it stripped, and a loader cloned from the parent no longer re-reads environ (which re-added the fd the main thread had already adopted) or already loaded .env files.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I checked the .isolatedCopy() removal in JSWorker.cpp — the env strings are consumed via to_utf8() on the parent thread inside WebWorker__create and then dropped by m_options.env = std::nullopt before the worker thread runs, so no StringImpl crosses threads. Also verified the extended WebWorker__create FFI signature matches parameter-for-parameter between WorkerMessagingProxy.cpp and web_worker.rs, and that the empty-env case (hasEnv true, zero pairs) hits ffi::slice's (null, 0) allowance.
Extended reasoning...
The two riskiest aspects of this diff for a Bun reviewer are cross-thread StringImpl lifetime (src/CLAUDE.md flags this explicitly) and C-ABI signature drift across the Rust/C++ boundary. Both were traced and hold: Bun::toString produces borrowing BunStrings over m_options.env's WTF strings, worker_env_loader copies bytes out via to_utf8() synchronously on the parent thread, and the backing Vector<pair<String,String>> is freed on the parent immediately after the call returns — the worker thread never sees those StringImpls. The three appended FFI parameters line up in order and type on both sides. The inline findings cover the remaining behavioral regression (nested-worker env snapshotting) and a test-quality nit; nothing further to add beyond recording what was ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
… the outer worker's env
There was a problem hiding this comment.
I re-reviewed after 71b733f and the nested-worker env leak I flagged is fixed — clone_for_worker now inherits did_load_process/default_files_loaded, for_worker_env marks every source loaded, and the new nested-worker test pins it. No further bugs found. Given the widened WebWorker__create FFI signature and the dropped .isolatedCopy() on env strings (now consumed parent-thread-only via to_utf8() before the worker thread spawns), a human pass on the cross-thread ownership is still worthwhile.
What was reviewed:
clone_for_worker/for_worker_envloaded-state flags vs. them_processEnvObject.isInitialized()gate at JSWorker.cpp:274 — the no-env nested path now clones a loader that will not re-readenviron.- FFI shape:
envPairs.size() / 2on the C++ side againstenv_pairs_len * 2on the Rust side, andbun_core::ffi::slicewith(null, 0)for an empty explicit env. options.envstring lifetime:Bun::toStringviews are copied into the loader map insideWebWorker__createon the parent thread, thenm_options.env = std::nulloptdrops the originals before any worker-thread access.jsEditWindowsEnvVarmain-thread gate and theNODE_CHANNEL_FDhoist against Node's workerMapKVStoresemantics.
Extended reasoning...
Overview
This PR reroutes a Worker's { env } option through the worker VM's bun_dotenv::Loader instead of only its process.env JS object, so native consumers (fetch() proxy resolution, NODE_TLS_REJECT_UNAUTHORIZED, Bun.spawn default env) honor the worker's own environment. It widens the WebWorker__create FFI to carry [key, value, ...] BunString pairs, adds Loader::for_worker_env and updates clone_for_worker to inherit loaded-state flags, drops the eager C++-side m_processEnvObject construction in ZigGlobalObject.cpp in favor of the lazy createEnvironmentVariablesMap path, gates jsEditWindowsEnvVar and NODE_CHANNEL_FD consumption to the main thread, and adds five tests in test/js/web/workers/worker-env.test.ts plus a fixture.
Security risks
The change touches how NODE_TLS_REJECT_UNAUTHORIZED and HTTP(S)_PROXY/NO_PROXY are resolved inside workers. The direction is toward more isolation (a worker given an empty env no longer inherits the main thread's proxy), which matches Node with NODE_USE_ENV_PROXY=1. I did not find a path where a worker's TLS-reject default becomes laxer than before without the caller explicitly passing NODE_TLS_REJECT_UNAUTHORIZED: "0" in the worker's env. for_worker_env starts reject_unauthorized: Cell::new(None), so the value is re-derived from the worker's own map on first read.
Level of scrutiny
High. The change spans a C++/Rust FFI boundary with a signature change on both sides, drops .isolatedCopy() on strings that previously crossed threads (now consumed synchronously on the parent thread — I traced that worker_env_loader runs to completion inside WebWorker__create before the thread spawn and copies bytes via to_utf8()/map.put), removes a ~35-line #if OS(WINDOWS)-branched block whose Windows behavior now flows through createEnvironmentVariablesMap, and alters per-VM env-loader semantics that many native readers depend on. A maintainer familiar with the env-loader and worker init ordering should confirm the Windows lazy path and the proxy_env_slots handling for the has_env branch (which now leaves proxy_env_slots at default rather than cloning the parent's — intentional, since the worker's env is authoritative).
Other factors
My earlier blocking finding (nested Web Worker without env leaking the launch environment because clone_for_worker left did_load_process=false) was addressed in 2d4f6db/71b733f8: clone_for_worker now inherits both flags, for_worker_env sets them to loaded, and a dedicated test asserts the inner worker sees only ONLY_IN_OUTER and not SET_AT_LAUNCH. The optional nit about run() blanking stderr on exitCode === 0 was not taken; it remains a minor test-hygiene point and does not block. Test coverage is good — five scenarios across both Worker constructors, plus child-process inheritance, nested-worker inheritance, and per-worker TLS-reject.
|
I opened #41995 for the same root (the worker's native env map built from
|
The native env map of a SHARE_ENV worker was still a clone of the parent VM's map. A Bun.spawn child of such a worker got a variable the parent had deleted from process.env and missed one the parent set at runtime. startWorkerGlobalScope now copies the store (SharedEnvStore::entries) into the pairs that WebWorker__create builds the worker's loader from. Ported from c66fc44 (#41995). An entry with an empty name or a NUL byte is left out of the loader. On Windows, jsEditWindowsEnvVar in a worker skips only the write to the OS environment. The proxy variables still reach the worker's env map, which main now updates from that function. The worker-env tests run one at a time and start Web Workers where the Worker kind does not matter, so each one stays under the default timeout on a debug build. They also cover a process launched with NODE_TLS_REJECT_UNAUTHORIZED=0 whose worker env omits it.
…n comment as it was
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/js/node/process/process.test.js— nit: the Windows skip on this worker-env test is now stale, so Windows loses coverage the change makes possible. test/js/node/process/process.test.js:273 skips because the base built a plain object for an explicit-env worker's process.env on Windows; the diff deletes that branch (ZigGlobalObject.cpp) and such a worker now gets the same windowsEnv Proxy as the main thread, whose set/defineProperty traps coerce values, throw on symbol keys and reject accessor descriptors. Fix: drop the skipIf(isWindows) (or replace it with a comment naming an observed Windows failure) so the EnvSetter semantics the change now provides on Windows are asserted there too.Why this was flagged
test/js/node/process/process.test.js:273 is
it.skipIf(isWindows)("process.env keeps Node's EnvSetter semantics inside a snapshot-env worker", ...); it starts a node:worker_threads Worker withenv: { SEEDED: "yes" }and assertsprocess.env.X = 42reads back "42", a symbol key throws TypeError, and an accessor descriptor throws ERR_INVALID_OBJECT_DEFINE_PROPERTY. On the base branch the worker's process.env on Windows wasJSC::constructEmptyObject(the removed#if OS(WINDOWS)branch in src/jsc/bindings/ZigGlobalObject.cpp), which enforces none of that, hence the skip. After the diff the worker's process.env is created lazily by createEnvironmentVariablesMap (src/jsc/bindings/JSEnvironmentVariableMap.cpp:981), which on Windows wraps the object in the windowsEnv Proxy (src/js/builtins/ProcessObjectInternals.ts:482): its set trap throws on symbol keys (line 507), and defineProperty throws $ERR_INVALID_OBJECT_DEFINE_PROPERTY for accessor descriptors (line 553). Leaving the skip means Windows never runs these assertions. No safeguard applies; this is a coverage nit, not a runtime failure.Verification: nit. Triggering condition: any Windows CI run of test/js/node/process/process.test.js, which now skips a case the merged code would pass. test/js/node/process/process.test.js:273 is
it.skipIf(isWindows)("process.env keeps Node's EnvSetter semantics inside a snapshot-env worker", ...). Nothing breaks at runtime; the base also skipped on Windows, so this is a missed-coverage nit, not a regression.
…worker A VM reads three settings from its env map at start: whether the resolver preserves symlinks (NODE_PRESERVE_SYMLINKS), the GC stress level and whether stack traces hide Bun's own frames. A worker given its own environment derived them from that environment. The resolver's directory cache is shared by every thread, so a worker whose symlink setting differed from the process's got whichever form of a path was cached first. load_extra_env_and_source_code_printer now derives these only for a VM that is not a worker. WebWorker::create copies them from the parent VM, so every worker has the process's values whatever its env holds, as before. NODE_CHANNEL_FD handling is back to the text on main: a worker returns before it. worker_env_loader uses as_chunks (clippy::chunks_exact_to_as_chunks). Tests: the node:worker_threads test rejects on 'exit', the Web Worker helpers reject on 'close', and two tests are new: the env setter semantics of a worker's process.env on every platform, and NODE_PRESERVE_SYMLINKS as a process setting.
|
On the finding outside the diff (the Also in 330c47e: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The latest push addresses the two points from my previous pass (the worker now inherits the parent's resolver preserve_symlinks and process settings in WebWorker::create, and the worker_threads test rejects on exit). Beyond the inline finding, I also checked whether seeding a restricted worker loader changes auto-install: the PackageManager singleton still takes the first VM's Transpiler-owned loader exactly as it did before this change (the worker already had its own cloned loader on the base branch), so this PR only changes which variables that loader holds, which the description already calls out as a downside.
Extended reasoning...
The change routes a Worker's env option and the SHARE_ENV store snapshot into the worker VM's native env loader via an extended WebWorker__create FFI signature, replaces the eager JS process.env build in ZigGlobalObject.cpp with lazy creation, and copies process-level settings (hide_bun_stackframes, GC level, preserve_symlinks) from the parent VM. It touches cross-thread string hand-off and the C++/Rust FFI boundary but no auth, crypto, or injection surface. Not approved because this is a multi-file native threading/FFI change with a posted inline finding and further verified findings not shown here, so a human maintainer should still look.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/VirtualMachine.rs— nit: maintainers get a dead branch and a stale comment: the early return at src/jsc/VirtualMachine.rs:4251 already leaves for every non-main VM, so the innerif self.is_main_thread()at src/jsc/VirtualMachine.rs:4274 is always true and the comment above it about workers describes a path that no longer exists. Fix: drop the inner guard and its worker comment so the NODE_CHANNEL_FD block reads as main-thread-only code, keeping the early return as the single gate.Why this was flagged
The change adds
if !self.is_main_thread() { return; }at src/jsc/VirtualMachine.rs:4251 ahead of the launch-env settings. The NODE_CHANNEL_FD block below still wraps its body inif self.is_main_thread()at src/jsc/VirtualMachine.rs:4274, with the comment at 4271-4273 saying a worker sees the same inherited variables but must not open a second endpoint. After the early return that inner condition can only be true, so the branch and the comment are dead. On the base branch the inner guard was the only thing keeping workers from adopting the channel, so it was live there. No user-visible behavior differs; this is leftover code the diff itself made unreachable.Verification: nit. Trigger: any read of this function after the PR. The diff adds at src/jsc/VirtualMachine.rs:4251-4253
if !self.is_main_thread() { return; }, so everything below runs only on the main thread. The NODE_CHANNEL_FD block still wraps its body inif self.is_main_thread() {at line 4274, which is now unconditionally true. On the base branch the inner guard was live. Nothing misbehaves at runtime.
Problem
fetch()ignores the worker'senv(HTTP_PROXY,NO_PROXY,NODE_TLS_REJECT_UNAUTHORIZED) and uses the main thread's. ABun.spawnchild gets the launch environment.options.envonly built the worker'sprocess.envobject (ZigGlobalObject.cpp:597). The env loader those readers use stayed a clone of the parent's (web_worker.rs:390).Fix
WebWorker__createbuilds the worker's loader fromoptions.env, or from theSHARE_ENVstore at worker start. Otherwise it clones the parent's loader, with no second read ofenvironor.env.process.envis built from that loader, as on the main thread.process.envand every native reader, as in Node.test/js/web/workers/worker-env.test.ts(10 tests, 7 fail on main), plus the worker and process.env suites.Background
bun_dotenv::Loader, a map filled fromenvironand.envfiles. The fetch proxy and TLS defaults,Bun.whichandBun.spawnread it.process.envobject in each native reader: one property lookup per fetch and per spawn. One loader per worker covers all readers.Downsides
envnow restricts children and lookups too:Bun.spawnwithoutenvpasses only the worker's variables, andBun.whichuses itsPATH.NODE_ENVnow picks its JSX runtime. Workers no longer re-read.env, and a runtimeNODE_TLS_REJECT_UNAUTHORIZED=0in the parent holds in later workers.process.envwrite stays in the worker. Speed and size are not measured:perf,valgrind,strace,bloatyare not installed.Notes
Decisions for the reviewer
packages/bun-types/bun.d.tssays of theenvoption ofBun.spawn: "Defaults toprocess.envas it was when the current Bun process launched." Inside a worker that is no longer true with this PR. The sentence is unchanged here until a maintainer decides. Proposed addition: "In a worker, the default is the worker's environment: itsenvoption, or the parent'sprocess.envwhen the worker started."Bun.which()and the executable lookup ofBun.spawninside a worker use the worker's ownPATH. A worker givenenv: { FOO }with noPATHfinds nothing by name (Bun.which("ls")is null,Bun.spawnSync(["ls"])still runs).jsEditWindowsEnvVarwrites to the OS environment block only on the main thread. Before, a worker without an env snapshot wrote through, and a worker with one had a plain object that never did.NODE_ENV: the worker's transpiler reads it from the worker's env, so the JSX runtime agrees with the worker's ownprocess.env.NODE_ENV. In a process launched withNODE_ENV=production, a worker withenv: {}now compiles JSX for the development runtime (main: the production runtime), and a worker withenv: { NODE_ENV: "production" }in a development process now gets the production runtime.What moves
env: { ONLY: "1" }under a parent that setHTTP_PROXYat runtime fetches through the parent's proxy, itsBun.spawnchild gets the whole launch environment, and a worker with its ownHTTP_PROXYgoes direct. With this PR: direct,ONLYalone, through the proxy. Found by an internal audit, not a user issue.SHARE_ENV: the worker's loader is a copy of the shared store taken when the worker starts (SharedEnvStore::entries(), ported from c66fc44 of Worker: build the worker's native env map from its env option, not the parent's launch environment #41995). Before, aBun.spawnchild of aSHARE_ENVworker still got a variable the parent had deleted fromprocess.envand missed one the parent set at runtime. A write through the sharedprocess.envafter the worker started reaches every thread'sprocess.envbut no thread's loader, the same as a main-thread write today.env: the cloned loader keepsdid_load_processanddefault_files_loaded. On main each worker readenvironover the clone again and re-read the default.envfiles, so a key added to.envafter launch appeared in later workers, and a proxy or TLS variable the parent set at runtime was overwritten by the launch value for the worker's native readers while the worker'sprocess.envshowed the runtime value.Bun.spawnrejects a NUL in an explicitenv. The worker'sprocess.envdoes not show it either (Node's worker shows it). A lone surrogate in a value arrives as U+FFFD.Object.keys(process.env)in the worker follows the parent's order.NODE_CHANNEL_FD: a worker's loader keeps the variable when the worker's env has it (test-worker-unsupported-things.jsputs it there on purpose). The main thread adopts the IPC channel and strips the variables before itsprocess.envexists, so a snapshot or clone never carries them. A worker inside a forked child still has noprocess.send.WorkerOptions.envis aVector<std::pair<String, String>>consumed on the parent thread, so the strings are no longerisolatedCopy()'d.What does not move: the process settings
load_extra_env_and_source_code_printer):NODE_PRESERVE_SYMLINKS, the GC stress level, andBUN_SHOW_BUN_STACKFRAMES. A worker does not read them from its own environment.WebWorker::createcopies them from the parent VM, so every worker has the process's values, as on main.env, it readsNODE_PRESERVE_SYMLINKSfrom the worker's env (HandleEnvOptionsinnode_worker.cc). Bun keeps one setting for the process, as before this PR.Per worker, counted from the diff (not measured)
startWorkerGlobalScope, one inWebWorker::create, and two byte scans per env entry.environpass on the worker thread (one map insert per variable), the default.envprobes and reads, two string copies per env entry on the parent thread, and the eager build of the worker'sprocess.envobject (it is now lazy, like the main thread's).Merge with main (620b50f)
jsEditWindowsEnvVar. The worker gate there now skips only the OS write, so a proxy variable assigned in a worker still reaches the worker's loader.Suites run on the debug build of 330c47e (Linux x64)
worker-env.test.ts10 of 10. On main 7 fail. The other 3 are guards: the nested worker, the symlink setting, and the env setter semantics, which main only lacks on Windows.worker_threads.test.ts151 of 151.process.test.jsenv tests, the worker tests intest/cli/run/env.test.ts,fetch.tls.test.ts: all pass.worker.test.ts40 of 42. Twoterminate()tests need more than the 5 s default on this debug build and pass with a longer timeout.test-worker-*.jsandtest-process-env*.js: 114 of 115.test-worker-arraybuffer-zerofill.jscallsdescribeoutside the test runner and fails the same way on main.cargo clippy -p bun_jsc -p bun_dotenvis clean. It was red on 633db06 (chunks_exact_to_as_chunksinworker_env_loader).Overlapping open PRs
new Worker(url)with no options) is a hoist of the sameJSWorker.cppblock and rebases on this in a few lines.WebWorker__createfor per-worker flags. With the loader seeded here, a per-workerNODE_USE_SYSTEM_CAcan readenv_loader.get()increate()instead of a new parameter. Worker / console / --hot: remove unsafe from web_worker, ConsoleObject, hot_reloader #40420 rewritesweb_worker.rs. Whichever lands second takes a small rebase increate().Review
envreadenvironover the outer worker's map, and a worker with its own env had a symlink setting that differed from the process's.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/workers/worker-env.test.ts