mirror of
https://github.com/servo/servo.git
synced 2025-08-03 20:50:07 +01:00
Resolves remaining code review issues
* Don't pass a boolean to the HSTSEntry constructor, use an enum instead * Don't clone when securing load data * Comment about the Url bug * Change remaining assert!(... == ...) to assert_eq!(..., ...)
This commit is contained in:
Sam Gibson
parent
29a34dbdb5
commit
02bd5cdc1b
2 changed files with 58 additions and 39 deletions
|
|
@ -200,14 +200,20 @@ pub struct HSTSEntry {
|
|||
pub timestamp: Option<u64>
|
||||
}
|
||||
|
||||
#[derive(PartialEq, Copy, Clone)]
|
||||
pub enum Subdomains {
|
||||
Included,
|
||||
NotIncluded
|
||||
}
|
||||
|
||||
impl HSTSEntry {
|
||||
pub fn new(host: String, include_subdomains: bool, max_age: Option<u64>) -> Option<HSTSEntry> {
|
||||
pub fn new(host: String, subdomains: Subdomains, max_age: Option<u64>) -> Option<HSTSEntry> {
|
||||
if IPV4_REGEX.is_match(&host) || IPV6_REGEX.is_match(&host) {
|
||||
None
|
||||
} else {
|
||||
Some(HSTSEntry {
|
||||
host: host,
|
||||
include_subdomains: include_subdomains,
|
||||
include_subdomains: (subdomains == Subdomains::Included),
|
||||
max_age: max_age,
|
||||
timestamp: Some(time::get_time().sec as u64)
|
||||
})
|
||||
|
|
@ -292,6 +298,10 @@ pub fn secure_load_data(load_data: &LoadData) -> LoadData {
|
|||
let mut secure_load_data = load_data.clone();
|
||||
let mut secure_url = load_data.url.clone();
|
||||
secure_url.scheme = "https".to_string();
|
||||
// The Url struct parses the port for a known scheme only once.
|
||||
// Updating the scheme doesn't update the port internally, resulting in
|
||||
// HTTPS connections attempted on port 80. Serialising and re-parsing
|
||||
// the Url is a hack to get around this.
|
||||
secure_load_data.url = Url::parse(&secure_url.serialize()).unwrap();
|
||||
|
||||
secure_load_data
|
||||
|
|
@ -351,7 +361,13 @@ impl ResourceChannelManager {
|
|||
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, max_age) {
|
||||
let subdomains = if include_subdomains {
|
||||
Subdomains::Included
|
||||
} else {
|
||||
Subdomains::NotIncluded
|
||||
};
|
||||
|
||||
if let Some(entry) = HSTSEntry::new(host, subdomains, max_age) {
|
||||
self.resource_manager.add_hsts_entry(entry)
|
||||
}
|
||||
}
|
||||
|
|
@ -415,6 +431,15 @@ impl ResourceManager {
|
|||
}
|
||||
}
|
||||
|
||||
fn load_data_must_be_secured(&self, load_data: &LoadData) -> bool {
|
||||
match (self.hsts_list.as_ref(), load_data.url.domain()) {
|
||||
(Some(ref l), Some(ref h)) => {
|
||||
l.is_host_secure(h)
|
||||
},
|
||||
_ => false
|
||||
}
|
||||
}
|
||||
|
||||
fn load(&mut self, mut load_data: LoadData, consumer: LoadConsumer) {
|
||||
unsafe {
|
||||
if let Some(host_table) = HOST_TABLE {
|
||||
|
|
@ -426,16 +451,9 @@ impl ResourceManager {
|
|||
load_data.preserved_headers.set(UserAgent(ua.clone()));
|
||||
});
|
||||
|
||||
load_data = match (self.hsts_list.as_ref(), load_data.url.domain()) {
|
||||
(Some(ref l), Some(ref h)) => {
|
||||
if l.is_host_secure(h) {
|
||||
secure_load_data(&load_data)
|
||||
} else {
|
||||
load_data.clone()
|
||||
}
|
||||
},
|
||||
_ => load_data.clone()
|
||||
};
|
||||
if self.load_data_must_be_secured(&load_data) {
|
||||
load_data = secure_load_data(&load_data)
|
||||
}
|
||||
|
||||
fn from_factory(factory: fn(LoadData, LoadConsumer, Arc<MIMEClassifier>))
|
||||
-> Box<FnBox(LoadData, LoadConsumer, Arc<MIMEClassifier>) + Send> {
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ use net::resource_task::parse_hostsfile;
|
|||
use net::resource_task::replace_hosts;
|
||||
use net::resource_task::HSTSList;
|
||||
use net::resource_task::HSTSEntry;
|
||||
use net::resource_task::Subdomains;
|
||||
use net::resource_task::secure_load_data;
|
||||
use net::resource_task::ResourceManager;
|
||||
use net_traits::{ControlMsg, LoadData, LoadConsumer};
|
||||
|
|
@ -34,7 +35,7 @@ fn test_add_hsts_entry_to_resource_manager_adds_an_hsts_entry() {
|
|||
let mut manager = ResourceManager::new(None, tx, Some(list), None);
|
||||
|
||||
let entry = HSTSEntry::new(
|
||||
"mozilla.org".to_string(), false, None
|
||||
"mozilla.org".to_string(), Subdomains::NotIncluded, None
|
||||
);
|
||||
|
||||
assert!(!manager.is_host_sts("mozilla.org"));
|
||||
|
|
@ -83,7 +84,7 @@ fn test_hsts_entry_is_expired_when_it_has_reached_its_max_age() {
|
|||
#[test]
|
||||
fn test_hsts_entry_cant_be_created_with_ipv6_address_as_host() {
|
||||
let entry = HSTSEntry::new(
|
||||
"2001:0db8:0000:0000:0000:ff00:0042:8329".to_string(), false, None
|
||||
"2001:0db8:0000:0000:0000:ff00:0042:8329".to_string(), Subdomains::NotIncluded, None
|
||||
);
|
||||
|
||||
assert!(entry.is_none(), "able to create HSTSEntry with IPv6 host");
|
||||
|
|
@ -92,7 +93,7 @@ fn test_hsts_entry_cant_be_created_with_ipv6_address_as_host() {
|
|||
#[test]
|
||||
fn test_hsts_entry_cant_be_created_with_ipv4_address_as_host() {
|
||||
let entry = HSTSEntry::new(
|
||||
"4.4.4.4".to_string(), false, None
|
||||
"4.4.4.4".to_string(), Subdomains::NotIncluded, None
|
||||
);
|
||||
|
||||
assert!(entry.is_none(), "able to create HSTSEntry with IPv4 host");
|
||||
|
|
@ -101,10 +102,10 @@ fn test_hsts_entry_cant_be_created_with_ipv4_address_as_host() {
|
|||
#[test]
|
||||
fn test_push_entry_with_0_max_age_evicts_entry_from_list() {
|
||||
let mut list = HSTSList {
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), false, Some(500000u64)).unwrap())
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, Some(500000u64)).unwrap())
|
||||
};
|
||||
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), false, Some(0)).unwrap());
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, Some(0)).unwrap());
|
||||
|
||||
assert!(list.is_host_secure("mozilla.org") == false)
|
||||
}
|
||||
|
|
@ -112,10 +113,10 @@ fn test_push_entry_with_0_max_age_evicts_entry_from_list() {
|
|||
#[test]
|
||||
fn test_push_entry_to_hsts_list_should_not_add_subdomains_whose_superdomain_is_already_matched() {
|
||||
let mut list = HSTSList {
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap())
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap())
|
||||
};
|
||||
|
||||
list.push(HSTSEntry::new("servo.mozilla.org".to_string(), false, None).unwrap());
|
||||
list.push(HSTSEntry::new("servo.mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap());
|
||||
|
||||
assert!(list.entries.len() == 1)
|
||||
}
|
||||
|
|
@ -123,12 +124,12 @@ fn test_push_entry_to_hsts_list_should_not_add_subdomains_whose_superdomain_is_a
|
|||
#[test]
|
||||
fn test_push_entry_to_hsts_list_should_update_existing_domain_entrys_include_subdomains() {
|
||||
let mut list = HSTSList {
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap())
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap())
|
||||
};
|
||||
|
||||
assert!(list.is_host_secure("servo.mozilla.org"));
|
||||
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), false, None).unwrap());
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap());
|
||||
|
||||
assert!(!list.is_host_secure("servo.mozilla.org"))
|
||||
}
|
||||
|
|
@ -136,10 +137,10 @@ fn test_push_entry_to_hsts_list_should_update_existing_domain_entrys_include_sub
|
|||
#[test]
|
||||
fn test_push_entry_to_hsts_list_should_not_create_duplicate_entry() {
|
||||
let mut list = HSTSList {
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), false, None).unwrap())
|
||||
entries: vec!(HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap())
|
||||
};
|
||||
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), false, None).unwrap());
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap());
|
||||
|
||||
assert!(list.entries.len() == 1)
|
||||
}
|
||||
|
|
@ -157,8 +158,8 @@ fn test_push_entry_to_hsts_list_should_add_an_entry() {
|
|||
assert!(!list.is_host_secure("mozilla.org"));
|
||||
assert!(!list.is_host_secure("bugzilla.org"));
|
||||
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap());
|
||||
list.push(HSTSEntry::new("bugzilla.org".to_string(), true, None).unwrap());
|
||||
list.push(HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap());
|
||||
list.push(HSTSEntry::new("bugzilla.org".to_string(), Subdomains::Included, None).unwrap());
|
||||
|
||||
assert!(list.is_host_secure("mozilla.org"));
|
||||
assert!(list.is_host_secure("bugzilla.org"));
|
||||
|
|
@ -187,8 +188,8 @@ fn test_parse_hsts_preload_should_decode_host_and_includes_subdomains() {
|
|||
let hsts_list = HSTSList::new_from_preload(mock_preload_content);
|
||||
let entries = hsts_list.unwrap().entries;
|
||||
|
||||
assert!(entries[0].host == "mozilla.org");
|
||||
assert!(entries[0].include_subdomains == false);
|
||||
assert_eq!(entries[0].host, "mozilla.org");
|
||||
assert!(!entries[0].include_subdomains);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -197,52 +198,52 @@ fn test_hsts_list_with_no_entries_does_not_is_host_secure() {
|
|||
entries: Vec::new()
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("mozilla.org") == false);
|
||||
assert!(!hsts_list.is_host_secure("mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_hsts_list_with_exact_domain_entry_is_is_host_secure() {
|
||||
let hsts_list = HSTSList {
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), false, None).unwrap()]
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap()]
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("mozilla.org") == true);
|
||||
assert!(hsts_list.is_host_secure("mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_hsts_list_with_subdomain_when_include_subdomains_is_true_is_is_host_secure() {
|
||||
let hsts_list = HSTSList {
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap()]
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap()]
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("servo.mozilla.org") == true);
|
||||
assert!(hsts_list.is_host_secure("servo.mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_hsts_list_with_subdomain_when_include_subdomains_is_false_is_not_is_host_secure() {
|
||||
let hsts_list = HSTSList {
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), false, None).unwrap()]
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), Subdomains::NotIncluded, None).unwrap()]
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("servo.mozilla.org") == false);
|
||||
assert!(!hsts_list.is_host_secure("servo.mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_hsts_list_with_subdomain_when_host_is_not_a_subdomain_is_not_is_host_secure() {
|
||||
let hsts_list = HSTSList {
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap()]
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap()]
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("servo-mozilla.org") == false);
|
||||
assert!(!hsts_list.is_host_secure("servo-mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_hsts_list_with_subdomain_when_host_is_exact_match_is_is_host_secure() {
|
||||
let hsts_list = HSTSList {
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap()]
|
||||
entries: vec![HSTSEntry::new("mozilla.org".to_string(), Subdomains::Included, None).unwrap()]
|
||||
};
|
||||
|
||||
assert!(hsts_list.is_host_secure("mozilla.org") == true);
|
||||
assert!(hsts_list.is_host_secure("mozilla.org"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue