napi: propagate Proxy trap exceptions from napi_get_all_property_names descriptor filter#32263
napi: propagate Proxy trap exceptions from napi_get_all_property_names descriptor filter#32263robobun wants to merge 3 commits into
Conversation
|
Updated 8:11 PM PT - Jun 24th, 2026
❌ @robobun, your commit 13321a2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32263That installs a local version of the PR into your bun-32263 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesProxy trap exception handling in
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-app/module.js`:
- Around line 962-985: The test_get_all_property_names_throwing_proxy function
currently only exercises the path where getOwnPropertyDescriptor throws, but the
PR fix adds exception checks after both getOwnPropertyDescriptor and
getPrototypeOf calls. Add a second Proxy test case named throwingPrototype that
has a working ownKeys and getOwnPropertyDescriptor but throws in the
getPrototypeOf trap to verify the prototype chain walking path is also fixed.
Add two test calls for this new throwingPrototype proxy (one with key filter
mode 1 for napi_key_own_only and one with mode 0 for
napi_key_include_prototypes) to ensure both property name collection modes
properly handle getPrototypeOf exceptions.
In `@test/napi/napi.test.ts`:
- Around line 931-938: In the test "returns napi_pending_exception when a Proxy
trap throws during descriptor filtering" within the describe block
"napi_get_all_property_names", verify whether the corresponding module.js test
fixture has been expanded to include a getPrototypeOf throwing case. If it has
been added, include an additional expect assertion to verify the exception
message for the getPrototypeOf trap throwing scenario. Additionally, replace the
individual expect calls checking for status lines with a more robust assertion
that filters the output for all status lines and verifies the exact count
matches the number of test cases being run, to catch unexpected output changes
and ensure the test remains maintainable as more cases are added.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6591cd5a-da10-47e3-a289-ee6fa9fb7cc8
📒 Files selected for processing (4)
src/jsc/bindings/napi.cpptest/napi/napi-app/module.jstest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for clarifying the fixture comment; the NAPI_RETURN_IF_EXCEPTION insertions and loop restructuring look correct.
Extended reasoning...
Overview
This PR fixes napi_get_all_property_names to propagate exceptions thrown by Proxy traps (getOwnPropertyDescriptor / getPrototypeOf) during the descriptor-filter loop, instead of continuing with a pending exception and hitting assertNoException() (segfault in release). The core change in src/jsc/bindings/napi.cpp (~15 lines) inserts NAPI_RETURN_IF_EXCEPTION(env) after each potentially-throwing JSC call (exportKeys->get, toPropertyKey, getOwnPropertyDescriptor, getPrototype, filteredKeys->push), hoists toPropertyKey out of the inner loop, and restructures the prototype walk from while(!getOwnPropertyDescriptor(...)) into an explicit while(true)/break body so the exception check runs regardless of the return value. The remaining three files add a native test helper, a JS fixture, and a checkSameOutput regression test comparing Bun against Node for both napi_key_own_only and napi_key_include_prototypes.
Security risks
None. This is a defensive correctness fix in the N-API bindings — it adds early-return-on-exception checks, which can only narrow behavior toward Node compatibility. No auth, crypto, permissions, filesystem, or network surface is touched.
Level of scrutiny
Low-to-medium. The NAPI_RETURN_IF_EXCEPTION macro is the established idiom throughout napi.cpp for exactly this purpose, and each insertion follows a call that is documented to invoke user-observable traps. The loop rewrite is a mechanical equivalence transform (while(!X){Y} → while(true){x=X; check; if(x)break; Y; check;}). The non-throwing filter paths remain covered by the existing test/napi/node-napi-tests/test/js-native-api/test_object/ suite per the PR description.
Other factors
- The bug-hunting system found no issues.
- All inline review threads are resolved: CodeRabbit withdrew both suggestions, and my earlier nit about
getPrototypeOfcoverage was addressed — the author explained convincingly that acheckSameOutputtest for that branch is infeasible because V8 and JSC walk the prototype chain a different number of times, and pushed d5a8dcb to scope the fixture comment to what it actually exercises. - The duplicate-PR bot noted overlap with #29642 / #24985; the PR description already explains this is the focused napi-only fix with a regression test and covers additional throw sites those PRs do not. That's a merge-ordering concern for maintainers, not a correctness concern for this diff.
There was a problem hiding this comment.
LGTM — the comment clarification addresses my earlier nit, and the exception guards follow the established pattern in this file.
Extended reasoning...
Overview
This PR adds NAPI_RETURN_IF_EXCEPTION(env) guards after each potentially-throwing JSC call inside the descriptor-filter loop of napi_get_all_property_names in src/jsc/bindings/napi.cpp (~20 lines net), restructures the prototype-walk while condition into an explicit body so the exception check runs unconditionally, and hoists toPropertyKey out of the inner loop. It adds a native test helper in standalone_tests.cpp, a JS fixture in module.js, and a checkSameOutput test in napi.test.ts that asserts byte-identical output against Node.js for both napi_key_own_only and napi_key_include_prototypes.
Security risks
None. This is defensive exception-propagation in a property-enumeration path; no auth, crypto, permissions, or untrusted-input parsing is involved. The change strictly narrows behavior (early-return on pending exception) rather than adding new code paths.
Level of scrutiny
Low-to-moderate. While napi.cpp is core runtime code, the change is mechanical: it inserts the same NAPI_RETURN_IF_EXCEPTION macro already used at line 1832 (immediately above this loop) after each call that can invoke a Proxy trap. The while(true)/break restructuring is a transparent semantic equivalent of the original while(!getOwnPropertyDescriptor(...)) condition. The fix replaces a segfault / assertNoException() violation with the Node-matching napi_pending_exception return.
Other factors
- The bug hunting system found no issues.
- My earlier nit (test coverage for the
getPrototypecheck) was addressed: the author explained why acheckSameOutputtest cannot deterministically reach that line across V8/JSC, and updated the fixture comment in commit d5a8dcb to accurately describe only what it exercises. Both CodeRabbit threads were similarly resolved and withdrawn. - The PR description explicitly addresses overlap with #29642 and #24985, noting this is the focused napi-only fix with additional throw-site coverage and a dedicated regression test.
- No CODEOWNERS entry covers these paths.
…s descriptor filter When napi_get_all_property_names is called with napi_key_enumerable, napi_key_writable, or napi_key_configurable, it iterates the collected keys and calls getOwnPropertyDescriptor/getPrototype on the target to filter by descriptor attributes. If the target is a Proxy whose getOwnPropertyDescriptor or getPrototypeOf trap throws, the loop kept iterating with a pending exception and then NAPI_RETURN_SUCCESS would assert (debug) or return napi_ok with the exception still pending (release). In release builds the next iteration would dereference an empty JSValue from exportKeys->get and segfault. Add NAPI_RETURN_IF_EXCEPTION after every call in the filter loop that can run user JS (get, toPropertyKey, getOwnPropertyDescriptor, getPrototype, push) and restructure the prototype walk so the exception check runs regardless of getOwnPropertyDescriptor's return value.
The fixture only exercises getOwnPropertyDescriptor throwing; V8 and JSC walk the prototype chain a different number of times so a getPrototypeOf case cannot be expressed as a checkSameOutput test.
robobun
force-pushed
the
farm/cac7b2a8/napi-get-all-property-names-exception-check
branch
from
e715a66 to
2d35d72
Compare
There was a problem hiding this comment.
LGTM — comment clarification addresses my earlier note; the exception checks are mechanical and follow the existing NAPI_RETURN_IF_EXCEPTION pattern.
Extended reasoning...
Overview
This PR adds exception-propagation guards to the descriptor-filter loop in napi_get_all_property_names (src/jsc/bindings/napi.cpp). It inserts NAPI_RETURN_IF_EXCEPTION(env) after each JSC call that can throw (exportKeys->get, toPropertyKey, getOwnPropertyDescriptor, getPrototype, filteredKeys->push), restructures the prototype-walk while(!getOwnPropertyDescriptor(...)) into an explicit while(true)/break body so the exception check runs regardless of the boolean result, and hoists toPropertyKey to once per key. A native test helper, JS fixture, and checkSameOutput test are added covering both napi_key_own_only and napi_key_include_prototypes with a Proxy whose getOwnPropertyDescriptor trap throws.
Security risks
None. This is pure exception-safety plumbing in the NAPI binding layer — it changes a crash / napi_ok-with-pending-exception path into the correct napi_pending_exception return. No new data flow, no auth/crypto/permission surface.
Level of scrutiny
Low–medium. The production change is ~30 lines, entirely mechanical, and applies the same NAPI_RETURN_IF_EXCEPTION macro used throughout napi.cpp. The restructured loop is semantically identical on the non-throwing path (protoValue.getObject() returns nullptr for non-objects including null, preserving the original break). The existing test_object Node-API suite covers the non-throwing filter paths, and the new checkSameOutput test pins behavior to Node for the throwing path.
Other factors
- No bugs found by the bug-hunting system.
- All prior review threads (CodeRabbit ×2, my earlier nit on
getPrototypeOfcoverage) are resolved; the author explained why a cross-enginecheckSameOutputtest for the innergetPrototypecheck is not feasible and pushed 2d35d72 to align the fixture comment with what the test actually exercises. - No CODEOWNERS entry for
src/jsc/bindings/napi.cpp. - The PR description notes overlap with #29642 / #24985 and explains why this is the focused, more complete change with a regression test.
The native helper writes via printf, which emits CRLF line endings on Windows. Splitting on a bare LF left trailing CR on each line and failed the toEqual check. checkSameOutput already verifies Bun and Node produce identical bytes; this only fixes the supplementary per-line assertion. From #32629.
There was a problem hiding this comment.
LGTM — focused exception-propagation fix following the established NAPI_RETURN_IF_EXCEPTION pattern, with a Node-comparison regression test.
Extended reasoning...
Overview
This PR adds NAPI_RETURN_IF_EXCEPTION(env) checks after each JSC call that can throw inside the descriptor-filter loop of napi_get_all_property_names (src/jsc/bindings/napi.cpp:1838-1881): exportKeys->get, toPropertyKey, getOwnPropertyDescriptor, getPrototype, and filteredKeys->push. The prototype walk is restructured from while(!getOwnPropertyDescriptor(...)) into an explicit while(true) body so the exception check runs regardless of the boolean return. toPropertyKey is hoisted to compute once per key. A new native test helper plus JS/TS fixtures verify status napi_pending_exception (10) for both napi_key_own_only and napi_key_include_prototypes against Node via checkSameOutput. An incidental split(/\r?\n/) fix is included for the adjacent napi_is_arraybuffer test on Windows.
Security risks
None. This is purely additive exception-check plumbing in an existing N-API entry point; no new inputs, no auth/crypto/permissions surface, no data exposure paths. The only behavioral change is returning napi_pending_exception instead of napi_ok (and avoiding a segfault) when a Proxy trap throws — strictly a correctness/safety improvement.
Level of scrutiny
Low-to-moderate. The change is mechanical: every added line is the established NAPI_RETURN_IF_EXCEPTION macro applied immediately after a call that can run JS (Proxy traps). The restructured loop is semantically equivalent to the original modulo the new exception checks. No CODEOWNERS paths are touched. The regression test is byte-for-byte compared against Node, so divergent behavior would be caught in CI.
Other factors
I previously left a nit about getPrototypeOf coverage; the author explained convincingly that V8 and JSC walk the prototype chain a different number of times, making a checkSameOutput-style test for that branch inherently engine-divergent, and adjusted the fixture comment to accurately describe what it covers. CodeRabbit raised and withdrew the same point. All inline threads are resolved, the bug-hunting system found nothing, and the existing test_object N-API suite continues to exercise the non-throwing paths.
|
CI status for 13321a2 (build 64566): 277 jobs passed, 3 failed, all unrelated to this diff.
The new |
1 participant
Summary
napi_get_all_property_nameswith a descriptor-basedkey_filter(napi_key_enumerable/napi_key_writable/napi_key_configurable) iterates the collected keys and callsgetOwnPropertyDescriptor/getPrototypeon the target to filter by attribute. When the target is a Proxy whosegetOwnPropertyDescriptororgetPrototypeOftrap throws, the loop kept iterating with a pending exception and then hitNAPI_RETURN_SUCCESS, which callsassertNoException().Repro
Node.js returns
napi_pending_exception. Bun returnednapi_okwith the exception still pending (release) and then segfaulted at address0x5on the next call withnapi_key_include_prototypes:Fix
Add
NAPI_RETURN_IF_EXCEPTION(env)after every call in the filter loop that can throw:exportKeys->get,toPropertyKey,getOwnPropertyDescriptor,getPrototype, andfilteredKeys->push. The prototype walk is restructured from awhile(!getOwnPropertyDescriptor(...))condition into an explicit body so the exception check runs regardless of the return value, andtoPropertyKeyis hoisted out of the loop so it is computed (and checked) once per key.Related: #29642 also touches this loop as part of a broader
Bun.inspectfix; this PR is the focused napi change with a napi regression test and covers the additional throw sites that PR does not (get/toPropertyKey/push, and the case wheregetOwnPropertyDescriptorthrows after returning true from the while condition).Test
test/napi/napi.test.ts→napi_get_all_property_names > returns napi_pending_exception when a Proxy trap throws during descriptor filteringcompares Bun against Node for bothnapi_key_own_onlyandnapi_key_include_prototypes. The existingtest/napi/node-napi-tests/test/js-native-api/test_object/suite (which exercises the non-throwing filter paths) continues to pass.