Now just one is_cors_safelisted_request_header, with closer spec alignment

components/net/fetch/methods.rs

@@ -12,15 +12,17 @@ use content_security_policy as csp;
 use crossbeam_channel::{unbounded, Receiver, Sender};
 use devtools_traits::DevtoolsControlMsg;
 use headers::{AccessControlExposeHeaders, ContentType, HeaderMapExt, Range};
-use http::header::{self, HeaderMap, HeaderName, HeaderValue};
+use http::header::{self, HeaderMap, HeaderName};
 use hyper::Method;
 use hyper::StatusCode;
 use ipc_channel::ipc::IpcReceiver;
 use mime::{self, Mime};
 use net_traits::blob_url_store::{parse_blob_url, BlobURLStoreError};
 use net_traits::filemanager_thread::RelativePos;
+use net_traits::request::{
+    is_cors_safelisted_method, is_cors_safelisted_request_header, Origin, ResponseTainting, Window,
+};
 use net_traits::request::{CredentialsMode, Destination, Referrer, Request, RequestMode};
-use net_traits::request::{Origin, ResponseTainting, Window};
 use net_traits::response::{Response, ResponseBody, ResponseType};
 use net_traits::{FetchTaskTarget, NetworkError, ReferrerPolicy, ResourceFetchTiming};
 use net_traits::{ResourceAttribute, ResourceTimeValue};
@@ -793,31 +795,6 @@ fn scheme_fetch(
     }
 }
 
-/// <https://fetch.spec.whatwg.org/#cors-safelisted-request-header>
-pub fn is_cors_safelisted_request_header(name: &HeaderName, value: &HeaderValue) -> bool {
-    if name == header::CONTENT_TYPE {
-        if let Some(m) = value.to_str().ok().and_then(|s| s.parse::<Mime>().ok()) {
-            m.type_() == mime::TEXT && m.subtype() == mime::PLAIN ||
-                m.type_() == mime::APPLICATION && m.subtype() == mime::WWW_FORM_URLENCODED ||
-                m.type_() == mime::MULTIPART && m.subtype() == mime::FORM_DATA
-        } else {
-            false
-        }
-    } else {
-        name == header::ACCEPT ||
-            name == header::ACCEPT_LANGUAGE ||
-            name == header::CONTENT_LANGUAGE
-    }
-}
-
-/// <https://fetch.spec.whatwg.org/#cors-safelisted-method>
-pub fn is_cors_safelisted_method(m: &Method) -> bool {
-    match *m {
-        Method::GET | Method::HEAD | Method::POST => true,
-        _ => false,
-    }
-}
-
 fn is_null_body_status(status: &Option<(StatusCode, String)>) -> bool {
     match *status {
         Some((status, _)) => match status {

components/net/http_loader.rs

@@ -7,10 +7,7 @@ use crate::cookie;
 use crate::cookie_storage::CookieStorage;
 use crate::decoder::Decoder;
 use crate::fetch::cors_cache::CorsCache;
-use crate::fetch::methods::{
+use crate::fetch::methods::{main_fetch, Data, DoneChannel, FetchContext, Target};
-    is_cors_safelisted_method, is_cors_safelisted_request_header, main_fetch,
-};
-use crate::fetch::methods::{Data, DoneChannel, FetchContext, Target};
 use crate::hsts::HstsList;
 use crate::http_cache::{CacheKey, HttpCache};
 use crate::resource_thread::AuthCache;
@@ -38,6 +35,7 @@ use hyper_serde::Serde;
 use msg::constellation_msg::{HistoryStateId, PipelineId};
 use net_traits::quality::{quality_to_value, Quality, QualityItem};
 use net_traits::request::Origin::Origin as SpecificOrigin;
+use net_traits::request::{is_cors_safelisted_method, is_cors_safelisted_request_header};
 use net_traits::request::{CacheMode, CredentialsMode, Destination, Origin};
 use net_traits::request::{RedirectMode, Referrer, Request, RequestBuilder, RequestMode};
 use net_traits::request::{ResponseTainting, ServiceWorkersMode};

components/net_traits/request.rs

@@ -7,6 +7,7 @@ use crate::ResourceTimingType;
 use content_security_policy::{self as csp, CspList};
 use http::HeaderMap;
 use hyper::Method;
+use mime::Mime;
 use msg::constellation_msg::PipelineId;
 use servo_url::{ImmutableOrigin, ServoUrl};
 
@@ -469,3 +470,105 @@ impl Referrer {
         }
     }
 }
+
+// https://fetch.spec.whatwg.org/#cors-unsafe-request-header-byte
+// TODO: values in the control-code range are being quietly stripped out by
+// HeaderMap and never reach this function to be loudly rejected!
+fn is_cors_unsafe_request_header_byte(value: &u8) -> bool {
+    match value {
+        0x00..=0x08 |
+        0x10..=0x19 |
+        0x22 |
+        0x28 |
+        0x29 |
+        0x3A |
+        0x3C |
+        0x3E |
+        0x3F |
+        0x40 |
+        0x5B |
+        0x5C |
+        0x5D |
+        0x7B |
+        0x7D |
+        0x7F => true,
+        _ => false,
+    }
+}
+
+// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+// subclause `accept`
+fn is_cors_safelisted_request_accept(value: &[u8]) -> bool {
+    !(value.iter().any(is_cors_unsafe_request_header_byte))
+}
+
+// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+// subclauses `accept-language`, `content-language`
+fn is_cors_safelisted_language(value: &[u8]) -> bool {
+    value.iter().all(|&x| match x {
+        0x30..=0x39 |
+        0x41..=0x5A |
+        0x61..=0x7A |
+        0x20 |
+        0x2A |
+        0x2C |
+        0x2D |
+        0x2E |
+        0x3B |
+        0x3D => true,
+        _ => false,
+    })
+}
+
+// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+// subclause `content-type`
+fn is_cors_safelisted_request_content_type(value: &[u8]) -> bool {
+    // step 1
+    if value.iter().any(is_cors_unsafe_request_header_byte) {
+        return false;
+    }
+    // step 2
+    let value_string = if let Ok(s) = std::str::from_utf8(value) {
+        s
+    } else {
+        return false;
+    };
+    let value_mime_result: Result<Mime, _> = value_string.parse();
+    match value_mime_result {
+        Err(_) => false, // step 3
+        Ok(value_mime) => match (value_mime.type_(), value_mime.subtype()) {
+            (mime::APPLICATION, mime::WWW_FORM_URLENCODED) |
+            (mime::MULTIPART, mime::FORM_DATA) |
+            (mime::TEXT, mime::PLAIN) => true,
+            _ => false, // step 4
+        },
+    }
+}
+
+// TODO: "DPR", "Downlink", "Save-Data", "Viewport-Width", "Width":
+// ... once parsed, the value should not be failure.
+// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+pub fn is_cors_safelisted_request_header<N: AsRef<str>, V: AsRef<[u8]>>(
+    name: &N,
+    value: &V,
+) -> bool {
+    let name: &str = name.as_ref();
+    let value: &[u8] = value.as_ref();
+    if value.len() > 128 {
+        return false;
+    }
+    match name {
+        "accept" => is_cors_safelisted_request_accept(value),
+        "accept-language" | "content-language" => is_cors_safelisted_language(value),
+        "content-type" => is_cors_safelisted_request_content_type(value),
+        _ => false,
+    }
+}
+
+/// <https://fetch.spec.whatwg.org/#cors-safelisted-method>
+pub fn is_cors_safelisted_method(m: &Method) -> bool {
+    match *m {
+        Method::GET | Method::HEAD | Method::POST => true,
+        _ => false,
+    }
+}

components/script/dom/headers.rs

@@ -14,9 +14,8 @@ use crate::dom::bindings::str::{is_token, ByteString};
 use crate::dom::globalscope::GlobalScope;
 use dom_struct::dom_struct;
 use http::header::{self, HeaderMap as HyperHeaders, HeaderName, HeaderValue};
-use mime::{self, Mime};
+use net_traits::request::is_cors_safelisted_request_header;
 use std::cell::Cell;
-use std::result::Result;
 use std::str::{self, FromStr};
 
 #[dom_struct]
@@ -28,7 +27,7 @@ pub struct Headers {
 }
 
 // https://fetch.spec.whatwg.org/#concept-headers-guard
-#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
+#[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf, PartialEq)]
 pub enum Guard {
     Immutable,
     Request,
@@ -88,6 +87,9 @@ impl HeadersMethods for Headers {
             return Ok(());
         }
         // Step 7
+        // FIXME: this is NOT what WHATWG says to do when appending
+        // another copy of an existing header. HyperHeaders
+        // might not expose the information we need to do it right.
         let mut combined_value: Vec<u8> = vec![];
         if let Some(v) = self
             .header_list
@@ -301,35 +303,6 @@ impl Iterable for Headers {
     }
 }
 
-fn is_cors_safelisted_request_content_type(value: &[u8]) -> bool {
-    let value_string = if let Ok(s) = str::from_utf8(value) {
-        s
-    } else {
-        return false;
-    };
-    let value_mime_result: Result<Mime, _> = value_string.parse();
-    match value_mime_result {
-        Err(_) => false,
-        Ok(value_mime) => match (value_mime.type_(), value_mime.subtype()) {
-            (mime::APPLICATION, mime::WWW_FORM_URLENCODED) |
-            (mime::MULTIPART, mime::FORM_DATA) |
-            (mime::TEXT, mime::PLAIN) => true,
-            _ => false,
-        },
-    }
-}
-
-// TODO: "DPR", "Downlink", "Save-Data", "Viewport-Width", "Width":
-// ... once parsed, the value should not be failure.
-// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
-fn is_cors_safelisted_request_header(name: &str, value: &[u8]) -> bool {
-    match name {
-        "accept" | "accept-language" | "content-language" => true,
-        "content-type" => is_cors_safelisted_request_content_type(value),
-        _ => false,
-    }
-}
-
 // https://fetch.spec.whatwg.org/#forbidden-response-header-name
 fn is_forbidden_response_header(name: &str) -> bool {
     match name {
@@ -394,11 +367,18 @@ pub fn is_forbidden_header_name(name: &str) -> bool {
 // [2] https://tools.ietf.org/html/rfc7230#section-3.2
 // [3] https://tools.ietf.org/html/rfc7230#section-3.2.6
 // [4] https://www.rfc-editor.org/errata_search.php?rfc=7230
+//
+// As of December 2019 WHATWG, isn't even using grammar productions for value;
+// https://fetch.spec.whatg.org/#concept-header-value just says not to have
+// newlines, nulls, or leading/trailing whitespace.
 fn validate_name_and_value(name: ByteString, value: ByteString) -> Fallible<(String, Vec<u8>)> {
     let valid_name = validate_name(name)?;
+
+    // this is probably out of date
     if !is_field_content(&value) {
         return Err(Error::Type("Value is not valid".to_string()));
     }
+
     Ok((valid_name, value.into()))
 }
 

components/script/dom/request.rs

@@ -90,59 +90,63 @@ impl Request {
         // Step 4
         let base_url = global.api_base_url();
 
+        // Step 5 TODO: "Let signal be null."
+
         match input {
-            // Step 5
+            // Step 6
             RequestInfo::USVString(USVString(ref usv_string)) => {
-                // Step 5.1
+                // Step 6.1
                 let parsed_url = base_url.join(&usv_string);
-                // Step 5.2
+                // Step 6.2
                 if parsed_url.is_err() {
                     return Err(Error::Type("Url could not be parsed".to_string()));
                 }
-                // Step 5.3
+                // Step 6.3
                 let url = parsed_url.unwrap();
                 if includes_credentials(&url) {
                     return Err(Error::Type("Url includes credentials".to_string()));
                 }
-                // Step 5.4
+                // Step 6.4
                 temporary_request = net_request_from_global(global, url);
-                // Step 5.5
+                // Step 6.5
                 fallback_mode = Some(NetTraitsRequestMode::CorsMode);
-                // Step 5.6
+                // Step 6.6
                 fallback_credentials = Some(NetTraitsRequestCredentials::CredentialsSameOrigin);
             },
-            // Step 6
+            // Step 7
             RequestInfo::Request(ref input_request) => {
-                // Step 6.1
+                // This looks like Step 38
+                // TODO do this in the right place to not mask other errors
                 if request_is_disturbed(input_request) || request_is_locked(input_request) {
                     return Err(Error::Type("Input is disturbed or locked".to_string()));
                 }
-                // Step 6.2
+                // Step 7.1
                 temporary_request = input_request.request.borrow().clone();
+                // Step 7.2 TODO: "Set signal to input's signal."
             },
         }
 
-        // Step 7
+        // Step 8
         // TODO: `entry settings object` is not implemented yet.
         let origin = base_url.origin();
 
-        // Step 8
+        // Step 9
         let mut window = Window::Client;
 
-        // Step 9
+        // Step 10
         // TODO: `environment settings object` is not implemented in Servo yet.
 
-        // Step 10
+        // Step 11
         if !init.window.handle().is_null_or_undefined() {
             return Err(Error::Type("Window is present and is not null".to_string()));
         }
 
-        // Step 11
+        // Step 12
         if !init.window.handle().is_undefined() {
             window = Window::NoWindow;
         }
 
-        // Step 12
+        // Step 13
         let mut request: NetTraitsRequest;
         request = net_request_from_global(global, temporary_request.current_url());
         request.method = temporary_request.method;
@@ -159,7 +163,7 @@ impl Request {
         request.redirect_mode = temporary_request.redirect_mode;
         request.integrity_metadata = temporary_request.integrity_metadata;
 
-        // Step 13
+        // Step 14
         if init.body.is_some() ||
             init.cache.is_some() ||
             init.credentials.is_some() ||
@@ -172,31 +176,33 @@ impl Request {
             init.referrerPolicy.is_some() ||
             !init.window.handle().is_undefined()
         {
-            // Step 13.1
+            // Step 14.1
             if request.mode == NetTraitsRequestMode::Navigate {
                 request.mode = NetTraitsRequestMode::SameOrigin;
             }
-            // Step 13.2
+            // Step 14.2 TODO: "Unset request's reload-navigation flag."
+            // Step 14.3 TODO: "Unset request's history-navigation flag."
+            // Step 14.4
             request.referrer = NetTraitsRequestReferrer::Client;
-            // Step 13.3
+            // Step 14.5
             request.referrer_policy = None;
         }
 
-        // Step 14
+        // Step 15
         if let Some(init_referrer) = init.referrer.as_ref() {
-            // Step 14.1
+            // Step 15.1
             let ref referrer = init_referrer.0;
-            // Step 14.2
+            // Step 15.2
             if referrer.is_empty() {
                 request.referrer = NetTraitsRequestReferrer::NoReferrer;
             } else {
-                // Step 14.3
+                // Step 15.3.1
                 let parsed_referrer = base_url.join(referrer);
-                // Step 14.4
+                // Step 15.3.2
                 if parsed_referrer.is_err() {
                     return Err(Error::Type("Failed to parse referrer url".to_string()));
                 }
-                // Step 14.5
+                // Step 15.3.3
                 if let Ok(parsed_referrer) = parsed_referrer {
                     if (parsed_referrer.cannot_be_a_base() &&
                         parsed_referrer.scheme() == "about" &&
@@ -205,55 +211,55 @@ impl Request {
                     {
                         request.referrer = NetTraitsRequestReferrer::Client;
                     } else {
-                        // Step 14.6
+                        // Step 15.3.4
                         request.referrer = NetTraitsRequestReferrer::ReferrerUrl(parsed_referrer);
                     }
                 }
             }
         }
 
-        // Step 15
+        // Step 16
         if let Some(init_referrerpolicy) = init.referrerPolicy.as_ref() {
             let init_referrer_policy = init_referrerpolicy.clone().into();
             request.referrer_policy = Some(init_referrer_policy);
         }
 
-        // Step 16
+        // Step 17
         let mode = init
             .mode
             .as_ref()
             .map(|m| m.clone().into())
             .or(fallback_mode);
 
-        // Step 17
+        // Step 18
         if let Some(NetTraitsRequestMode::Navigate) = mode {
             return Err(Error::Type("Request mode is Navigate".to_string()));
         }
 
-        // Step 18
+        // Step 19
         if let Some(m) = mode {
             request.mode = m;
         }
 
-        // Step 19
+        // Step 20
         let credentials = init
             .credentials
             .as_ref()
             .map(|m| m.clone().into())
             .or(fallback_credentials);
 
-        // Step 20
+        // Step 21
         if let Some(c) = credentials {
             request.credentials_mode = c;
         }
 
-        // Step 21
+        // Step 22
         if let Some(init_cache) = init.cache.as_ref() {
             let cache = init_cache.clone().into();
             request.cache_mode = cache;
         }
 
-        // Step 22
+        // Step 23
         if request.cache_mode == NetTraitsRequestCache::OnlyIfCached {
             if request.mode != NetTraitsRequestMode::SameOrigin {
                 return Err(Error::Type(
@@ -262,45 +268,55 @@ impl Request {
             }
         }
 
-        // Step 23
+        // Step 24
         if let Some(init_redirect) = init.redirect.as_ref() {
             let redirect = init_redirect.clone().into();
             request.redirect_mode = redirect;
         }
 
-        // Step 24
+        // Step 25
         if let Some(init_integrity) = init.integrity.as_ref() {
             let integrity = init_integrity.clone().to_string();
             request.integrity_metadata = integrity;
         }
 
-        // Step 25
+        // Step 26 TODO: "If init["keepalive"] exists..."
+
+        // Step 27.1
         if let Some(init_method) = init.method.as_ref() {
-            // Step 25.1
+            // Step 27.2
             if !is_method(&init_method) {
                 return Err(Error::Type("Method is not a method".to_string()));
             }
             if is_forbidden_method(&init_method) {
                 return Err(Error::Type("Method is forbidden".to_string()));
             }
-            // Step 25.2
+            // Step 27.3
             let method = match init_method.as_str() {
                 Some(s) => normalize_method(s)
                     .map_err(|e| Error::Type(format!("Method is not valid: {:?}", e)))?,
                 None => return Err(Error::Type("Method is not a valid UTF8".to_string())),
             };
-            // Step 25.3
+            // Step 27.4
             request.method = method;
         }
 
-        // Step 26
+        // Step 28 TODO: "If init["signal"] exists..."
+
+        // Step 29
         let r = Request::from_net_request(global, request);
+
+        // Step 30 TODO: "If signal is not null..."
+
+        // Step 31
+        // "or_init" looks unclear here
         r.headers.or_init(|| Headers::for_request(&r.global()));
 
-        // Step 27
+        // Step 32 - but spec says this should only be when non-empty init?
+        // Step 32.1
         let mut headers_copy = r.Headers();
 
-        // Step 28
+        // Step 32.2
         if let Some(possible_header) = init.headers.as_ref() {
             match possible_header {
                 &HeadersInit::Headers(ref init_headers) => {
@@ -319,7 +335,7 @@ impl Request {
             }
         }
 
-        // Step 29
+        // Step 32.3
         // We cannot empty `r.Headers().header_list` because
         // we would undo the Step 27 above.  One alternative is to set
         // `headers_copy` as a deep copy of `r.Headers()`. However,
@@ -328,21 +344,21 @@ impl Request {
         // mutable reference, we cannot mutate `r.Headers()` to be the
         // deep copied headers in Step 27.
 
-        // Step 30
+        // Step 32.4
         if r.request.borrow().mode == NetTraitsRequestMode::NoCors {
             let borrowed_request = r.request.borrow();
-            // Step 30.1
+            // Step 32.4.1
             if !is_cors_safelisted_method(&borrowed_request.method) {
                 return Err(Error::Type(
                     "The mode is 'no-cors' but the method is not a cors-safelisted method"
                         .to_string(),
                 ));
             }
-            // Step 30.2
+            // Step 32.4.2
             r.Headers().set_guard(Guard::RequestNoCors);
         }
 
-        // Step 31
+        // Step 32.5
         match init.headers {
             None => {
                 // This is equivalent to the specification's concept of
@@ -360,10 +376,11 @@ impl Request {
             _ => {},
         }
 
+        // Step 32.5-6 depending on how we got here
         // Copy the headers list onto the headers of net_traits::Request
         r.request.borrow_mut().headers = r.Headers().get_headers_list();
 
-        // Step 32
+        // Step 33
         let mut input_body = if let RequestInfo::Request(ref input_request) = input {
             let input_request_request = input_request.request.borrow();
             input_request_request.body.clone()
@@ -371,7 +388,7 @@ impl Request {
             None
         };
 
-        // Step 33
+        // Step 34
         if let Some(init_body_option) = init.body.as_ref() {
             if init_body_option.is_some() || input_body.is_some() {
                 let req = r.request.borrow();
@@ -392,14 +409,16 @@ impl Request {
             }
         }
 
-        // Step 34
+        // Step 35-36
         if let Some(Some(ref init_body)) = init.body {
-            // Step 34.2
+            // Step 36.2 TODO "If init["keepalive"] exists and is true..."
+
+            // Step 36.3
             let extracted_body_tmp = init_body.extract();
             input_body = Some(extracted_body_tmp.0);
             let content_type = extracted_body_tmp.1;
 
-            // Step 34.3
+            // Step 36.4
             if let Some(contents) = content_type {
                 if !r
                     .Headers()
@@ -414,17 +433,23 @@ impl Request {
             }
         }
 
-        // Step 35
+        // Step 37 "TODO if body is non-null and body's source is null..."
+        // This looks like where we need to set the use-preflight flag
+        // if the request has a body and nothing else has set the flag.
+
+        // Step 38 is done earlier
+
+        // Step 39
+        // TODO: `ReadableStream` object is not implemented in Servo yet.
+
+        // Step 40
         r.request.borrow_mut().body = input_body;
 
-        // Step 36
+        // Step 41
         let extracted_mime_type = r.Headers().extract_mime_type();
         *r.mime_type.borrow_mut() = extracted_mime_type;
 
-        // Step 37
+        // Step 42
-        // TODO: `ReadableStream` object is not implemented in Servo yet.
-
-        // Step 38
         Ok(r)
     }
 

tests/wpt/metadata/fetch/api/headers/headers-no-cors.window.js.ini

@@ -1,25 +1,10 @@
 [headers-no-cors.window.html]
-  ["no-cors" Headers object cannot have accept-language/@ as header]
-    expected: FAIL
-
   ["no-cors" Headers object cannot have accept-language/\x01 as header]
     expected: FAIL
 
-  ["no-cors" Headers object cannot have content-type/text/plain; long=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901 as header]
-    expected: FAIL
-
-  ["no-cors" Headers object cannot have content-language/@ as header]
-    expected: FAIL
-
   ["no-cors" Headers object cannot have content-language/\x01 as header]
     expected: FAIL
 
-  ["no-cors" Headers object cannot have accept/" as header]
-    expected: FAIL
-
-  ["no-cors" Headers object cannot have accept/012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 as header]
-    expected: FAIL
-
   ["no-cors" Headers object cannot have content-type set to text/plain;ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss, text/plain]
     expected: FAIL