From 6471843d722c52168c22ab238525e8aeb3171ee6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87?= Date: Tue, 2 Jun 2026 16:55:00 -0700 Subject: [PATCH] JSC napi: stop polluting global Object.prototype from napi_define_class JSObjectMakeConstructor(ctx, nullptr, ...) returns a function whose `prototype` property points at the global Object.prototype. Any napi caller that writes to constructor.prototype (either internally via napi_define_class for instance members, or in user code via ctor.Get("prototype").Set(...)) was mutating Object.prototype itself, breaking `'X' in {}` and `for..in` across the whole runtime. The previous workaround set the constructor's `[[Prototype]]` internal slot to a fresh object and re-routed napi_get_prototype + CallAsConstructor through it. That kept napi_define_class out of Object.prototype but left the user-visible `.prototype` property still pointing at Object.prototype, so user code (e.g. the File polyfill in JsRH#169) hit the pollution. Fix: - ConstructorInfo::Create installs a fresh per-class prototype object as the constructor's `.prototype` PROPERTY (DontEnum|DontDelete) with a non-enumerable `.constructor` back-reference, matching ECMAScript semantics for normal constructors. - CallAsConstructor sets the new instance's [[Prototype]] from the constructor's `.prototype` property, not from its [[Prototype]] slot. - napi_define_class looks up the `.prototype` property directly instead of going through napi_get_prototype (which per spec returns the [[Prototype]] internal slot = Function.prototype for a constructor). Adds a regression test using Blob (which uses InstanceAccessor + InstanceMethod) to assert Blob.prototype is a fresh object and no Blob instance members leak onto the global Object.prototype. Fixes #172. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Source/js_native_api_javascriptcore.cc | 45 +++++++++++++++---- Tests/UnitTests/Scripts/tests.ts | 17 +++++++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 2d0ab4ab..af8124ac 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -323,17 +323,24 @@ namespace { void* data, napi_value* result) { JSObjectRef constructor{JSObjectMakeConstructor(env->context, nullptr, CallAsConstructor)}; - // BEGIN TODO: This extra prototype should no longer be needed, but for some reason removing it leads to errors - // when setting properties on some prototypes. This should be investigated and removed. + + // JSObjectMakeConstructor(ctx, nullptr, ...) leaves the resulting function's + // `prototype` property pointing at the global `Object.prototype`. Writing to + // it (e.g. via napi_define_properties for instance members, or user code via + // ctor.Get("prototype").Set(...)) would pollute the global Object.prototype + // and corrupt every object in the runtime. Install a fresh per-class + // prototype object whose `[[Prototype]]` defaults to Object.prototype and + // whose `constructor` back-reference points at the new constructor. JSObjectRef prototype{JSObjectMake(env->context, nullptr, nullptr)}; - JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); - JSObjectSetPrototype(env->context, constructor, prototype); JSValueRef exception{}; JSObjectSetProperty(env->context, prototype, JSString("constructor"), constructor, - kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, &exception); + kJSPropertyAttributeDontEnum, &exception); + CHECK_JSC(env, exception); + + JSObjectSetProperty(env->context, constructor, JSString("prototype"), prototype, + kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, &exception); CHECK_JSC(env, exception); - // END TODO ConstructorInfo* info{new ConstructorInfo(env, utf8name, length, cb, data)}; if (info == nullptr) { @@ -379,8 +386,20 @@ namespace { // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(env); + // New instance's [[Prototype]] is the constructor's `prototype` property + // (the fresh per-class object installed by ConstructorInfo::Create), NOT + // the constructor's [[Prototype]] slot (which is Function.prototype). + JSValueRef protoValue{JSObjectGetProperty(ctx, constructor, JSString("prototype"), exception)}; + if (*exception) { + return nullptr; + } + JSObjectRef prototype{JSValueToObject(ctx, protoValue, exception)}; + if (*exception) { + return nullptr; + } + JSObjectRef instance{JSObjectMake(ctx, nullptr, nullptr)}; - JSObjectSetPrototype(ctx, instance, JSObjectGetPrototype(ctx, constructor)); + JSObjectSetPrototype(ctx, instance, prototype); napi_callback_info__ cbinfo{}; cbinfo.thisArg = ToNapi(instance); @@ -932,8 +951,16 @@ napi_status napi_define_class(napi_env env, } if (instancePropertyCount > 0) { - napi_value prototype{}; - CHECK_NAPI(napi_get_prototype(env, constructor, &prototype)); + // Look up the constructor's `prototype` property (the per-class object set up + // by ConstructorInfo::Create), not napi_get_prototype which per spec returns + // the [[Prototype]] internal slot (Function.prototype for constructors). + JSValueRef exception{}; + JSValueRef protoValue{JSObjectGetProperty(env->context, ToJSObject(env, constructor), + JSString("prototype"), &exception)}; + CHECK_JSC(env, exception); + JSObjectRef prototypeObj{JSValueToObject(env->context, protoValue, &exception)}; + CHECK_JSC(env, exception); + napi_value prototype{ToNapi(prototypeObj)}; CHECK_NAPI(napi_define_properties(env, prototype, diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index a08121d7..a4001664 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1199,6 +1199,23 @@ describe("Blob", function () { expect(modelGltfJson.type).to.equal("model/gltf+json"); }); + // -------------------------------- Prototype isolation (regression: JsRH#172) -------------------------------- + it("Blob.prototype is a fresh per-class object, not Object.prototype", function () { + expect(Blob.prototype).to.not.equal(Object.prototype); + expect(Object.getPrototypeOf(Blob.prototype)).to.equal(Object.prototype); + expect(Object.getPrototypeOf(new Blob([]))).to.equal(Blob.prototype); + }); + + it("does not leak instance members onto the global Object.prototype", function () { + // Blob's instance accessors live on Blob.prototype. Before the JSC napi-shim + // fix in JsRH#172, napi_define_class wrote them onto the global Object.prototype, + // corrupting every object in the runtime. + for (const key of ["size", "type", "arrayBuffer", "text", "bytes"]) { + expect(key in {}).to.equal(false, `Object.prototype was polluted with '${key}'`); + expect(Object.prototype.hasOwnProperty.call(Object.prototype, key)).to.equal(false); + } + }); + // -------------------------------- Blob.text() -------------------------------- it("returns empty string for empty blobs", async function () { for (const blob of emptyBlobs) {