Skip to content

Fix .prototype pollution on JSC by passing JSClass to JSObjectMakeConstructor#177

Merged
bghgary merged 5 commits into
BabylonJS:mainfrom
bghgary:jrh-172-jsc-prototype-fix
Jun 3, 2026
Merged

Fix .prototype pollution on JSC by passing JSClass to JSObjectMakeConstructor#177
bghgary merged 5 commits into
BabylonJS:mainfrom
bghgary:jrh-172-jsc-prototype-fix

Conversation

@bghgary
Copy link
Copy Markdown
Contributor

@bghgary bghgary commented Jun 3, 2026

[Created by Copilot on behalf of @bghgary]

Context

Closes #172.

JSC's JSObjectMakeConstructor defaults the constructor's .prototype property to the global Object.prototype when invoked with a null JSClassRef, then installs it with the ReadOnly attribute. That gives us two distinct bugs:

  1. Pollution. Every napi-defined class shares Object.prototype. Writing to Foo.prototype.x mutates Object.prototype.x; {} instanceof Foo returns true for every class.
  2. Silent failure on retry. The ReadOnly bit means JSObjectSetProperty(constructor, "prototype", ...) cannot overwrite the default afterward — the write is silently dropped (or throws in strict mode).

Fix

Pass the existing per-constructor JSClassRef — the one we already create for the sentinel — into JSObjectMakeConstructor. WebKit then assigns a fresh per-class JSCallbackObject (built from the class's auto-generated prototypeClass) as .prototype, sidestepping both bugs at the source.

The vestigial "extra prototype" hack in ConstructorInfo::Create — kept since #101 with a TODO to remove — goes away. Two ancillary call sites needed a small update because they were reading the [[Prototype]] internal slot when they meant the .prototype property:

  • CallAsConstructor chains instance.__proto__ from constructor.prototype (the property), not constructor.__proto__ (the slot = Function.prototype).
  • napi_define_class installs instance properties on constructor.prototype via napi_get_named_property. napi_get_prototype keeps its N-API-spec contract of returning the [[Prototype]] slot and is unchanged.

Tests

Added a napi class prototype isolation describe block using Blob (a napi-defined class available on every engine). Six invariants — all standard JS semantics that V8/Chakra already satisfy and that JSC fails today:

  • Blob.prototype !== Object.prototype
  • Object.getPrototypeOf(Blob.prototype) === Object.prototype
  • Blob.prototype.constructor === Blob
  • Object.getPrototypeOf(new Blob([])) === Blob.prototype (and instanceof true)
  • ({} instanceof Blob) === false
  • Writes to Blob.prototype do not appear on {}

Lifecycle note

JSCallbackConstructor does not invoke the JSClass finalize callback; only JSCallbackObject does. The auto-prototype WebKit builds from _class->prototype() uses a separate prototypeClass that explicitly clears finalize. So sharing _class between the constructor, the sentinel, and the auto-prototype carries no double-free risk — only the sentinel ever calls Finalize on our info pointer.

Pass the per-constructor JSClass (already used for the sentinel) into
JSObjectMakeConstructor. WebKit then assigns a fresh per-class
JSCallbackObject as the constructor's `.prototype` property instead of
defaulting it to the global Object.prototype — eliminating both the
pollution bug (writes to `Foo.prototype` mutate `Object.prototype`) and
the silent-failure-on-write bug (JSC installs the default .prototype
with the ReadOnly attribute, so `JSObjectSetProperty(constructor,
"prototype", ...)` cannot overwrite it after the fact).

Changes:
- `ConstructorInfo::Create`: construct `ConstructorInfo` first so
  `info->_class` exists, pass it to `JSObjectMakeConstructor`, and
  install a `constructor` back-reference on the fresh prototype with
  ECMAScript-spec attributes (DontEnum only -- not ReadOnly|DontDelete).
- `CallAsConstructor`: chain instances through the `.prototype` property
  (where instance methods live) instead of the [[Prototype]] internal
  slot (which points at Function.prototype).
- `napi_define_class`: install instance properties on the `.prototype`
  property directly via `napi_get_named_property`. `napi_get_prototype`
  returns the [[Prototype]] slot per N-API spec -- Function.prototype
  for our constructors -- and is unchanged.
- Removed the long-vestigial "extra prototype" hack (TODO since BabylonJS#101
  switched sentinel attachment to a non-enumerable symbol property).

Also fixes a latent correctness bug: `{} instanceof Foo` previously
returned true for every napi-defined class because `Foo.prototype` was
literally `Object.prototype`, which is in every object's prototype
chain.

`JSCallbackConstructor` does not invoke the JSClass `finalize` callback
(only `JSCallbackObject` does), and the auto-prototype returned by
`_class->prototype()` is built from a separate `prototypeClass` whose
definition WebKit explicitly clears `finalize` on -- so sharing
`_class` between the constructor, the sentinel, and the auto-prototype
carries no double-free risk.

Closes BabylonJS#172.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 3, 2026 15:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a JavaScriptCore-specific N-API class constructor bug where JSObjectMakeConstructor (when passed a null JSClassRef) causes all N-API constructors to share Object.prototype as their .prototype, leading to global prototype pollution and incorrect instanceof behavior. The change ensures each N-API-defined class gets a fresh per-class .prototype object, and updates call sites that incorrectly used the constructor’s [[Prototype]] slot instead of the .prototype property.

Changes:

  • Create JSC constructors with a non-null JSClassRef so .prototype is per-class (fixes pollution + read-only .prototype issue).
  • Fix instance creation and napi_define_class to use constructor.prototype (property) rather than constructor.__proto__ / [[Prototype]] (slot).
  • Add regression tests asserting per-class prototype isolation using Blob.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Tests/UnitTests/Scripts/tests.ts Adds regression tests for per-class .prototype isolation and correct instanceof behavior.
Core/Node-API/Source/js_native_api_javascriptcore.cc Fixes JSC constructor creation to produce a fresh .prototype, and updates prototype usage in constructor calls and napi_define_class.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Core/Node-API/Source/js_native_api_javascriptcore.cc
Comment thread Core/Node-API/Source/js_native_api_javascriptcore.cc
Chakra's napi_wrap inserts an external object between the instance and the prototype set by the constructor, so Object.getPrototypeOf(new Blob([])) !== Blob.prototype at depth 1 on Chakra (by design). isPrototypeOf walks the chain and passes on every backend.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary and others added 2 commits June 3, 2026 08:57
1. Create the sentinel immediately after the constructor so JSC's GC
   takes ownership of `info`. Without this, any of the three CHECK_JSC
   early-returns in the prototype wiring would leak `info` (introduced
   by moving the new ConstructorInfo allocation above those CHECK_JSC
   calls so info->_class is available for JSObjectMakeConstructor).

2. Per ECMAScript GetPrototypeFromConstructor, fall back to
   %Object.prototype% when constructor.prototype is not an object.
   JSObjectMake(ctx, nullptr, nullptr) already gives the new instance
   that default, so guarding the SetPrototype call is enough.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fallback added in 3f919d6 is dead code: JSObjectMakeConstructor
installs .prototype with ReadOnly | DontDelete | DontEnum, so neither
JS (Foo.prototype = 5, Object.defineProperty) nor any N-API public
surface can reassign the property to a non-object. JSObjectGetProperty
is guaranteed to return the original per-class prototype object.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary bghgary merged commit 9cd456c into BabylonJS:main Jun 3, 2026
23 checks passed
bghgary added a commit to bghgary/JsRuntimeHost that referenced this pull request Jun 3, 2026
With this PR's Chakra napi_wrap fix in place, Object.getPrototypeOf(blob)
now returns Blob.prototype directly on Chakra (no hidden external object
interposed in the chain), so the loose isPrototypeOf walk added in BabylonJS#177
can be tightened to a strict ===. Drops the TODO(BabylonJS#178) note and the
redundant instanceof check that was only there as a sanity backup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft added a commit to bkaradzic-microsoft/JsRuntimeHost that referenced this pull request Jun 3, 2026
…onJS#177 landed

BabylonJS#177 fixed the JSC napi shim's Object.prototype pollution by passing the
per-class JSClassRef to JSObjectMakeConstructor, so napi-defined classes
now get a real per-class .prototype object instead of aliasing the global
Object.prototype.

That removes the entire reason for the JS_PROTOTYPE_CHAIN_SHIM and the
Napi::Eval / try/catch dance: we can wire File.prototype's [[Prototype]]
to Blob.prototype with a direct Object.setPrototypeOf call. Drops ~50
lines of explanatory comment + shim + Eval-with-IsExceptionPending guard
in File::Initialize down to a 4-line napi call.

`file instanceof Blob` regression coverage remains in tests.ts:1564 and
BabylonJS#177's `describe("napi class prototype isolation (BabylonJS#172)")` block at
tests.ts:1271 (uses Blob — and File extends Blob — to assert that
napi-defined classes don't share Object.prototype). Local Chakra build
clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary added a commit that referenced this pull request Jun 3, 2026
[Created by Copilot on behalf of @bghgary]

Fixes #178.

## Context

Chakra's `napi_wrap` inserted the external object into the wrapped
value's prototype chain, leaving `Object.getPrototypeOf(new Foo())`
returning that external rather than `Foo.prototype`. V8 and JSC don't do
this; their `napi_wrap` is transparent to the prototype chain.

## Fix

Attach the external as a hidden own property keyed by
`"\x01napi_external"`, defined non-enumerable + non-writable +
configurable so it stays out of `for..in` / `Object.keys` /
`JSON.stringify`, can't be silently overwritten by assignment, but can
still be removed by `napi_remove_wrap`.

`FindWrapper` / `Unwrap` / `napi_remove_wrap` switch from
prototype-chain walking / splicing to property lookup / deletion. Public
N-API ABI is unchanged.

## Validation

Once this and #177 both land, the workaround in #177's test 4
(`Blob.prototype.isPrototypeOf(blob)`) can be tightened back to
`Object.getPrototypeOf(blob) === Blob.prototype`.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft added a commit to bkaradzic-microsoft/JsRuntimeHost that referenced this pull request Jun 4, 2026
…onJS#177 landed

BabylonJS#177 fixed the JSC napi shim's Object.prototype pollution by passing the
per-class JSClassRef to JSObjectMakeConstructor, so napi-defined classes
now get a real per-class .prototype object instead of aliasing the global
Object.prototype.

That removes the entire reason for the JS_PROTOTYPE_CHAIN_SHIM and the
Napi::Eval / try/catch dance: we can wire File.prototype's [[Prototype]]
to Blob.prototype with a direct Object.setPrototypeOf call. Drops ~50
lines of explanatory comment + shim + Eval-with-IsExceptionPending guard
in File::Initialize down to a 4-line napi call.

`file instanceof Blob` regression coverage remains in tests.ts:1564 and
BabylonJS#177's `describe("napi class prototype isolation (BabylonJS#172)")` block at
tests.ts:1271 (uses Blob — and File extends Blob — to assert that
napi-defined classes don't share Object.prototype). Local Chakra build
clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft added a commit to bkaradzic-microsoft/JsRuntimeHost that referenced this pull request Jun 4, 2026
…ment

Per bghgary's review nit: the "BabylonJS#177 fixed JSC .prototype pollution"
context is now in main's git history, doesn't need to live in the
comment block. Comment now focuses on WHY we wire File→Blob, not on
which past bug enabled the wiring to be one-liner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSC napi shim: writing to constructor.prototype pollutes global Object.prototype

3 participants