fix(fetch): address PR review comments for iterators, URL validation, and intrinsics

Rajdeep Singh · Rajdeep Singh · commit c1c256735921 · 2026-03-14T04:33:00.000+05:30
diff --git a/core/engine/src/builtins/string/mod.rs b/core/engine/src/builtins/string/mod.rs
@@ -13,8 +13,8 @@ use crate::{
     Context, JsArgs, JsExpect, JsResult, JsString, JsValue,
     builtins::{Array, BuiltInObject, Number, RegExp},
     context::intrinsics::{Intrinsics, StandardConstructor, StandardConstructors},
-    error::RuntimeLimitError,
     error::JsNativeError,
+    error::RuntimeLimitError,
     js_string,
     object::{JsObject, internal_methods::get_prototype_from_constructor},
     property::{Attribute, PropertyDescriptor},
@@ -721,8 +721,7 @@ impl String {
 
         let n = u64::try_from(n).expect("n was checked to be non-negative");
 
-        if n
-            .checked_mul(len as u64)
+        if n.checked_mul(len as u64)
             .is_none_or(|total_len| total_len > (Self::MAX_STRING_LENGTH as u64))
         {
             return Err(JsNativeError::range()
diff --git a/core/engine/src/builtins/string/tests.rs b/core/engine/src/builtins/string/tests.rs
@@ -2,9 +2,7 @@ use boa_macros::js_str;
 use indoc::indoc;
 
 use crate::{
-    JsNativeErrorKind, JsValue, TestAction,
-    error::RuntimeLimitError,
-    js_string, run_test_actions,
+    JsNativeErrorKind, JsValue, TestAction, error::RuntimeLimitError, js_string, run_test_actions,
 };
 
 #[test]
diff --git a/core/engine/src/module/loader/mod.rs b/core/engine/src/module/loader/mod.rs
@@ -62,6 +62,7 @@ pub fn resolve_module_specifier(
 
     // On Windows, also replace `/` with `\`. JavaScript imports use `/` as path separator.
     #[cfg(target_family = "windows")]
+    #[allow(clippy::disallowed_methods)]
     let specifier = specifier.replace('/', "\\");
 
     let short_path = Path::new(&specifier);
diff --git a/core/runtime/src/fetch/headers.rs b/core/runtime/src/fetch/headers.rs
@@ -3,17 +3,17 @@
 //! See <https://developer.mozilla.org/en-US/docs/Web/API/Headers>.
 #![allow(clippy::needless_pass_by_value)]
 
+use boa_engine::builtins::iterable::create_iter_result_object;
 use boa_engine::interop::JsClass;
-use boa_engine::object::builtins::{JsArray, TypedJsFunction};
+use boa_engine::native_function::NativeFunction;
 use boa_engine::object::FunctionObjectBuilder;
+use boa_engine::object::builtins::{JsArray, TypedJsFunction};
 use boa_engine::property::PropertyDescriptor;
 use boa_engine::value::{Convert, TryFromJs};
 use boa_engine::{
     Context, Finalize, JsData, JsObject, JsResult, JsString, JsValue, Trace, boa_class, js_error,
     js_string,
 };
-use boa_engine::builtins::iterable::create_iter_result_object;
-use boa_engine::native_function::NativeFunction;
 use http::header::HeaderMap as HttpHeaderMap;
 use http::{HeaderName, HeaderValue};
 use std::cell::RefCell;
@@ -44,44 +44,54 @@ enum HeadersIteratorKind {
 ///  - [WHATWG spec](https://fetch.spec.whatwg.org/#headers-class)
 #[derive(Debug, Clone, JsData, Trace, Finalize)]
 struct HeadersIterator {
-    headers: JsHeaders,
+    headers: Vec<(JsString, JsString)>,
     index: usize,
     kind: HeadersIteratorKind,
 }
 
 impl HeadersIterator {
     /// Creates a new Headers iterator.
     fn new(headers: JsHeaders, kind: HeadersIteratorKind) -> Self {
+        let headers_vec: Vec<(JsString, JsString)> = {
+            let headers_data = headers.headers.borrow();
+            headers_data
+                .iter()
+                .map(|(key, value)| {
+                    (
+                        JsString::from(key.as_str()),
+                        JsString::from(value.to_str().unwrap_or_default()),
+                    )
+                })
+                .collect()
+        };
+
         Self {
-            headers,
+            headers: headers_vec,
             index: 0,
             kind,
         }
     }
 
     /// Gets the next entry in the headers iterator.
-    fn next(&mut self, context: &mut Context) -> JsResult<JsValue> {
-        let headers_data = self.headers.headers.borrow();
-        let headers_vec: Vec<_> = headers_data.iter().collect();
-
-        if self.index >= headers_vec.len() {
-            return Ok(create_iter_result_object(JsValue::undefined(), true, context));
+    fn next(&mut self, context: &mut Context) -> JsValue {
+        if self.index >= self.headers.len() {
+            return create_iter_result_object(JsValue::undefined(), true, context);
         }
 
-        let (key, value) = headers_vec[self.index];
+        let (key, value) = &self.headers[self.index];
         self.index += 1;
 
         let value_obj = match self.kind {
             HeadersIteratorKind::Entries => {
-                let key_val: JsValue = JsString::from(key.as_str()).into();
-                let val_val: JsValue = JsString::from(value.to_str().unwrap_or_default()).into();
+                let key_val: JsValue = key.clone().into();
+                let val_val: JsValue = value.clone().into();
                 JsArray::from_iter([key_val, val_val], context).into()
             }
-            HeadersIteratorKind::Keys => JsString::from(key.as_str()).into(),
-            HeadersIteratorKind::Values => JsString::from(value.to_str().unwrap_or_default()).into(),
+            HeadersIteratorKind::Keys => key.clone().into(),
+            HeadersIteratorKind::Values => value.clone().into(),
         };
 
-        Ok(create_iter_result_object(value_obj, false, context))
+        create_iter_result_object(value_obj, false, context)
     }
 }
 
@@ -122,12 +132,19 @@ fn headers_iterator_next(
         .and_then(JsObject::downcast_mut::<HeadersIterator>)
         .ok_or_else(|| js_error!(TypeError: "`this` is not a Headers iterator"))?;
 
-    iterator.next(context)
+    Ok(iterator.next(context))
 }
 
 /// Creates a Headers iterator object wrapper.
-fn create_headers_iterator_object(iterator: HeadersIterator, context: &mut Context) -> JsValue {
-    let proto = context.intrinsics().objects().iterator_prototypes().iterator();
+fn create_headers_iterator_object(
+    iterator: HeadersIterator,
+    context: &mut Context,
+) -> JsResult<JsValue> {
+    let proto = context
+        .intrinsics()
+        .objects()
+        .iterator_prototypes()
+        .iterator();
     let iterator_obj = JsObject::from_proto_and_data(proto, iterator);
 
     let next_fn = FunctionObjectBuilder::new(
@@ -139,18 +156,17 @@ fn create_headers_iterator_object(iterator: HeadersIterator, context: &mut Conte
     .constructor(false)
     .build();
 
-    #[allow(let_underscore_drop)]
-    let _ = iterator_obj.define_property_or_throw(
+    iterator_obj.define_property_or_throw(
         js_string!("next"),
         PropertyDescriptor::builder()
             .value(next_fn)
             .writable(true)
             .enumerable(false)
             .configurable(true),
         context,
-    );
+    )?;
 
-    iterator_obj.into()
+    Ok(iterator_obj.into())
 }
 
 /// A JavaScript wrapper for the `Headers` object.
@@ -266,10 +282,13 @@ impl JsHeaders {
 
     /// Returns an iterator allowing to go through all key/value pairs contained in this object.
     ///
+    /// # Errors
+    /// Returns an error if the iterator object cannot be created.
+    ///
     /// More information:
     ///  - [WHATWG spec](https://fetch.spec.whatwg.org/#headers-class)
     #[boa(method)]
-    pub fn entries(this: JsClass<Self>, context: &mut Context) -> JsValue {
+    pub fn entries(this: JsClass<Self>, context: &mut Context) -> JsResult<JsValue> {
         let iterator = HeadersIterator::new(this.clone_inner(), HeadersIteratorKind::Entries);
         create_headers_iterator_object(iterator, context)
     }
@@ -348,10 +367,13 @@ impl JsHeaders {
     /// Returns an iterator allowing you to go through all keys of the key/value pairs
     /// contained in this object.
     ///
+    /// # Errors
+    /// Returns an error if the iterator object cannot be created.
+    ///
     /// More information:
     ///  - [WHATWG spec](https://fetch.spec.whatwg.org/#headers-class)
     #[boa(method)]
-    fn keys(this: JsClass<Self>, context: &mut Context) -> JsValue {
+    fn keys(this: JsClass<Self>, context: &mut Context) -> JsResult<JsValue> {
         let iterator = HeadersIterator::new(this.clone_inner(), HeadersIteratorKind::Keys);
         create_headers_iterator_object(iterator, context)
     }
@@ -367,10 +389,13 @@ impl JsHeaders {
 
     /// Returns an iterator allowing you to go through all values in the Headers object.
     ///
+    /// # Errors
+    /// Returns an error if the iterator object cannot be created.
+    ///
     /// More information:
     ///  - [WHATWG spec](https://fetch.spec.whatwg.org/#headers-class)
     #[boa(method)]
-    pub fn values(this: JsClass<Self>, context: &mut Context) -> JsValue {
+    pub fn values(this: JsClass<Self>, context: &mut Context) -> JsResult<JsValue> {
         let iterator = HeadersIterator::new(this.clone_inner(), HeadersIteratorKind::Values);
         create_headers_iterator_object(iterator, context)
     }
diff --git a/core/runtime/src/fetch/mod.rs b/core/runtime/src/fetch/mod.rs
@@ -11,12 +11,11 @@ use crate::fetch::headers::JsHeaders;
 use crate::fetch::request::{JsRequest, RequestInit};
 use crate::fetch::response::JsResponse;
 use boa_engine::class::Class;
-use boa_engine::object::FunctionObjectBuilder;
 use boa_engine::property::PropertyDescriptor;
 use boa_engine::realm::Realm;
 use boa_engine::{
     Context, Finalize, JsData, JsError, JsObject, JsResult, JsString, JsSymbol, JsValue,
-    NativeObject, Trace, boa_module, js_error, js_string, native_function::NativeFunction,
+    NativeObject, Trace, boa_module, js_error, js_string,
 };
 use either::Either;
 use http::{HeaderName, HeaderValue, Request as HttpRequest, Request};
@@ -204,22 +203,6 @@ pub mod js_module {
 #[doc(inline)]
 pub use js_module::fetch;
 
-fn headers_iterator(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
-    // Call the entries method through the JavaScript object mechanism
-    let this_object = this.as_object()
-        .ok_or_else(|| {
-            js_error!(TypeError: "`Headers.prototype[Symbol.iterator]` requires a `Headers` object")
-        })?;
-
-    // Get the entries method from the prototype and call it
-    let entries_fn = this_object
-        .get(js_string!("entries"), context)?;
-    
-    entries_fn.as_function()
-        .ok_or_else(|| js_error!(TypeError: "entries is not a function"))?
-        .call(this, &[], context)
-}
-
 /// Register the `fetch` function in the realm, as well as ALL supporting classes.
 /// Pass `None` as the realm to register globally.
 ///
@@ -246,19 +229,12 @@ pub fn register<F: Fetcher>(
     .ok_or_else(|| js_error!(Error: "Headers class should be registered"))?
     .prototype();
 
-    let iterator = FunctionObjectBuilder::new(
-        context.realm(),
-        NativeFunction::from_fn_ptr(headers_iterator),
-    )
-    .name(js_string!("[Symbol.iterator]"))
-    .length(0)
-    .constructor(false)
-    .build();
+    let entries_fn = headers_proto.get(js_string!("entries"), context)?;
 
     headers_proto.define_property_or_throw(
         JsSymbol::iterator(),
         PropertyDescriptor::builder()
-            .value(iterator)
+            .value(entries_fn)
             .writable(true)
             .enumerable(false)
             .configurable(true),
diff --git a/core/runtime/src/fetch/response.rs b/core/runtime/src/fetch/response.rs
@@ -179,12 +179,19 @@ impl JsResponse {
         }
 
         let mut js_response = Self::basic(js_string!(""), http::Response::new(Vec::new()));
-        js_response.status = Some(StatusCode::from_u16(status).expect("guaranteed to be a valid status by preceding match"));
+        js_response.status = Some(
+            StatusCode::from_u16(status)
+                .expect("guaranteed to be a valid status by preceding match"),
+        );
+
+        let url_str = url.to_std_string_escaped();
+        let parsed_url = url::Url::parse(&url_str)
+            .map_err(|_| JsNativeError::typ().with_message("Invalid redirect URL"))?;
 
         let mut headers = JsHeaders::default();
         headers.append(
             Convert::from("Location".to_string()),
-            Convert::from(url.to_std_string_escaped()),
+            Convert::from(parsed_url.to_string()),
         )?;
         js_response.headers = headers;
 
@@ -201,16 +208,13 @@ impl JsResponse {
         init: Option<JsResponseOptions>,
         context: &mut Context,
     ) -> JsResult<Self> {
-        let json = context.global_object().get(js_string!("JSON"), context)?;
-        let stringify = json
-            .as_object()
-            .ok_or_else(|| JsNativeError::typ().with_message("JSON is not an object"))?
-            .get(js_string!("stringify"), context)?;
+        let json = context.intrinsics().objects().json();
+        let stringify = json.get(js_string!("stringify"), context)?;
 
         let text = stringify
             .as_callable()
             .ok_or_else(|| JsNativeError::typ().with_message("stringify is not a function"))?
-            .call(&json, &[data], context)?;
+            .call(&json.into(), &[data], context)?;
 
         let text = if let Some(s) = text.as_string() {
             s.to_std_string_escaped()
diff --git a/core/runtime/src/fetch/tests/headers.rs b/core/runtime/src/fetch/tests/headers.rs
@@ -24,3 +24,40 @@ fn headers_are_iterable() {
         ),
     ]);
 }
+
+#[test]
+fn headers_iterators() {
+    run_test_actions([
+        TestAction::harness(),
+        TestAction::inspect_context(register),
+        TestAction::run(
+            r#"
+                const headers = new Headers([["a", "1"], ["b", "2"]]);
+                
+                // entries()
+                const entriesList = [...headers.entries()];
+                assertEq(entriesList.length, 2);
+                assertEq(entriesList[0][0], "a");
+                assertEq(entriesList[0][1], "1");
+                
+                // keys()
+                const keysList = [...headers.keys()];
+                assertEq(keysList.length, 2);
+                assertEq(keysList[0], "a");
+                assertEq(keysList[1], "b");
+                
+                // values()
+                const valuesList = [...headers.values()];
+                assertEq(valuesList.length, 2);
+                assertEq(valuesList[0], "1");
+                assertEq(valuesList[1], "2");
+                
+                // .next() directly
+                const it = headers.entries();
+                const first = it.next();
+                assertEq(first.done, false);
+                assertEq(first.value[0], "a");
+            "#,
+        ),
+    ]);
+}
