process/worker: env descriptor validation, worker execArgv policy table with per-worker --expose-gc (+3 tests, worker 74%→76%) - #34654
Conversation
Object.defineProperty(process.env, k, { value: v }) silently succeeded where
node throws ERR_INVALID_OBJECT_DEFINE_PROPERTY. process.env was a plain object
with per-key custom getters and no defineOwnProperty hook, so nothing validated
the descriptor. Workers build their env through a separate path in
ZigGlobalObject, which had the same gap.
Both paths now use a JSProcessEnvMap that rejects a descriptor which is not a
full data descriptor -- missing writable, enumerable or configurable, as node
requires.
Accessors, empty descriptors and Object.freeze are deliberately still accepted.
Node rejects all three, but test/js/node/worker_threads/worker_threads.test.ts
asserts accessors are allowed and documents the divergence as intentional, so
widening this to node's full rule is left as a separate decision.
Note JSProcessEnvMap derives from JSNonFinalObject, which forbids inline
storage, so process.env no longer pre-sizes its property slots; only
JSFinalObject gets inline slots and it is final, so a defineOwnProperty hook and
inline storage are mutually exclusive here.
Adds test-worker-process-env from Node v26.3.0, verbatim.
|
Updated 7:21 PM PT - Aug 7th, 2026
❌ @robobun, your commit c52ec62 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34654That installs a local version of the PR into your bun-34654 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Zig__GlobalObject__create is entered from Rust with no exception scope, and numeric env keys now reach JSProcessEnvMap::defineOwnProperty (a throwing path) through putDirectMayBeIndex. Declare the outermost scope in initializeWorker, check between puts, and assert nothing is pending at the end. Fixes the exception-check validation abort in worker.test.ts on the asan runner.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs, but this warrants a human look: the author explicitly flags two tradeoffs for a reviewer to weigh (loss of inline storage on process.env and a now-reachable Windows envMapList ordering hazard), and the diff also adds two [ ASAN ] ... [ CRASH ] quarantine entries to test/expectations.txt that silently drop whole-file coverage.
What was reviewed:
validateEnvPropertyDescriptoragainst Node'sEnvDefinersemantics; accessor/empty descriptors intentionally still pass through.JSProcessEnvMapsharesplainObjectSpace()viaSTATIC_ASSERT_ISO_SUBSPACE_SHARABLE, no instance fields — same shape asJSSharedEnvMap.- Exception-scope discipline in the worker init loop (
DECLARE_TOP_EXCEPTION_SCOPE+ per-iteration check +assertNoException) — descriptors defined there are always full/permissive so the hook can't reject. createErrorwithERR_INVALID_OBJECT_DEFINE_PROPERTYproduces aTypeErrormatching the vendored test'sname/code/message.
Extended reasoning...
Overview
Replaces the plain constructEmptyObject() backing process.env with a new JSProcessEnvMap : JSNonFinalObject whose only method-table override is defineOwnProperty, so partial data descriptors throw ERR_INVALID_OBJECT_DEFINE_PROPERTY to match Node. The same validator is wired into the existing JSSharedEnvMap::defineOwnProperty (SHARE_ENV path) and the worker-snapshot path in ZigGlobalObject.cpp is switched to createEmptyProcessEnvMap. Also vendors Node's test-worker-process-env.js and adds two ASAN quarantine entries to test/expectations.txt.
Security risks
None apparent. This tightens validation (rejects previously-accepted descriptors) rather than loosening it, and doesn't touch auth/crypto/permissions.
Level of scrutiny
Medium-high. process.env is on every startup path and every spawn env merge; swapping its backing class from JSFinalObject to a JSNonFinalObject subclass is a structural change with a stated perf cost (no inline storage). The C++ itself is small and follows the neighboring JSSharedEnvMap pattern closely, and 343 tests were run green — but the author is explicitly asking a maintainer to accept the inline-storage tradeoff vs. the JS-wrapper alternative, and to acknowledge the Windows Proxy ordering hazard that this change makes reachable. Those are maintainer calls, not correctness bugs.
Other factors
- expectations.txt additions: two new
[ ASAN ] ... [ CRASH ]entries quarantinetest-worker-message-port-transfer-terminate.jsandworker-transfer-terminate-stress.test.tson the ASAN lane. The comments are thorough and reference tracked issues (#34095, #34690), but per the file's own header this drops the entire files from the run — a maintainer should confirm that's acceptable and that these are genuinely pre-existing (the PR is stacked on #34424). - Duplicate PRs: the bot flagged #19382 and #31831 as covering the same ground with a near-identical approach; a human should decide which to land.
- The exception-scope handling around
putDirectMayBeIndexlooks correct: numeric keys route through the newdefineOwnPropertybut the loop only ever supplies full permissive descriptors, soassertNoException()holds; the per-iteration check is there for scope-discipline underBUN_JSC_validateExceptionChecks=1.
There was a problem hiding this comment.
I didn't find any bugs, but this needs a human look: it changes the class of process.env (JSC bindings, method-table hook, inline-storage tradeoff), the description explicitly asks a reviewer to weigh two design points (the lost inline storage and the now-reachable Windows envMapList ordering hazard), and it quarantines two ASAN tests in expectations.txt. There are also two overlapping open PRs (#19382, #31831) to coordinate with.
What was reviewed:
validateEnvPropertyDescriptorgating — accessor/empty descriptors still fall through, matching the documented Bun divergence and the existingworker_threads.test.tsaccessor test.- The new
DECLARE_TOP_EXCEPTION_SCOPEininitializeWorkerand the per-iteration exception check afterputDirectMayBeIndex— the loop only writes fully-permissive data descriptors, soassertNoException()at the end is sound. JSProcessEnvMapshape: no instance fields,STATIC_ASSERT_ISO_SUBSPACE_SHARABLE+plainObjectSpace(), so no custom subspace needed;RELEASE_AND_RETURNon the Base delegate is correct.
Extended reasoning...
Overview
The PR replaces process.env's backing object (previously a plain constructEmptyObject() / JSFinalObject) with a new JSProcessEnvMap : JSNonFinalObject whose only method-table override is defineOwnProperty, so it can reject partial data descriptors with ERR_INVALID_OBJECT_DEFINE_PROPERTY to match Node. The same validator is wired into the existing JSSharedEnvMap::defineOwnProperty (SHARE_ENV path) and the worker snapshot-env path in ZigGlobalObject.cpp. It adds Node's verbatim test-worker-process-env.js, and quarantines two ASAN worker-terminate tests in test/expectations.txt.
Security risks
None identified. The change tightens validation (rejecting descriptors that were previously silently accepted) rather than loosening it. No untrusted-input parsing, no auth/crypto surface.
Level of scrutiny
High. process.env is one of the most-touched objects in every Bun process; changing its class from JSFinalObject to a custom JSNonFinalObject subclass is a structural change with a documented perf cost (loss of inline storage) that the author explicitly flags for reviewer judgment. JSC bindings work involving method tables, subspaces, and exception-scope discipline is exactly the category the repo review guide singles out as most-blocked. The author also names a Windows ordering hazard in ProcessObjectInternals.ts that is now reachable but deliberately not fixed here.
Other factors
- Quarantined tests: two new
[ ASAN ]entries inexpectations.txt. The comments are thorough and cite tracking issues (#34095, #34690) and fix PRs, but per the repo guide, disabling coverage warrants human sign-off — especially confirming these crashes are genuinely pre-existing on main and not introduced/amplified by this PR's addedDECLARE_TOP_EXCEPTION_SCOPEin worker bootstrap. - Duplicate PRs: the bot flagged #19382 and #31831 as covering the same fix; someone should decide which lands.
- Author explicitly requests reviewer input on the inline-storage tradeoff ("If that cost is unacceptable, the alternative is validating in JS at the process.env wrapper instead") — that's a design call, not something to auto-approve past.
- Stacked on #34424, so merge order matters.
`new Worker(url, { execArgv })` accepted any array and honored almost
none of it. Match node_worker.cc (verified empirically on node v26.3.0):
- New policy table (src/runtime/cli/worker_exec_argv.rs) built from
Bun's own RUNTIME/TRANSPILER CLI param tables plus the node
env/isolate options Bun tolerates as no-ops. Unknown flags,
per-process flags (--title, ...), V8 flags (--max-old-space-size,
...), and missing required values now throw
ERR_WORKER_INVALID_EXEC_ARGV synchronously with node's exact message
format, including the "X requires an argument" and multi-flag forms.
- Validate NODE_OPTIONS from an explicitly provided worker env the way
node does: skipped when byte-identical to the parent's (so passing
process.env through never throws), positionals pass through, V8
options are tolerated, and disallowed/unknown options throw
"X is not allowed in NODE_OPTIONS".
- Honor --require/-r/--preload/--import from an explicit execArgv as
worker preloads (resolved worker-side, so a bad path errors at
runtime like node, not at construction).
- Honor --expose-gc per worker (gc() is per-global in JSC): an explicit
execArgv wins; inheriting workers chain the parent worker's value or
the process execArgv (compile_exec_argv + BUN_OPTIONS for compiled
executables). Deliberate superset of node, which rejects --expose-gc
in worker execArgv; same for --stack-trace-limit (honored via
pre_execution) and Bun-only runtime flags.
- The previous ad-hoc scanner (--no-addons/--use-system-ca/--cpu-prof)
is replaced by the same table-driven scanner, parsed once per worker
at create() and reused at start, so the honored set is always a
subset of the accepted set.
Vendor test-worker-execargv-invalid.js and
test-worker-stdio-from-preload-module.js from node v26.3.0 (byte
identical; pass with this change, fail without it, pass on real node).
Not vendored: test-worker-execargv.js needs node's exact warning text
routed through worker stderr ("(node:pid) Warning ... at
Object.<anonymous>"); test-worker-cli-options.js needs
require('internal/options') inside a fresh worker and asserts the
--expose-gc rejection this change deliberately supersets;
test-worker-eval-typescript.js depends on --input-type strictness Bun's
transpiler does not implement.
cirospaciari
changed the title
The execArgv policy table was built from RUNTIME_PARAMS_ and TRANSPILER_PARAMS_ only, so run-surface flags like --bun were rejected with ERR_WORKER_INVALID_EXEC_ARGV. Next.js forwards --bun from process.execArgv into its build workers' NODE_OPTIONS, which broke next build under --bun. Chain AUTO_OR_RUN_PARAMS into the table so the run-surface flags are accepted; unknown and per-process flags are still rejected.
The per-worker --expose-gc honoring calls JSC__JSGlobalObject__addGc from start_vm before the worker thread enters holdAPILock, and putDirectNativeFunction allocates a weak handle that asserts the lock (WeakSet::allocate, seen as a SIGABRT in the napi test_instance_data worker phase). JSLock is recursive, so the main-path caller that already holds it is unaffected. [allow size]
- table_map now chains AUTO_ONLY_PARAMS and BASE_PARAMS_ so every flag
create_exec_argv can emit into process.execArgv round-trips through
new Worker({ execArgv: process.execArgv }) (--silent, --cwd, -c, ...),
and scan_process_exec_argv consumes their values instead of treating
a --cwd argument as the first positional (which hid --expose-gc from
inheriting workers).
- a rejected flag with a required value now consumes its value token, so
the ERR_WORKER_INVALID_EXEC_ARGV message lists every invalid flag.
- reworded the NODE_OPTIONS parent-comparison comments (Rust + JSWorker)
to describe what the env_loader snapshot actually is.
- test hygiene: the fire-and-forget worker tests await exits; the
inheriting --expose-gc test uses await using + piped stderr; new tests
for --silent/--cwd round-trip, --cwd + --expose-gc inheritance, and
rejected-flag value consumption.
[allow size]
|
🤖 CI status: every lane is green except ubuntu-25.04-x64, where |
There was a problem hiding this comment.
Thanks — 52f6889 addresses everything raised earlier (AUTO_ONLY_PARAMS/BASE_PARAMS_ coverage with regression tests, rejected-flag value consumption, awaited workers, subprocess assertion shape). No new issues found this pass. Still deferring to a human for the two tradeoffs the description explicitly flags (JSProcessEnvMap dropping inline storage, and the now-reachable Windows envMapList ordering hazard), plus the hand-maintained NODE_FLAGS policy table and the two new ASAN quarantine entries in test/expectations.txt.
What was reviewed:
table_map()now chains RUNTIME_PARAMS_ + TRANSPILER_PARAMS_ + AUTO_ONLY_PARAMS + BASE_PARAMS_, matchingcreate_exec_argv's AUTO_PARAMS;--silent/--cwdround-trip and--cwd <dir> --expose-gcinheritance are covered by tests.scan_exec_argvReject branch consumes the value token; verified against the new[--max-old-space-size, 4096, --foo]test.JSC__JSGlobalObject__addGctakes JSLockHolder beforeputDirectNativeFunction; recursive on the main path.- The
--expose-gctest now awaits both workers viaonce(w, "message"); the run-surface-flags test awaits all four viaonce(w, "exit").
Extended reasoning...
Overview
This PR spans two features across 15 files: (1) a new JSProcessEnvMap JSC class so process.env gains a defineOwnProperty hook that rejects partial data descriptors with ERR_INVALID_OBJECT_DEFINE_PROPERTY (main thread, worker snapshot env, and SHARE_ENV paths), and (2) a new 546-line worker_exec_argv.rs module implementing node_worker.cc-style validation of new Worker(url, { execArgv, env: { NODE_OPTIONS } }) plus per-worker honouring of --expose-gc/--require. Supporting changes: WorkerExecArgv grew expose_gc/preloads fields (losing Copy), execArgv parsing moved from start_vm to create(), JSC__JSGlobalObject__addGc acquires the JSLock, and two ASAN worker-terminate tests are quarantined in test/expectations.txt with tracked issue numbers.
Security risks
None identified. The new validation tightens what worker construction accepts; it rejects with a typed error rather than executing anything. NODE_OPTIONS tokenization reuses append_options_env (the existing BUN_OPTIONS parser) rather than hand-rolling. No new untrusted-input parsing reaches allocation or filesystem paths.
Level of scrutiny
High — this warrants human review, not bot approval. It changes user-visible behavior on a public API (new Worker now throws ERR_WORKER_INVALID_EXEC_ARGV where it previously accepted silently), introduces a ~100-entry hand-maintained NODE_FLAGS table encoding Node's per-flag worker/env policy (a compat-taxonomy design decision), replaces process.env's object class with a documented startup-perf tradeoff, and adds two ASAN quarantine entries. The PR description itself asks a reviewer to weigh two specific tradeoffs.
Other factors
Earlier review rounds found a real regression (Bun's own process.execArgv failing validation because table_map() covered a strict subset of AUTO_PARAMS); it took two follow-up commits to fully close. Commit 52f6889 addresses every prior finding, each with a targeted test, and this run's bug hunt found nothing new — including re-checking the --stack-trace-limit doc claim and the --expose-gc test's worker cleanup, both ruled out. The change is well-tested (3 vendored Node tests + ~160 lines of Bun-owned tests), but the scope, the explicit reviewer-should-weigh items, and the expectations.txt additions put it outside what should merge without a maintainer look.
|
🤖 Update on the onGC wedge documented above: it has spread beyond this PR. |
There was a problem hiding this comment.
No new findings this pass, but this touches enough JSC/worker-lifecycle surface (new JSProcessEnvMap method-table class, worker-VM init with JSLockHolder/exception-scope changes, 600-line execArgv policy module, Windows env-proxy trap rework) that it's worth a human look before landing.
What was reviewed:
JSProcessEnvMap/JSSharedEnvMapdefineOwnProperty/preventExtensionshooks — descriptor validation ordering, symbol-key handling, value stringification, RELEASE_AND_RETURN placement.scan_process_exec_argv's process-globalLazyLock— initializer only runs on the main thread (nested workers take theSome(parent_worker)branch), soVirtualMachine::get()is safe there.collect_process_exec_argv_tokensnormalization vs. bun_clap arity (One/Many vs OneOptional),--prefixed values, NODE_SHORT_ALIASES handling.- Windows
definePropertytrap: define-before-bookkeeping ordering,envMapListpredicate parity withsettrap, string-coercion of accessor reads beforeeditWindowsEnvVar.
Extended reasoning...
Overview
This PR adds node-parity process.env descriptor validation and a worker execArgv policy table. It touches 19 files: a new JSProcessEnvMap C++ class with defineOwnProperty/preventExtensions method-table hooks (plus the same on JSSharedEnvMap); a new 632-line worker_exec_argv.rs module implementing the flag-policy table, argv re-parser, and NODE_OPTIONS validator with two extern-C entry points; worker-VM init changes in web_worker.rs/ZigGlobalObject.cpp (per-worker --expose-gc, JSLockHolder in addGc, top-level exception scope during env population); the Windows windowsEnv proxy defineProperty trap rework; and process.execArgv construction moved onto the shared token builder.
Security risks
None identified. The execArgv/NODE_OPTIONS validators only classify tokens against a static table; they don't execute or resolve anything. The env-map hooks tighten validation (reject where node rejects) rather than loosen it. No auth/crypto/permissions surface.
Level of scrutiny
High. This is not a simple or mechanical change: it adds a JSC class with method-table overrides (subspace sharing, structure creation), changes worker-VM startup ordering (JSLockHolder, exception scopes at global creation), and introduces a substantial parsing module whose correctness depends on matching both bun_clap's arity semantics and node's option-parser behavior. The PR went through ~20 review rounds that caught real bugs (including 🔴-severity ones around glued short-flag handling, OneOptional arity, and the Windows trap ordering), which confirms the surface is subtle.
Other factors
All prior inline findings are marked resolved and the current diff reflects the fixes (envMapList predicate, string-coercion in the Windows trap, subprocess-isolated tests, One|Many-only TAKES_VALUE, rejected-flag value-token consumption, NODE_SHORT_ALIASES in table_map). The bug-hunting system found nothing new this run. Test coverage is thorough (14 new execArgv tests, descriptor-matrix tests across all three env-map variants, 3 vendored upstream tests). The earlier ride-along deletions and VM.rs change have dropped out of the diff after the merge with main, so those findings are moot. Given the scope and the JSC/threading touchpoints, deferring to a human reviewer rather than auto-approving.
cpu-prof.test.ts: keep both the worker --cpu-prof-name inheritance test and main's PATH_MAX error tests. No-Verification-Needed: merge-conflict resolution in a test file only; CI verifies the merged tree
There was a problem hiding this comment.
No new findings this pass; all prior inline comments are resolved. Deferring to a human reviewer given the scope — this is a 300-file stacked change reaching the HTTP/1 parser's Transfer-Encoding handling (request-smuggling surface), per-SNI client-certificate policy and a new fetch TLS session cache in openssl.c, plus the HTMLRewriter async-suspension fork switch and a CompressionStream rewrite alongside the titled worker execArgv / process.env work.
What was reviewed:
JSProcessEnvMap::defineOwnProperty/preventExtensionsand the Windows proxy trap against node's EnvDefiner semantics; the accessor divergence is preserved and the value is stringified beforeBase::defineOwnProperty.getTransferEncodingrework: chunked-must-be-last is enforced per token across multiple TE fields,multipleCodingsonly tightens Bun.serve, anduseLenientTransferEncodingclears only the coding-shape verdict (TE+CL conflict still rejects).- The fetch session cache: sink is armed only after
checkServerIdentitypasses and is scoped to the per-SSLConfig context, so an unverified session is not laundered into a later strict caller.
Extended reasoning...
Overview
The PR title describes worker execArgv policy and process.env descriptor validation, but the diff is a 300-file, ~856 KB stacked change on #34424. Beyond the titled work (JSEnvironmentVariableMap.cpp, ProcessObjectInternals.ts, worker_exec_argv.rs, web_worker.rs, ZigGlobalObject.cpp), it carries: a rework of uWS HttpParser.h Transfer-Encoding parsing and maxHeaderSize accounting plus a node:http flood-prevention spill/replay path through JSNodeHTTPServerSocket.cpp; a new client-side TLS session cache (session_cache.rs, us_ssl_set_session_sink in openssl.c) and per-SNI client-certificate policy (us_ssl_ctx_set_sni_policy); an HTMLRewriter async-handler suspension redesign switching lol-html to the oven-sh fork with a new HTMLRewriterSink; a native CompressionStream/DecompressionStream replacing the JS builtin path; substantial node:http2 changes (perf_hooks entries, END_STREAM-on-final-DATA, session close grace); node:v8 GCProfiler/isStringOneByteRepresentation; napi external-string and NapiRef::ref semantics; JS printer/parser inlining fixes; large V8-shim additions; and a broad dead-code sweep across FFI sys crates and codegen.
Security risks
Several hunks sit directly on request-smuggling and TLS trust boundaries. HttpParser.h changes how multiple Transfer-Encoding headers and chunked-not-last are classified, adds a useLenientTransferEncoding bit wired from node:http's insecureHTTPParser/httpValidation, and switches maxHeaderSize to llhttp-style byte accounting with a framing slack constant — all of which change what a proxy/backend split can disagree on. openssl.c adds per-serverName verify_mode reapplication and a distinct session-id context so a session established under one SNI entry cannot be resumed under another (skipping client auth); the fetch session cache keys on (host, port, proxy_auth_hash) per HTTPContext and only inserts after the native identity check passes. These look correct on read-through, but they are exactly the class of change REVIEW.md flags for human sign-off.
Level of scrutiny
High. The titled worker/env work by itself is medium-risk and well-tested (three vendored node tests, worker_threads.test.ts at 112/0, Windows-specific bookkeeping tests), but it cannot be separated from the stacked base here, and the base reaches the HTTP parser, TLS handshake path, GC observation, and a vendored-dependency fork switch. This is well past the "simple, mechanical, self-contained" bar for auto-approval.
Other factors
Every prior inline finding from earlier passes (worker_exec_argv token pairing, env descriptor edge cases, Windows defineProperty trap ordering/predicate, expectations.txt cleanup, dead JSC__VM__clearHasTerminationRequest) is marked resolved, and the bug-hunting system found nothing new this run. The PR description accurately documents the intentional accessor divergence and the #35182 cherry-pick. Given no outstanding findings but very high scope, deferring once to a human reviewer is the right call rather than approving or staying silent.
…-descriptor-validation No-Verification-Needed: comment-only conflict resolution (duplicate SAFETY comment wording)
There was a problem hiding this comment.
No new findings this pass. This is a substantial change — a new JSProcessEnvMap JSC class replacing the plain-object process.env, a 600+ line worker execArgv policy module with a hand-maintained NODE_FLAGS table, new Rust↔C++ FFI entry points, and behavioral tightening (previously-accepted defineProperty/execArgv/NODE_OPTIONS inputs now throw) — so it warrants a human look.
What was reviewed:
- All three env-map variants (
JSProcessEnvMap,JSSharedEnvMap, and the WindowswindowsEnvproxy) route through the sharedvalidateEnvPropertyDescriptorand each fails[[PreventExtensions]]. scan_process_exec_argv'sLazyLockcache is process-global but only reads process-constant argv/BUN_OPTIONS, so the per-VM/worker state rule doesn't apply.- The
JSLockHolderadded inJSC__JSGlobalObject__addGcand the top exception scope in worker env init cover the two new call sites reached before the worker thread holds the API lock / has a scope. - Prior rounds' Windows
defineProperty-trap fixes (envMapList predicate, post-define string coercion, subprocess test) are present at HEAD; the earlierexpectations.txt/VM.rshunks are no longer in the diff after the merges.
Extended reasoning...
Overview
This PR spans four layers: (1) a new JSProcessEnvMap JSC class in JSEnvironmentVariableMap.cpp that replaces the plain JSFinalObject backing process.env so defineOwnProperty/preventExtensions can be hooked for node parity, applied to the ordinary map, the worker snapshot map (ZigGlobalObject.cpp), and JSSharedEnvMap; (2) the Windows windowsEnv proxy defineProperty trap reordered and hardened; (3) a new src/runtime/cli/worker_exec_argv.rs implementing node's worker execArgv/NODE_OPTIONS policy over Bun's AUTO_PARAMS plus a large hand-curated NODE_FLAGS table, wired through two new extern "C" validators called from JSWorker.cpp and a parse_worker_exec_argv hook consumed in web_worker.rs; (4) process.execArgv construction moved onto a shared collect_process_exec_argv_tokens normalizer so the inherit-path scan and the JS-visible array agree. Roughly 1,200 lines added across 18 files, plus 3 vendored upstream node tests and ~350 lines of new in-tree tests.
Security risks
Low. The new validation paths are strictly tightening — inputs that used to be silently accepted now throw ERR_WORKER_INVALID_EXEC_ARGV / ERR_INVALID_OBJECT_DEFINE_PROPERTY. No new privilege, no new filesystem/network surface. The NODE_OPTIONS validator only inspects an explicit worker env value (skipped when byte-equal to the parent's), so it can't be used to reject an inherited environment. The FFI boundary passes borrowed WTF::StringImpl* arrays with a length and null-checks each entry.
Level of scrutiny
High. This is not a mechanical change: it swaps the concrete class of a hot, process-lifetime object (process.env) from an inline-capacity JSFinalObject to a JSNonFinalObject subclass (the diff comment acknowledges the loss of inline slots); it introduces a large policy table whose correctness is empirical ("verified against node v26.3.0") and will need maintenance as node's option set evolves; it changes user-visible behavior in ways that could break tooling that forwards arbitrary flags into worker execArgv/NODE_OPTIONS; and it touches worker startup ordering (--expose-gc install, exception-scope placement, JSLockHolder) where mistakes manifest as debug-only aborts or races. All of those are areas where a maintainer should sign off on the design choices, particularly the NODE_FLAGS accept/reject/env split and the deliberate accessor-descriptor divergence from node.
Other factors
The PR has been through many review iterations — every prior inline finding (Windows trap predicate, accessor-read coercion, vacuous-test shape, stale bookkeeping, dead extern) is marked resolved and the corresponding fixes are visible at HEAD. This run's bug hunt found nothing new. Test coverage is thorough for the surface it targets (descriptor matrix, freeze/seal, symbol keys, SHARE_ENV variant, glued/chained short flags, --expose-gc inherit chain, NODE_OPTIONS glued forms). It is stacked on #34424, so some of the surrounding web_worker.rs/Worker.cpp context belongs to the base branch. Given the scope and the number of judgment calls encoded in the policy table, deferring to a human reviewer rather than auto-approving.
Resolve the process.env overlap with the merged process compat work (#31831): main's JSEnvironmentVariableMap and windowsEnv implementations supersede the parallel JSProcessEnvMap machinery from this branch. Keep this branch's SHARE_ENV descriptor tightening by porting it onto main's JSSharedEnvMap (full data descriptor required, symbol keys throw after validation, preventExtensions fails), and keep the JSLockHolder in addGc for per-worker --expose-gc.
|
🤖 Adopted. Latest at 945f6c3: symbol-key handling aligned across all env write paths — |
…-descriptor-validation
main's byte-search source lint rejects scalar iter().position loops. No-Verification-Needed: one-line lint conformance; the source-lint test passes locally
After the main merge took #31831's design, initializeWorker's Windows branch stored a bare object, so a worker's process.env (explicit env or the no-option snapshot) had no case-insensitivity and no set/defineProperty validation; test-worker-process-env.js failed with a missing ERR_INVALID_OBJECT_DEFINE_PROPERTY on both Windows lanes. The proxy wrap is factored out of createEnvironmentVariablesMap and reused with the OS-env sink disabled so worker writes stay thread-local like node.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/JSEnvironmentVariableMap.h:44-47— The merge with main dropped thepreventExtensionsoverride fromJSEnvironmentVariableMap(and never added a trap to the WindowswindowsEnvProxy) — onlyJSSharedEnvMaphas it (JSEnvironmentVariableMap.cpp:613). So on POSIX main-thread and Windows,Object.preventExtensions(process.env)succeeds (Node throws) and a caughtObject.freeze(process.env)leaves the env non-extensible with the wrong error code, contradicting the PR's own "Both env maps fail [[PreventExtensions]]" claim. Addstatic bool preventExtensions(JSObject*, JSGlobalObject*) { return false; }toJSEnvironmentVariableMapand apreventExtensionstrap returningfalsetowindowsEnv.Extended reasoning...
What the bug is
The PR description states "Both env maps fail
[[PreventExtensions]]so freeze/seal throw at that step (plain TypeError, no code) and the env stays extensible", and the timeline confirms commit 490b699 implemented this for bothJSEnvironmentVariableMap(POSIXprocess.env) andJSSharedEnvMap(SHARE_ENV workers). But after the 2026-08-07 merge with main — which brought in #31831'sJSEnvironmentVariableMapimplementation — the merge only ported thepreventExtensionsoverride toJSSharedEnvMap. Robobun's own merge summary ("preventExtensions fails so freeze/seal throw and the map stays extensible" — listed only under "SHARE_ENV descriptor tightening, ported onto main's JSSharedEnvMap") confirms the omission.JSEnvironmentVariableMap's header at lines 44-47 declaresput/putByIndex/defineOwnProperty/deletePropertybut notpreventExtensions, soCREATE_METHOD_TABLE(JSEnvironmentVariableMap)inheritsJSNonFinalObject::preventExtensions, which marks the structure non-extensible and returnstrue. The WindowswindowsEnvProxy inProcessObjectInternals.tshas nopreventExtensionstrap either (grep confirms zero hits), so the default trap forwards to the plaininternalEnvtarget and succeeds.Step-by-step proof
On POSIX main-thread (or Windows via the proxy),
process.envis aJSEnvironmentVariableMap(seecreateEnvironmentVariablesMapin JSEnvironmentVariableMap.cpp / the#if OS(WINDOWS)branch that wraps a plain object in thewindowsEnvProxy):Object.preventExtensions(process.env)→JSObject::preventExtensions(inherited) marks the structure non-extensible and returnstrue. Node throwsTypeError: Cannot prevent extensions; here it silently succeeds.Object.isExtensible(process.env)now returnsfalse(Node:true).try { Object.freeze(process.env) } catch {}→Object.freezefirst calls[[PreventExtensions]]→ succeeds (env now non-extensible), then walks own keys calling[[DefineOwnProperty]]with{writable:false, configurable:false}. The first key hits the newJSEnvironmentVariableMap::defineOwnPropertyoverride, which throwsERR_INVALID_OBJECT_DEFINE_PROPERTY("only accepts a configurable, writable, and enumerable data descriptor"). The user catches it. Node instead throws a plainTypeError: Cannot freeze(no.code) at the[[PreventExtensions]]step, and the env stays extensible afterwards.- After step 2,
process.env.NEW_VAR = 'x'now fails (silently in sloppy mode, throws in strict mode) because the structure is non-extensible — state corruption on the common main-thread path that Node does not exhibit.
On Windows the same sequence goes through the
windowsEnvProxy: nopreventExtensionstrap → default trap callsReflect.preventExtensions(internalEnv)on the plain-object target → succeeds → same divergence.Why nothing prevents it
- Grep of
JSEnvironmentVariableMap.cppforpreventExtensionsreturns exactly one hit at line 613, insideJSSharedEnvMap's class body.JSEnvironmentVariableMaphas no override. git show 45eda514(main's #31831) confirmsJSEnvironmentVariableMapwas introduced withoutpreventExtensions; the merge commit 3bb4d69 did not add it.ProcessObjectInternals.tshas zeropreventExtensions/isExtensibletraps.- The only test coverage (worker_threads.test.ts:478-479,
Object.freeze(process.env)…out.extensibleAfterFreeze = Object.isExtensible(process.env)) runs inside a SHARE_ENV worker child, so it exercisesJSSharedEnvMapand passes.process.test.jshas zerofreeze/preventExtensions/isExtensibleassertions on the main-thread map.
Why this is not a mere description mismatch
The rubric's "PR-description mismatches are
nitby default" exception does not apply: the code is clearly the code that's wrong — it produces incorrect behavior (accepts[[PreventExtensions]], leaves the env non-extensible after a caught freeze, throws the wrong error class) independent of what the description says. This is the PR's headline feature ("env descriptor validation", theObject.freeze / seal / preventExtensionsrow in the description table), the branch had it right pre-merge, and the merge silently dropped it for 2 of 3 parallel implementations. REVIEW.md: "Fix the whole class in the same PR — parallel switch arms, … POSIX/Windows branches" and "Cover the variant matrix, not just the repro".How to fix
Two changes, both one-liners:
- In
JSEnvironmentVariableMap.h, add alongside the existing method-table overrides (and mirror it in the .cpp if defined out-of-line, matchingJSSharedEnvMap's in-class definition at line 613):static bool preventExtensions(JSC::JSObject*, JSC::JSGlobalObject*) { return false; }
- In
ProcessObjectInternals.tswindowsEnv, add a Proxy trap:preventExtensions() { return false; },
And add a main-thread pin in
process.test.js(spawned subprocess, all platforms) assertingObject.preventExtensions(process.env)throws,Object.isExtensible(process.env)staystrueafter a caughtObject.freeze, and the freeze error has no.code.
…Windows proxy The main merge took #31831's JSEnvironmentVariableMap, which lacks the preventExtensions override 490b699 had added (only JSSharedEnvMap kept it), and the windowsEnv Proxy never had the trap. Object.preventExtensions succeeded where node throws, and a caught Object.freeze left process.env non-extensible with the wrong error. Both now return false like JSSharedEnvMap, pinned in-process across platforms.
…; drop a redundant forward decl
…-descriptor-validation ErrorCode.ts: keep both new codes. node_process.rs: keep this branch's design - the inline execArgv parser main re-added was deliberately replaced by worker_exec_argv::collect_process_exec_argv_tokens. No-Verification-Needed: merge resolution restoring the branch's CI-green design; CI verifies the merged tree
…-descriptor-validation node_process.rs: keep this branch's design again - the inline parser lives in worker_exec_argv here. No-Verification-Needed: merge resolution restoring the branch's CI-green design; CI verifies the merged tree
…tforms JSSharedEnvMap::put silently accepted symbol keys via Base::put while its defineOwnProperty (and node, the regular map, the Windows set trap) throw the conversion TypeError; the two write hooks on one object disagreed. The windowsEnv defineProperty trap checked the symbol key before the descriptor, reporting the plain TypeError where POSIX and node report ERR_INVALID_OBJECT_DEFINE_PROPERTY for an invalid descriptor. Pinned via the SHARE_ENV subprocess matrix (symbolSet) and the descriptor-first order in process.test.js.
There was a problem hiding this comment.
I reviewed this PR again at 945f6c3 and found no new issues — the symbol-key ordering finding from the previous pass is addressed (JSSharedEnvMap::put now throws on symbols, and the Windows defineProperty trap validates the descriptor before the key). Given the scope — a new ~630-line worker execArgv policy module with Rust↔C++ FFI, worker-thread init changes (JSLockHolder in addGc, expose_gc plumbing), and platform-gated Windows env-snapshot wrapping — a human sign-off is still warranted.
Checked this pass: the three preventExtensions sites (JSEnvironmentVariableMap, JSSharedEnvMap, windowsEnv Proxy) plus the process.test.js pin are all present; scan_process_exec_argv's LazyLock cache is process-constant so per-worker staleness isn't a concern; Bun__Worker__validateExecArgv's reinterpret_cast<WTF::StringImpl* const*>(execArgv.begin()) is guarded by the static_assert(sizeof(WTF::String) == sizeof(WTF::StringImpl*)); the Windows worker-snapshot path uses assertNoException on seed puts (JSString values, OOM-only).
Extended reasoning...
Overview
This PR spans 18 files across Rust (worker_exec_argv.rs new, web_worker.rs, jsc_hooks.rs, node_process.rs, VirtualMachine.rs), C++ (JSEnvironmentVariableMap.{cpp,h}, ZigGlobalObject.cpp, JSWorker.cpp, Worker.cpp), builtin JS (ProcessObjectInternals.ts), and tests. It adds Node-parity process.env descriptor validation across all three env-map implementations (POSIX exotic, SHARE_ENV, Windows Proxy), a worker execArgv/NODE_OPTIONS policy table with per-worker --expose-gc, and a shared process.execArgv token normalizer.
Security risks
Low. The new validation paths reject more inputs than before (fail-closed). NODE_OPTIONS tokenization reuses the existing append_options_env quote-aware parser; no new shell/exec surface. The FFI boundary passes WTF::StringImpl* arrays read-only with a size assertion. No auth/crypto/permissions code touched.
Level of scrutiny
High. This is not a mechanical change: it introduces a 630-line policy module derived from Node's option tables, rewires worker-thread init to parse execArgv at create() time (storing results on WebWorker for later start_vm use), adds a JSLockHolder to addGc because the new call site runs before the worker holds the API lock, and platform-gates a new Windows worker-env-snapshot proxy wrap. The WorkerExecArgv struct changed from Copy to Clone (it now owns a Vec), and scan_process_exec_argv caches per-process in a LazyLock. Each of these is individually reasonable but the interaction surface (worker init ordering, cross-thread state, platform branches) is exactly where subtle bugs hide.
Other factors
The PR has been through ~19 iterations; every prior finding I raised (preventExtensions merge regression, stale test title, redundant forward decl, byte-search lint, symbol-key ordering) is addressed at HEAD, with tests pinning each fix. Test coverage is thorough (17 new execArgv test cases, cross-platform env-descriptor matrix, SHARE_ENV subprocess). The vendored test-worker-execargv-invalid.js skip-passes (documented in the description); its cases are pinned in worker_threads.test.ts. A maintainer (cirospaciari) has been resolving threads. Given the breadth, a human should confirm the NODE_FLAGS policy table entries and the Windows worker-snapshot proxy wrap before merge.
|
🤖 Adopted. Latest at c52ec62: the regular map's put path now uses node/V8's exact "Cannot convert a Symbol value to a string" (the last of five write paths to align), and the inherit-path preload scope is documented honestly — an inheriting worker does not re-run CLI |
The validator gated the pop on the token not starting with a dash, so NODE_OPTIONS '--redirect-warnings --no-warnings' failed with 'requires an argument' while the execArgv sibling (and node's parser) consume the next token as the value. Drop the gate so the two validators agree.
…path; document the inherit-path preload scope The fifth sibling still said 'Cannot convert a symbol to a string'; node/V8 and the other four write paths say 'Cannot convert a Symbol value to a string'. The parse_worker_exec_argv doc claimed the parent VM carries preloads for inheriting workers; only cpu-prof has that fallback, so say plainly that inherit does not re-run CLI -r and widening is a behavior decision. PR description scoped the round-trip claim the same way.
There was a problem hiding this comment.
I reviewed this PR again after c52ec62 and found no new issues — the last three nits (regular-map Symbol message, NODE_OPTIONS required-value dash gate, inherit-path preload doc/claim) are all addressed. Given the scope — a new 560-line execArgv policy subsystem, JSC method-table overrides across three process.env implementations, worker-init lifecycle changes (JSLockHolder, execArgv parse moved to create()), and Windows-gated Proxy/C++ paths — a human look is still worthwhile.
What was reviewed:
preventExtensionsnow on all three env maps (regular/shared C++ + windowsEnv Proxy) with the restoredprocess.test.jspin.- All five symbol-key write paths agree on validation order and the exact V8 message;
JSSharedEnvMap::putno longer silently accepts symbols. scan_exec_argvand the NODE_OPTIONS validator now agree on required-value token consumption (unconditional pop), pinned by the new dash-value test.- Inherit vs. explicit execArgv preload divergence is now documented as intentional in
VirtualMachine.rsand the PR description; the round-trip claim is scoped to validation/--expose-gc.
Extended reasoning...
Overview
Two Node-compat features across 18 files: (1) process.env defineOwnProperty/preventExtensions validation on all three backing implementations (JSEnvironmentVariableMap, JSSharedEnvMap, the Windows windowsEnv Proxy) plus the Windows worker-snapshot path via a new wrapInWindowsEnvProxy; and (2) a worker execArgv/NODE_OPTIONS policy validator (src/runtime/cli/worker_exec_argv.rs, ~560 lines) driven from JSWorker.cpp, with per-worker --expose-gc and --require honoring wired through web_worker.rs. process.execArgv re-parsing is unified with the worker validator via collect_process_exec_argv_tokens(). ~340 lines of new tests in worker_threads.test.ts plus three vendored upstream files (one skip-passes by design, noted in the description).
Security risks
Low. Nothing auth/crypto/permissions-adjacent. The new validators parse user-supplied execArgv/NODE_OPTIONS strings but only to reject — no shell, no path resolution beyond what preloads already did, and preloads via explicit execArgv were already honorable via the preload option. FFI surface (Bun__Worker__validateExecArgv/...NodeOptions) copies WTF strings into owned Vec<u8> before parsing, so no borrowed-slice lifetime hazard. transferToWTFString() consumes the Rust-allocated BunString on the reject path, so no leak.
Level of scrutiny
High. This adds JSC method-table overrides (preventExtensions, defineOwnProperty on JSSharedEnvMap), reorders worker-init (execArgv parse moved from start_vm to create(), JSLockHolder added around addGc), introduces a hand-rolled argv re-parser that must agree with bun_clap's short-flag/glued/chained forms, and has substantial #if OS(WINDOWS) branching. The PR has been through 24 iterations; earlier rounds caught a merge-dropped preventExtensions override, sibling-divergent validators, and a JSLockHolder-missing assertion abort — all fixed, but that history argues for a human pass on the final shape.
Other factors
All prior automated findings (including the three from 2026-08-08) are resolved at HEAD c52ec62, and this run's bug hunt found nothing. Test coverage is thorough (variant matrices for glued/chained short flags, both validators pinned to agree, SHARE_ENV descriptor validation in a subprocess, freeze/isExtensible pinned on the regular map). The one open behavior decision — inherit-path workers not re-running CLI -r where node does — is now explicitly documented as deferred rather than claimed as parity. The PR is stacked on #34424 and carries a cherry-pick of #35182; a maintainer should confirm the base-branch CI red mentioned by robobun (!exception() termination race) is indeed unrelated.
3 participants
process.envdescriptor validation and workerexecArgvhandling, verified against the node v26.3.0 binary. Adds 3 vendored upstream tests (2 active;test-worker-execargv-invalid.jsskip-passes under Bun'snode_without_node_optionsgate, and its cases are pinned by the in-tree worker_threads tests instead). Stacked on #34424. Also cherry-picks #35182 (deletes the three flaky--expose-gcGC-observation node tests) plus drops their staleexpectations.txt/expected-durations.jsonentries.process.env defineProperty validation
Object.defineProperty(process.env, k, { value: v })silently succeeded where node throwsERR_INVALID_OBJECT_DEFINE_PROPERTY.process.envhad nodefineOwnPropertyhook; workers build their env through a separate path (ZigGlobalObject.cpp) with the same gap, so the fix covers both.{value: 42}(partial){}emptyObject.freeze/seal/preventExtensionsThe accessor row is a documented maintainer decision (
worker_threads.test.tsdeliberately asserts accessors are allowed); this narrows to the uncontroversial half rather than silently reversing it. A full data descriptor's value is stringified to match node'sEnvDefinerand the string-only contract the Windows env sync relies on. Both env maps fail[[PreventExtensions]]so freeze/seal throw at that step (plain TypeError, no code) and the env stays extensible, matching node exactly. The WindowsdefinePropertyproxy trap uses thesettrap's envMapList predicate (so a first-time define of the always-present HTTP_PROXY/TZ/TLS accessor names stays enumerable) and string-coerces the post-define read before the OS sync.Worker execArgv policy
Worker
execArgvis validated against a real flag policy table (chaining the BASE/AUTO_ONLY/AUTO_OR_RUN param sets) instead of ad-hoc checks: node-illegal flags throwERR_WORKER_INVALID_EXEC_ARGVlisting every invalid flag, rejected required-value flags consume their value token (node arity), and per-worker--expose-gcinstallsgc()in that worker only. The--expose-gcinstall path takes the JSC API lock (JSLockHolder) since it previously ran before the worker thread held the lock, aborting underWeakSet::allocate's assertion in napi worker tests. Env population during worker init holds an exception scope with per-put checks.process.execArgvconstruction and the inherit-path honoring scan share one argv re-parser (collect_process_exec_argv_tokens): bun_clap's glued/chained short-flag forms are normalized to the separate-token shape the worker validator accepts, value tokens are paired regardless of leading-, onlyOne/Manyflags claim the next token (anOneOptionallike--inspect/--configdoes not), andNODE_SHORT_ALIASES(-pe) are recognized, sonew Worker(url, {execArgv: process.execArgv})andnew Worker(url)round-trip across the CLI's accepted launch shapes for validation and--expose-gc. Preloads are honored only by an explicit execArgv: an inheriting worker does not re-run CLI-r(node re-runs it in both; widening that is a separate behavior decision).Verification
test-worker-process-env3/3 with 5 tamper points each exiting 1; descriptor parity re-checked against node across all cases including freeze/seal/preventExtensions + isExtensible; execArgv subset 14/14 andworker_threads.test.ts112/0; napitest_instance_datafixture 10/10 post lock fix; WindowsdefinePropertybookkeeping tests 3/3 on a native Windows build; also run on the debug binary for the JSC structure assertions release skips.Compat impact (upstream node v26.3.0 test files vendored, in-tree = passing)
no test proof · iteration 24 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js
Ride-along: cherry-pick of #35182
This branch carries a cherry-pick of #35182 (merged to main), which deletes three flaky GC-observation vendored tests (
test-gc-http-client-connaborted.js,test-net-connect-memleak.js,test-tls-connect-memleak.js) with their staleexpectations.txt/expected-durations.jsonentries; rationale in the #35182 thread (repeatedcollected-never-true flakes on main). The diff disappears once the branch rebases past the main merge-base.