-
-
Notifications
You must be signed in to change notification settings - Fork 163
Fix URL component reflection with prototype accessors #10997
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,13 @@ | ||
| Fixed URL component reflection (#10823). | ||
|
|
||
| URL instances now keep their twelve component values in internal numbered slots | ||
| and inherit enumerable, configurable accessors from `URL.prototype`. This makes | ||
| `Object.keys(url)` and `Object.getOwnPropertyNames(url)` empty while preserving | ||
| component reads, setters, and `Object.getPrototypeOf(url) === URL.prototype`. | ||
| The first ordinary or `Object.defineProperty` expando reserves the internal | ||
| slots before appending a user key, so it cannot overwrite `href` or the | ||
| `searchParams` owner link. The `host` accessor now also updates hostname and | ||
| an optional valid port. | ||
|
|
||
| Verified with a compiled TypeScript regression covering descriptors, borrowed | ||
| getters, setters, both expando paths, and URLSearchParams behavior on Linux. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -410,6 +410,11 @@ pub(crate) fn set_field_by_name_object_tail( | |
| let obj = obj_handle.get_raw_mut_ptr::<ObjectHeader>(); | ||
| let value = value_handle.get_nanbox_f64(); | ||
| match key_str.as_str() { | ||
| "origin" | "searchParams" => return, | ||
| "host" => { | ||
| crate::url::js_url_set_host(obj, value); | ||
| return; | ||
| } | ||
|
Comment on lines
+413
to
+417
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 '350,500p' crates/perry-runtime/src/object/field_set_by_name/tail.rsRepository: PerryTS/perry Length of output: 7046 🏁 Script executed: #!/bin/bash
sed -n '1,360p' crates/perry-runtime/src/object/field_set_by_name/tail.rs
printf '\n--- descriptor/accessor and URL references ---\n'
rg -n -C 4 'getter|setter|descriptor|define_property|defineProperty|origin|searchParams|is_url_object_shape|js_url_set_host' crates/perry-runtime/src crates/perry-runtime/tests 2>/dev/null | head -n 800Repository: PerryTS/perry Length of output: 42314 🏁 Script executed: #!/bin/bash
printf '%s\n' '--- generic write path and immutable writes ---'
rg -n -C 12 'get_accessor_descriptor|throw_immutable_write|own_key_present|own_descriptors_skip_key|plain_data_write_may_intercept' crates/perry-runtime/src/object crates/perry-runtime/src | head -n 1000
printf '%s\n' '--- URL definitions and property installation ---'
rg -n -C 12 'js_url_set_host|is_url_object_shape|searchParams|origin|set_builtin_accessor_descriptor|install_builtin_getter|AccessorDescriptor' crates/perry-runtime/src/url* crates/perry-runtime/src | head -n 1200Repository: PerryTS/perry Length of output: 42986 Apply own-property and accessor semantics before URL dispatch. The URL branch runs before the generic own-property and descriptor checks. A writable own The Resolve own properties first. Invoke the inherited URL setter only when no own property shadows it. Use the immutable-write path when the inherited accessor has no setter. 🤖 Prompt for AI Agents |
||
| "pathname" => { | ||
| crate::url::js_url_set_pathname(obj, value); | ||
| return; | ||
|
|
@@ -488,7 +493,7 @@ pub(crate) fn set_field_by_name_object_tail( | |
| // transition-cache fast path, whose `prev_shape_id` is read AFTER | ||
| // this — then appends at the floor. Seeding allocates, so every raw | ||
| // local is re-read through its handle. | ||
| if keys.is_null() && crate::object::reserved_slot_floor_for_class_id((*obj).class_id) != 0 { | ||
| if keys.is_null() && crate::object::reserved_slot_floor_for_object(obj) != 0 { | ||
| let seeded = crate::object::ensure_reserved_floor_keys(obj); | ||
| refresh_roots_after_alloc!(); | ||
| keys = crate::object::object_keys_array(obj); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| //! WebIDL URL accessors. The URL's numbered fields are private storage; | ||
| //! property lookup and reflection use these descriptors on URL.prototype. | ||
|
|
||
| use super::parse::*; | ||
| use super::url_class::*; | ||
| use super::*; | ||
|
|
||
| fn require_url_receiver(name: &str) -> *mut ObjectHeader { | ||
| let this = crate::object::js_implicit_this_get(); | ||
| if let Some(obj) = object_from_f64(this) { | ||
| if is_url_object_shape(obj) { | ||
|
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 | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: sed -n '1,80p' crates/perry-runtime/src/url/prototype.rs
sed -n '495,540p' crates/perry-runtime/src/url/url_class.rs
rg -n 'is_url_object_shape' crates/perry-runtime/src -nRepository: PerryTS/perry Length of output: 8098 The finding remains supported, but the consequence is a URL brand violation rather than established memory-unsafe behavior. A JavaScript-created ordinary object can satisfy the structural check if it has enough live slots, a valid absolute URL string in slot 0, and a slot-9 object whose owner slot points back to it. The accessors then accept that object. Getters return its stored values, and setters can mutate its URL-shaped slots instead of throwing The current check also requires valid GC-backed objects and sufficient live slots, so the inspected code does not establish an out-of-bounds access. Keep the fix focused on a non-forgeable URL brand and apply it to every 🤖 Prompt for AI Agents |
||
| return obj; | ||
| } | ||
| } | ||
| let message = format!("Value of URL.prototype.{name} called on an incompatible receiver"); | ||
| let msg = js_string_from_bytes(message.as_ptr(), message.len() as u32); | ||
| let err = crate::error::js_typeerror_new(msg); | ||
| crate::exception::js_throw(crate::value::js_nanbox_pointer(err as i64)); | ||
| } | ||
|
|
||
| macro_rules! url_getter { | ||
| ($fn_name:ident, $name:literal, $slot:expr) => { | ||
| extern "C" fn $fn_name(_closure: *const crate::closure::ClosureHeader) -> f64 { | ||
| let obj = require_url_receiver($name); | ||
| crate::object::js_object_get_field_f64(obj, $slot) | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| macro_rules! url_setter { | ||
| ($fn_name:ident, $name:literal, $setter:path) => { | ||
| extern "C" fn $fn_name(_closure: *const crate::closure::ClosureHeader, value: f64) -> f64 { | ||
| let obj = require_url_receiver($name); | ||
| $setter(obj, value); | ||
| f64::from_bits(crate::value::TAG_UNDEFINED) | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| url_getter!(get_href, "href", URL_HREF); | ||
| url_getter!(get_protocol, "protocol", URL_PROTOCOL); | ||
| url_getter!(get_host, "host", URL_HOST); | ||
| url_getter!(get_hostname, "hostname", URL_HOSTNAME); | ||
| url_getter!(get_port, "port", URL_PORT); | ||
| url_getter!(get_pathname, "pathname", URL_PATHNAME); | ||
| url_getter!(get_search, "search", URL_SEARCH); | ||
| url_getter!(get_hash, "hash", URL_HASH); | ||
| url_getter!(get_origin, "origin", URL_ORIGIN); | ||
| url_getter!(get_search_params, "searchParams", URL_SEARCH_PARAMS); | ||
| url_getter!(get_username, "username", URL_USERNAME); | ||
| url_getter!(get_password, "password", URL_PASSWORD); | ||
| url_setter!(set_href, "href", js_url_set_href); | ||
| url_setter!(set_protocol, "protocol", js_url_set_protocol); | ||
| url_setter!(set_host, "host", js_url_set_host); | ||
| url_setter!(set_hostname, "hostname", js_url_set_hostname); | ||
| url_setter!(set_port, "port", js_url_set_port); | ||
| url_setter!(set_pathname, "pathname", js_url_set_pathname); | ||
| url_setter!(set_search, "search", js_url_set_search); | ||
| url_setter!(set_hash, "hash", js_url_set_hash); | ||
| url_setter!(set_username, "username", js_url_set_username); | ||
| url_setter!(set_password, "password", js_url_set_password); | ||
|
|
||
| pub(crate) fn install_url_prototype_accessors(proto: *mut ObjectHeader) { | ||
| let entries: &[(&str, *const u8, Option<*const u8>)] = &[ | ||
| ("href", get_href as *const u8, Some(set_href as *const u8)), | ||
| ("origin", get_origin as *const u8, None), | ||
| ( | ||
| "protocol", | ||
| get_protocol as *const u8, | ||
| Some(set_protocol as *const u8), | ||
| ), | ||
| ( | ||
| "username", | ||
| get_username as *const u8, | ||
| Some(set_username as *const u8), | ||
| ), | ||
| ( | ||
| "password", | ||
| get_password as *const u8, | ||
| Some(set_password as *const u8), | ||
| ), | ||
| ("host", get_host as *const u8, Some(set_host as *const u8)), | ||
| ( | ||
| "hostname", | ||
| get_hostname as *const u8, | ||
| Some(set_hostname as *const u8), | ||
| ), | ||
| ("port", get_port as *const u8, Some(set_port as *const u8)), | ||
| ( | ||
| "pathname", | ||
| get_pathname as *const u8, | ||
| Some(set_pathname as *const u8), | ||
| ), | ||
| ( | ||
| "search", | ||
| get_search as *const u8, | ||
| Some(set_search as *const u8), | ||
| ), | ||
| ("searchParams", get_search_params as *const u8, None), | ||
| ("hash", get_hash as *const u8, Some(set_hash as *const u8)), | ||
| ]; | ||
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let proto_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(proto as i64)); | ||
| for &(name, getter, setter) in entries { | ||
| unsafe { | ||
| crate::closure::js_register_closure_arity(getter, 0); | ||
| let get = crate::closure::js_closure_alloc(getter, 0); | ||
| let get_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(get as i64)); | ||
| crate::object::native_module::set_bound_native_closure_name( | ||
| get, | ||
| &format!("get {name}"), | ||
| ); | ||
| let get = crate::value::js_nanbox_get_pointer(get_h.get_nanbox_f64()) as usize; | ||
| crate::object::native_module::set_builtin_closure_length(get, 0); | ||
| crate::object::native_module::set_builtin_closure_non_constructable(get); | ||
| let set_h = setter.map(|func| { | ||
| crate::closure::js_register_closure_arity(func, 1); | ||
| let set = crate::closure::js_closure_alloc(func, 0); | ||
| let handle = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(set as i64)); | ||
| crate::object::native_module::set_bound_native_closure_name( | ||
| set, | ||
| &format!("set {name}"), | ||
| ); | ||
| let set = crate::value::js_nanbox_get_pointer(handle.get_nanbox_f64()) as usize; | ||
| crate::object::native_module::set_builtin_closure_length(set, 1); | ||
| crate::object::native_module::set_builtin_closure_non_constructable(set); | ||
| handle | ||
| }); | ||
| let proto = | ||
| crate::value::js_nanbox_get_pointer(proto_h.get_nanbox_f64()) as *mut ObjectHeader; | ||
| let get_bits = get_h.get_nanbox_f64().to_bits(); | ||
| crate::object::install_builtin_getter(proto, name, get_bits); | ||
| let proto = | ||
| crate::value::js_nanbox_get_pointer(proto_h.get_nanbox_f64()) as *mut ObjectHeader; | ||
| crate::object::set_builtin_accessor_descriptor( | ||
| proto as usize, | ||
| name.to_string(), | ||
| crate::object::AccessorDescriptor { | ||
| get: get_h.get_nanbox_f64().to_bits(), | ||
| set: set_h.as_ref().map_or(0, |h| h.get_nanbox_f64().to_bits()), | ||
| }, | ||
| crate::object::PropertyAttrs::new(true, true, true), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
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 | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '340,410p' crates/perry-runtime/src/url/url_class.rsRepository: PerryTS/perry
Length of output: 2762
Reject invalid explicit ports before changing the host.
js_url_set_hostmust reject the complete assignment when the input contains an invalid explicit port. The current code updatesURL_HOSTNAMEfirst and only updatesURL_PORTwhen parsing succeeds. Therefore,"new.example:bad"and"new.example:99999"change the hostname while retaining the previous port."new.example:8080abc"is truncated to"8080"and incorrectly accepted instead of being rejected.A missing port can preserve the existing port, but an explicit invalid port must leave the URL unchanged.
🤖 Prompt for AI Agents