servo: Merge #14445 - Redesign CookieStorage and Implement Leave Secure Cookie Alone (from mrnayak:netSecurity); r=jdm
authorRaghav <rmuddur@gmail.com>
Sun, 04 Dec 2016 15:30:46 -0800
changeset 340342 afa259cb27c04633ca1db3c5a6354341f60d828e
parent 340341 190e7a2c6cf7ac9ce43d7394cad4249d079bd378
child 340343 3bff31cb6f5c31fe0b988e3fc655369778f5a091
push id86548
push userkwierso@gmail.com
push dateSat, 04 Feb 2017 01:35:21 +0000
treeherdermozilla-inbound@e7b96d015d03 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
servo: Merge #14445 - Redesign CookieStorage and Implement Leave Secure Cookie Alone (from mrnayak:netSecurity); r=jdm CookieStorage has been refactored to use HashMap with the base domain as the key. Values of hashmap are vector of cookies. CookieStorage now has max_per_host which restricts maximum cookies that can be added per base domain. Cookie eviction does not take place if max_per_host is not reached. Cookie eviction logic implemented here does following steps 1) Evict all expired cookies 2) Remove oldest accessed non-secure cookie If any 3) When no non-secure cookie exists, remove oldest accessed secure cookie if new cookie being added is secure. Else ignore new cookie --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: b05c27cb58e8d625f4f436b6e9e1f0c29e908f21
servo/components/net/cookie_storage.rs
servo/components/net/http_loader.rs
servo/components/net/resource_thread.rs
servo/tests/unit/net/cookie.rs
servo/tests/unit/net/cookie_http_state.rs
--- a/servo/components/net/cookie_storage.rs
+++ b/servo/components/net/cookie_storage.rs
@@ -3,51 +3,61 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Implementation of cookie storage as specified in
 //! http://tools.ietf.org/html/rfc6265
 
 use cookie::Cookie;
 use cookie_rs;
 use net_traits::CookieSource;
+use net_traits::pub_domains::reg_suffix;
 use servo_url::ServoUrl;
 use std::cmp::Ordering;
+use std::collections::HashMap;
+use time::Tm;
+
+extern crate time;
 
 #[derive(Clone, Debug, RustcDecodable, RustcEncodable)]
 pub struct CookieStorage {
     version: u32,
-    cookies: Vec<Cookie>
+    cookies_map: HashMap<String, Vec<Cookie>>,
+    max_per_host: usize,
 }
 
 impl CookieStorage {
-    pub fn new() -> CookieStorage {
+    pub fn new(max_cookies: usize) -> CookieStorage {
         CookieStorage {
             version: 1,
-            cookies: Vec::new()
+            cookies_map: HashMap::new(),
+            max_per_host: max_cookies,
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
     pub fn remove(&mut self, cookie: &Cookie, source: CookieSource) -> Result<Option<Cookie>, ()> {
+        let domain = reg_host(cookie.cookie.domain.as_ref().unwrap_or(&"".to_string()));
+        let cookies = self.cookies_map.entry(domain).or_insert(vec![]);
+
         // Step 1
-        let position = self.cookies.iter().position(|c| {
+        let position = cookies.iter().position(|c| {
             c.cookie.domain == cookie.cookie.domain &&
             c.cookie.path == cookie.cookie.path &&
             c.cookie.name == cookie.cookie.name
         });
 
         if let Some(ind) = position {
-            let c = self.cookies.remove(ind);
+            let c = cookies.remove(ind);
 
             // http://tools.ietf.org/html/rfc6265#section-5.3 step 11.2
             if !c.cookie.httponly || source == CookieSource::HTTP {
                 Ok(Some(c))
             } else {
                 // Undo the removal.
-                self.cookies.push(c);
+                cookies.push(c);
                 Err(())
             }
         } else {
             Ok(None)
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
@@ -60,17 +70,30 @@ impl CookieStorage {
 
         // Step 11
         if let Some(old_cookie) = old_cookie.unwrap() {
             // Step 11.3
             cookie.creation_time = old_cookie.creation_time;
         }
 
         // Step 12
-        self.cookies.push(cookie);
+        let domain = reg_host(&cookie.cookie.domain.as_ref().unwrap_or(&"".to_string()));
+        let mut cookies = self.cookies_map.entry(domain).or_insert(vec![]);
+
+        if cookies.len() == self.max_per_host {
+            let old_len = cookies.len();
+            cookies.retain(|c| !is_cookie_expired(&c));
+            let new_len = cookies.len();
+
+            // https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone
+            if new_len == old_len && !evict_one_cookie(cookie.cookie.secure, cookies) {
+                return;
+            }
+        }
+        cookies.push(cookie);
     }
 
     pub fn cookie_comparator(a: &Cookie, b: &Cookie) -> Ordering {
         let a_path_len = a.cookie.path.as_ref().map_or(0, |p| p.len());
         let b_path_len = b.cookie.path.as_ref().map_or(0, |p| p.len());
         match a_path_len.cmp(&b_path_len) {
             Ordering::Equal => {
                 let a_creation_time = a.creation_time.to_timespec();
@@ -82,45 +105,97 @@ impl CookieStorage {
             Ordering::Less => Ordering::Greater,
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.4
     pub fn cookies_for_url(&mut self, url: &ServoUrl, source: CookieSource) -> Option<String> {
         let filterer = |c: &&mut Cookie| -> bool {
             info!(" === SENT COOKIE : {} {} {:?} {:?}",
-                  c.cookie.name, c.cookie.value, c.cookie.domain, c.cookie.path);
-            info!(" === SENT COOKIE RESULT {}", c.appropriate_for_url(url, source));
+                  c.cookie.name,
+                  c.cookie.value,
+                  c.cookie.domain,
+                  c.cookie.path);
+            info!(" === SENT COOKIE RESULT {}",
+                  c.appropriate_for_url(url, source));
             // Step 1
             c.appropriate_for_url(url, source)
         };
 
         // Step 2
-        let mut url_cookies: Vec<&mut Cookie> = self.cookies.iter_mut().filter(filterer).collect();
+        let domain = reg_host(url.host_str().unwrap_or(""));
+        let cookies = self.cookies_map.entry(domain).or_insert(vec![]);
+
+        let mut url_cookies: Vec<&mut Cookie> = cookies.iter_mut().filter(filterer).collect();
         url_cookies.sort_by(|a, b| CookieStorage::cookie_comparator(*a, *b));
 
         let reducer = |acc: String, c: &mut &mut Cookie| -> String {
             // Step 3
             c.touch();
 
             // Step 4
             (match acc.len() {
                 0 => acc,
-                _ => acc + "; "
+                _ => acc + "; ",
             }) + &c.cookie.name + "=" + &c.cookie.value
         };
         let result = url_cookies.iter_mut().fold("".to_owned(), reducer);
 
         info!(" === COOKIES SENT: {}", result);
         match result.len() {
             0 => None,
-            _ => Some(result)
+            _ => Some(result),
         }
     }
 
-    pub fn cookies_data_for_url<'a>(&'a mut self, url: &'a ServoUrl,
-                                    source: CookieSource) -> Box<Iterator<Item=cookie_rs::Cookie> + 'a> {
-        Box::new(self.cookies.iter_mut().filter(move |c| { c.appropriate_for_url(url, source) }).map(|c| {
+    pub fn cookies_data_for_url<'a>(&'a mut self,
+                                    url: &'a ServoUrl,
+                                    source: CookieSource)
+                                    -> Box<Iterator<Item = cookie_rs::Cookie> + 'a> {
+        let domain = reg_host(url.host_str().unwrap_or(""));
+        let cookies = self.cookies_map.entry(domain).or_insert(vec![]);
+
+        Box::new(cookies.iter_mut().filter(move |c| c.appropriate_for_url(url, source)).map(|c| {
             c.touch();
             c.cookie.clone()
         }))
     }
 }
+fn reg_host<'a>(url: &'a str) -> String {
+    reg_suffix(url).to_string()
+}
+
+fn is_cookie_expired(cookie: &Cookie) -> bool {
+    match cookie.expiry_time {
+        Some(t) => t.to_timespec() <= time::get_time(),
+        None => false,
+    }
+}
+
+fn evict_one_cookie(is_secure_cookie: bool, cookies: &mut Vec<Cookie>) -> bool {
+    // Remove non-secure cookie with oldest access time
+    let oldest_accessed: Option<(usize, Tm)> = get_oldest_accessed(false, cookies);
+
+    if let Some((index, _)) = oldest_accessed {
+        cookies.remove(index);
+    } else {
+        // All secure cookies were found
+        if !is_secure_cookie {
+            return false;
+        }
+        let oldest_accessed: Option<(usize, Tm)> = get_oldest_accessed(true, cookies);
+        if let Some((index, _)) = oldest_accessed {
+            cookies.remove(index);
+        }
+    }
+    return true;
+}
+
+fn get_oldest_accessed(is_secure_cookie: bool, cookies: &mut Vec<Cookie>) -> Option<(usize, Tm)> {
+    let mut oldest_accessed: Option<(usize, Tm)> = None;
+    for (i, c) in cookies.iter().enumerate() {
+        if (c.cookie.secure == is_secure_cookie) &&
+           oldest_accessed.as_ref().map_or(true, |a| c.last_access < a.1) {
+            oldest_accessed = Some((i, c.last_access));
+        }
+    }
+    oldest_accessed
+}
--- a/servo/components/net/http_loader.rs
+++ b/servo/components/net/http_loader.rs
@@ -74,17 +74,17 @@ pub struct HttpState {
     pub auth_cache: Arc<RwLock<AuthCache>>,
     pub blocked_content: Arc<Option<RuleList>>,
 }
 
 impl HttpState {
     pub fn new() -> HttpState {
         HttpState {
             hsts_list: Arc::new(RwLock::new(HstsList::new())),
-            cookie_jar: Arc::new(RwLock::new(CookieStorage::new())),
+            cookie_jar: Arc::new(RwLock::new(CookieStorage::new(150))),
             auth_cache: Arc::new(RwLock::new(AuthCache::new())),
             blocked_content: Arc::new(None),
         }
     }
 }
 
 fn precise_time_ms() -> u64 {
     time::precise_time_ns() / (1000 * 1000)
--- a/servo/components/net/resource_thread.rs
+++ b/servo/components/net/resource_thread.rs
@@ -180,30 +180,30 @@ struct ResourceChannelManager {
     resource_manager: CoreResourceManager,
     config_dir: Option<PathBuf>,
 }
 
 fn create_resource_groups(config_dir: Option<&Path>)
                           -> (ResourceGroup, ResourceGroup) {
     let mut hsts_list = HstsList::from_servo_preload();
     let mut auth_cache = AuthCache::new();
-    let mut cookie_jar = CookieStorage::new();
+    let mut cookie_jar = CookieStorage::new(150);
     if let Some(config_dir) = config_dir {
         read_json_from_file(&mut auth_cache, config_dir, "auth_cache.json");
         read_json_from_file(&mut hsts_list, config_dir, "hsts_list.json");
         read_json_from_file(&mut cookie_jar, config_dir, "cookie_jar.json");
     }
     let resource_group = ResourceGroup {
         cookie_jar: Arc::new(RwLock::new(cookie_jar)),
         auth_cache: Arc::new(RwLock::new(auth_cache)),
         hsts_list: Arc::new(RwLock::new(hsts_list.clone())),
         connector: create_http_connector(),
     };
     let private_resource_group = ResourceGroup {
-        cookie_jar: Arc::new(RwLock::new(CookieStorage::new())),
+        cookie_jar: Arc::new(RwLock::new(CookieStorage::new(150))),
         auth_cache: Arc::new(RwLock::new(AuthCache::new())),
         hsts_list: Arc::new(RwLock::new(HstsList::new())),
         connector: create_http_connector(),
     };
     (resource_group, private_resource_group)
 }
 
 impl ResourceChannelManager {
--- a/servo/tests/unit/net/cookie.rs
+++ b/servo/tests/unit/net/cookie.rs
@@ -1,13 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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 cookie_rs;
+use hyper::header::{Header, SetCookie};
 use net::cookie::Cookie;
 use net::cookie_storage::CookieStorage;
 use net_traits::CookieSource;
 use servo_url::ServoUrl;
 
 #[test]
 fn test_domain_match() {
     assert!(Cookie::domain_match("foo.com", "foo.com"));
@@ -105,18 +106,17 @@ fn delay_to_ensure_different_timestamp()
     use std::time::Duration;
 
     // time::now()'s resolution on some platforms isn't granular enought to ensure
     // that two back-to-back calls to Cookie::new_wrapped generate different timestamps .
     thread::sleep(Duration::from_millis(500));
 }
 
 #[cfg(not(target_os = "windows"))]
-fn delay_to_ensure_different_timestamp() {
-}
+fn delay_to_ensure_different_timestamp() {}
 
 #[test]
 fn test_sort_order() {
     use std::cmp::Ordering;
 
     let url = &ServoUrl::parse("http://example.com/foo").unwrap();
     let a_wrapped = cookie_rs::Cookie::parse("baz=bar; Path=/foo/bar/").unwrap();
     let a = Cookie::new_wrapped(a_wrapped.clone(), url, CookieSource::HTTP).unwrap();
@@ -127,8 +127,107 @@ fn test_sort_order() {
 
     assert!(b.cookie.path.as_ref().unwrap().len() > a.cookie.path.as_ref().unwrap().len());
     assert!(CookieStorage::cookie_comparator(&a, &b) == Ordering::Greater);
     assert!(CookieStorage::cookie_comparator(&b, &a) == Ordering::Less);
     assert!(CookieStorage::cookie_comparator(&a, &a_prime) == Ordering::Less);
     assert!(CookieStorage::cookie_comparator(&a_prime, &a) == Ordering::Greater);
     assert!(CookieStorage::cookie_comparator(&a, &a) == Ordering::Equal);
 }
+
+
+fn add_retrieve_cookies(set_location: &str,
+                        set_cookies: &[String],
+                        final_location: &str)
+                        -> String {
+    let mut storage = CookieStorage::new(5);
+    let url = ServoUrl::parse(set_location).unwrap();
+    let source = CookieSource::HTTP;
+
+    // Add all cookies to the store
+    for str_cookie in set_cookies {
+        let bytes = str_cookie.to_string().into_bytes();
+        let header = Header::parse_header(&[bytes]).unwrap();
+        let SetCookie(cookies) = header;
+        for bare_cookie in cookies {
+            let cookie = Cookie::new_wrapped(bare_cookie, &url, source).unwrap();
+            storage.push(cookie, source);
+        }
+    }
+
+    // Get cookies for the test location
+    let url = ServoUrl::parse(final_location).unwrap();
+    storage.cookies_for_url(&url, source).unwrap_or("".to_string())
+}
+
+
+#[test]
+fn test_cookie_eviction_expired() {
+    let mut vec = Vec::new();
+    for i in 1..6 {
+        let st = format!("extra{}=bar; Secure; expires=Sun, 18-Apr-2000 21:06:29 GMT",
+                         i);
+        vec.push(st);
+    }
+    vec.push("foo=bar; Secure; expires=Sun, 18-Apr-2027 21:06:29 GMT".to_owned());
+    let r = add_retrieve_cookies("https://home.example.org:8888/cookie-parser?0001",
+                                 &vec, "https://home.example.org:8888/cookie-parser-result?0001");
+    assert_eq!(&r, "foo=bar");
+}
+
+
+#[test]
+fn test_cookie_eviction_all_secure_one_nonsecure() {
+    let mut vec = Vec::new();
+    for i in 1..5 {
+        let st = format!("extra{}=bar; Secure; expires=Sun, 18-Apr-2026 21:06:29 GMT",
+                         i);
+        vec.push(st);
+    }
+    vec.push("foo=bar; expires=Sun, 18-Apr-2026 21:06:29 GMT".to_owned());
+    vec.push("foo2=bar; Secure; expires=Sun, 18-Apr-2028 21:06:29 GMT".to_owned());
+    let r = add_retrieve_cookies("https://home.example.org:8888/cookie-parser?0001",
+                                 &vec, "https://home.example.org:8888/cookie-parser-result?0001");
+    assert_eq!(&r, "extra1=bar; extra2=bar; extra3=bar; extra4=bar; foo2=bar");
+}
+
+
+#[test]
+fn test_cookie_eviction_all_secure_new_nonsecure() {
+    let mut vec = Vec::new();
+    for i in 1..6 {
+        let st = format!("extra{}=bar; Secure; expires=Sun, 18-Apr-2026 21:06:29 GMT",
+                         i);
+        vec.push(st);
+    }
+    vec.push("foo=bar; expires=Sun, 18-Apr-2077 21:06:29 GMT".to_owned());
+    let r = add_retrieve_cookies("https://home.example.org:8888/cookie-parser?0001",
+                                 &vec, "https://home.example.org:8888/cookie-parser-result?0001");
+    assert_eq!(&r, "extra1=bar; extra2=bar; extra3=bar; extra4=bar; extra5=bar");
+}
+
+
+#[test]
+fn test_cookie_eviction_all_nonsecure_new_secure() {
+    let mut vec = Vec::new();
+    for i in 1..6 {
+        let st = format!("extra{}=bar; expires=Sun, 18-Apr-2026 21:06:29 GMT", i);
+        vec.push(st);
+    }
+    vec.push("foo=bar; Secure; expires=Sun, 18-Apr-2077 21:06:29 GMT".to_owned());
+    let r = add_retrieve_cookies("https://home.example.org:8888/cookie-parser?0001",
+                                 &vec, "https://home.example.org:8888/cookie-parser-result?0001");
+    assert_eq!(&r, "extra2=bar; extra3=bar; extra4=bar; extra5=bar; foo=bar");
+}
+
+
+#[test]
+fn test_cookie_eviction_all_nonsecure_new_nonsecure() {
+    let mut vec = Vec::new();
+    for i in 1..6 {
+        let st = format!("extra{}=bar; expires=Sun, 18-Apr-2026 21:06:29 GMT", i);
+        vec.push(st);
+    }
+    vec.push("foo=bar; expires=Sun, 18-Apr-2077 21:06:29 GMT".to_owned());
+    let r = add_retrieve_cookies("https://home.example.org:8888/cookie-parser?0001",
+                                 &vec, "https://home.example.org:8888/cookie-parser-result?0001");
+    assert_eq!(&r, "extra2=bar; extra3=bar; extra4=bar; extra5=bar; foo=bar");
+}
--- a/servo/tests/unit/net/cookie_http_state.rs
+++ b/servo/tests/unit/net/cookie_http_state.rs
@@ -5,17 +5,17 @@
 use hyper::header::{Header, SetCookie};
 use net::cookie::Cookie;
 use net::cookie_storage::CookieStorage;
 use net_traits::CookieSource;
 use servo_url::ServoUrl;
 
 
 fn run(set_location: &str, set_cookies: &[&str], final_location: &str) -> String {
-    let mut storage = CookieStorage::new();
+    let mut storage = CookieStorage::new(150);
     let url = ServoUrl::parse(set_location).unwrap();
     let source = CookieSource::HTTP;
 
     // Add all cookies to the store
     for str_cookie in set_cookies {
         let bytes = str_cookie.to_string().into_bytes();
         let header = Header::parse_header(&[bytes]);
         if let Ok(SetCookie(cookies)) = header {