Improve spec conformance around request header validation (#33418)

* fix: improve spec conformance around request header validation

Signed-off-by: Shane Handley <shanehandley@fastmail.com>

* account for additional test passes

Signed-off-by: Shane Handley <shanehandley@fastmail.com>

* fix: remove redundant .to_vec call

Signed-off-by: Shane Handley <shanehandley@fastmail.com>

---------

Signed-off-by: Shane Handley <shanehandley@fastmail.com>

components/script/dom/headers.rs

@@ -9,7 +9,9 @@ use data_url::mime::Mime as DataUrlMime;
 use dom_struct::dom_struct;
 use http::header::{HeaderMap as HyperHeaders, HeaderName, HeaderValue};
 use js::rust::HandleObject;
-use net_traits::fetch::headers::get_value_from_header_list;
+use net_traits::fetch::headers::{
+    get_decode_and_split_header_value, get_value_from_header_list, is_forbidden_method,
+};
 use net_traits::request::is_cors_safelisted_request_header;
 
 use crate::dom::bindings::cell::DomRefCell;
@@ -85,12 +87,15 @@ impl HeadersMethods for Headers {
         // Step 2
         // https://fetch.spec.whatwg.org/#headers-validate
         let (mut valid_name, valid_value) = validate_name_and_value(name, value)?;
+
         valid_name = valid_name.to_lowercase();
 
         if self.guard.get() == Guard::Immutable {
             return Err(Error::Type("Guard is immutable".to_string()));
         }
-        if self.guard.get() == Guard::Request && is_forbidden_header_name(&valid_name) {
+        if self.guard.get() == Guard::Request &&
+            is_forbidden_request_header(&valid_name, &valid_value)
+        {
             return Ok(());
         }
         if self.guard.get() == Guard::Response && is_forbidden_response_header(&valid_name) {
@@ -141,13 +146,18 @@ impl HeadersMethods for Headers {
     // https://fetch.spec.whatwg.org/#dom-headers-delete
     fn Delete(&self, name: ByteString) -> ErrorResult {
         // Step 1
-        let valid_name = validate_name(name)?;
+        let (mut valid_name, valid_value) = validate_name_and_value(name, ByteString::new(vec![]))?;
+
+        valid_name = valid_name.to_lowercase();
+
         // Step 2
         if self.guard.get() == Guard::Immutable {
             return Err(Error::Type("Guard is immutable".to_string()));
         }
         // Step 3
-        if self.guard.get() == Guard::Request && is_forbidden_header_name(&valid_name) {
+        if self.guard.get() == Guard::Request &&
+            is_forbidden_request_header(&valid_name, &valid_value)
+        {
             return Ok(());
         }
         // Step 4
@@ -205,7 +215,9 @@ impl HeadersMethods for Headers {
             return Err(Error::Type("Guard is immutable".to_string()));
         }
         // Step 4
-        if self.guard.get() == Guard::Request && is_forbidden_header_name(&valid_name) {
+        if self.guard.get() == Guard::Request &&
+            is_forbidden_request_header(&valid_name, &valid_value)
+        {
             return Ok(());
         }
         // Step 5
@@ -358,14 +370,12 @@ impl Iterable for Headers {
     }
 }
 
-// https://fetch.spec.whatwg.org/#forbidden-response-header-name
-fn is_forbidden_response_header(name: &str) -> bool {
-    matches!(name, "set-cookie" | "set-cookie2")
-}
-
-// https://fetch.spec.whatwg.org/#forbidden-header-name
-pub fn is_forbidden_header_name(name: &str) -> bool {
-    let disallowed_headers = [
+/// This function will internally convert `name` to lowercase for matching, so explicitly converting
+/// before calling is not necessary
+///
+/// <https://fetch.spec.whatwg.org/#forbidden-request-header>
+pub fn is_forbidden_request_header(name: &str, value: &[u8]) -> bool {
+    let forbidden_header_names = [
         "accept-charset",
         "accept-encoding",
         "access-control-request-headers",
@@ -386,14 +396,61 @@ pub fn is_forbidden_header_name(name: &str) -> bool {
         "transfer-encoding",
         "upgrade",
         "via",
+        // This list is defined in the fetch spec, however the draft spec for private-network-access
+        // proposes this additional forbidden name, which is currently included in WPT tests. See:
+        // https://wicg.github.io/private-network-access/#forbidden-header-names
+        "access-control-request-private-network",
     ];
 
-    let disallowed_header_prefixes = ["sec-", "proxy-"];
+    // Step 1: If name is a byte-case-insensitive match for one of (forbidden_header_names), return
+    // true
+    let lowercase_name = name.to_lowercase();
 
-    disallowed_headers.iter().any(|header| *header == name) ||
-        disallowed_header_prefixes
+    if forbidden_header_names
+        .iter()
+        .any(|header| *header == lowercase_name.as_str())
+    {
+        return true;
+    }
+
+    let forbidden_header_prefixes = ["sec-", "proxy-"];
+
+    // Step 2: If name when byte-lowercased starts with `proxy-` or `sec-`, then return true.
+    if forbidden_header_prefixes
+        .iter()
+        .any(|prefix| lowercase_name.starts_with(prefix))
+    {
+        return true;
+    }
+
+    let potentially_forbidden_header_names = [
+        "x-http-method",
+        "x-http-method-override",
+        "x-method-override",
+    ];
+
+    // Step 3: If name is a byte-case-insensitive match for one of (potentially_forbidden_header_names)
+    if potentially_forbidden_header_names
+        .iter()
+        .any(|header| *header == lowercase_name)
+    {
+        // Step 3.1: Let parsedValues be the result of getting, decoding, and splitting value.
+        let parsed_values = get_decode_and_split_header_value(value.to_vec());
+
+        // Step 3.2: For each method of parsedValues: if the isomorphic encoding of method is a
+        // forbidden method, then return true.
+        return parsed_values
             .iter()
-            .any(|prefix| name.starts_with(prefix))
+            .any(|s| is_forbidden_method(s.as_bytes()));
+    }
+
+    // Step 4: Return false.
+    false
+}
+
+// https://fetch.spec.whatwg.org/#forbidden-response-header-name
+fn is_forbidden_response_header(name: &str) -> bool {
+    matches!(name, "set-cookie" | "set-cookie2")
 }
 
 // There is some unresolved confusion over the definition of a name and a value.

components/script/dom/request.rs

@@ -13,6 +13,7 @@ use http::method::InvalidMethod;
 use http::Method as HttpMethod;
 use js::jsapi::JSObject;
 use js::rust::HandleObject;
+use net_traits::fetch::headers::is_forbidden_method;
 use net_traits::request::{
     CacheMode as NetTraitsRequestCache, CredentialsMode as NetTraitsRequestCredentials,
     Destination as NetTraitsRequestDestination, Origin, RedirectMode as NetTraitsRequestRedirect,
@@ -503,14 +504,6 @@ fn is_method(m: &ByteString) -> bool {
     m.as_str().is_some()
 }
 
-// https://fetch.spec.whatwg.org/#forbidden-method
-fn is_forbidden_method(m: &ByteString) -> bool {
-    matches!(
-        m.to_lower().as_str(),
-        Some("connect") | Some("trace") | Some("track")
-    )
-}
-
 // https://fetch.spec.whatwg.org/#cors-safelisted-method
 fn is_cors_safelisted_method(m: &HttpMethod) -> bool {
     m == HttpMethod::GET || m == HttpMethod::HEAD || m == HttpMethod::POST

components/script/dom/xmlhttprequest.rs

@@ -59,7 +59,7 @@ use crate::dom::document::{Document, DocumentSource, HasBrowsingContext, IsHTMLD
 use crate::dom::event::{Event, EventBubbles, EventCancelable};
 use crate::dom::eventtarget::EventTarget;
 use crate::dom::globalscope::GlobalScope;
-use crate::dom::headers::{extract_mime_type, is_forbidden_header_name};
+use crate::dom::headers::{extract_mime_type, is_forbidden_request_header};
 use crate::dom::node::Node;
 use crate::dom::performanceresourcetiming::InitiatorType;
 use crate::dom::progressevent::ProgressEvent;
@@ -460,41 +460,37 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
 
     /// <https://xhr.spec.whatwg.org/#the-setrequestheader()-method>
     fn SetRequestHeader(&self, name: ByteString, value: ByteString) -> ErrorResult {
-        // Step 1, 2
+        // Step 1: If this’s state is not opened, then throw an "InvalidStateError" DOMException.
+        // Step 2: If this’s send() flag is set, then throw an "InvalidStateError" DOMException.
         if self.ready_state.get() != XMLHttpRequestState::Opened || self.send_flag.get() {
             return Err(Error::InvalidState);
         }
 
-        // Step 3
+        // Step 3: Normalize value.
         let value = trim_http_whitespace(&value);
 
-        // Step 4
+        // Step 4: If name is not a header name or value is not a header value, then throw a
+        // "SyntaxError" DOMException.
         if !is_token(&name) || !is_field_value(value) {
             return Err(Error::Syntax);
         }
-        let name_lower = name.to_lower();
-        let name_str = match name_lower.as_str() {
-            Some(s) => {
-                // Step 5
-                // Disallowed headers and header prefixes:
-                // https://fetch.spec.whatwg.org/#forbidden-header-name
-                if is_forbidden_header_name(s) {
-                    return Ok(());
-                } else {
-                    s
-                }
-            },
-            None => unreachable!(),
-        };
+
+        let name_str = name.as_str().ok_or(Error::Syntax)?;
+
+        // Step 5: If (name, value) is a forbidden request-header, then return.
+        if is_forbidden_request_header(name_str, &value) {
+            return Ok(());
+        }
 
         debug!(
             "SetRequestHeader: name={:?}, value={:?}",
-            name.as_str(),
+            name_str,
             str::from_utf8(value).ok()
         );
         let mut headers = self.request_headers.borrow_mut();
 
-        // Step 6
+        // Step 6: Combine (name, value) in this’s author request headers.
+        // https://fetch.spec.whatwg.org/#concept-header-list-combine
         let value = match headers.get(name_str).map(HeaderValue::as_bytes) {
             Some(raw) => {
                 let mut buf = raw.to_vec();