From 11f5be6d854634f5bbc8c4bb9d5a0d2fdd15cbb3 Mon Sep 17 00:00:00 2001 From: Sam Gibson Date: Sun, 19 Jul 2015 09:54:20 +1000 Subject: [PATCH] Responds to more code review feedback * Use regex from resource task * Don't have an option of an HSTS list, default to empty --- components/net/hsts.rs | 13 +++++++------ components/net/http_loader.rs | 14 +++++++------- components/net/resource_task.rs | 27 +++++++++++---------------- tests/unit/net/hsts.rs | 20 ++++++++++++++------ 4 files changed, 39 insertions(+), 35 deletions(-) diff --git a/components/net/hsts.rs b/components/net/hsts.rs index 74756a63afa..f7c7e995a17 100644 --- a/components/net/hsts.rs +++ b/components/net/hsts.rs @@ -2,20 +2,15 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use regex::Regex; use rustc_serialize::json::{decode}; use time; use url::Url; +use resource_task::{IPV4_REGEX, IPV6_REGEX}; use std::str::{from_utf8}; use util::resource_files::read_resource_file; -static IPV4_REGEX: Regex = regex!( - r"^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$" -); -static IPV6_REGEX: Regex = regex!(r"^([a-fA-F0-9]{0,4}[:]?){1,8}(/\d{1,3})?$"); - #[derive(RustcDecodable, RustcEncodable, Clone)] pub struct HSTSEntry { pub host: String, @@ -69,6 +64,12 @@ pub struct HSTSList { } impl HSTSList { + pub fn new() -> HSTSList { + HSTSList { + entries: vec![] + } + } + pub fn new_from_preload(preload_content: &str) -> Option { decode(preload_content).ok() } diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 9ade3469746..cac53a0b608 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -36,7 +36,7 @@ use std::boxed::FnBox; pub fn factory(cookies_chan: Sender, devtools_chan: Option>, - hsts_list: Option) + hsts_list: HSTSList) -> Box) + Send> { box move |load_data, senders, classifier| { spawn_named("http_loader".to_owned(), @@ -72,10 +72,10 @@ fn read_block(reader: &mut R) -> Result { } } -fn request_must_be_secured(hsts_list: Option<&HSTSList>, url: &Url) -> bool { - match (hsts_list.as_ref(), url.domain()) { - (Some(ref l), Some(ref h)) => { - l.is_host_secure(h) +fn request_must_be_secured(hsts_list: &HSTSList, url: &Url) -> bool { + match url.domain() { + Some(ref h) => { + hsts_list.is_host_secure(h) }, _ => false } @@ -86,7 +86,7 @@ fn load(mut load_data: LoadData, classifier: Arc, cookies_chan: Sender, devtools_chan: Option>, - hsts_list: Option) { + hsts_list: HSTSList) { // FIXME: At the time of writing this FIXME, servo didn't have any central // location for configuration. If you're reading this and such a // repository DOES exist, please update this constant to use it. @@ -117,7 +117,7 @@ fn load(mut load_data: LoadData, loop { iters = iters + 1; - if request_must_be_secured(hsts_list.as_ref(), &url) { + if request_must_be_secured(&hsts_list, &url) { 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 8ddaf11db83..8d5f23343b7 100644 --- a/components/net/resource_task.rs +++ b/components/net/resource_task.rs @@ -19,10 +19,7 @@ use util::opts; use util::task::spawn_named; use url::Url; -use hsts::HSTSList; -use hsts::HSTSEntry; -use hsts::Subdomains; -use hsts::preload_hsts_domains; +use hsts::{HSTSList, HSTSEntry, Subdomains, preload_hsts_domains}; use devtools_traits::{DevtoolsControlMsg}; use hyper::header::{ContentType, Header, SetCookie, UserAgent}; @@ -39,10 +36,10 @@ use std::sync::Arc; use std::sync::mpsc::{channel, Receiver, Sender}; static mut HOST_TABLE: Option<*mut HashMap> = None; -static IPV4_REGEX: Regex = regex!( +pub static IPV4_REGEX: Regex = regex!( r"^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$" ); -static IPV6_REGEX: Regex = regex!(r"^([a-fA-F0-9]{0,4}[:]?){1,8}(/\d{1,3})?$"); +pub static IPV6_REGEX: Regex = regex!(r"^([a-fA-F0-9]{0,4}[:]?){1,8}(/\d{1,3})?$"); pub fn global_init() { //TODO: handle bad file path @@ -165,7 +162,10 @@ pub fn start_sending_opt(start_chan: LoadConsumer, metadata: Metadata) -> Result /// Create a ResourceTask pub fn new_resource_task(user_agent: Option, devtools_chan: Option>) -> ResourceTask { - let hsts_preload = preload_hsts_domains(); + let hsts_preload = match preload_hsts_domains() { + Some(list) => list, + None => HSTSList::new() + }; let (setup_chan, setup_port) = channel(); let setup_chan_clone = setup_chan.clone(); @@ -260,13 +260,13 @@ pub struct ResourceManager { resource_task: Sender, mime_classifier: Arc, devtools_chan: Option>, - hsts_list: Option + hsts_list: HSTSList } impl ResourceManager { pub fn new(user_agent: Option, resource_task: Sender, - hsts_list: Option, + hsts_list: HSTSList, devtools_channel: Option>) -> ResourceManager { ResourceManager { user_agent: user_agent, @@ -293,16 +293,11 @@ impl ResourceManager { } pub fn add_hsts_entry(&mut self, entry: HSTSEntry) { - if let Some(list) = self.hsts_list.as_mut() { - list.push(entry) - } + self.hsts_list.push(entry); } pub fn is_host_sts(&self, host: &str) -> bool { - match self.hsts_list.as_ref() { - Some(list) => list.is_host_secure(host), - None => false - } + self.hsts_list.is_host_secure(host) } fn load(&mut self, mut load_data: LoadData, consumer: LoadConsumer) { diff --git a/tests/unit/net/hsts.rs b/tests/unit/net/hsts.rs index 31b5182d83e..bdefa6b4645 100644 --- a/tests/unit/net/hsts.rs +++ b/tests/unit/net/hsts.rs @@ -7,7 +7,6 @@ use net::hsts::HSTSEntry; use net::hsts::Subdomains; use net::hsts::secure_url; use net::resource_task::ResourceManager; -use net_traits::LoadData; use std::sync::mpsc::channel; use url::Url; use time; @@ -19,7 +18,7 @@ fn test_add_hsts_entry_to_resource_manager_adds_an_hsts_entry() { }; let (tx, _) = channel(); - let mut manager = ResourceManager::new(None, tx, Some(list), None); + let mut manager = ResourceManager::new(None, tx, list, None); let entry = HSTSEntry::new( "mozilla.org".to_string(), Subdomains::NotIncluded, None @@ -134,10 +133,6 @@ fn test_push_entry_to_hsts_list_should_not_create_duplicate_entry() { #[test] fn test_push_multiple_entrie_to_hsts_list_should_add_them_all() { -} - -#[test] -fn test_push_entry_to_hsts_list_should_add_an_entry() { let mut list = HSTSList { entries: Vec::new() }; @@ -152,6 +147,19 @@ fn test_push_entry_to_hsts_list_should_add_an_entry() { assert!(list.is_host_secure("bugzilla.org")); } +#[test] +fn test_push_entry_to_hsts_list_should_add_an_entry() { + let mut list = HSTSList { + entries: Vec::new() + }; + + assert!(!list.is_host_secure("mozilla.org")); + + list.push(HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap()); + + assert!(list.is_host_secure("mozilla.org")); +} + #[test] fn test_parse_hsts_preload_should_return_none_when_json_invalid() { let mock_preload_content = "derp";