Fix napi_get_property_names conformance on JSC, Chakra and QuickJS

[bkaradzic-microsoft]

Fixes #216.napi_get_property_names is specified to return the enumerable string-keyed properties of an object and of its prototype chain — the same set a for...in loop visits. Only the V8 backend did that. Every other backend diverged:| backend | enumerable-only | includes prototypes | works at all ||---|---|---|---|| V8 | yes | yes | yes || JavaScriptCore | n/a | n/a | no — always threw || Chakra | no | no | yes || QuickJS | yes | no | yes || JSI | n/a | n/a | no — TODO stub |- JavaScriptCore called Object.getOwnPropertyNames with argc 0, so the object argument was never used and the call always threw TypeError: undefined is not an object.- Chakra used JsGetOwnPropertyNames, which is own-only and also reports non-enumerable properties.- QuickJS used JS_GetOwnPropertyNames with JS_GPN_ENUM_ONLY — enumerable-only, but still own-only.- JSI (found while fixing the above) had Object::GetPropertyNames as an unimplemented stub that threw std::runtime_error{"TODO"}.## ApproachNone of JSC, Chakra or QuickJS exposes a native equivalent of V8's kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS key collection, so rather than write three subtly different prototype walks, this adds one shared implementation (Source/js_native_api_shared.{h,cc}) written purely against the public napi_* surface, and has all three delegate to it.Per prototype level it takes:- Object.keys — the properties for...in reports at that level, already in specification order;- Object.getOwnPropertyNames — the shadowing set. A non-enumerable own property is not reported itself, but it does hide a same-named enumerable property further up the chain.JSObjectCopyPropertyNames was considered for JavaScriptCore since it walks the chain natively, but it does not apply that shadowing rule. JSObject::getPropertyNames calls getOwnPropertyNames per level with DontEnumPropertiesMode::Exclude, so a non-enumerable own property is never added to the array and cannot suppress a same-named enumerable property further up — the inherited name is reported where for...in correctly omits it. The shared walk is therefore used there too, and all backends stay consistent. The input is coerced with napi_coerce_to_object, matching V8's CHECK_TO_OBJECT.JSI keeps its own path: jsi::Object::getPropertyNames already has exactly the specified semantics, so Object::GetPropertyNames just forwards to it.## Termination and error reportingTwo defects in the shared walk, both raised in review:Cyclic prototype chains hung the process. A getPrototypeOf Proxy trap may return an object already on the chain — the invariants on that trap constrain only the non-extensible case, so Object.getPrototypeOf(p) === p is reachable from script today. V8 survives it because its walk is recursive and dies on the stack with a RangeError; this walk is iterative, so it spun. Reproduced against a real build before fixing: the test process burned 56 seconds of CPU and never returned. No JavaScript-level timeout can preempt this, since control never re-enters the engine — it would have taken CI down rather than failing a test.The walk now stops when it reaches a level it has already visited. That is exact rather than a bail-out: every level adds its full Object.getOwnPropertyNames set to the shadowing set before the walk advances, and in a cycle that branch is always taken, so a level reached a second time can only re-encounter names that are already shadowed. Breaking there yields precisely the fixed point the non-terminating walk converges on.The returned status disagreed with napi_get_last_error_info. The shared walk is written against the public napi_* surface, so it cannot reach napi_set_last_error; CHECK_NAPI only propagates the status, and the napi_typeof performed immediately before the rejection calls napi_clear_last_error on its way out. A caller therefore saw napi_object_expected returned while the recorded error code still read napi_ok. The success path had the mirror-image problem — it left whatever error a previous call had recorded in place. Both directions are now handled explicitly at all three call sites.## Two further bugs this uncovered1. napi_get_prototype on JavaScriptCore (fixed here — the shared walk depends on it). It ran the result of JSObjectGetPrototype through JSValueToObject. At the top of a prototype chain that value is null, so the conversion threw TypeError: null is not an object instead of reporting the end of the chain, making the chain impossible to walk. It had no other callers in this repository — Blob.cpp deliberately uses Object.getPrototypeOf instead.The conversion was on the wrong operand: it belongs on the argument, which is where V8 puts it (CHECK_TO_OBJECT). Applying it there fixes a second, latent defect on the same line. The argument was being handed to ToJSObject, which only asserts that its input is an object — so a primitive tripped the assert in debug builds and, in release, reinterpreted a non-object JSValueRef as a JSObjectRef before passing it to JSObjectGetPrototype, which is undefined behaviour rather than a status. Coercing the argument now matches V8 exactly: a primitive yields its wrapper's prototype, and only null/undefined are rejected with napi_object_expected. The shared walk never reaches this, since it only recurses into values it has already found to be object-like.2. Hermes' for...in does not implement the shadowing rule (not fixed — engine bug). CI showed napi_get_property_names returning the correct ['own', 'middle'] while Hermes' own for...in returned ['own', 'middle', 'deep'], i.e. it reports an inherited property that a non-enumerable own property is supposed to hide. The test therefore probes for that behaviour at runtime rather than naming engines, and only uses for...in as an oracle where it holds; the explicit expected-value assertion still runs everywhere. This is tracked by #219.## TestingThirteen script tests, exercised through a new napiGetPropertyNames global that calls Napi::Object::GetPropertyNames.Conformance:- own enumerable string keys- enumerable properties inherited from the prototype chain- non-enumerable own properties excluded- symbol keys excluded- a shadowed inherited property reported only once- an inherited property shadowed by a non-enumerable own property omitted- class methods excluded (they are non-enumerable)- array indices as strings, length omitted- equality with for...in over a multi-level prototype chainTermination:- a proxy that is its own prototype- a two-object cycle, with each level reported once- a long acyclic chain still reported in full, showing the termination check does not truncate legitimate chains- a throwing getPrototypeOf trap propagating as an exception instead of hangingPlus a native regression test, NodeApi.GetPropertyNamesReportsLastErrorConsistently, asserting that the returned status and the recorded error code agree on rejection and that the success path clears.Verified locally on Chakra — 235 passing, 0 failing, and the native test green. The remaining backends are covered by CI; all 24 legs are green.As a negative control, restoring the old Chakra implementation makes five of the nine conformance tests fail, confirming the tests are load-bearing rather than vacuous:napi_get_property_names (#216) ✅ returns own enumerable string keys 1) includes enumerable properties inherited from the prototype chain 2) excludes non-enumerable own properties ✅ excludes symbol keys ✅ reports a shadowed inherited property only once 3) omits an inherited property shadowed by a non-enumerable own property ✅ excludes class methods, which are non-enumerable 4) reports array indices as strings and omits the non-enumerable length 5) matches for...in over a multi-level prototype chainReverting the termination fix hangs the suite, and reverting the last_error fix fails the native test on exactly the two assertions it should.## Note on test scopeTwo of the new groups are deliberately narrower than the rest, in both cases because the behaviour belongs to an engine rather than to this change:- The three proxy-dependent termination tests are skipped on JavaScriptCore. Its napi_get_prototype calls JSObjectGetPrototype, which reads the internal [[Prototype]] slot and never runs a getPrototypeOf trap — so a trapped chain reports the target's real prototype, the cycle is invisible, and that backend was never at risk. This is not a JSC deficiency: V8 behaves the same way and says so in a comment on napi_get_prototype (js_native_api_v8.cc:1539). Chakra and QuickJS are the trap-aware outliers, and they are exactly where the hang was reachable. The acyclic-chain test needs no trap and runs everywhere.- The native error-reporting test covers only the three backends that share the walk. V8's napi_get_property_names is vendored upstream Node code that keeps neither half of the contract: a rejection leaves a pending exception, so a following call reports napi_pending_exception rather than napi_object_expected, and its success path returns bare napi_ok through GET_RETURN_STATUS without clearing. Both are upstream's to define, not this PR's to redefine.## Note on scopenapi_get_all_property_names is still only implemented by the V8 backend; the others would fail to link if a consumer called it. That is a separate gap and is left alone here.

[Copilot]

Pull request overview

This PR aligns napi_get_property_names behavior across the JavaScriptCore, ChakraCore, and QuickJS backends to match the Node-API/V8 semantics: enumerable, string-keyed property names including the prototype chain (i.e., matching for...in, including the “non-enumerable own property shadows inherited enumerable” rule).

Changes:

• Added an engine-agnostic implementation (napi_shared::GetEnumerablePropertyNames) that walks the prototype chain using Object.keys plus Object.getOwnPropertyNames to enforce for...in-equivalent semantics.
• Updated JavaScriptCore, ChakraCore, and QuickJS napi_get_property_names implementations to delegate to the shared helper (and added missing CHECK_ARG(env, object) on JSC/Chakra).
• Added script-level regression tests and a test harness global (napiGetPropertyNames) to validate behavior against for...in.

Reviewed changes

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

Show a summary per file

File	Description
Tests/UnitTests/Shared/Shared.cpp	Exposes napiGetPropertyNames to script tests via the C++ harness.
Tests/UnitTests/Scripts/tests.ts	Adds regression tests for napi_get_property_names semantics (prototype chain, enumerability, symbols, shadowing, arrays).
Core/Node-API/Source/js_native_api_shared.h	Declares shared helper for napi_get_property_names semantics.
Core/Node-API/Source/js_native_api_shared.cc	Implements the shared prototype-chain walk using only public napi_* APIs.
Core/Node-API/Source/js_native_api_quickjs.cc	Replaces own-only QuickJS implementation with shared helper.
Core/Node-API/Source/js_native_api_javascriptcore.cc	Fixes JSC implementation (previously throwing) by delegating to shared helper and validating object.
Core/Node-API/Source/js_native_api_chakra.cc	Replaces own-only / non-enumerable-including Chakra implementation with shared helper and validates object.
Core/Node-API/CMakeLists.txt	Adds shared helper sources to non-V8 backend builds.

---

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

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

Core/Node-API/Source/js_native_api_chakra.cc:688

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Since CHECK_NAPI assumes last_error is already set, callers may observe stale napi_get_last_error_info data. Set last_error explicitly for this status before returning it.

  // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties,
  // so use the shared prototype-chain walk instead.
  CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result));

Core/Node-API/Source/js_native_api_javascriptcore.cc:976

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Because CHECK_NAPI assumes last_error was already set, this can leave stale last-error info visible via napi_get_last_error_info when an error is returned. Set last_error explicitly for this status before returning.

  // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but
  // silently drops properties shadowed by a non-enumerable own property, so use
  // the shared prototype-chain walk instead.
  CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result));

  return napi_ok;

Tests/UnitTests/Shared/Shared.cpp:142

• When napi_get_property_names fails with a pending JavaScript exception, the current code clears and discards the original exception and instead throws a generic status-based error. This makes debugging failures harder and can hide the real engine error. If an exception is pending, rethrow that exception value instead of discarding it.

                    // A failed call may or may not have left a JavaScript
                    // exception pending; surface either as a thrown error so
                    // that the script tests can assert on it uniformly.
                    bool isExceptionPending{};
                    if (napi_is_exception_pending(rawEnv, &isExceptionPending) == napi_ok && isExceptionPending)
                    {
                        napi_value error{};
                        napi_get_and_clear_last_exception(rawEnv, &error);
                    }

Core/Node-API/Source/js_native_api_quickjs.cc:1403

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Because CHECK_NAPI assumes the callee already set last_error, this can leave stale last-error info visible via napi_get_last_error_info even though the API returned an error. Handle this status explicitly and set last_error before returning it.

  // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain
  // walk instead.
  CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result));

  napi_clear_last_error(env);

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

LGTM. Comments inline.

[matthargett]

thanks for coming at N-API! hopefully this means you can review my existing work in this area :D a few review notes:

• This does not duplicate the implementation work in #189, #217, rebeckerspecialties#8, or #10 but it does overlap with PR #116’s already-vendored test_object conformance case, although that directory is not currently enabled.

• Please use PR #189’s JSR_NAPI_ENGINE_* definition family rather than adding a parallel engine-name macro. The current merge of this PR branch against my N-API stack has one Android CMake conflict for exactly this reason.

• Make a focused v1-compatible NodeApi/CTS addon durable ABI test, retaining this PR’s additional shadowing, coercion, poisoned-intrinsic, proxy-cycle, and JSI cases.

• Please design the shared collector so it can support v6 napi_get_all_property_names, or link a required follow-up; the combined v7 stack (#189 ) otherwise still exposes a non-V8 symbol gap.

a couple of other security things to guard against:

• rethrow the pending value in the raw test bridge instead of clearing and replacing it with a generic error
• avoid UTF-8 strings as property-key identity because distinct lone-surrogate keys may collapse

[matthargett]

GetEnumerablePropertyNames returns napi_object_expected directly for null/undefined without setting env->last_error. This caller (and others in this PR's commits) uses CHECK_NAPI,and that contract assumes the callee already set last_error. On QuickJS the preceding napi_typeof in the shared macro clears the last error, so the returned status and napi_get_last_error_info()->error_code disagree. I'd recommend adding a unit test for both of these gaps, as the e2e failure with only one of the JS runtimes would be pretty subtle.

[bkaradzic-microsoft]

Correct, fixed in 0d7ab06.

The mechanism, for the record, since it is a little indirect: the shared walk is written against the public napi_* surface, so it has no way to reach napi_set_last_error. CHECK_NAPI (js_native_api_quickjs.h:111) only propagates the status -- the house contract is that the callee already recorded it -- and the napi_typeof the walk performs immediately before the rejection calls napi_clear_last_error on its way out (js_native_api_quickjs.cc:596). So a caller saw napi_object_expected returned while napi_get_last_error_info()->error_code still read napi_ok.

Writing the test turned up the mirror-image problem, which I had not expected: the success path had no clear either, so a successful napi_get_property_names left whatever error a previous call had recorded in place. Both directions are now handled explicitly at all three call sites.

Covered by a native regression test, NodeApi.GetPropertyNamesReportsLastErrorConsistently, which asserts the returned status and the recorded error agree for null and undefined and that the success path clears. I ran it against a reverted build as a negative control -- it fails on exactly the two assertions it should.

[matthargett]

grabbing global.Object.keys and Object.getOwnPropertyNames on every call allows user monkey-patching to change native Node-API behavior. if a BabylonNative app will be executing user-generated code, this is an attack surface. PR #116 already established the opposite invariant by capturing canonical Function.prototype.call. is there a reason not to use protected pristine intrinsics or engine-native key enumeration beyond sharing code? To pin this, I suggest adding a poisoned-intrinsics regression integration test. If you all have an internal fuzzer harness, you should ask why it didn't find/highlight this.

[bkaradzic-microsoft]

I looked into this one carefully and I am going to push back, though not on the premise.

The premise is sound: script can redefine Object.keys, and a native caller of napi_get_property_names would then observe the redefined behaviour, where on V8 it would not. That is a real difference.

Where I disagree is that it is a property of this change. js_native_api_javascriptcore.cc:1073-1075 already resolves global -> "Object" -> "hasOwnProperty" at runtime, and has for as long as napi_has_own_property has existed on that backend. This PR did not introduce the pattern, and fixing it here would leave the identical exposure one function away, which is the sort of half-measure that reads as fixed and is not.

Two of the supporting points do not hold up. PR #116 has not "established the opposite invariant" -- it is still open and unmerged, so there is nothing to be consistent with yet. And "use engine-native key enumeration instead" is not available on JavaScriptCore: its public C API offers only JSObjectCopyPropertyNames, which walks the prototype chain itself and silently drops properties shadowed by a non-enumerable own property. That is precisely the bug #216 reported and precisely why this PR does the walk in shared code rather than per-engine. Taking the native route would mean re-forking the three implementations this change just unified, in order to trade a documented, spec-visible correctness bug for a hardening property that the surrounding code does not have anyway.

So: worth doing, worth doing repo-wide against the whole intrinsic-dependent surface, and worth doing with a considered mechanism -- a per-env snapshot of the intrinsics taken at init, rather than ad-hoc lookups. That is a different change from this one, and I would rather it be scoped and reviewed on its own than smuggled in here. I am deliberately not filing an issue for it; if the team wants it, it should be prioritised as its own piece of work rather than parked in the backlog.

[matthargett]

TL;DR: please merge #116 first, then this PR, then we/I can do a hardening pass across the repo to prevent prototype injection attacks.

ok, I agree that the repo-wide pristine-intrinsic hardening can be handled as a separate PR, but I want to clarify two points after some cross-checking rather than leave misunderstandings for other folks/agents reading.

First, you are correct that #116 has not merged. “Established” was imprecise wording on my part: I meant that #116 has already implemented the invariant I believe this project should adopt, and that I have deployed in my N-API modules in my BabylonNative app, not that the invariant is already present on main. The existing PR captures the canonical Function.prototype.call once during environment initialization so that user mutation cannot alter napi_call_function behavior, and it introduces the portable Node-API conformance harness. Doing work in N-APi without at least merging the conformance suite PR first will probably yield more friction as time goes on -- unless you and the other project maintainers are purposefully deviating from Node API conformance?

I checked the JavaScriptCore claim because I was pretty sure that statement was incorrect. Looking at Apple’s public SDK (rather than WebKit SPI) and Bun's fork of JSC, JSObjectCopyPropertyNames and JSObjectGetPrototype are both public and available at my deployment floors—Apple currently lists iOS 16+ and macOS 10.5+. Looking at recent adjustments to CI, it looks like Microsoft's deployment floors are higher than that even?

I do see that even the current public JSC API has no configurable own/prototype, enumerable, string/symbol, descriptor-aware enumeration operation equivalent to V8’s key-collection modes. I also verified the behavior against shipping JSC: given an enumerable inherited property shadowed by a non-enumerable own property, JSObjectCopyPropertyNames returns the inherited name while for...in correctly omits it. Therefore, it is not a conforming replacement for the shared walk. The shadowing wording above is slightly ambiguous: JSC does not drop the shadowed inherited property; it fails to let the non-enumerable own property suppress that inherited name. Also, #216 reported the existing JSC backend’s unconditional throw and the cross-backend prototype/enumerability differences; this shadowing case is useful additional conformance coverage discovered while fixing it.

sorry for the wall of text, but you/agent got some things wrong and there were some nuances that are important to get full understanding on if we're going to do a good job of supporting N-API across the diversity of open souce JS VMs that's been expanded over the last several months (QuickJS, Hermes, etc).

[bkaradzic-microsoft]

You're right on the shadowing point, and I've corrected it in 26a4a99 — thanks for pushing on it.

I checked it against WebKit rather than just taking the correction: JSObject::getPropertyNames walks the chain calling getOwnPropertyNames per level with DontEnumPropertiesMode::Exclude. Because non-enumerable own properties are never added to the array in that mode, they cannot suppress a same-named enumerable property further up — so the inherited name survives where for...in correctly omits it. That is the opposite of what my comment said, and "silently drops properties shadowed by a non-enumerable own property" described the correct behaviour as if it were the bug. Fixed in the source comment and in the PR description, with the mechanism spelled out so the next reader doesn't have to re-derive it. The conclusion and the code are unchanged. This case is covered by the omits an inherited property shadowed by a non-enumerable own property test.

Two smaller clarifications, in the same spirit of not leaving misunderstandings behind:

I didn't claim JSObjectCopyPropertyNames or JSObjectGetPrototype are unavailable — what I said was that the public C API has no own-only enumeration, and offers only JSObjectCopyPropertyNames, which chain-walks. That phrasing presupposes it's available; it's the own-only capability that's missing. You reached the same conclusion independently ("no configurable own/prototype, enumerable, string/symbol, descriptor-aware enumeration operation equivalent to V8's key-collection modes"), so I think we agree on the substance and this was just my wording being read more strongly than intended.

And no, there's no deliberate deviation from Node-API conformance — this PR is a conformance fix, and CI now covers the shadowing rule, symbol exclusion, coercion, prototype-chain ordering and for...in equivalence across all six backends. Thank you for the review; the cycle and last_error findings were both real and both worth the round trip.

Merge order for #116 is bghgary's call, not mine, so I'll leave that to him. I have no objection to it landing first; I'd just rather not block a fix for a reported bug on it. The two don't conflict — nothing here touches napi_call_function or intrinsic capture — so whichever order they land in, the rebase is trivial.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Core/Node-API/Source/js_native_api_shared.cc:23

• GetUtf8Value converts property names via napi_get_value_string_utf8 and then uses the UTF-8 bytes for shadowing/deduping. In this repo’s V8 implementation, napi_get_value_string_utf8 uses REPLACE_INVALID_UTF8, which can make distinct JS strings (e.g., ones containing unpaired surrogates) collapse to the same UTF-8 sequence. That can break the shadowing rule and cause incorrect results for exotic-but-valid property names. Prefer tracking keys using UTF-16 code units (napi_get_value_string_utf16 + std::u16string) or otherwise comparing actual JS strings rather than their UTF-8 encoding.

    napi_status GetUtf8Value(napi_env env, napi_value value, std::string& result) {
      size_t length{};
      RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, nullptr, 0, &length));

      std::vector<char> buffer(length + 1);