require.extensions: do not register a handler the object refused to store

[robobun]

Problem

• A store to require.extensions / Module._extensions that the object itself refuses still registers the handler with the loader, so require() dispatches to a function that never became an entry of the object. Refused stores that reproduce this on bun 1.4.0 and on main: assignment to a frozen require.extensions or to an entry made non-writable (silently refused in sloppy mode, TypeError in strict mode), Reflect.set returning false, Object.defineProperty / Reflect.defineProperty on a non-configurable entry, and assignment or defineProperty of a new entry on a non-extensible object. Node dispatches to the entry the object actually holds in every one of these cases.
• The same code also handles defineProperty calls that only change attributes as an unregister, because the descriptor carries no value. Object.freeze(require.extensions) performs exactly such a call for every entry, so on 1.4.0 freezing the object silently drops every custom hook registered before it (a .hooked handler registered and then frozen in place is ignored and the file is loaded as JavaScript); Object.defineProperty(require.extensions, ".x", { writable: false }) drops the .x hook the same way.
• Cause: JSCommonJSExtensions::put and JSCommonJSExtensions::defineOwnProperty (src/jsc/bindings/JSCommonJSExtensions.cpp) called onAssign, which writes the native table, before Base::put / Base::defineOwnProperty decided whether the operation is allowed, and ignored the result. defineOwnProperty additionally mirrored descriptor.value(), which is empty for attribute-only descriptors. deleteProperty already did it the other way round.

Fix

• put, defineOwnProperty and deleteProperty now share one helper: read the object's own value for the key, apply the base class operation, and if it succeeded and the own value is now a different value, mirror that value (or its absence) into the native table.
• Why this is correct: require() in Node reads Module._extensions[ext] at load time, so what it dispatches on is, by definition, the entry the object holds. Bun dispatches on a native table instead, and the table is only correct while it tracks the entries the object holds. Reading the entry back after the base class has applied the operation makes the table follow the object whatever the outcome: a refused store leaves the entry unchanged and is not mirrored, an attribute-only define leaves the value unchanged and is not mirrored, a delete or an accepted store changes it and is mirrored. Mirroring the assigned value on success would have been enough for refused stores, but Object.freeze would then have re-registered every builtin entry as an explicit handler, and an explicitly registered .js handler loads files with the JSX-less loader (test/regression/issue/require-extensions-override.test.ts pins that), so freezing would have changed how .js files load. The read-back keeps freeze a no-op for dispatch, as it is in Node.
• Behavior that is unchanged: accepted stores, restores of a builtin handler, deletes, the packages that are not allowed to mutate the object (isAllowedToMutateExtensions still runs first for every key), and accessor descriptors (still mirrored as an unregister, the limitation tracked with the Module._extensions work in node:module: route CJS entrypoint and CJS-via-ESM-import through Module._extensions #35774). Keys that do not start with . go straight to the base class as before.
• Independent of require.extensions: assignments from a repeated statement no longer bypass the loader table #38854 (which makes every execution of an assignment reach put; this PR changes what put does once reached). The two were built together locally and both test files pass.
• Verified with test/js/node/module/require-extensions.test.ts: three new tests. One in-process test makes a custom entry non-writable, checks it stays registered, and checks that a throwing assignment and a false-returning Reflect.set do not change what require() dispatches to; one spawned sloppy-mode test freezes the object and checks that refused stores to a builtin entry, a custom entry and a new entry are not dispatched to, that the hook registered before the freeze survives it, and that a .js file using JSX still loads afterwards; one spawned test covers a non-configurable entry (rejected defineProperty, Reflect.defineProperty, assignment and delete) and a non-extensible object (rejected assignment and defineProperty of new entries). On 1.4.0 all three fail (the refused handlers are dispatched to and the frozen hook is dropped); with this change the file passes 13/13. The fixtures of the two spawned tests print the same values under Node (without the JSX file).
• Also run with the debug build: the rest of test/js/node/module/, test/regression/issue/require-extensions-override.test.ts, test/regression/issue/22929-module-extensions-asi.test.ts, Node's test-module-multi-extensions.js, test-require-extensions-main.js and test-module-main-extension-lookup.js, and the new tests under BUN_JSC_validateExceptionChecks=1: all pass.

Background

• require.extensions is one JSCommonJSExtensions object per global. Its properties are only a mirror: the table require() actually consults is commonjs_custom_extensions in src/jsc/NodeModuleModule.rs, written through onAssign -> NodeModuleModule__onRequireExtensionModify / ...NonFunction and read by Bun__transpileFile. Builtin entries are not in the table until someone assigns them; assigning one of the builtin handler functions registers the corresponding fixed loader (the .js handler registers the JS loader, while an untouched .js entry uses the default loader for .js files, which also accepts JSX).
• put / defineOwnProperty / deleteProperty are JSC's per-class hooks for [[Set]], [[DefineOwnProperty]] and [[Delete]]. The base class versions implement the language rules (non-writable, non-configurable, non-extensible) and return false, or throw when the caller asked for that, when the operation is refused. getDirect reads an own property straight out of the object's storage without running any JavaScript, which is why it is safe to call before and after the operation.
• Object.freeze(obj) is [[PreventExtensions]] followed by a [[DefineOwnProperty]] on every own property with { configurable: false, writable: false }, so it reaches defineOwnProperty once per entry with a descriptor that has no value.

Repro on 1.4.0

// a.js: module.exports = "builtin";
const original = require.extensions[".js"];
Object.freeze(require.extensions);
let invoked = false;
require.extensions[".js"] = m => { invoked = true; m.exports = "hook"; }; // sloppy mode: silently refused
console.log(require.extensions[".js"] === original);   // true in both
console.log(require("./a.js"), invoked);               // bun 1.4.0: "hook" true    node: "builtin" false

// mod.hooked: module.exports = "loaded as javascript";
require.extensions[".hooked"] = m => { m.exports = "kept"; };
Object.freeze(require.extensions);
console.log(require("./mod.hooked"));                  // bun 1.4.0: "loaded as javascript"    node: "kept"

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 8 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: c12eca07-a39f-44e1-a708-881310cdeb9c

📥 Commits

Reviewing files that changed from the base of the PR and between 732491c and 522c0ff.

📒 Files selected for processing (2)

• src/jsc/bindings/JSCommonJSExtensions.cpp
• test/js/node/module/require-extensions.test.ts

---

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

[robobun]

Status: reproduced and fixed, waiting on CI.

Reproduced on bun 1.4.0 (USE_SYSTEM_BUN=1 bun test test/js/node/module/require-extensions.test.ts): the three new tests fail there (a refused assignment to a frozen require.extensions, to a non-writable entry, to a non-configurable entry and to a non-extensible object is still dispatched to by require(), and Object.freeze(require.extensions) drops the hooks registered before it). With this branch the file passes 13/13 on a debug build; the fixtures print the same values under Node.

[claude]

I reviewed this PR and didn't find any bugs. The change is small and well-reasoned, but it rewires the JSC method-table hooks (put/defineOwnProperty/deleteProperty) that keep the native require() dispatch table in sync, so a maintainer familiar with JSCommonJSExtensions / NodeModuleModule should give it a look.

What was reviewed:

• mutateExtension control flow: refused ops return false before the second getDirect, so a pending TypeError from strict-mode Base::put/defineOwnProperty is propagated without touching the table.
• isExtensionName's publicName() null-check subsumes the old isSymbol() guard; non-. keys still gate on isAllowedToMutateExtensions before falling through to the base op, matching the old order.
• The after != before short-circuit means a same-value re-assignment (and Object.freeze's attribute-only redefines) no longer call onAssign — the PR argues this is required so freeze doesn't flip builtin .js to the JSX-less loader; the new classic-jsx.js fixture pins that.
• Accessor descriptors: getDirect returns the GetterSetter cell, getCallData on it is None, so it still unregisters — same limitation as before, no regression.

Extended reasoning...

Overview

The PR touches one C++ file (src/jsc/bindings/JSCommonJSExtensions.cpp) and adds three tests to test/js/node/module/require-extensions.test.ts. It fixes two related bugs: (1) a store to require.extensions that the object refuses (frozen, non-writable, non-extensible) still updated the native dispatch table, so require() would call a handler that never became a property of the object; (2) an attribute-only defineProperty (as Object.freeze performs on every key) was treated as an unregister because descriptor.value() is empty. The fix collapses put/defineOwnProperty/deleteProperty into a shared mutateExtension helper that reads the own value with getDirect before and after the base-class operation and only mirrors into the native table when the operation succeeded and the stored value actually changed.

Security risks

None identified. No untrusted input parsing, no allocation, no path handling. The change narrows when the native table is updated; it cannot cause a handler to be registered that wasn't before.

Level of scrutiny

Medium-high. The diff is ~30 net lines, but it sits on the JSC method-table hooks for a per-global object whose native mirror is what CommonJS require() actually consults. Subtleties I checked: exception propagation (a throwing Base::put returns false, so we bail before the second getDirect and never call onAssign with an exception pending — matches the author's BUN_JSC_validateExceptionChecks=1 claim), empty-vs-empty JSValue equality (two default JSValue() compare equal, so deleting a non-existent key or a rejected new-key store is a no-op), and the uncheckedDowncast<JSCommonJSExtensions>(cell) used to get a JSObject* for getDirect (safe: these static hooks are only reached on cells of this class, and Base = JSDestructibleObject is a JSObject).

Other factors

The one behavior change beyond the headline fix is that after != before now skips onAssign when the same value is re-assigned. The PR description argues convincingly that this is required (otherwise Object.freeze would re-register every builtin as an explicit loader and switch .js from the default JSX-accepting loader to the strict JS loader), and the new test's classic-jsx.js fixture pins it. Existing tests that restore require.extensions['.js'] = original after overriding it still trigger onAssign because before (the mock) ≠ after (the builtin), so no regression there. Test coverage is thorough across the variant matrix (frozen / non-writable / non-configurable / non-extensible, strict throw vs sloppy silent vs Reflect.set false, in-process and spawned), and the author reports the full test/js/node/module/ suite plus the two related regression tests pass. Given the subtlety of the JSC-bindings surface and the intentional secondary behavior change, a human sign-off from someone who owns this code is still worthwhile.

[robobun]

Confirming the one behavior change the review calls out is intentional: a store that leaves the entry holding the same value (a same-value re-assignment, or the attribute-only redefines Object.freeze performs) no longer touches the native table. That is what keeps freezing the object from re-registering the builtin entries as explicit handlers; the frozen-object test pins it with the JSX fixture.

[robobun]

^{Updated 7:05 AM PT - Aug 15th, 2026}

❌ @robobun, your commit 522c0ff has some failures in Build #97908 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38991

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

bun-38991 --bun

[robobun]

^{Updated 10:12 AM PT - Aug 15th, 2026}

✅ @robobun, your commit 522c0ff8808eb6b40065c070569e1920db3652f9 passed in Build #97908! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38991

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

bun-38991 --bun