From c093ce8a5a73ea405745c8eb1c0b3554175cf645 Mon Sep 17 00:00:00 2001 From: Sam Gibson Date: Sat, 15 Aug 2015 17:22:45 +1000 Subject: [PATCH] Only use the resource manager's HSTS list. Simplifies a bunch of stuff. --- components/net/http_loader.rs | 34 ++++++++--------- components/net/resource_task.rs | 48 ++++++++++++------------ components/net_traits/lib.rs | 1 + tests/unit/net/http_loader.rs | 66 ++++++++++++++++++++------------- 4 files changed, 81 insertions(+), 68 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 8bd03e86e54..d79f90e1d06 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -9,7 +9,7 @@ use net_traits::ProgressMsg::{Payload, Done}; use net_traits::hosts::replace_hosts; use net_traits::{ControlMsg, CookieSource, LoadData, Metadata, LoadConsumer, IncludeSubdomains}; use resource_task::{start_sending_opt, start_sending_sniffed_opt}; -use hsts::{HSTSList, secure_url}; +use hsts::secure_url; use file_loader; use file_loader; @@ -37,7 +37,6 @@ use std::collections::HashSet; use std::error::Error; use std::io::{self, Read, Write}; use std::sync::Arc; -use std::sync::Mutex; use std::sync::mpsc::{Sender, channel}; use util::task::spawn_named; use util::resource_files::resources_dir_path; @@ -52,12 +51,11 @@ use uuid; use std::fs::File; pub fn factory(resource_mgr_chan: IpcSender, - devtools_chan: Option>, - hsts_list: Arc>) + devtools_chan: Option>) -> Box) + Send> { box move |load_data, senders, classifier| { spawn_named(format!("http_loader for {}", load_data.url.serialize()), - move || load_for_consumer(load_data, senders, classifier, resource_mgr_chan, devtools_chan, hsts_list)) + move || load_for_consumer(load_data, senders, classifier, resource_mgr_chan, devtools_chan)) } } @@ -89,15 +87,6 @@ fn read_block(reader: &mut R) -> Result { } } -fn request_must_be_secured(hsts_list: &HSTSList, url: &Url) -> bool { - match url.domain() { - Some(ref h) => { - hsts_list.is_host_secure(h) - }, - _ => false - } -} - fn inner_url(url: &Url) -> Url { let inner_url = url.non_relative_scheme_data().unwrap(); Url::parse(inner_url).unwrap() @@ -107,10 +96,9 @@ fn load_for_consumer(load_data: LoadData, start_chan: LoadConsumer, classifier: Arc, resource_mgr_chan: IpcSender, - devtools_chan: Option>, - hsts_list: Arc>) { + devtools_chan: Option>) { - match load::(load_data, resource_mgr_chan, devtools_chan, hsts_list, &NetworkHttpRequestFactory) { + match load::(load_data, resource_mgr_chan, devtools_chan, &NetworkHttpRequestFactory) { Err(LoadError::UnsupportedScheme(url)) => { let s = format!("{} request, but we don't support that scheme", &*url.scheme); send_error(url, s, start_chan) @@ -320,10 +308,18 @@ fn set_cookies_from_response(url: Url, response: &HttpResponse, resource_mgr_cha } } +fn request_must_be_secured(url: &Url, resource_mgr_chan: &IpcSender) -> bool { + let (tx, rx) = ipc::channel().unwrap(); + resource_mgr_chan.send( + ControlMsg::GetHostMustBeSecured(url.domain().unwrap().to_string(), tx) + ).unwrap(); + + rx.recv().unwrap() +} + pub fn load(mut load_data: LoadData, resource_mgr_chan: IpcSender, devtools_chan: Option>, - hsts_list: Arc>, request_factory: &HttpRequestFactory) -> Result<(Box, Metadata), LoadError> where A: HttpRequest + 'static { // FIXME: At the time of writing this FIXME, servo didn't have any central @@ -352,7 +348,7 @@ pub fn load(mut load_data: LoadData, loop { iters = iters + 1; - if &*url.scheme != "https" && request_must_be_secured(&hsts_list.lock().unwrap(), &url) { + if &*url.scheme != "https" && request_must_be_secured(&url, &resource_mgr_chan) { info!("{} is in the strict transport security list, requesting secure host", url); url = secure_url(&url); } diff --git a/components/net/resource_task.rs b/components/net/resource_task.rs index 62566f71c70..d232db51bff 100644 --- a/components/net/resource_task.rs +++ b/components/net/resource_task.rs @@ -29,7 +29,6 @@ use ipc_channel::ipc::{self, IpcReceiver, IpcSender}; use std::borrow::ToOwned; use std::boxed::FnBox; use std::sync::Arc; -use std::sync::Mutex; use std::sync::mpsc::{channel, Sender}; pub enum ProgressSender { @@ -161,23 +160,26 @@ impl ResourceChannelManager { fn start(&mut self) { loop { match self.from_client.recv().unwrap() { - ControlMsg::Load(load_data, consumer) => { - self.resource_manager.load(load_data, consumer) - } - ControlMsg::SetCookiesForUrl(request, cookie_list, source) => { - self.resource_manager.set_cookies_for_url(request, cookie_list, source) - } - ControlMsg::GetCookiesForUrl(url, consumer, source) => { - consumer.send(self.resource_manager.cookie_storage.cookies_for_url(&url, source)).unwrap(); - } - ControlMsg::SetHSTSEntryForHost(host, include_subdomains, max_age) => { - if let Some(entry) = HSTSEntry::new(host, include_subdomains, Some(max_age)) { - self.resource_manager.add_hsts_entry(entry) - } - } - ControlMsg::Exit => { - break - } + ControlMsg::Load(load_data, consumer) => { + self.resource_manager.load(load_data, consumer) + } + ControlMsg::SetCookiesForUrl(request, cookie_list, source) => { + self.resource_manager.set_cookies_for_url(request, cookie_list, source) + } + ControlMsg::GetCookiesForUrl(url, consumer, source) => { + consumer.send(self.resource_manager.cookie_storage.cookies_for_url(&url, source)).unwrap(); + } + ControlMsg::SetHSTSEntryForHost(host, include_subdomains, max_age) => { + if let Some(entry) = HSTSEntry::new(host, include_subdomains, Some(max_age)) { + self.resource_manager.add_hsts_entry(entry) + } + } + ControlMsg::GetHostMustBeSecured(host, consumer) => { + consumer.send(self.resource_manager.is_host_sts(&*host)).unwrap(); + } + ControlMsg::Exit => { + break + } } } } @@ -189,7 +191,7 @@ pub struct ResourceManager { resource_task: IpcSender, mime_classifier: Arc, devtools_chan: Option>, - hsts_list: Arc> + hsts_list: HSTSList } impl ResourceManager { @@ -203,7 +205,7 @@ impl ResourceManager { resource_task: resource_task, mime_classifier: Arc::new(MIMEClassifier::new()), devtools_chan: devtools_channel, - hsts_list: Arc::new(Mutex::new(hsts_list)) + hsts_list: hsts_list } } } @@ -221,11 +223,11 @@ impl ResourceManager { } pub fn add_hsts_entry(&mut self, entry: HSTSEntry) { - self.hsts_list.lock().unwrap().push(entry); + self.hsts_list.push(entry); } pub fn is_host_sts(&self, host: &str) -> bool { - self.hsts_list.lock().unwrap().is_host_secure(host) + self.hsts_list.is_host_secure(host) } fn load(&mut self, mut load_data: LoadData, consumer: LoadConsumer) { @@ -241,7 +243,7 @@ impl ResourceManager { let loader = match &*load_data.url.scheme { "file" => from_factory(file_loader::factory), "http" | "https" | "view-source" => - http_loader::factory(self.resource_task.clone(), self.devtools_chan.clone(), self.hsts_list.clone()), + http_loader::factory(self.resource_task.clone(), self.devtools_chan.clone()), "data" => from_factory(data_loader::factory), "about" => from_factory(about_loader::factory), _ => { diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index e4a93f56673..3f9db2a7ebc 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -161,6 +161,7 @@ pub enum ControlMsg { GetCookiesForUrl(Url, IpcSender>, CookieSource), /// Store a domain's STS information SetHSTSEntryForHost(String, IncludeSubdomains, u64), + GetHostMustBeSecured(String, IpcSender), Exit } diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 25662619cb8..a787f46e3f5 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -6,10 +6,8 @@ use net::http_loader::{load, LoadError, HttpRequestFactory, HttpRequest, HttpRes use net::resource_task::new_resource_task; use net_traits::{ResourceTask, ControlMsg, CookieSource}; use url::Url; -use std::sync::{Arc, Mutex}; use ipc_channel::ipc; use net_traits::LoadData; -use net::hsts::HSTSList; use hyper::method::Method; use hyper::http::RawStatus; use hyper::status::StatusCode; @@ -190,6 +188,34 @@ fn assert_cookie_for_domain(resource_mgr: &ResourceTask, domain: &str, cookie: & } } +#[test] +fn test_load_adds_host_to_sts_list_when_url_is_https_and_sts_headers_are_present() { + struct Factory; + + impl HttpRequestFactory for Factory { + type R=MockRequest; + + fn create(&self, _: Url, _: Method) -> Result { + let content = <[_]>::to_vec("Yay!".as_bytes()); + let mut headers = Headers::new(); + headers.set_raw("Strict-Transport-Security", vec![b"max-age=31536000".to_vec()]); + Ok(MockRequest::new(RequestType::WithHeaders(content, headers))) + } + } + + let url = Url::parse("https://mozilla.com").unwrap(); + let resource_mgr = new_resource_task(None, None); + + let load_data = LoadData::new(url.clone(), None); + + let _ = load::(load_data, resource_mgr.clone(), None, &Factory); + + let (tx, rx) = ipc::channel().unwrap(); + resource_mgr.send(ControlMsg::GetHostMustBeSecured("mozilla.com".to_string(), tx)).unwrap(); + + assert!(rx.recv().unwrap()); +} + #[test] fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_in_response() { struct Factory; @@ -210,9 +236,8 @@ fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_ assert_cookie_for_domain(&resource_mgr, "http://mozilla.com", ""); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - let _ = load::(load_data, resource_mgr.clone(), None, hsts_list, &Factory); + let _ = load::(load_data, resource_mgr.clone(), None, &Factory); assert_cookie_for_domain(&resource_mgr, "http://mozilla.com", "mozillaIs=theBest"); } @@ -231,9 +256,7 @@ fn test_load_sets_requests_cookies_header_for_url_by_getting_cookies_from_the_re let mut cookie = Headers::new(); cookie.set_raw("Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIs=theBest".as_bytes())]); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - - let _ = load::(load_data.clone(), resource_mgr, None, hsts_list, &AssertMustHaveHeadersRequestFactory { + let _ = load::(load_data.clone(), resource_mgr, None, &AssertMustHaveHeadersRequestFactory { expected_headers: cookie, body: <[_]>::to_vec(&*load_data.data.unwrap()) }); @@ -251,9 +274,7 @@ fn test_load_sets_content_length_to_length_of_request_body() { let mut content_len_headers= Headers::new(); content_len_headers.set_raw("Content-Length".to_owned(), vec![<[_]>::to_vec(&*format!("{}", content.len()).as_bytes())]); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - - let _ = load::(load_data.clone(), resource_mgr, None, hsts_list, &AssertMustHaveHeadersRequestFactory { + let _ = load::(load_data.clone(), resource_mgr, None, &AssertMustHaveHeadersRequestFactory { expected_headers: content_len_headers, body: <[_]>::to_vec(&*load_data.data.unwrap()) }); @@ -268,9 +289,8 @@ fn test_load_sets_default_accept_to_html_xhtml_xml_and_then_anything_else() { let resource_mgr = new_resource_task(None, None); let mut load_data = LoadData::new(url.clone(), None); load_data.data = Some(<[_]>::to_vec("Yay!".as_bytes())); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - let _ = load::(load_data, resource_mgr, None, hsts_list, &AssertMustHaveHeadersRequestFactory { + let _ = load::(load_data, resource_mgr, None, &AssertMustHaveHeadersRequestFactory { expected_headers: accept_headers, body: <[_]>::to_vec("Yay!".as_bytes()) }); @@ -285,9 +305,8 @@ fn test_load_sets_default_accept_encoding_to_gzip_and_deflate() { let resource_mgr = new_resource_task(None, None); let mut load_data = LoadData::new(url.clone(), None); load_data.data = Some(<[_]>::to_vec("Yay!".as_bytes())); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - let _ = load::(load_data, resource_mgr, None, hsts_list, &AssertMustHaveHeadersRequestFactory { + let _ = load::(load_data, resource_mgr, None, &AssertMustHaveHeadersRequestFactory { expected_headers: accept_encoding_headers, body: <[_]>::to_vec("Yay!".as_bytes()) }); @@ -314,9 +333,8 @@ fn test_load_errors_when_there_a_redirect_loop() { let url = Url::parse("http://mozilla.com").unwrap(); let resource_mgr = new_resource_task(None, None); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - match load::(load_data, resource_mgr, None, hsts_list, &Factory) { + match load::(load_data, resource_mgr, None, &Factory) { Err(LoadError::InvalidRedirect(_, msg)) => { assert_eq!(msg, "redirect loop"); }, @@ -343,9 +361,8 @@ fn test_load_errors_when_there_is_too_many_redirects() { let url = Url::parse("http://mozilla.com").unwrap(); let resource_mgr = new_resource_task(None, None); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - match load::(load_data, resource_mgr, None, hsts_list, &Factory) { + match load::(load_data, resource_mgr, None, &Factory) { Err(LoadError::MaxRedirects(url)) => { assert_eq!(url.domain().unwrap(), "mozilla.com") }, @@ -380,9 +397,8 @@ fn test_load_follows_a_redirect() { let url = Url::parse("http://mozilla.com").unwrap(); let resource_mgr = new_resource_task(None, None); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - match load::(load_data, resource_mgr, None, hsts_list, &Factory) { + match load::(load_data, resource_mgr, None, &Factory) { Err(_) => panic!("expected to follow a redirect"), Ok((mut r, _)) => { let response = read_response(&mut *r); @@ -404,11 +420,10 @@ impl HttpRequestFactory for DontConnectFactory { #[test] fn test_load_errors_when_scheme_is_not_http_or_https() { let url = Url::parse("ftp://not-supported").unwrap(); - let (cookies_chan, _) = ipc::channel().unwrap(); + let resource_mgr = new_resource_task(None, None); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - match load::(load_data, cookies_chan, None, hsts_list, &DontConnectFactory) { + match load::(load_data, resource_mgr, None, &DontConnectFactory) { Err(LoadError::UnsupportedScheme(_)) => {} _ => panic!("expected ftp scheme to be unsupported") } @@ -417,11 +432,10 @@ fn test_load_errors_when_scheme_is_not_http_or_https() { #[test] fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_https() { let url = Url::parse("view-source:ftp://not-supported").unwrap(); - let (cookies_chan, _) = ipc::channel().unwrap(); + let resource_mgr = new_resource_task(None, None); let load_data = LoadData::new(url.clone(), None); - let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() })); - match load::(load_data, cookies_chan, None, hsts_list, &DontConnectFactory) { + match load::(load_data, resource_mgr, None, &DontConnectFactory) { Err(LoadError::UnsupportedScheme(_)) => {} _ => panic!("expected ftp scheme to be unsupported") }