Auto merge of #9780 - bobthekingofegypt:github_hsts_bug, r=jdm

correctly send secure cookies after hsts url match

Fixes #8100, where sites in the hsts list were not recieving secure
cookies if the site was originally loading using a plain http url.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9780)
<!-- Reviewable:end -->

components/net/http_loader.rs

@@ -513,14 +513,14 @@ fn request_must_be_secured(url: &Url, hsts_list: &Arc<RwLock<HSTSList>>) -> bool
 }
 
 pub fn modify_request_headers(headers: &mut Headers,
-                              doc_url: &Url,
+                              url: &Url,
                               user_agent: &str,
                               cookie_jar: &Arc<RwLock<CookieStorage>>,
                               load_data: &LoadData) {
     // Ensure that the host header is set from the original url
     let host = Host {
-        hostname: doc_url.serialize_host().unwrap(),
+        hostname: url.serialize_host().unwrap(),
-        port: doc_url.port_or_default()
+        port: url.port_or_default()
     };
     headers.set(host);
 
@@ -538,11 +538,11 @@ pub fn modify_request_headers(headers: &mut Headers,
     set_default_accept_encoding(headers);
     // https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch step 11
     if load_data.credentials_flag {
-        set_request_cookies(doc_url.clone(), headers, cookie_jar);
+        set_request_cookies(url.clone(), headers, cookie_jar);
 
         // https://fetch.spec.whatwg.org/#http-network-or-cache-fetch step 12
         if !headers.has::<Authorization<Basic>>() {
-            if let Some(auth) = auth_from_url(doc_url) {
+            if let Some(auth) = auth_from_url(url) {
                 headers.set(auth);
             }
         }
@@ -563,7 +563,6 @@ fn auth_from_url(doc_url: &Url) -> Option<Authorization<Basic>> {
 
 pub fn process_response_headers(response: &HttpResponse,
                                 url: &Url,
-                                doc_url: &Url,
                                 cookie_jar: &Arc<RwLock<CookieStorage>>,
                                 hsts_list: &Arc<RwLock<HSTSList>>,
                                 load_data: &LoadData) {
@@ -576,7 +575,7 @@ pub fn process_response_headers(response: &HttpResponse,
 
     // https://fetch.spec.whatwg.org/#concept-http-network-fetch step 9
     if load_data.credentials_flag {
-        set_cookies_from_response(doc_url.clone(), response, cookie_jar);
+        set_cookies_from_response(url.clone(), response, cookie_jar);
     }
     update_sts_list_from_response(url, response, hsts_list);
 }
@@ -595,17 +594,18 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
                           -> Result<A::R, LoadError> where A: HttpRequest + 'static  {
 
     let response;
+    let connection_url = replace_hosts(&url);
 
     // loop trying connections in connection pool
     // they may have grown stale (disconnected), in which case we'll get
     // a ConnectionAborted error. this loop tries again with a new
     // connection.
     loop {
-        let mut req = try!(request_factory.create(url.clone(), method.clone()));
+        let mut req = try!(request_factory.create(connection_url.clone(), method.clone()));
         *req.headers_mut() = request_headers.clone();
 
         if cancel_listener.is_cancelled() {
-            return Err(LoadError::Cancelled(url.clone(), "load cancelled".to_owned()));
+            return Err(LoadError::Cancelled(connection_url.clone(), "load cancelled".to_owned()));
         }
 
         if log_enabled!(log::LogLevel::Info) {
@@ -677,48 +677,44 @@ pub fn load<A>(load_data: LoadData,
     let mut iters = 0;
     // URL of the document being loaded, as seen by all the higher-level code.
     let mut doc_url = load_data.url.clone();
-    // URL that we actually fetch from the network, after applying the replacements
-    // specified in the hosts file.
-    let mut url = replace_hosts(&load_data.url);
     let mut redirected_to = HashSet::new();
     let mut method = load_data.method.clone();
 
     if cancel_listener.is_cancelled() {
-        return Err(LoadError::Cancelled(url, "load cancelled".to_owned()));
+        return Err(LoadError::Cancelled(doc_url, "load cancelled".to_owned()));
     }
 
     // If the URL is a view-source scheme then the scheme data contains the
     // real URL that should be used for which the source is to be viewed.
     // Change our existing URL to that and keep note that we are viewing
     // the source rather than rendering the contents of the URL.
-    let viewing_source = url.scheme == "view-source";
+    let viewing_source = doc_url.scheme == "view-source";
     if viewing_source {
-        url = inner_url(&load_data.url);
+        doc_url = inner_url(&load_data.url);
-        doc_url = url.clone();
     }
 
     // Loop to handle redirects.
     loop {
         iters = iters + 1;
 
-        if &*url.scheme == "http" && request_must_be_secured(&url, &hsts_list) {
+        if &*doc_url.scheme == "http" && request_must_be_secured(&doc_url, &hsts_list) {
-            info!("{} is in the strict transport security list, requesting secure host", url);
+            info!("{} is in the strict transport security list, requesting secure host", doc_url);
-            url = secure_url(&url);
+            doc_url = secure_url(&doc_url);
         }
 
         if iters > max_redirects {
-            return Err(LoadError::MaxRedirects(url));
+            return Err(LoadError::MaxRedirects(doc_url));
         }
 
-        if &*url.scheme != "http" && &*url.scheme != "https" {
+        if &*doc_url.scheme != "http" && &*doc_url.scheme != "https" {
-            return Err(LoadError::UnsupportedScheme(url));
+            return Err(LoadError::UnsupportedScheme(doc_url));
         }
 
         if cancel_listener.is_cancelled() {
-            return Err(LoadError::Cancelled(url, "load cancelled".to_owned()));
+            return Err(LoadError::Cancelled(doc_url, "load cancelled".to_owned()));
         }
 
-        info!("requesting {}", url.serialize());
+        info!("requesting {}", doc_url.serialize());
 
         // Avoid automatically preserving request headers when redirects occur.
         // See https://bugzilla.mozilla.org/show_bug.cgi?id=401564 and
@@ -736,11 +732,11 @@ pub fn load<A>(load_data: LoadData,
 
         modify_request_headers(&mut request_headers, &doc_url, &user_agent, &cookie_jar, &load_data);
 
-        let response = try!(obtain_response(request_factory, &url, &method, &request_headers,
+        let response = try!(obtain_response(request_factory, &doc_url, &method, &request_headers,
                                             &cancel_listener, &load_data.data, &load_data.method,
                                             &load_data.pipeline_id, iters, &devtools_chan, &request_id));
 
-        process_response_headers(&response, &url, &doc_url, &cookie_jar, &hsts_list, &load_data);
+        process_response_headers(&response, &doc_url, &cookie_jar, &hsts_list, &load_data);
 
         // --- Loop if there's a redirect
         if response.status().class() == StatusClass::Redirection {
@@ -750,7 +746,7 @@ pub fn load<A>(load_data: LoadData,
                     if c.preflight {
                         return Err(
                             LoadError::Cors(
-                                url,
+                                doc_url,
                                 "Preflight fetch inconsistent with main fetch".to_owned()));
                     } else {
                         // XXXManishearth There are some CORS-related steps here,
@@ -765,10 +761,6 @@ pub fn load<A>(load_data: LoadData,
                     }
                 };
 
-                info!("redirecting to {}", new_doc_url);
-                url = replace_hosts(&new_doc_url);
-                doc_url = new_doc_url;
-
                 // According to https://tools.ietf.org/html/rfc7231#section-6.4.2,
                 // historically UAs have rewritten POST->GET on 301 and 302 responses.
                 if method == Method::Post &&
@@ -777,10 +769,13 @@ pub fn load<A>(load_data: LoadData,
                     method = Method::Get;
                 }
 
-                if redirected_to.contains(&url) {
+                if redirected_to.contains(&new_doc_url) {
                     return Err(LoadError::InvalidRedirect(doc_url, "redirect loop".to_owned()));
                 }
 
+                info!("redirecting to {}", new_doc_url);
+                doc_url = new_doc_url;
+
                 redirected_to.insert(doc_url.clone());
                 continue;
             }

tests/unit/net/http_loader.rs

@@ -18,10 +18,10 @@ use hyper::status::StatusCode;
 use msg::constellation_msg::PipelineId;
 use net::cookie::Cookie;
 use net::cookie_storage::CookieStorage;
-use net::hsts::{HSTSList};
+use net::hsts::{HSTSList, HSTSEntry};
 use net::http_loader::{load, LoadError, HttpRequestFactory, HttpRequest, HttpResponse};
 use net::resource_thread::CancellationListener;
-use net_traits::{LoadData, CookieSource, LoadContext};
+use net_traits::{LoadData, CookieSource, LoadContext, IncludeSubdomains};
 use std::borrow::Cow;
 use std::io::{self, Write, Read, Cursor};
 use std::sync::mpsc::Receiver;
@@ -813,6 +813,50 @@ fn test_load_sets_requests_cookies_header_for_url_by_getting_cookies_from_the_re
                                                     &CancellationListener::new(None));
 }
 
+#[test]
+fn test_load_sends_secure_cookie_if_http_changed_to_https_due_to_entry_in_hsts_store() {
+    let url = url!("http://mozilla.com");
+    let secured_url = url!("https://mozilla.com");
+
+    let hsts_list = Arc::new(RwLock::new(HSTSList::new()));
+    let cookie_jar = Arc::new(RwLock::new(CookieStorage::new()));
+
+    {
+        let mut hsts_list = hsts_list.write().unwrap();
+        let entry = HSTSEntry::new(
+            "mozilla.com".to_owned(), IncludeSubdomains::Included, Some(1000000)
+        ).unwrap();
+        hsts_list.push(entry);
+    }
+
+    {
+        let mut cookie_jar = cookie_jar.write().unwrap();
+        let cookie_url = secured_url.clone();
+        let mut cookie_pair = CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned());
+        cookie_pair.secure = true;
+
+        let cookie = Cookie::new_wrapped(
+            cookie_pair,
+            &cookie_url,
+            CookieSource::HTTP
+        ).unwrap();
+        cookie_jar.push(cookie, CookieSource::HTTP);
+    }
+
+    let mut load_data = LoadData::new(LoadContext::Browsing, url, None);
+    load_data.data = Some(<[_]>::to_vec("Yay!".as_bytes()));
+
+    let mut headers = Headers::new();
+    headers.set_raw("Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIs=theBest".as_bytes())]);
+
+    let _ = load::<AssertRequestMustIncludeHeaders>(
+        load_data.clone(), hsts_list, cookie_jar, None,
+        &AssertMustIncludeHeadersRequestFactory {
+            expected_headers: headers,
+            body: <[_]>::to_vec(&*load_data.data.unwrap())
+        }, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None));
+}
+
 #[test]
 fn test_load_sends_cookie_if_nonhttp() {
     let url = url!("http://mozilla.com");