Auto merge of #25236 - pshaughn:safelistct, r=jdm

De-deplicate is_cors_safelisted_request_header helper functions

<!-- Please describe your changes on the following line: -->
Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25235 and some but not all subcases in #25175

<!-- Either: -->
- [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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