process: reject accessor/partial descriptors in Object.defineProperty(process.env, ...) - #34727
process: reject accessor/partial descriptors in Object.defineProperty(process.env, ...)#34727robobun wants to merge 6 commits into
Conversation
…erty(process.env, ...)
Node.js's process.env EnvDefiner rejects any descriptor that is not a
fully-specified {value, writable: true, enumerable: true, configurable: true}
data descriptor with ERR_INVALID_OBJECT_DEFINE_PROPERTY, because a
getter/setter cannot be reflected into the real environment block. Bun
previously accepted such descriptors silently.
The regular process.env is now a thin JSNonFinalObject subclass whose only
override is defineOwnProperty to perform this validation; the Windows Proxy
trap and the SHARE_ENV worker map do the same check.
WalkthroughChanges
process.env descriptor enforcement
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:59 PM PT - Jul 19th, 2026
❌ @robobun, your commit 22a0e9d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34727That installs a local version of the PR into your bun-34727 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Workers spawned with env: {...} build process.env via constructEmptyObject
in ZigGlobalObject.cpp, bypassing JSProcessEnvMap entirely. Route that path
through createEmptyProcessEnvMap so defineOwnProperty validation applies
there too. Adds Node's test-worker-process-env.js verbatim.
Co-authored-by: Ciro Spaciari <6379399+cirospaciari@users.noreply.github.com>
|
Overlaps with #34654, which I'd missed. That PR uses the same #34654 also catches the snapshot-env worker path ( Net difference vs #34654: accessor/empty/ |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/JSEnvironmentVariableMap.cpp:817-818— There's a fourthprocess.envcreation site this PR doesn't cover:ZigGlobalObject.cpp:565builds a snapshot-env worker'sprocess.envviaJSC::constructEmptyObject(...)— a plainJSFinalObjectwith the stockdefineOwnProperty— soObject.defineProperty(process.env, 'x', {get(){}})insidenew Worker(..., { env: {...} })still succeeds silently. Node applies the sameEnvDefinerinterceptor to every worker regardless of theenvoption, so after this PR Bun diverges both from Node and from its own main-thread / SHARE_ENV behavior. The fix is a one-line swap ofconstructEmptyObjectforJSProcessEnvMap::createat that site (or an explicit call-out that it's intentionally excluded).Extended reasoning...
What the bug is
This PR introduces
JSProcessEnvMap(whose only method-table override isdefineOwnPropertyrunningthrowIfInvalidEnvDescriptor) and wires it into three of the fourprocess.envcreation sites:createEnvironmentVariablesMap(main thread / default workers),JSSharedEnvMap::defineOwnProperty(SHARE_ENV workers), and the WindowswindowsEnvProxy trap. The fourth site —ZigGlobalObject.cpp:565— is not touched:if (options.env.has_value()) { HashMap<String, String> map = *std::exchange(options.env, std::nullopt); ... auto env = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), ...); ... globalObject->m_processEnvObject.set(vm, globalObject, env); }
constructEmptyObjectreturns a plainJSFinalObjectwhose method table has the defaultJSObject::defineOwnProperty, sothrowIfInvalidEnvDescriptoris never reached for this worker'sprocess.env.The code path that triggers it
options.envis populated inJSWorker.cppwhenever the user passes an explicitenv: {...}object tonew Worker(...), and also (via them_processEnvObject.isInitialized()branch) when noenvoption is given but the parent has already touchedprocess.env— the parent's env is snapshotted intooptions.env. In both cases the branch above runs,m_processEnvObject.set()is called directly at line 572, and the lazyinitLaterthat would have calledcreateEnvironmentVariablesMap(and thus produced aJSProcessEnvMap) is bypassed. On Windows thewindowsEnvProxy is likewise bypassed here, so the new Proxy trap doesn't cover this path either.Why existing code doesn't prevent it
The PR's updated
worker_threads.test.tsonly exercisesenv: SHARE_ENV; there is no test forenv: {...}or the default-snapshot path, so this gap is invisible to the test suite.Impact
After this PR, Bun's own
process.envvariants diverge from each other: the main thread and SHARE_ENV workers throwERR_INVALID_OBJECT_DEFINE_PROPERTYon an accessor descriptor, while snapshot-env workers silently accept it. Node.js applies oneenv_proxy_template/EnvDefinerinterceptor to every realm regardless of whichKVStore(RealEnvStorevsMapKVStore) backs it, so Node rejects accessors uniformly. Per the repo's "fix the whole class in the same PR" rule this is exactly the kind of sibling site that should be swept in — it's the same fix, not scope creep.Step-by-step proof
const { Worker } = require('worker_threads'); new Worker( `try { Object.defineProperty(process.env, 'x', { get() {} }); console.log('accepted'); } catch (e) { console.log(e.code); }`, { eval: true, env: { A: '1' } }, );
new Worker(..., { env: { A: '1' } })setsoptions.envto{ A: '1' }.- In the worker's global setup,
options.env.has_value()is true →ZigGlobalObject.cpp:565buildsenvviaconstructEmptyObjectand stores it directly withm_processEnvObject.set(...). - Inside the worker,
process.envis that plainJSFinalObject.Object.defineProperty(process.env, 'x', { get(){} })dispatches toJSObject::defineOwnProperty, which installs the accessor without complaint. - Node v26 prints
ERR_INVALID_OBJECT_DEFINE_PROPERTY; Bun after this PR printsaccepted.
Fix
Swap the plain object for the new class at
ZigGlobalObject.cpp:565(and exportJSProcessEnvMapfrom the.cppor add a small factory alongsidecreateSharedEnvironmentVariablesMap):auto* structure = Bun::JSProcessEnvMap::createStructure(vm, globalObject, globalObject->objectPrototype()); auto* env = Bun::JSProcessEnvMap::create(vm, structure);
Then extend the worker test to also probe
{ eval: true, env: { A: '1' } }with the same accessor-descriptor assertion.
|
Already addressed in 3a8672d (pushed before this review landed): |
… setup putDirectMayBeIndex on a JSNonFinalObject dispatches index-like keys through the method table's defineOwnProperty (canDoFastPutDirectIndex is false for non-JSFinalObject), which declares a ThrowScope. initializeWorker runs before topEntryFrame is set, so that scope's destructor leaves m_needExceptionCheck set and the next ThrowScope asserts under validateExceptionChecks=1. Wrap the population loop in a TopExceptionScope (whose destructor does not re-simulate) and acknowledge each put.
Node's EnvDefiner delegates to EnvSetter which coerces the value to a string. The Windows Proxy trap and JSSharedEnvMap already stringify; without this the regular POSIX map would store the raw value and diverge from both Windows and Node.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 394-449: Update JSProcessEnvMap::defineOwnProperty to coerce a
present descriptor value to a string before delegating to
Base::defineOwnProperty, while preserving the existing descriptor validation and
attributes. Ensure Object.defineProperty assignments to process.env store string
values for numeric and object inputs, and add regression coverage for both
cases.
🪄 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: 94e1aa64-9003-4825-94e1-d141d99b42f7
📒 Files selected for processing (8)
src/js/builtins/ProcessObjectInternals.tssrc/jsc/bindings/JSEnvironmentVariableMap.cppsrc/jsc/bindings/JSEnvironmentVariableMap.hsrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/process/process.test.jstest/js/node/test/parallel/test-process-env-ignore-getter-setter.jstest/js/node/test/parallel/test-worker-process-env.jstest/js/node/worker_threads/worker_threads.test.ts
SerializedScriptValue's ObjectStartState gate admits only JSFinalObject, NapiPrototype, and ObjectPrototype. Switching process.env to JSProcessEnvMap (a JSNonFinalObject with its own ClassInfo) made structuredClone(process.env), postMessage(process.env), and workerData: process.env throw DataCloneError. Expose isProcessEnvClassInfo() and admit both env map classes at the gate.
On Windows process.env is a Proxy and the structured-clone serializer rejects Proxy objects. This is pre-existing behavior independent of JSProcessEnvMap; the test only guards the POSIX JSFinalObject->JSNonFinalObject change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/JSEnvironmentVariableMap.cpp (1)
421-432: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve side effects for special environment variables.
Base::defineOwnPropertyreplaces existing custom accessors with a plain data property. ForTZ,NODE_TLS_REJECT_UNAUTHORIZED,BUN_CONFIG_VERBOSE_FETCH, and proxy variables, this bypasses the corresponding timezone/TLS/verbose-fetch/native-environment updates and disables the special setter for later writes. Route accepted descriptors through the same side-effecting update path and preserve special accessor behavior where required.🤖 Prompt for 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. In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp` around lines 421 - 432, Update defineOwnProperty in JSEnvironmentVariableMap so accepted descriptors use the existing side-effecting environment-variable update path instead of directly calling Base::defineOwnProperty, preserving custom accessors for TZ, NODE_TLS_REJECT_UNAUTHORIZED, BUN_CONFIG_VERBOSE_FETCH, and proxy variables. Retain string coercion and descriptor validation, while ensuring later writes continue invoking each variable’s special setter.test/js/node/process/process.test.js (1)
221-258: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the process.env test hermetic and exception-safe.
The test assumes
fooandgooare unset, and cleanup occurs only after assertions. Use unique keys and wrap all mutations intry/finally, restoring any prior values so failures cannot contaminate subsequent tests.As per coding guidelines, tests must isolate process-global state and restore globals in finally blocks.
🤖 Prompt for 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. In `@test/js/node/process/process.test.js` around lines 221 - 258, Make the Object.defineProperty process.env test hermetic by generating unique keys instead of using fixed “foo” and “goo” names, and capture each key’s prior value/state. Wrap all process.env mutations and assertions in try/finally blocks, restoring or deleting the keys in finally so cleanup occurs even when an assertion fails; preserve the existing descriptor validation and string-coercion coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 421-432: Update defineOwnProperty in JSEnvironmentVariableMap so
accepted descriptors use the existing side-effecting environment-variable update
path instead of directly calling Base::defineOwnProperty, preserving custom
accessors for TZ, NODE_TLS_REJECT_UNAUTHORIZED, BUN_CONFIG_VERBOSE_FETCH, and
proxy variables. Retain string coercion and descriptor validation, while
ensuring later writes continue invoking each variable’s special setter.
In `@test/js/node/process/process.test.js`:
- Around line 221-258: Make the Object.defineProperty process.env test hermetic
by generating unique keys instead of using fixed “foo” and “goo” names, and
capture each key’s prior value/state. Wrap all process.env mutations and
assertions in try/finally blocks, restoring or deleting the keys in finally so
cleanup occurs even when an assertion fails; preserve the existing descriptor
validation and string-coercion coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76c1f02d-954c-4151-afa3-20bdc55f170e
📒 Files selected for processing (4)
src/jsc/bindings/JSEnvironmentVariableMap.cppsrc/jsc/bindings/JSEnvironmentVariableMap.hsrc/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/node/process/process.test.js
|
Re the two coderabbit findings on 22a0e9d: Special-variable side effects ( Routing accepted descriptors through the side-effecting setters would be a behavior change layered on top of the validation this PR adds; leaving it for a follow-up to keep scope bounded. Test hermeticity: the |
|
Build #75903 has no failures in the files this PR touches. Remaining reds are |
There was a problem hiding this comment.
No new issues found — both earlier inline concerns (value stringification, structured-clone whitelist) are addressed. Deferring to human review: this changes process.env's underlying class from JSFinalObject to a custom JSNonFinalObject with a method-table override, touches the structured-clone serializer and worker-global setup, and overlaps with #34654 (one will need closing).
What was reviewed:
throwIfInvalidEnvDescriptoragainst Node'sEnvDefinercases (accessor, partial,{attr:false}, empty) — messages match verbatim.JSProcessEnvMapsubspace/structure setup (STATIC_ASSERT_ISO_SUBSPACE_SHARABLE,plainObjectSpace, no instance state) and thatPropertyDescriptor(value, 0)fromputDirectIndexpasses the validator during snapshot-env worker population.- Windows
definePropertytrap: validation now precedesenvMapList/SetEnvironmentVariableWwrites, and the oldeditWindowsEnvVar(k, internalEnv[k])(which passed the old value) is fixed. - Symbol-key handling: Node also throws
ERR_INVALID_OBJECT_DEFINE_PROPERTYon symbol keys, so no divergence there.
Extended reasoning...
Overview
The PR replaces process.env's backing object (a plain JSFinalObject) with a new JSProcessEnvMap : JSNonFinalObject whose only method-table override is defineOwnProperty, which validates descriptors to match Node's EnvDefiner (rejects accessors, partial descriptors, and any {writable|enumerable|configurable: false}). The same validator is wired into JSSharedEnvMap (SHARE_ENV), the Windows Proxy trap, and the snapshot-env worker path in ZigGlobalObject.cpp. Because the class change broke the exact-classInfo() whitelist in SerializedScriptValue.cpp, an isProcessEnvClassInfo() helper is added to that gate. Two Node parallel tests are imported verbatim, plus targeted coverage in process.test.js and an updated SHARE_ENV accessor test.
Security risks
None identified. This tightens validation (rejects more inputs than before) and does not expose new capability. The structured-clone whitelist addition admits objects that behave as plain string maps — the same content that was clonable before the class change.
Level of scrutiny
Moderate-to-high. Changing the underlying JSC class of process.env is the kind of change with non-local consequences: any code path that keys on JSFinalObject::info(), FinalObjectType, or inline-capacity fast paths may now take a different branch. Two such consequences already surfaced during review (structured-clone rejection; putDirectMayBeIndex dispatching through the method table's defineOwnProperty during worker-env population, requiring a top-level exception scope). Both are handled, but the pattern suggests a human should sanity-check for other FinalObject-keyed fast paths.
Other factors
- PR overlap: #34654 implements a subset of this via the same
JSProcessEnvMapapproach; a maintainer needs to decide which lands. - Prior review: I left two inline findings (stringify in
JSProcessEnvMap::defineOwnProperty; structured-clone whitelist), both resolved in 64c4b25 and 2d862d6. - CI: green on the touched files; remaining reds are tagged flaky/pre-existing.
- Windows: the
structuredClone(process.env)test is skipped there becauseprocess.envis a Proxy (pre-existing serializer limitation, noted in the test comment).
What does this PR do?
Node.js throws
ERR_INVALID_OBJECT_DEFINE_PROPERTYwhenObject.definePropertyis called onprocess.envwith anything other than a fully-specified{value, writable: true, enumerable: true, configurable: true}data descriptor (seenode_env_var.ccEnvDefiner). An accessor descriptor can never be reflected into the real environment block, so Node rejects it outright.Bun previously accepted such descriptors silently, leaving an env key whose value never reaches child processes.
How did you verify your code works?
test/js/node/process/process.test.js: newObject.defineProperty on process.env ...case covering accessor, partial, andfalse-attribute descriptors plus the valid form.test/js/node/test/parallel/test-process-env-ignore-getter-setter.js: imported verbatim from Node.js, passes with exit 0.test/js/node/worker_threads/worker_threads.test.ts: the existing SHARE_ENV accessor test is updated to assert the shared map now matches the regular map (both throw).Implementation
JSEnvironmentVariableMap.cpp: addedJSProcessEnvMap, aJSNonFinalObjectsubclass whose only method-table override isdefineOwnProperty, which validates the descriptor viathrowIfInvalidEnvDescriptorand delegates toBaseon success.createEnvironmentVariablesMapnow instantiates this class instead of a plainJSFinalObject.JSSharedEnvMap::defineOwnPropertynow runs the same validation first; the old store-shadowing branch (only reachable for accessors/partial descriptors) is dead and removed.ProcessObjectInternals.ts(Windows proxy): thedefinePropertytrap validates before touchingenvMapListor callingSetEnvironmentVariableW, then writes the stringified value through the same path asset.no test proof · iteration 1 · 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