From 759099c78dcd92fc1916d6fdc3d0a8305cadb361 Mon Sep 17 00:00:00 2001 From: Bob Date: Sat, 27 Feb 2016 15:05:23 +0000 Subject: [PATCH 1/4] correctly send secure cookies after hsts url match Fix for #8100, where sites in the hsts list were not recieving secure cookies if the site was originally loading using a plain http url. --- components/net/http_loader.rs | 5 ++-- tests/unit/net/http_loader.rs | 48 +++++++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index d1e0bef1068..3d231423f93 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -513,6 +513,7 @@ fn request_must_be_secured(url: &Url, hsts_list: &Arc>) -> bool } pub fn modify_request_headers(headers: &mut Headers, + url: &Url, doc_url: &Url, user_agent: &str, cookie_jar: &Arc>, @@ -529,7 +530,7 @@ 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::>() { @@ -725,7 +726,7 @@ pub fn load(load_data: LoadData, let request_id = uuid::Uuid::new_v4().to_simple_string(); - modify_request_headers(&mut request_headers, &doc_url, &user_agent, &cookie_jar, &load_data); + modify_request_headers(&mut request_headers, &url, &doc_url, &user_agent, &cookie_jar, &load_data); let response = try!(obtain_response(request_factory, &url, &method, &request_headers, &cancel_listener, &load_data.data, &load_data.method, diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 70d4944b7ae..c144e4bd3a8 100644 --- a/tests/unit/net/http_loader.rs +++ b/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::NonHTTP + ).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::( + 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"); From 06ffdd68e85854790f48f0c59f638b415c8dd695 Mon Sep 17 00:00:00 2001 From: Bob Date: Tue, 1 Mar 2016 18:10:58 +0000 Subject: [PATCH 2/4] refactor http_loader hostname/htst order Changed hostname rewrite to happen inside obtain response after any htst changes. Removed url from load leaving just doc_url to avoid confusion --- components/net/http_loader.rs | 60 ++++++++++++++++------------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 3d231423f93..5b66c877579 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -514,14 +514,13 @@ fn request_must_be_secured(url: &Url, hsts_list: &Arc>) -> bool pub fn modify_request_headers(headers: &mut Headers, url: &Url, - doc_url: &Url, user_agent: &str, cookie_jar: &Arc>, load_data: &LoadData) { // Ensure that the host header is set from the original url let host = Host { - hostname: doc_url.serialize_host().unwrap(), - port: doc_url.port_or_default() + hostname: url.serialize_host().unwrap(), + port: url.port_or_default() }; headers.set(host); headers.set(UserAgent(user_agent.to_owned())); @@ -534,7 +533,7 @@ pub fn modify_request_headers(headers: &mut Headers, // https://fetch.spec.whatwg.org/#http-network-or-cache-fetch step 12 if !headers.has::>() { - if let Some(auth) = auth_from_url(doc_url) { + if let Some(auth) = auth_from_url(url) { headers.set(auth); } } @@ -555,7 +554,6 @@ fn auth_from_url(doc_url: &Url) -> Option> { pub fn process_response_headers(response: &HttpResponse, url: &Url, - doc_url: &Url, cookie_jar: &Arc>, hsts_list: &Arc>, load_data: &LoadData) { @@ -568,7 +566,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); } @@ -587,17 +585,18 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, -> Result 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) { @@ -633,7 +632,7 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, if let Some(pipeline_id) = *pipeline_id { send_request_to_devtools( devtools_chan.clone(), request_id.clone().into(), - url.clone(), method.clone(), request_headers.clone(), + connection_url.clone(), method.clone(), request_headers.clone(), cloned_data, pipeline_id, time::now() ); } @@ -669,48 +668,44 @@ pub fn load(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 = url.clone(); + doc_url = inner_url(&load_data.url); } // Loop to handle redirects. loop { iters = iters + 1; - if &*url.scheme == "http" && request_must_be_secured(&url, &hsts_list) { - info!("{} is in the strict transport security list, requesting secure host", url); - url = secure_url(&url); + if &*doc_url.scheme == "http" && request_must_be_secured(&doc_url, &hsts_list) { + info!("{} is in the strict transport security list, requesting secure host", doc_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" { - return Err(LoadError::UnsupportedScheme(url)); + if &*doc_url.scheme != "http" && &*doc_url.scheme != "https" { + 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 @@ -726,13 +721,13 @@ pub fn load(load_data: LoadData, let request_id = uuid::Uuid::new_v4().to_simple_string(); - modify_request_headers(&mut request_headers, &url, &doc_url, &user_agent, &cookie_jar, &load_data); + 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 { @@ -742,7 +737,7 @@ pub fn load(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, @@ -757,10 +752,6 @@ pub fn load(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 && @@ -769,10 +760,13 @@ pub fn load(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; } From 17e600768583cd4991452f5609dd120658aa641b Mon Sep 17 00:00:00 2001 From: Bob Date: Thu, 3 Mar 2016 17:15:29 +0000 Subject: [PATCH 3/4] send correct url to devtool on request Send url that was not modified by the hosts file to the dev tools --- components/net/http_loader.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 5b66c877579..c4c546000c6 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -632,7 +632,7 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, if let Some(pipeline_id) = *pipeline_id { send_request_to_devtools( devtools_chan.clone(), request_id.clone().into(), - connection_url.clone(), method.clone(), request_headers.clone(), + url.clone(), method.clone(), request_headers.clone(), cloned_data, pipeline_id, time::now() ); } From 248e84e9281f464b438ef36b9c36de467dfb13a8 Mon Sep 17 00:00:00 2001 From: Bob Date: Thu, 3 Mar 2016 17:16:07 +0000 Subject: [PATCH 4/4] create cookie as HTTP instead of NonHttp in test Cleanup for readability and correctness of unit test in http_loader --- tests/unit/net/http_loader.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index c144e4bd3a8..3f0aa134862 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -838,7 +838,7 @@ fn test_load_sends_secure_cookie_if_http_changed_to_https_due_to_entry_in_hsts_s let cookie = Cookie::new_wrapped( cookie_pair, &cookie_url, - CookieSource::NonHTTP + CookieSource::HTTP ).unwrap(); cookie_jar.push(cookie, CookieSource::HTTP); }