From 522c0ff8808eb6b40065c070569e1920db3652f9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 10:31:39 +0000 Subject: [PATCH] require.extensions: only register a handler once the object accepted the store JSCommonJSExtensions::put and defineOwnProperty updated the native loader table before asking the base class to apply the operation and ignored its result, so a store refused by a frozen require.extensions, a non-writable or non-configurable entry, or a non-extensible object was still registered and require() dispatched to a handler the object never accepted. An attribute-only defineProperty (what Object.freeze performs on every entry) carried no value and was mirrored as an unregister, dropping every custom hook. All three mutators now apply the base operation first and mirror the entry only when its own value actually changed, reading the value back from the object. --- src/jsc/bindings/JSCommonJSExtensions.cpp | 57 ++++--- .../js/node/module/require-extensions.test.ts | 149 +++++++++++++++++- 2 files changed, 185 insertions(+), 21 deletions(-) diff --git a/src/jsc/bindings/JSCommonJSExtensions.cpp b/src/jsc/bindings/JSCommonJSExtensions.cpp index eb8baaf5dee3..361656d9bce2 100644 --- a/src/jsc/bindings/JSCommonJSExtensions.cpp +++ b/src/jsc/bindings/JSCommonJSExtensions.cpp @@ -145,12 +145,15 @@ extern "C" void NodeModuleModule__onRequireExtensionModifyNonFunction( Zig::GlobalObject* globalObject, const BunString* key); -void onAssign(Zig::GlobalObject* globalObject, JSC::PropertyName propertyName, JSC::JSValue value) +static bool isExtensionName(JSC::PropertyName propertyName) { - if (propertyName.isSymbol()) return; auto* name = propertyName.publicName(); - if (!name->startsWith('.')) return; - BunString ext = Bun::toString(name); + return name && name->startsWith('.'); +} + +static void onAssign(Zig::GlobalObject* globalObject, JSC::PropertyName propertyName, JSC::JSValue value) +{ + BunString ext = Bun::toString(propertyName.publicName()); JSC::CallData callData = JSC::getCallData(value); if (callData.type == JSC::CallData::Type::None) { return NodeModuleModule__onRequireExtensionModifyNonFunction(globalObject, &ext); @@ -172,33 +175,47 @@ void onAssign(Zig::GlobalObject* globalObject, JSC::PropertyName propertyName, J NodeModuleModule__onRequireExtensionModify(globalObject, &ext, loader, value); } -bool JSCommonJSExtensions::defineOwnProperty(JSC::JSObject* object, JSC::JSGlobalObject* globalObject, JSC::PropertyName propertyName, const JSC::PropertyDescriptor& descriptor, bool shouldThrow) +// require() dispatches on the native table, so the table mirrors this object's own +// entries. The entry is read back once the base class has applied the operation: a +// store the object refuses (frozen object, non-writable entry, non-extensible object) +// or a define that only changes attributes (Object.freeze) leaves the entry as it was, +// so it leaves the table alone too. That is what Node's require() observes as well, +// since it reads Module._extensions directly. +template +static bool mutateExtension(JSC::JSObject* object, JSC::JSGlobalObject* globalObject, JSC::PropertyName propertyName, Operation operation) { if (!isAllowedToMutateExtensions(globalObject)) return true; - JSValue value = descriptor.value(); - if (value) { - onAssign(defaultGlobalObject(globalObject), propertyName, value); - } else { - onAssign(defaultGlobalObject(globalObject), propertyName, JSC::jsUndefined()); + if (!isExtensionName(propertyName)) return operation(); + + JSC::VM& vm = JSC::getVM(globalObject); + JSC::JSValue before = object->getDirect(vm, propertyName); + if (!operation()) return false; + JSC::JSValue after = object->getDirect(vm, propertyName); + if (after != before) { + onAssign(defaultGlobalObject(globalObject), propertyName, after ? after : JSC::jsUndefined()); } - return Base::defineOwnProperty(object, globalObject, propertyName, descriptor, shouldThrow); + return true; +} + +bool JSCommonJSExtensions::defineOwnProperty(JSC::JSObject* object, JSC::JSGlobalObject* globalObject, JSC::PropertyName propertyName, const JSC::PropertyDescriptor& descriptor, bool shouldThrow) +{ + return mutateExtension(object, globalObject, propertyName, [&] { + return Base::defineOwnProperty(object, globalObject, propertyName, descriptor, shouldThrow); + }); } bool JSCommonJSExtensions::put(JSC::JSCell* cell, JSC::JSGlobalObject* globalObject, JSC::PropertyName propertyName, JSC::JSValue value, JSC::PutPropertySlot& slot) { - if (!isAllowedToMutateExtensions(globalObject)) return true; - onAssign(defaultGlobalObject(globalObject), propertyName, value); - return Base::put(cell, globalObject, propertyName, value, slot); + return mutateExtension(uncheckedDowncast(cell), globalObject, propertyName, [&] { + return Base::put(cell, globalObject, propertyName, value, slot); + }); } bool JSCommonJSExtensions::deleteProperty(JSC::JSCell* cell, JSC::JSGlobalObject* globalObject, JSC::PropertyName propertyName, JSC::DeletePropertySlot& slot) { - if (!isAllowedToMutateExtensions(globalObject)) return true; - bool deleted = Base::deleteProperty(cell, globalObject, propertyName, slot); - if (deleted) { - onAssign(defaultGlobalObject(globalObject), propertyName, JSC::jsUndefined()); - } - return deleted; + return mutateExtension(uncheckedDowncast(cell), globalObject, propertyName, [&] { + return Base::deleteProperty(cell, globalObject, propertyName, slot); + }); } extern "C" uint32_t JSCommonJSExtensions__appendFunction(Zig::GlobalObject* globalObject, JSC::JSValue value) diff --git a/test/js/node/module/require-extensions.test.ts b/test/js/node/module/require-extensions.test.ts index 7ca6fd493c20..1efd21d9f648 100644 --- a/test/js/node/module/require-extensions.test.ts +++ b/test/js/node/module/require-extensions.test.ts @@ -1,6 +1,6 @@ import assert from "assert"; import { expect, mock, test } from "bun:test"; -import { tempDir } from "harness"; +import { bunEnv, bunExe, tempDir } from "harness"; import path from "path"; test("require.extensions shape makes sense", () => { @@ -191,3 +191,150 @@ test("mutating extensions is banned by some files", () => { expect(globalThis.pass).toBe(n); } }); +test("a store refused by a non-writable entry does not reach the loader", () => { + using dir = tempDir("extensions-non-writable", { + "mod.locked": `module.exports = "loaded as javascript";`, + }); + const file = require.resolve(path.join(String(dir), "mod.locked")); + const load = () => { + delete require.cache[file]; + return require(file); + }; + const held = mock(module => { + module.exports = "held"; + }); + const refused = mock(module => { + module.exports = "refused"; + }); + + require.extensions[".locked"] = held; + try { + // Only the attributes change here, so the entry has to stay registered. + Object.defineProperty(require.extensions, ".locked", { writable: false }); + expect(load()).toBe("held"); + + // This file is a module, so the refused store throws rather than failing silently. + expect(() => { + require.extensions[".locked"] = refused; + }).toThrow(TypeError); + expect(Reflect.set(require.extensions, ".locked", refused)).toBe(false); + expect(require.extensions[".locked"]).toBe(held); + expect(load()).toBe("held"); + expect(refused).not.toHaveBeenCalled(); + expect(held).toHaveBeenCalledTimes(2); + } finally { + // The entry is still configurable, so it can be removed. + delete require.extensions[".locked"]; + } + expect(".locked" in require.extensions).toBe(false); + expect(load()).toBe("loaded as javascript"); + delete require.cache[file]; +}); +test.concurrent("stores refused by a frozen require.extensions do not reach the loader", async () => { + using dir = tempDir("extensions-frozen", { + "index.cjs": ` + const assert = require("assert"); + const calls = []; + require.extensions[".hooked"] = m => { calls.push("kept"); m.exports = "kept"; }; + const originalJs = require.extensions[".js"]; + Object.freeze(require.extensions); + + // This file is sloppy mode, so the frozen object refuses these silently. + require.extensions[".js"] = m => { calls.push(".js"); m.exports = "refused .js hook"; }; + require.extensions[".hooked"] = m => { calls.push(".hooked"); m.exports = "refused .hooked hook"; }; + require.extensions[".added"] = m => { calls.push(".added"); m.exports = "refused .added hook"; }; + assert.strictEqual(require.extensions[".js"], originalJs); + assert.strictEqual(Object.hasOwn(require.extensions, ".added"), false); + + console.log(JSON.stringify({ + js: require("./plain.js"), + jsx: require("./classic-jsx.js"), + hooked: require("./mod.hooked"), + added: require("./mod.added"), + calls, + })); + `, + "plain.js": `module.exports = "plain";`, + // Object.freeze redefines the attributes of every builtin entry. The builtin ".js" entry + // must stay builtin afterwards: the default loader for .js files accepts JSX, while an + // explicitly registered require.extensions[".js"] handler does not. + "classic-jsx.js": ` + /* @jsxRuntime classic */ + /* @jsx h */ + function h(tag) { return "jsx:" + tag; } + module.exports =
; + `, + "mod.hooked": `module.exports = "loaded as javascript";`, + "mod.added": `module.exports = "loaded as javascript";`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "index.cjs"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual({ + js: "plain", + jsx: "jsx:div", + hooked: "kept", + added: "loaded as javascript", + calls: ["kept"], + }); + expect(exitCode).toBe(0); +}); +test.concurrent( + "stores refused by a non-configurable entry or a non-extensible require.extensions do not reach the loader", + async () => { + using dir = tempDir("extensions-non-configurable", { + "index.cjs": ` + const assert = require("assert"); + const calls = []; + const refused = how => m => { calls.push(how); m.exports = "refused " + how; }; + + require.extensions[".pinned"] = m => { calls.push("held"); m.exports = "held"; }; + Object.defineProperty(require.extensions, ".pinned", { configurable: false, writable: false }); + assert.throws(() => Object.defineProperty(require.extensions, ".pinned", { value: refused("defineProperty") }), TypeError); + assert.strictEqual(Reflect.defineProperty(require.extensions, ".pinned", { value: refused("Reflect.defineProperty") }), false); + require.extensions[".pinned"] = refused("assignment"); + assert.strictEqual(delete require.extensions[".pinned"], false); + + Object.preventExtensions(require.extensions); + require.extensions[".fresh"] = refused("assignment of a new entry"); + assert.throws(() => Object.defineProperty(require.extensions, ".defined", { value: refused("defineProperty of a new entry") }), TypeError); + assert.strictEqual(Object.hasOwn(require.extensions, ".fresh"), false); + assert.strictEqual(Object.hasOwn(require.extensions, ".defined"), false); + + console.log(JSON.stringify({ + pinned: require("./mod.pinned"), + fresh: require("./mod.fresh"), + defined: require("./mod.defined"), + calls, + })); + `, + "mod.pinned": `module.exports = "loaded as javascript";`, + "mod.fresh": `module.exports = "loaded as javascript";`, + "mod.defined": `module.exports = "loaded as javascript";`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "index.cjs"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual({ + pinned: "held", + fresh: "loaded as javascript", + defined: "loaded as javascript", + calls: ["held"], + }); + expect(exitCode).toBe(0); + }, +);