process.env: fix assignment to a proxy var after its property is redefined

[robobun]

Repro

// key absent from the OS env
Object.defineProperty(process.env, "HTTP_PROXY", { enumerable: false });
process.env.HTTP_PROXY = "http://127.0.0.1:1";

Debug builds abort:

ASSERTION FAILED: value.isCustomGetterSetter()
vendor/WebKit/Source/JavaScriptCore/runtime/JSObject.cpp(2259) : bool JSC::JSObject::putDirectCustomAccessor(VM &, PropertyName, JSValue, unsigned int)

The jest-style descriptor snapshot/restore hits the same path, as do Bun.env, HTTPS_PROXY/NO_PROXY, and the lowercase variants:

const desc = Object.getOwnPropertyDescriptor(process.env, "HTTPS_PROXY");
delete process.env.HTTPS_PROXY;
Object.defineProperty(process.env, "HTTPS_PROXY", desc);
process.env.HTTPS_PROXY = "http://127.0.0.1:2";

Cause

The proxy env vars are installed as CustomAccessors, DontEnum when absent from the OS env at startup. jsSetterProxyEnvironmentVariable clears DontEnum on write by deleting the property and re-installing whatever getDirect() returned via putDirectCustomAccessor.

Object.defineProperty() over a CustomAccessor reifies it into a plain GetterSetter of wrapper functions, and assignment then reaches the native setter through the set wrapper. The tail passed that GetterSetter to putDirectCustomAccessor, which requires a CustomGetterSetter: an assertion failure on debug builds, and on release builds a slot with Accessor|CustomValue attributes holding a GetterSetter, a state JSC never otherwise creates (latent there: the Accessor branch wins on the probed paths, so reads/writes/enumeration still behave).

Fix

Branch on what getDirect() returned:

• CustomGetterSetter: the existing delete + putDirectCustomAccessor path.
• GetterSetter (reified): re-install with putDirectAccessor, clearing DontEnum. Same observable behavior release builds already had, without the mismatched attributes.
• Anything else (the property was redefined as a data property and a captured set wrapper was invoked): leave the user's property untouched.

Note: #34727 proposes rejecting accessor/partial descriptors on process.env at the defineProperty level. If that lands first, the repro above starts throwing and the new test needs adjusting; independent of that decision, the setter should not corrupt the slot it is given.

Verification

Test added next to the existing proxy-var coverage in test/js/node/process/process.test.js, covering both variants plus value readback and {...process.env} pickup. Without the fix it fails with the assertion above (child aborts); with the fix it passes, as do the neighboring proxy env tests and the rest of the file.

---

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

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b219b7d4-a24c-4c16-b60a-28e42a3ab5ac

📥 Commits

Reviewing files that changed from the base of the PR and between 0ffabf6 and 8a4c078.

📒 Files selected for processing (2)

• src/jsc/bindings/JSEnvironmentVariableMap.cpp
• test/js/node/process/process.test.js

---

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

[claude]

LGTM — targeted crash fix in the proxy-var setter with matching regression coverage.

What was reviewed:

• Verified the new isGetterSetter() branch: uncheckedDowncast<GetterSetter> is guarded by the type check, and putDirectAccessor is the correct call for a reified plain accessor (matches existing usage in JSMockFunction.cpp for replacing accessor slots).
• Checked that the attributes read from getDirect on a reified slot carry Accessor and not CustomAccessor, so putDirectAccessor(..., attributes & ~DontEnum) is valid.
• Traced both test scenarios through the Windows process.env Proxy traps (defineProperty/set/ownKeys/getOwnPropertyDescriptor) to confirm the assertions hold there too.
• Test is a spawned subprocess asserting the combined {stdout, stderr, exitCode}, so an unfixed debug build's assertion abort would fail it.

Extended reasoning...

Overview

The PR fixes a debug assertion / latent attribute corruption in jsSetterProxyEnvironmentVariable (src/jsc/bindings/JSEnvironmentVariableMap.cpp). When Object.defineProperty() reifies a proxy-var CustomAccessor into a plain GetterSetter and the native setter is subsequently invoked through the set wrapper, the DontEnum-clearing tail was passing that GetterSetter to putDirectCustomAccessor, which asserts value.isCustomGetterSetter(). The fix branches on what getDirect() returned: CustomGetterSetter → existing path; GetterSetter → putDirectAccessor; anything else → leave the user's slot alone. A regression test is added next to the existing proxy-var tests in test/js/node/process/process.test.js.

Security risks

None. This is a property-installation correctness fix on process.env; no untrusted input parsing, auth, or crypto is involved.

Level of scrutiny

Moderate. It touches JSC property-slot mechanics, which are subtle, but the change is ~15 lines confined to a single setter's tail and the mechanism is precisely explained. I confirmed uncheckedDowncast<T>(JSValue) is an established pattern in this codebase (widely used in generated bindings) and that putDirectAccessor(globalObject, name, GetterSetter*, attrs) is the right call for installing/replacing a plain accessor slot (same usage as spyOn in JSMockFunction.cpp). The reified slot's attributes from getDirect include Accessor and exclude CustomAccessor, so passing them (minus DontEnum) to putDirectAccessor is valid. The fall-through case (data property with a captured set wrapper) correctly leaves the user's configuration untouched rather than clobbering it.

Other factors

• The test spawns a subprocess with the proxy vars stripped from bunEnv, exercising the DontEnum-at-startup path, and asserts {stdout, stderr, exitCode} together so a debug assert abort is caught.
• I walked both scenarios through the Windows process.env Proxy (ProcessObjectInternals.ts): defineProperty forwards to internalEnv, set writes through the reified accessor, and ownKeys/getOwnPropertyDescriptor see the now-enumerable slot — the assertions hold there as well.
• The PR description already flags the interaction with #34727 (rejecting accessor descriptors on process.env); this fix is orthogonal and correct regardless.
• No prior reviewer comments to address; the bug-hunting system found nothing.

[claude]

No bugs found; the branch on isCustomGetterSetter() / isGetterSetter() looks correct and putDirectAccessor doesn't need the delete-first dance. Deferring to a human because this is JSC property-slot manipulation and the comment-cop bot has two open notes on the new inline comments that you may want to address before merge.

What was reviewed:

• uncheckedDowncast<GetterSetter>(JSValue) after isGetterSetter() — matches existing usage (e.g. BunJSCModule.h:702), and putDirectAccessor accepts a GetterSetter* and overwrites in place, so skipping the deleteProperty on that branch is fine.
• Exception scopes: RETURN_IF_EXCEPTION placement after deleteProperty / putDirectAccessor is consistent with the surrounding code.
• Test strips proxy vars from bunEnv so the DontEnum-at-startup path is actually exercised, and asserts {stdout, stderr, exitCode} together.

Extended reasoning...

Overview

The PR fixes a debug-build assertion (value.isCustomGetterSetter()) in jsSetterProxyEnvironmentVariable. When Object.defineProperty is applied to a proxy env var (HTTP_PROXY etc.), JSC reifies the CustomAccessor into a plain GetterSetter of wrapper functions; a subsequent assignment reaches the native setter through the wrapper, and the DontEnum-clearing tail then passed that GetterSetter to putDirectCustomAccessor, which requires a CustomGetterSetter. The fix branches on getDirect()'s result: CustomGetterSetter → existing path; GetterSetter → putDirectAccessor; anything else → leave the user's property alone. A subprocess test covers both the attribute-only redefine and the jest-style descriptor snapshot/restore.

Security risks

None. This is property-attribute bookkeeping on process.env's proxy-var accessors; no untrusted input parsing, no auth/crypto, no memory ownership changes.

Level of scrutiny

Medium-high. The diff is small (~15 lines of C++), but it manipulates JSC property slots directly — an area where the wrong putDirect* variant produces subtle attribute corruption rather than an immediate failure. That said, the change is narrow, well-explained, and guarded by explicit type checks, and the neighboring code already uses the same delete/re-install pattern.

Other factors

• The comment-cop GitHub Action left two open inline notes on the new multi-line comments (lines ~122 and ~129). They're automated style flags rather than correctness issues, but per the repo's own review rules ("one line") the author may want to trim them.
• putDirectAccessor does not assert NewProperty (unlike putDirectCustomAccessor), so the new branch correctly omits the deleteProperty step.
• The test mirrors the adjacent "proxy env vars assigned at runtime propagate…" test's setup (spawnSync, env stripping, combined-object assertion), so it fits harness conventions.

Given it's non-trivial JSC bindings code with open bot comments, I'm leaving the final call to a human.

[robobun]

CI status: the diff is green on every lane that ran tests (195/196 jobs in build 89623, 95+ test lanes including debian 13 x64 ASAN, which exercises the debug assertion this PR fixes).

The single red in each of the two builds is infrastructure, not this change: a darwin tart job hung at the checkout-sync step before running any test (build 89609: darwin 14 aarch64 on agent darwin-challah-arm64-tart-15, timed out after ~45 minutes; build 89623 after a retrigger: darwin 26 aarch64 on darwin-challah-arm64-tart-26, same hang, no automatic retry). All test-level failures in both builds passed on retry and are in files this change does not touch.

Not pushing further retriggers. Ready for review.

[robobun]

Closing: #31831 landed after this PR's base and removes the reachable path to this bug.

On current main, JSEnvironmentVariableMap::defineOwnProperty (and the Windows env Proxy's defineProperty trap) rejects accessor descriptors and anything other than a fully permissive data descriptor with ERR_INVALID_OBJECT_DEFINE_PROPERTY, and routes the accepted shape through put(). The proxy-var CustomGetterSetter therefore can no longer be reified into a GetterSetter by user code, which is the precondition for jsSetterProxyEnvironmentVariable seeing a GetterSetter. The isGetterSetter() branch added here is unreachable on main, and the test in this PR can no longer be written to fail without the change.

Verified on an unmodified debug+ASAN build of main (07d38c1), with the proxy vars absent from the OS environment:

• attribute-only redefine ({ enumerable: false }) then assign: defineProperty throws ERR_INVALID_OBJECT_DEFINE_PROPERTY, the assignment then succeeds, the var becomes enumerable, exit 0, no assertion
• descriptor snapshot, delete, restore, then assign: the restore throws (accessor descriptor), the assignment succeeds, exit 0, no assertion
• the accepted descriptor shape ({ value, writable, enumerable, configurable: true }) on a proxy var: routed through the custom setter with the CustomGetterSetter still in the slot, value and enumeration correct, exit 0

This PR's base (0ffabf6) reproduced the assertion on both of the first two scripts; the same scripts on main no longer reach the setter. If the defensive branch is still wanted as hardening, this can be reopened, but as a bug fix there is nothing left to fix.