Skip to content

process.env: coerce assigned values to strings across all construction paths - #34728

Open
robobun wants to merge 9 commits into
mainfrom
farm/6a84d846/process-env-string-coercion
Open

process.env: coerce assigned values to strings across all construction paths#34728
robobun wants to merge 9 commits into
mainfrom
farm/6a84d846/process-env-string-coercion

Conversation

@robobun

@robobun robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Node.js documents that every process.env assignment coerces the value to a string. Bun stored the value verbatim on two of the three construction paths, so assigning a non-string read back as the raw value:

process.env.NUM = 1; process.env.BOOL = true; process.env.BIG = 42n;
Object.assign(process.env, { ASN: 7 });
console.log([process.env.NUM, process.env.BOOL, process.env.BIG, process.env.ASN].map(v => typeof v));
// bun: ["number","boolean","bigint","number"]   node: all "string"
process.env.SYM = Symbol("s");                   // bun: stored; node: TypeError
JSON.stringify(process.env);                     // bun: TypeError (BigInt); node: ok

Two user-visible consequences beyond the raw readback: after a BigInt assignment, JSON.stringify(process.env) throws for the rest of the process (breaking error reporters and config dumpers); and process.env.X = Symbol() is silently accepted instead of throwing like Node.

Cause

process.env is built in three places, only one of which coerced on assignment:

  • createEnvironmentVariablesMap (main thread, default): a plain JSObject populated with CustomValue getters for lazy OS-env reads. Those getters have no setter, so writes went straight to JSObject::put and stored whatever JSValue arrived. On Windows the same object is wrapped in the windowsEnv Proxy whose set trap called String(value), which coerces everything but special-cases Symbol (returns "Symbol(s)" instead of throwing).
  • ZigGlobalObject initializeWorker with { env: {...} }: a plain constructEmptyObject assigned directly to m_processEnvObject, bypassing createEnvironmentVariablesMap on every platform.
  • JSSharedEnvMap (SHARE_ENV): already coerced via toWTFString in its put hook.

Fix

  • Introduce JSProcessEnvMap, a JSNonFinalObject subclass whose put/putByIndex/defineOwnProperty call value.toString() (spec ToString: throws on Symbol) before delegating to Base. Symbol keys fall through unchanged, matching JSSharedEnvMap::put.
  • createEnvironmentVariablesMap uses it on POSIX. Windows keeps a plain object as the Proxy target, because windowsEnv() stores toJSON as an own function on that object directly and the Proxy's traps already coerce.
  • The Windows set/defineProperty traps now use template-literal coercion instead of String() so Symbol values throw TypeError the same as POSIX and Node. The defineProperty trap now also syncs the new value to the OS env after a successful define (it previously synced the old value or undefined, which tripped a debug ASSERT).
  • The worker-with-explicit-env path at ZigGlobalObject.cpp builds its object via a new createProcessEnvMapObject() factory, so workers on every platform coerce.
  • SerializedScriptValue whitelists the new class so structuredClone(process.env) keeps working.

Verification

  • test/js/node/process/process.test.js: new "process.env coerces assigned values to strings" case covering number, boolean, null, undefined, BigInt, array, function, object with toString, Object.assign, indexed key, Object.defineProperty, a throwing toString, Symbol-throws on all three paths, the Object.values(process.env) all-string invariant, and JSON.stringify(process.env) after a BigInt write; plus "... in a worker with an explicit env". Both fail on main, pass with the fix.
  • test/cli/run/env.test.ts: un-todos the pre-existing "setting process.env coerces the value to a string" test, which was todoOnPosix for exactly this reason.
  • test/js/node/worker_threads/worker_threads.test.ts: all SHARE_ENV tests still pass.

Related: #31831 is a broader process-compat omnibus that includes this among many other fixes; #34727 adds defineOwnProperty descriptor validation to the same class. Whichever lands first, the others rebase over it.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/env.test.ts test/js/node/process/process.test.js

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:52 AM PT - Jul 23rd, 2026

@robobun, your commit 372ec7f35d88ea64659d867e9b55522137fd701b passed in Build #78581! 🎉


🧪   To try this PR locally:

bunx bun-pr 34728

That installs a local version of the PR into your bun-34728 executable, so you can run:

bun-34728 --bun

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

process.env now stringifies assigned and defined values on Windows and non-Windows platforms. Worker environment snapshots use the coercing map, with expanded CLI, process, and worker tests covering conversion behavior and errors.

process.env string coercion

Layer / File(s) Summary
Platform-specific coercion implementation
src/js/builtins/ProcessObjectInternals.ts, src/jsc/bindings/JSEnvironmentVariableMap.cpp
Windows property definitions and non-Windows environment map assignments convert values to strings before storage while preserving platform-specific initialization.
Worker environment map integration
src/jsc/bindings/JSEnvironmentVariableMap.h, src/jsc/bindings/ZigGlobalObject.cpp
Worker options.env uses the bare coercing environment map and checks for exceptions after each insertion.
Coercion behavior tests
test/cli/run/env.test.ts, test/js/node/process/process.test.js
Tests cover stringification of assigned values, defineProperty, custom and throwing toString methods, worker values, and cleanup.

Possibly related PRs

  • oven-sh/bun#34727: Modifies related process.env defineProperty validation and environment map behavior.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely summarizes the main change: coercing process.env assignments to strings across construction paths.
Description check ✅ Passed The description covers the problem, cause, fix, and verification, including test evidence, even though it uses custom headings instead of the template.

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. process: port Node.js v26.3.0 process compatibility tests and fix the gaps they surface (env exotic-object/TZ semantics, warnings pipeline + CLI flags, uncaught origin/exit codes, execve throw, threadCpuUsage/finalization/loadEnvFile, native-module identity; +26 tests) #31831 - Implements the same process.env string coercion as part of a broader Node.js process compatibility overhaul ("process.env is now an exotic object with Node's semantics: every assigned value is coerced to a string")
  2. node: add custom setters to process.env #19382 - Introduces the same JSEnvironmentVariableMap C++ class with put/defineOwnProperty overrides that call toStringOrNull() before storing, matching this PR's approach

🤖 Generated with Claude Code

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Re the duplicate flag:

This PR is the focused 3-file version of just the coercion piece. Happy to close if #31831 is landing imminently; otherwise this can go in first and #31831 rebases over it (the JSProcessEnvMap here is a strict subset of what that PR adds). Same applies to #34727, which adds defineOwnProperty descriptor validation to the same class.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/js/builtins/ProcessObjectInternals.ts (1)

533-541: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Sync the Windows environment after a successful definition.

Line 540 writes internalEnv[k] before Line 541 defines the property, so Object.defineProperty(process.env, "X", { value: "new" }) propagates the old value (or deletion) to the OS environment. Define first, then sync the coerced value only on success.

🤖 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/js/builtins/ProcessObjectInternals.ts` around lines 533 - 541, Update the
defineProperty handling around editWindowsEnvVar and $Object.$defineProperty so
the property is defined first, then synchronize Windows using the coerced
resulting value only after successful definition. Preserve the existing
envMapList bookkeeping and ensure failed definitions do not update the OS
environment.
🤖 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/js/builtins/ProcessObjectInternals.ts`:
- Around line 530-536: Update the defineProperty trap to delegate non-string
property keys directly to internalEnv before converting keys or coercing
descriptor values. Preserve the existing string-key behavior, including String
conversion of descriptor values, while ensuring symbol keys and their values
remain unchanged.

In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 746-751: The comment above the default non-SHARE_ENV process.env
implementation exceeds the three-line limit. Condense it to three lines or fewer
while preserving the key behavior: assigned values are string-coerced, and
Windows relies on proxy traps with own functions stored directly.

In `@test/js/node/process/process.test.js`:
- Around line 235-288: Update the process.env test setup and its finally cleanup
to snapshot each targeted key’s original property descriptor before mutation,
then restore that descriptor afterward instead of unconditionally deleting the
keys. Apply this to the keys used by the COERCE_* assignments and "4242424242",
preserving absence for keys that were not originally present.

---

Outside diff comments:
In `@src/js/builtins/ProcessObjectInternals.ts`:
- Around line 533-541: Update the defineProperty handling around
editWindowsEnvVar and $Object.$defineProperty so the property is defined first,
then synchronize Windows using the coerced resulting value only after successful
definition. Preserve the existing envMapList bookkeeping and ensure failed
definitions do not update the OS environment.
🪄 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: 2e47aa95-1874-4b29-9464-6664362619d9

📥 Commits

Reviewing files that changed from the base of the PR and between 99fc2f8 and c032762.

📒 Files selected for processing (4)
  • src/js/builtins/ProcessObjectInternals.ts
  • src/jsc/bindings/JSEnvironmentVariableMap.cpp
  • test/cli/run/env.test.ts
  • test/js/node/process/process.test.js

Comment thread src/js/builtins/ProcessObjectInternals.ts
Comment thread src/jsc/bindings/JSEnvironmentVariableMap.cpp Outdated
Comment thread test/js/node/process/process.test.js

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:844-845 — On Windows, internalEnv is now a JSProcessEnvMap, so the direct (internalEnv as any).toJSON = () => {...} assignment inside windowsEnv() (ProcessObjectInternals.ts:464) hits JSProcessEnvMap::put and stores the arrow function's source text as a string instead of the function. This breaks JSON.stringify(process.env)'s original-case-key output and will fail the existing Windows test asserting typeof process.env.toJSON === 'function'. Either keep constructEmptyObject on Windows (the Proxy set trap already coerces) or install toJSON via putDirect/$putByIdDirect.

    Extended reasoning...

    What the bug is

    createEnvironmentVariablesMap now unconditionally builds the backing object as a JSProcessEnvMap — a JSNonFinalObject subclass with OverridesPut whose put() calls value.toString() on every non-symbol write. On Windows, that object is passed as internalEnv into the windowsEnv() builtin (src/js/builtins/ProcessObjectInternals.ts), which — before creating the Proxy — does:

    (internalEnv as any).toJSON = () => { ... };   // line 464

    This is a plain = assignment (compiles to put_by_id) directly on internalEnv, not through the Proxy. It therefore dispatches to JSProcessEnvMap::put, which sees a non-symbol key "toJSON", calls value.toString(globalObject) on the arrow function, and stores the function's source text as a JSString. The Bun.inspect.custom assignment at line 456 survives because it uses a Symbol key, which JSProcessEnvMap::put passes through to Base::put unchanged.

    Why the existing code doesn't prevent it

    The PR description says "On Windows the Proxy still wraps this object; its set trap already stringifies, so writes coerce once in JS and pass a string through." That reasoning is correct only for writes that go through the Proxy (process.env.X = ...). The internalEnv.toJSON = fn assignment happens inside windowsEnv() on the raw backing object, before new Proxy(internalEnv, {...}) is even constructed, so it bypasses the set trap entirely and hits the new C++ put hook directly. Before this PR internalEnv was a plain constructEmptyObject() JSObject with no put hook, so the function landed as-is.

    Step-by-step proof

    1. createEnvironmentVariablesMap (JSEnvironmentVariableMap.cpp:844-845) creates object = JSProcessEnvMap::create(...) — no #if !OS(WINDOWS) guard.
    2. Under #if OS(WINDOWS), object is passed as args[0] to processObjectInternalsWindowsEnvCodeGenerator, becoming the internalEnv parameter of windowsEnv().
    3. windowsEnv() executes (internalEnv as any).toJSON = () => {...} (line 464). propertyName is "toJSON" (not a symbol), so JSProcessEnvMap::put runs value.toString(globalObject) → the arrow's source text — and stores that string.
    4. windowsEnv() returns new Proxy(internalEnv, {...}) (line 475). Its get trap for "toJSON" uppercases to "TOJSON", which is not in internalEnv, then falls through to internalEnv["toJSON"] (line 490) — returning the stringified function body.
    5. Therefore typeof process.env.toJSON === 'string'.

    Impact

    • The existing Windows-only test "windows process.env exposes prototype methods and own props alongside case-insensitive vars" in test/js/node/process/process.test.js asserts expect(typeof process.env.toJSON).toBe("function") and expect(json.Bun_Test_Env_Proxy_Mixed).toBe("mixed") (which relies on the custom toJSON emitting original-case keys). Both will fail on Windows CI.
    • JSON.stringify(process.env) on Windows no longer routes through the custom toJSON, so its output loses the original-case key names and instead exposes the canonical UPPERCASE storage keys plus a spurious "toJSON" env-var-looking string property.

    How to fix

    The cleanest fix is to only construct JSProcessEnvMap on POSIX and keep constructEmptyObject on Windows, since the Proxy set/defineProperty traps already coerce every user-visible write — the C++ hook adds nothing there. Alternatively, install toJSON on the backing object via putDirect from C++ (or $putByIdDirect in the builtin) so it bypasses the coercing put, or special-case "toJSON" in JSProcessEnvMap::put. The first option is smallest and matches the PR's stated intent (the fix targets POSIX; Windows already coerced correctly).

  • 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:844-845 — There's a fourth process.env construction site this misses: a worker spawned with an explicit env dictionary — new Worker(url, { env: { KEY: 'v' } }) — builds its process.env via JSC::constructEmptyObject(...) at ZigGlobalObject.cpp:565 and assigns it directly to m_processEnvObject, so inside that worker process.env.X = 42 still stores the raw number. Per REVIEW.md ('Fix the whole class in the same PR — grep for every sibling site sharing the pattern'), that site should also become a JSProcessEnvMap — expose a small factory (e.g. createProcessEnvMapObject(globalObject)) from this file and call it there in place of constructEmptyObject.

    Extended reasoning...

    What the bug is

    The PR introduces JSProcessEnvMap so process.env coerces assigned values to strings, and the description enumerates three construction paths as covered: the default POSIX object (now JSProcessEnvMap), the Windows Proxy wrap (whose set trap already stringifies), and the SHARE_ENV path (JSSharedEnvMap::put already calls toWTFString). But there is a fourth path that constructs process.env, and it is not touched.

    At src/jsc/bindings/ZigGlobalObject.cpp:553-572, when a worker is created with an explicit env dictionary — new Worker(url, { env: { FOO: 'bar' } }) — the worker's global builds its process.env via:

    auto env = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), ...);
    // ...populate with putDirectMayBeIndex...
    globalObject->m_processEnvObject.set(vm, globalObject, env);

    This assigns a plain JSFinalObject directly to m_processEnvObject. It bypasses createEnvironmentVariablesMap entirely (so it is not a JSProcessEnvMap, and on Windows it gets no Proxy wrap either), and it is not the SHARE_ENV branch (that's the else if (options.sharedEnvStore) at line 573).

    Why existing code doesn't prevent it

    JSProcessEnvMap is only instantiated inside createEnvironmentVariablesMap, which is the lazy initializer for m_processEnvObject (line 2490-2493). The explicit-env worker path pre-populates m_processEnvObject before the lazy initializer ever runs, so createEnvironmentVariablesMap is never called for that global. The plain object it installs has no put/putByIndex/defineOwnProperty overrides, so writes go straight to JSObject::put and store whatever JSValue arrived — exactly the pre-PR POSIX behavior.

    Step-by-step proof

    1. Main thread runs new Worker('./w.js', { env: { FOO: 'bar' } }).
    2. Worker global initialization enters the options.env.has_value() branch at ZigGlobalObject.cpp:553.
    3. Line 565: env = JSC::constructEmptyObject(...) → a plain JSFinalObject.
    4. Line 572: globalObject->m_processEnvObject.set(vm, globalObject, env) — the LazyProperty is now set, so the initializer at line 2492 (which would have created a JSProcessEnvMap) never runs.
    5. Inside the worker, user code does process.env.X = 42.
    6. env is a JSFinalObject; its method table's put is JSObject::put, which stores jsNumber(42) verbatim.
    7. process.env.X reads back as the number 42, and process.env.Y = undefined reads back as undefined (not 'undefined') — the exact divergence this PR fixes on the main thread.

    Repro:

    const { Worker } = require('worker_threads');
    new Worker(
      "process.env.X = 42; require('assert').strictEqual(process.env.X, '42');",
      { eval: true, env: { FOO: 'bar' } }
    );

    Node passes; Bun (with this PR) throws 42 !== '42' inside the worker.

    Impact

    Inside any worker spawned with an explicit env: {...} option, process.env still fails to coerce on assignment — diverging from Node.js and from Bun's own main-thread behavior after this PR lands. This is exactly the bug class the PR sets out to close, at a sibling construction site. REVIEW.md is explicit on this: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR." The PR description does not note this exclusion.

    How to fix

    JSProcessEnvMap is currently file-local to JSEnvironmentVariableMap.cpp. Expose a small factory alongside createSharedEnvironmentVariablesMap:

    JSObject* createProcessEnvMapObject(Zig::GlobalObject* globalObject)
    {
        VM& vm = globalObject->vm();
        auto* structure = JSProcessEnvMap::createStructure(vm, globalObject, globalObject->objectPrototype());
        return JSProcessEnvMap::create(vm, structure);
    }

    declare it in the header, and at ZigGlobalObject.cpp:565 replace JSC::constructEmptyObject(...) with Bun::createProcessEnvMapObject(globalObject). The subsequent putDirectMayBeIndex loop populating the initial keys is unaffected (it writes directly, bypassing the coercing put hook, and the values are already jsStrings). Add a worker-with-explicit-env case to the new coercion test so this path is covered.

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Re the two claude[bot] findings:

  • Windows toJSON stringified: addressed in c032762 (review ran on the earlier commit). Windows now keeps a plain constructEmptyObject as the Proxy target so windowsEnv() can store toJSON as a function; coercion happens in the Proxy's set/defineProperty traps.
  • Worker with explicit env: {...} bypasses coercion: good catch, addressed in c3880dd. Exposed createProcessEnvMapObject() and routed ZigGlobalObject.cpp:565 through it; added a worker-explicit-env coercion test.

Also picked up the related pre-existing Windows bug the new test surfaced: the defineProperty trap was syncing internalEnv[k] (the old value, or undefined for a new key, which tripped a debug ASSERT) before the define. 5ef3cba now defines first and syncs the coerced new value.

Comment thread src/jsc/bindings/JSEnvironmentVariableMap.cpp Outdated
Comment thread src/js/builtins/ProcessObjectInternals.ts
Comment thread test/cli/run/env.test.ts
Comment thread src/jsc/bindings/JSEnvironmentVariableMap.cpp
Comment thread src/jsc/bindings/ZigGlobalObject.cpp
Comment thread test/js/node/process/process.test.js Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/js/builtins/ProcessObjectInternals.ts`:
- Around line 539-546: Update the defineProperty path around envMapList and
$Object.$defineProperty to use the same case-insensitive key matching as set(),
including hidden variables such as HTTP_PROXY, HTTPS_PROXY, and NO_PROXY.
Perform $Object.$defineProperty first, then add the canonical key to envMapList
only after success, preserving ownKeys() and spread visibility without leaving
entries when definition fails.

In `@test/js/node/process/process.test.js`:
- Around line 292-319: Update the worker test around the message posted by the
Worker to include process.env.SEED, then extend the expected parsed output to
assert SEED equals "seed". Preserve the existing assertions for X, Y, Z, and
their types.
🪄 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: 9bdda5cb-15a3-46da-92de-c1641fe334e2

📥 Commits

Reviewing files that changed from the base of the PR and between c032762 and 3f98162.

📒 Files selected for processing (6)
  • src/js/builtins/ProcessObjectInternals.ts
  • src/jsc/bindings/JSEnvironmentVariableMap.cpp
  • src/jsc/bindings/JSEnvironmentVariableMap.h
  • src/jsc/bindings/ZigGlobalObject.cpp
  • test/cli/run/env.test.ts
  • test/js/node/process/process.test.js

Comment thread src/js/builtins/ProcessObjectInternals.ts Outdated
Comment thread test/js/node/process/process.test.js
@robobun robobun changed the title process.env: coerce assigned values to strings on POSIX process.env: coerce assigned values to strings across all construction paths Jul 20, 2026
@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

CI at 372ec7f (build #78581, post-rebase): the diff's own tests (process.test.js, env.test.ts, worker.test.ts, worker_threads.test.ts, structured-clone.test.ts) pass on every lane. Remaining reds are unrelated flakes that passed on retry:

  • test/regression/issue/1632.test.ts (process.stdout broken pipe, ubuntu aarch64)
  • test/napi/napi.test.ts (N-API string output ordering, ubuntu aarch64)
  • test/js/bun/spawn/spawn.test.ts (ubuntu x64)
  • test/js/node/test/parallel/test-fastutf8stream-reopen.js (ubuntu x64)
  • test/cli/run/no-orphans.test.ts (darwin x64)

Ready for review.

robobun and others added 9 commits July 23, 2026 08:50
Node.js documents that every process.env assignment coerces the value
to a string. On POSIX, Bun's process.env was a plain JSObject with no
put hook, so assigning a non-string value stored it verbatim:

  process.env.foo = undefined;  // read back as undefined, not "undefined"
  process.env.bar = 42;         // typeof process.env.bar === "number"

This broke the common "=== 'undefined'" sentinel check and meant the
parent's readback diverged from what a spawned child actually received.

The Windows path (a Proxy with a set trap) and the SHARE_ENV path
(JSSharedEnvMap::put) already coerced. This change gives the default
POSIX object the same treatment by wrapping it in a JSProcessEnvMap
that overrides put/putByIndex/defineOwnProperty to call toString() on
the value before storing it. The CustomValue getters for lazy OS-env
loading are unchanged; they sit on the base object and still work.

Also un-todos the existing "setting process.env coerces" test in
env.test.ts, which was todoOnPosix for exactly this reason.
The windowsEnv Proxy stores toJSON and Bun.inspect.custom as own
properties on the target object before wrapping it. Routing those
assignments through JSProcessEnvMap::put coerced the toJSON function
to its string source, breaking typeof process.env.toJSON on Windows.

Windows already coerces in the Proxy's set trap, so keep the target
as a plain JSObject there and add the matching String(value) coercion
to the Proxy's defineProperty trap instead. JSProcessEnvMap is now
POSIX-only.
ZigGlobalObject's initializeWorker path builds process.env for
new Worker(url, { env: {...} }) as a plain constructEmptyObject and
assigns it to m_processEnvObject directly, so createEnvironmentVariablesMap
never runs and writes inside that worker stored raw values. Route that
construction through the same JSProcessEnvMap via a small factory.

JSProcessEnvMap is no longer #if-guarded: Windows still uses a plain
object inside createEnvironmentVariablesMap (the windowsEnv Proxy coerces
and stores toJSON on the target), but the worker-env path has no Proxy
on any platform and so uses JSProcessEnvMap everywhere.

Also condensed the class comment to three lines per review.
The Windows defineProperty trap called editWindowsEnvVar(k, internalEnv[k])
before defining the property, so a new key synced undefined (tripping the
isNull()||isString() assert in debug builds) and an existing key synced
its old value. Coerce the descriptor value, define, then sync the coerced
string only when a value was supplied.
JSProcessEnvMap is a JSNonFinalObject, so canDoFastPutDirectIndex()
is false and indexed putDirectMayBeIndex routes through
methodTable()->defineOwnProperty. That path declares ThrowScopes, whose
destructors simulate a throw to the caller; with no enclosing scope in
initializeWorker the next DECLARE_*_SCOPE elsewhere trips
verifyExceptionCheckNeedIsSatisfied. A TopExceptionScope (used the same
way elsewhere in bun for top-of-stack loops) absorbs the simulated
throw and asserts no real exception per iteration.

Also: add finally-cleanup to the un-todo'd env.test.ts coercion test.
JSProcessEnvMap carries its own ClassInfo, so the ObjectStartState check
in CloneSerializer returned DataCloneError for structuredClone(process.env)
and postMessage({env: process.env}). Node structured-clones process.env
as a plain object of its string entries, and Bun did too when the object
was a JSFinalObject. Whitelist both env-map classes alongside
NapiPrototype via a small isEnvironmentVariablesMapObject() predicate.

Test: the coercion tests now assert structuredClone(process.env) succeeds
and round-trips string values; the worker test also verifies the seed env
var is visible and process.env clones inside the worker. Dropped the
stderr-exactly-empty assertion per repo review rules.

windowsEnv defineProperty trap: record the key in envMapList only after
a successful define, using the same case-insensitive check as set().
Windows wraps process.env in a Proxy that structured-clone rejects;
that's pre-existing and unchanged by this PR. The worker test still
asserts structuredClone(process.env) on every platform (the worker
env is a JSProcessEnvMap on Windows too).
…SON.stringify

Template-literal coercion in the windowsEnv Proxy set/defineProperty traps so
Symbol values throw TypeError the same as POSIX and Node.js (String() special-
cases Symbol and would have silently stored "Symbol(s)").

Extends the coercion test to cover BigInt, array, function, Object.assign, the
Object.values all-string invariant, and JSON.stringify(process.env) surviving a
BigInt assignment.
@robobun
robobun force-pushed the farm/6a84d846/process-env-string-coercion branch from 85f4bbb to 372ec7f Compare July 23, 2026 09:09
@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on main (149 commits behind) and pushed 372ec7f:

  • Windows set/defineProperty traps now use template-literal coercion instead of String(), so Symbol values throw TypeError there too (previously would have stored "Symbol(s)").
  • Extended the coercion test to cover BigInt, array, function, Object.assign, Symbol-throws on all three assignment paths, the Object.values(process.env) all-string invariant, and JSON.stringify(process.env) surviving a BigInt write.

All touched test files pass locally on the debug build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find new issues after the rebase, but this introduces a new JSC class (JSProcessEnvMap) with method-table put/putByIndex/defineOwnProperty overrides, whitelists it in the structured-clone ObjectStartState check, and diverges the POSIX vs Windows process.env backing object — worth a human look, especially given the acknowledged Windows defineProperty Proxy-invariant edge case being deferred to #34727.

Checked: exception-scope handling in the three new hooks matches JSSharedEnvMap (Symbol keys / non-data descriptors pass through, toStringRETURN_IF_EXCEPTIONRELEASE_AND_RETURN); the worker seed loop's DECLARE_TOP_EXCEPTION_SCOPE + assertNoException() is sound because the values are pre-built JSString*s from strings; isEnvironmentVariablesMapObject covers both env-map ClassInfos so structuredClone/postMessage(process.env) don't regress; the existing CustomValue/CustomAccessor getters (TZ, proxy vars) still work under Base::put — the value arrives already coerced to a JSString, so their own toString/toWTFString is idempotent; the Windows defineProperty trap's return true matches the old return $Object.$defineProperty(...) (which returned the target object, also truthy).

Extended reasoning...

Overview

This PR makes process.env coerce all assigned values to strings, matching Node.js. It introduces JSProcessEnvMap (a JSNonFinalObject subclass with overridden put/putByIndex/defineOwnProperty), routes createEnvironmentVariablesMap through it on POSIX, routes the worker explicit-env path in ZigGlobalObject.cpp through a new createProcessEnvMapObject() factory, whitelists both env-map ClassInfos in SerializedScriptValue.cpp's ObjectStartState check, and updates the Windows windowsEnv Proxy's set/defineProperty traps to use template-literal coercion (so Symbol values throw) and to sync the new value to the OS env after a successful define. Two test files gain coverage; one todoOnPosix is un-todo'd.

Security risks

None identified. The change tightens behavior (rejects Symbol values, coerces to strings) rather than loosening it. The structured-clone whitelist addition is narrowly scoped to two internal ClassInfos.

Level of scrutiny

High. This is JSC bindings work: a new class with a custom method table, subspace sharing with plainObjectSpace, exception-scope discipline across three overrides, a change to the structured-clone serializer's object-acceptance predicate, and a platform split where POSIX and Windows now use different backing types for process.env. Earlier review passes on this PR found real regressions (structuredClone DataCloneError, exception-scope validator failure on the ASAN lane, a Windows defineProperty ordering bug), all of which were fixed — but that iteration history itself argues for a human sign-off on the final shape.

Other factors

  • The Windows defineProperty trap has an acknowledged edge case (non-string value + no writable/configurable on an all-uppercase new key trips the ES Proxy invariant with an engine TypeError) that the author deliberately deferred to #34727's descriptor validation. That's a reasonable call — Node rejects the same input — but the coordination with #34727 / #31831 is a merge-order decision a maintainer should own.
  • The Object.values(process.env).every(v => typeof v === "string") assertion depends on the pre-existing special-var getters (TZ, NODE_TLS_REJECT_UNAUTHORIZED, BUN_CONFIG_VERBOSE_FETCH) never being enumerable-and-undefined under bunEnv; that holds today because they're DontEnum when unset, but it's not something this PR changed.
  • The latest push (372ec7f) was a rebase over 149 commits plus a behavior change (template-literal coercion in the Windows traps); the timeline does not yet show a green CI on that commit, only "passes locally on the debug build".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants