fix(fetch): "prop" in Request/Response reports real properties (unblocks Auth.js login body parse)#6434
Conversation
… false
The `in` operator on a Web Fetch handle reported `false` for every key. Perry
represents `Request` / `Response` / `Headers` as native handle-band registry ids
(not heap objects), and `js_object_has_property` took a blanket shortcut —
returning `false` for all handle-band values to avoid dereferencing the id as a
pointer. But a `Request` genuinely has `body` / `method` / `url` / `headers`, so
`"body" in request` was wrongly `false`.
That broke request-body reading in real frameworks: Auth.js's body parser gates
on `if (!("body" in e) || !e.body || …) return`, so with `"body" in request`
false it skipped parsing the credentials POST body entirely — the `csrfToken`
form field never reached the CSRF check and every login failed with
`MissingCSRF` (redirecting to `/auth/error?error=Configuration` instead of
running the `authorize` callback).
Two arms of `js_object_has_property` now forward a STRING key to the same handle
property dispatcher that property *reads* use (safe for these ids — no heap
deref); the property exists iff it resolves to a non-undefined value, and a miss
falls through so real expandos still resolve:
- the Web-Fetch / zlib handle-band arm (a bare `Request`/`Response`/`Headers`);
- the `class X extends Request/Response` arm — a heap object whose native
members live on an underlying handle stashed in `__perry_fetch_handle__`
(`fetch_subclass_handle_id`). Next.js's `NextRequest` extends `Request`, so
the credentials request Auth.js inspects is a subclass instance.
A symbol key still reports `false` (no own-property meaning on these handles).
Gap test covers `"body"/"method"/"url"/"headers"/"bodyUsed" in request`,
`Response`, a `Request` subclass, and a non-existent key — byte-identical to
`node --experimental-strip-types`.
📝 WalkthroughWalkthroughThe runtime updates ChangesProperty presence checks
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InOperator
participant js_object_has_property
participant handle_property_dispatch
InOperator->>js_object_has_property: Check string key with in
js_object_has_property->>handle_property_dispatch: Dispatch fetch handle property
handle_property_dispatch-->>js_object_has_property: Return property value
js_object_has_property-->>InOperator: Return true when value is non-undefined
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
crates/perry-runtime/src/object/field_get_set/has_property.rs (2)
232-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRebuild static wrapper crates after runtime changes.
As per coding guidelines, when changing runtime code like
has_property.rs, ensure you rebuild the corresponding static wrapper crates (perry-runtime-staticandperry-stdlib-static). The runtime and stdlib crates emit only rlibs and may leave stale archives linked otherwise.🤖 Prompt for 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. In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines 232 - 263, After updating the handle-property logic in has_property.rs, rebuild the corresponding perry-runtime-static and perry-stdlib-static wrapper crates so their archives include the runtime changes and do not remain stale.Source: Coding guidelines
251-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract handle property dispatch into a reusable helper.
The unsafe logic to extract a string pointer, compute the name length, and invoke the handle property dispatcher is duplicated across multiple locations in this file. Extracting this into a helper function (e.g.,
check_handle_property(handle_id: i64, key: f64) -> bool) will DRY up the code and improve maintainability.
crates/perry-runtime/src/object/field_get_set/has_property.rs#L251-L266: extract the string pointer extraction and dispatch invocation into a helper.crates/perry-runtime/src/object/field_get_set/has_property.rs#L394-L409: reuse the new helper here.🤖 Prompt for 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. In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines 251 - 266, The string extraction and handle-property dispatch logic is duplicated in has_property.rs. Add a reusable check_handle_property(handle_id: i64, key: f64) -> bool helper containing the existing unsafe logic, update the dispatch block at crates/perry-runtime/src/object/field_get_set/has_property.rs#L251-L266 to call it, and replace the duplicate logic at crates/perry-runtime/src/object/field_get_set/has_property.rs#L394-L409 with the same helper while preserving the existing true/false behavior.
🤖 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 `@crates/perry-runtime/src/object/field_get_set/has_property.rs`:
- Around line 392-411: Reduce the overhead in the string-key path before calling
fetch_subclass_handle_id by adding a reliable fast-path guard that identifies
objects capable of subclass property dispatch. Update the surrounding logic in
has_property.rs, preserving dispatch behavior for eligible subclass objects
while bypassing fetch_subclass_handle_id for ordinary heap objects and retaining
the existing fallback property check.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/field_get_set/has_property.rs`:
- Around line 232-263: After updating the handle-property logic in
has_property.rs, rebuild the corresponding perry-runtime-static and
perry-stdlib-static wrapper crates so their archives include the runtime changes
and do not remain stale.
- Around line 251-266: The string extraction and handle-property dispatch logic
is duplicated in has_property.rs. Add a reusable
check_handle_property(handle_id: i64, key: f64) -> bool helper containing the
existing unsafe logic, update the dispatch block at
crates/perry-runtime/src/object/field_get_set/has_property.rs#L251-L266 to call
it, and replace the duplicate logic at
crates/perry-runtime/src/object/field_get_set/has_property.rs#L394-L409 with the
same helper while preserving the existing true/false behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8da0df50-fd72-409c-93f0-83c8dcc217b4
📒 Files selected for processing (2)
crates/perry-runtime/src/object/field_get_set/has_property.rstest-files/test_gap_fetch_handle_in_operator.ts
| if key_val.is_any_string() { | ||
| if let Some(handle_id) = unsafe { super::fetch_subclass_handle_id(obj_addr as usize) } { | ||
| if let Some(dispatch) = super::super::class_registry::handle_property_dispatch() { | ||
| unsafe { | ||
| let key_ptr = crate::value::js_get_string_pointer_unified(key) | ||
| as *const crate::StringHeader; | ||
| if !key_ptr.is_null() { | ||
| let name_ptr = | ||
| (key_ptr as *const u8).add(std::mem::size_of::<crate::StringHeader>()); | ||
| let name_len = (*key_ptr).byte_len as usize; | ||
| let result = dispatch(handle_id, name_ptr, name_len); | ||
| if result.to_bits() != crate::value::TAG_UNDEFINED { | ||
| return nanbox_true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Significant performance overhead for all in operator checks.
Invoking fetch_subclass_handle_id here creates a substantial performance regression. Because this block evaluates for any heap pointer with a string key, every standard in check (e.g., "foo" in my_object) now incurs the cost of a full property lookup for __perry_fetch_handle__. On a miss, this lookup will also traverse the object's entire prototype chain before falling through to the ordinary presence check.
To mitigate this overhead, consider adding a fast-path guard before calling fetch_subclass_handle_id. For example, you could check an internal bit flag on the object header, verify a specific class_id if one exists for subclasses, or limit this lookup to only known native fetch properties (like "body", "method", "url").
🤖 Prompt for 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.
In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines
392 - 411, Reduce the overhead in the string-key path before calling
fetch_subclass_handle_id by adding a reliable fast-path guard that identifies
objects capable of subclass property dispatch. Update the surrounding logic in
has_property.rs, preserving dispatch behavior for eligible subclass objects
while bypassing fetch_subclass_handle_id for ordinary heap objects and retaining
the existing fallback property check.
…in rewrite Rewriting the pointer `in` arm dropped the PerryTS#6406 buffer branch that answered a user own-property (`buf.foo = v`) and a `Buffer.prototype` method (`writeInt8`/`readUInt8`/…). The surviving typed-array arm covers indices, the view slots, and the %TypedArray% prototype chain, but NOT those two — Perry keeps buffers outside the object model, so both live in the buffer side tables. `"writeInt8" in buf` and `"foo" in buf` regressed to false (test_gap_buffer_own_props). Re-add the two checks in the typed-array arm, after the prototype-chain scan. Verified byte-identical to node; the PR's own test_gap_fetch_handle_in_operator still passes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/perry-runtime/src/object/field_get_set/has_property.rs`:
- Around line 563-567: In the property check surrounding buffer_get_own_prop,
evaluate is_buffer_method_name(name) before the mutex-acquiring
buffer_get_own_prop call so known buffer methods short-circuit without locking;
preserve the existing true result and fallback behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07ead4ec-4a49-42eb-9cef-d2f136df0f03
📒 Files selected for processing (1)
crates/perry-runtime/src/object/field_get_set/has_property.rs
| if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some() | ||
| || crate::object::buffer_dispatch::is_buffer_method_name(name) | ||
| { | ||
| return nanbox_true; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Swap the order of conditions to avoid an unnecessary mutex lock.
Based on the provided codebase context, buffer_get_own_prop acquires a global mutex (buffer_props().lock()). Since the || operator short-circuits, place is_buffer_method_name first to avoid acquiring the lock when the property is a known buffer method name.
⚡ Proposed fix
- if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
- || crate::object::buffer_dispatch::is_buffer_method_name(name)
+ if crate::object::buffer_dispatch::is_buffer_method_name(name)
+ || crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some() | |
| || crate::object::buffer_dispatch::is_buffer_method_name(name) | |
| { | |
| return nanbox_true; | |
| } | |
| if crate::object::buffer_dispatch::is_buffer_method_name(name) | |
| || crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some() | |
| { | |
| return nanbox_true; | |
| } |
🤖 Prompt for 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.
In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines
563 - 567, In the property check surrounding buffer_get_own_prop, evaluate
is_buffer_method_name(name) before the mutex-acquiring buffer_get_own_prop call
so known buffer methods short-circuit without locking; preserve the existing
true result and fallback behavior.
1 participant
The
inoperator on a Web Fetch handle reportedfalsefor every key.Perry represents
Request/Response/Headersas native handle-band registry ids (not heap objects), andjs_object_has_propertytook a blanket shortcut — returningfalsefor all handle-band values to avoid dereferencing the id as a pointer. But aRequestgenuinely hasbody/method/url/headers, so"body" in requestwas wronglyfalse.Real-world impact
Auth.js's request-body parser gates on
if (!("body" in e) || !e.body || …) return. With"body" in requestfalse it skipped parsing the credentials POST body entirely — thecsrfTokenform field never reached the CSRF check, so a Next.js app's credentials login failed withMissingCSRF(redirecting to/auth/error?error=Configurationinstead of runningauthorize). Getting there took peeling many layers; every other piece (body read,clone().text(),URLSearchParams+Object.fromEntries, cookie round-trip, content-type gate,crypto.subtle.digesthash) already matched Node — theinoperator was the blocker.Fix
Two arms of
js_object_has_propertynow forward a string key to the same handle property dispatcher that property reads use (safe for these ids — no heap deref); the property exists iff it resolves to a non-undefined value, and a miss falls through so real expandos still resolve:Request/Response/Headers);class X extends Request/Responsearm — a heap object whose native members live on an underlying handle stashed in__perry_fetch_handle__(fetch_subclass_handle_id, already used by property reads /instanceof). Next.js'sNextRequestextendsRequest, so the credentials request Auth.js inspects is a subclass instance.A symbol key still reports
false(no own-property meaning on these handles). Mirrors the existing Web-Streamsindelegation in the same function.Coverage
test_gap_fetch_handle_in_operator.ts—"body"/"method"/"url"/"headers"/"bodyUsed" in request,Response(body/status/ok), aRequestsubclass, and a non-existent key — all byte-identical tonode --experimental-strip-types. Verified end-to-end: a Next.js 16 credentials login now passes the CSRF check (theMissingCSRFis gone).Summary by CodeRabbit
inoperator behavior for Web FetchRequestandResponse, including subclasses, for properties likebody,method,url,headers,bodyUsed,status, andok.inhandling for Buffer/typed-array values so Buffer methods and user-defined own properties are reported correctly; nonexistent properties still returnfalse.inchecks onRequest/ResponseandRequestsubclasses.