-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(fetch): Response.json shares ctor init validation; --platform bun accepts null-body-status bodies (#10360) #10368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| Fixed `Response.json(value, init)` skipping the `ResponseInit` validation that | ||
| `new Response(body, init)` applies (#10360). Both now share one check, in | ||
| Node's order: status range (`RangeError`), then `statusText` (`TypeError`), then | ||
| the body/null-body-status conflict. So `Response.json({a: 1}, {status: 204})` | ||
| throws Node's `TypeError: Response constructor: Invalid response status code | ||
| 204` instead of returning a 204, and `Response.json({}, {status: 600})` throws | ||
| a `RangeError` instead of returning a 600. The fix covers both perry-stdlib and | ||
| perry-ext-fetch. | ||
|
|
||
| Programs compiled with `--platform bun` follow Bun instead: a body with a | ||
| null-body status (204/205/304) is accepted by both constructors, so | ||
| `new Response("", {status: 204})` works. The compiler seeds | ||
| `__perry_runtime.setBunPlatform()` into every module's init next to the #9599 | ||
| `globalThis.Bun` install, which sets a runtime flag | ||
| (`perry-runtime/src/bun_compat/platform.rs`, `js_set_bun_platform` / | ||
| `js_bun_platform_enabled`) before any dependency's top-level code runs. | ||
|
|
||
| Tests: `test-files/test_gap_response_null_body_status_10360.ts` (Node parity) | ||
| and `crates/perry/tests/issue_10360_bun_platform_response_null_body.rs` (Bun | ||
| 1.3.14 output under `--platform bun`, plus a node-platform control). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| //! Runtime view of `perry compile --platform bun` (#10360). | ||
| //! | ||
| //! The platform is a compile-time choice, but a few Web APIs differ between | ||
| //! Node and Bun at runtime. Under `--platform bun` the compiler seeds every | ||
| //! module's init with `__perry_runtime.setBunPlatform()` (next to the | ||
| //! `globalThis.Bun` install), so the flag is set before any user code runs. | ||
| //! It is process-global and only ever turns on. | ||
| //! | ||
| //! Both setter and getter are `#[no_mangle]` so perry-stdlib and the ext | ||
| //! crates (which link the runtime by symbol, not by Rust path) all read the | ||
| //! same flag. | ||
|
|
||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||
|
|
||
| static BUN_PLATFORM: AtomicBool = AtomicBool::new(false); | ||
|
|
||
| /// Called from generated module init under `--platform bun`. | ||
| #[no_mangle] | ||
| pub extern "C" fn js_set_bun_platform() { | ||
| BUN_PLATFORM.store(true, Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// 1 when the program was compiled with `--platform bun`, else 0. | ||
| #[no_mangle] | ||
| pub extern "C" fn js_bun_platform_enabled() -> i32 { | ||
| i32::from(BUN_PLATFORM.load(Ordering::Relaxed)) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) fn reset_bun_platform_for_test() { | ||
| BUN_PLATFORM.store(false, Ordering::Relaxed); | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn bun_platform_flag_defaults_off_and_turns_on() { | ||
| reset_bun_platform_for_test(); | ||
| assert_eq!(js_bun_platform_enabled(), 0); | ||
| js_set_bun_platform(); | ||
| assert_eq!(js_bun_platform_enabled(), 1); | ||
| reset_bun_platform_for_test(); | ||
| assert_eq!(js_bun_platform_enabled(), 0); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,30 +28,24 @@ pub(super) fn alloc_response( | |
| id | ||
| } | ||
|
|
||
| /// new Response(body, statusOpt, statusTextPtrOpt, headersHandleOpt) | ||
| /// - body_ptr: StringHeader for the body, or null for "" | ||
| /// - status: f64 (200 default) | ||
| /// - status_text_ptr: StringHeader for statusText, or null for "" | ||
| /// - headers_handle: f64 numeric handle from js_headers_new, or 0 | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn js_response_new( | ||
| body_ptr: *const StringHeader, | ||
| /// Validate a `ResponseInit` the way Node's `initializeResponse` does, in its | ||
| /// order: status range, then statusText, then the body/null-body-status | ||
| /// conflict. Shared by `new Response` and `Response.json` so the two | ||
| /// construction paths cannot disagree (#10360). Returns (status, statusText). | ||
| /// | ||
| /// - `status`: NaN / 0.0 are the codegen "no status field" sentinels. Node | ||
| /// defaults missing status to 200; any explicit value is truncated toward | ||
| /// zero then range-checked against 200..=599 (199.9 → RangeError, 599.9 → | ||
| /// 599). Refs #2640. | ||
| /// - `statusText`: Node defaults it to the empty string (NOT the canonical | ||
| /// reason phrase) and validates the reason-phrase token. Refs #2640. | ||
| /// - A body with a null-body status (204/205/304) is a TypeError in Node, but | ||
| /// Bun accepts it, so `--platform bun` programs skip the check. | ||
| pub(super) unsafe fn response_init( | ||
| status: f64, | ||
| status_text_ptr: *const StringHeader, | ||
| headers_handle: f64, | ||
| ) -> f64 { | ||
| let body_stream_id = take_pending_fetch_body_stream_id(); | ||
| // Consume before validation so a throwing constructor cannot leak body | ||
| // metadata into the next Response construction on this thread. | ||
| let body_content_type = take_pending_fetch_body_content_type(); | ||
| // Lossless raw-byte read so binary bodies survive byte-for-byte (#5435). | ||
| let body_opt = dispatch::body_bytes_from_header(body_ptr); | ||
| let body_present = body_opt.is_some() || body_stream_id.is_some(); | ||
| let body = body_opt.unwrap_or_default(); | ||
| // NaN / 0.0 are the codegen "no status field" sentinels. Node defaults | ||
| // missing status to 200; any explicit value is truncated toward zero | ||
| // then range-checked against 200..=599 (199.9 → RangeError, 599.9 → | ||
| // 599). Refs #2640. | ||
| body_present: bool, | ||
| ) -> (u16, String) { | ||
| let status_u16 = if status.is_nan() || status == 0.0 { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '25,120p' crates/perry-stdlib/src/fetch/response_ctor.rs
sed -n '85,125p' crates/perry-stdlib/src/fetch/body_clone.rs
rg -n 'response_init|status_val|init_status|js_response_static_json|js_response_new' crates/perry-codegen/src/lower_call crates/perry-runtime/src/object/global_thisRepository: PerryTS/perry Length of output: 8380 🏁 Script executed: sed -n '1068,1185p' crates/perry-codegen/src/lower_call/builtin.rs
sed -n '35,135p' crates/perry-codegen/src/lower_call/options/fetch.rs
sed -n '1015,1045p' crates/perry-runtime/src/object/global_this/fetch_globals.rs
sed -n '35,65p' crates/perry-stdlib/src/fetch/response_ctor.rs
rg -n -A8 -B8 'Response\.json|status.*0|status.*NaN|js_response_static_json|js_response_new' crates/perry-stdlib crates/perry-codegen crates/perry-runtime | head -220Repository: PerryTS/perry Length of output: 36494 Preserve explicit
🤖 Prompt for AI Agents |
||
| 200 | ||
| } else { | ||
|
|
@@ -63,8 +57,6 @@ pub unsafe extern "C" fn js_response_new( | |
| } | ||
| truncated as u16 | ||
| }; | ||
| // Node defaults statusText to the empty string (NOT the canonical | ||
| // reason phrase) and validates the reason-phrase token. Refs #2640. | ||
| let status_text = match string_from_header(status_text_ptr) { | ||
| Some(s) => { | ||
| if !is_valid_status_text(&s) { | ||
|
|
@@ -74,11 +66,38 @@ pub unsafe extern "C" fn js_response_new( | |
| } | ||
| None => String::new(), | ||
| }; | ||
| if body_present && is_null_body_status(status_u16) { | ||
| if body_present | ||
| && is_null_body_status(status_u16) | ||
| && perry_runtime::bun_compat::js_bun_platform_enabled() == 0 | ||
| { | ||
| throw_fetch_type_error(&format!( | ||
| "Response constructor: Invalid response status code {status_u16}" | ||
| )); | ||
| } | ||
| (status_u16, status_text) | ||
| } | ||
|
|
||
| /// new Response(body, statusOpt, statusTextPtrOpt, headersHandleOpt) | ||
| /// - body_ptr: StringHeader for the body, or null for "" | ||
| /// - status: f64 (200 default) | ||
| /// - status_text_ptr: StringHeader for statusText, or null for "" | ||
| /// - headers_handle: f64 numeric handle from js_headers_new, or 0 | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn js_response_new( | ||
| body_ptr: *const StringHeader, | ||
| status: f64, | ||
| status_text_ptr: *const StringHeader, | ||
| headers_handle: f64, | ||
| ) -> f64 { | ||
| let body_stream_id = take_pending_fetch_body_stream_id(); | ||
| // Consume before validation so a throwing constructor cannot leak body | ||
| // metadata into the next Response construction on this thread. | ||
| let body_content_type = take_pending_fetch_body_content_type(); | ||
| // Lossless raw-byte read so binary bodies survive byte-for-byte (#5435). | ||
| let body_opt = dispatch::body_bytes_from_header(body_ptr); | ||
| let body_present = body_opt.is_some() || body_stream_id.is_some(); | ||
| let body = body_opt.unwrap_or_default(); | ||
| let (status_u16, status_text) = response_init(status, status_text_ptr, body_present); | ||
| let headers_id = handle_id(headers_handle); | ||
| let registered = (headers_id != 0) | ||
| .then(|| HEADERS_REGISTRY.lock().unwrap().get(&headers_id).cloned()) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 7605
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 18044
Keep explicit
status: 0distinct from an omitted status.ResponseandResponse.jsonpass a literalstatus: 0as0.0to the sharedresponse_init. The validator treats0.0as omitted and returns 200. The ResponseInit contract requires aRangeErrorfor an explicit status outside 200 through 599.Preserve status presence separately or use a nonnumeric omission sentinel. Update both lowering paths and their runtime callers so omitted status does not use
0.0; the shared validator then covers both ext-fetch construction paths.🤖 Prompt for AI Agents