Differentiate between HTTP and non-HTTP APIs for cookie operations. Fix some incorrect cookie removal operation logic. Order the returned cookies according to the spec. Make cookie unit tests pass.

components/net/cookie.rs

@@ -5,6 +5,7 @@
 //! Implementation of cookie creation and matching as specified by
 //! http://tools.ietf.org/html/rfc6265
 
+use cookie_storage::CookieSource;
 use pub_domains::PUB_DOMAINS;
 
 use cookie_rs;
@@ -23,15 +24,15 @@ pub struct Cookie {
     pub cookie: cookie_rs::Cookie,
     pub host_only: bool,
     pub persistent: bool,
-    pub created_at: Tm,
+    pub creation_time: Tm,
     pub last_access: Tm,
-    pub scheme: String,
     pub expiry_time: Tm,
 }
 
 impl Cookie {
     /// http://tools.ietf.org/html/rfc6265#section-5.3
-    pub fn new_wrapped(mut cookie: cookie_rs::Cookie, request: &Url) -> Option<Cookie> {
+    pub fn new_wrapped(mut cookie: cookie_rs::Cookie, request: &Url, source: CookieSource)
+                       -> Option<Cookie> {
         // Step 3
         let (persistent, expiry_time) = match (&cookie.max_age, &cookie.expires) {
             (&Some(max_age), _) => (true, at(now().to_timespec() + Duration::seconds(max_age as i64))),
@@ -67,19 +68,15 @@ impl Cookie {
         // Step 7
         let mut path = cookie.path.unwrap_or("".to_owned());
         if path.is_empty() || path.char_at(0) != '/' {
-            let mut url_path: String = "".to_owned();
-            if let Some(paths) = request.path() {
-                for path in paths.iter() {
-                    url_path.extend(path.chars());
-                }
-            }
-            path = Cookie::default_path(url_path.as_slice());
+            let url_path = request.serialize_path();
+            let url_path = url_path.as_ref().map(|path| path.as_slice());
+            path = Cookie::default_path(url_path.unwrap_or(""));
         }
         cookie.path = Some(path);
 
 
         // Step 10
-        if cookie.httponly && !request.scheme.as_slice().starts_with("http") {
+        if cookie.httponly && source != CookieSource::HTTP {
             return None;
         }
 
@@ -87,9 +84,8 @@ impl Cookie {
             cookie: cookie,
             host_only: host_only,
             persistent: persistent,
-            created_at: now(),
+            creation_time: now(),
             last_access: now(),
-            scheme: request.scheme.clone(),
             expiry_time: expiry_time,
         })
     }
@@ -100,13 +96,14 @@ impl Cookie {
 
     // http://tools.ietf.org/html/rfc6265#section-5.1.4
     fn default_path(request_path: &str) -> String {
-        if request_path == "" || request_path.char_at(0) != '/' || request_path == "/" {
-            return "/".to_owned();
+        if request_path == "" || request_path.char_at(0) != '/' ||
+           request_path.chars().filter(|&c| c == '/').count() == 1 {
+            "/".to_owned()
+        } else if request_path.ends_with("/") {
+            request_path.slice_to(request_path.len()-1).to_owned()
+        } else {
+            request_path.to_owned()
         }
-        if request_path.ends_with("/") {
-            return request_path.slice_to(request_path.len()-1).to_string();
-        }
-        return request_path.to_owned();
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.1.4
@@ -131,7 +128,7 @@ impl Cookie {
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.4 step 1
-    pub fn appropriate_for_url(&self, url: Url) -> bool {
+    pub fn appropriate_for_url(&self, url: &Url, source: CookieSource) -> bool {
         let domain = url.host().map(|host| host.serialize());
         if self.host_only {
             if self.cookie.domain != domain {
@@ -154,7 +151,7 @@ impl Cookie {
         if self.cookie.secure && url.scheme != "https".to_string() {
             return false;
         }
-        if self.cookie.httponly && !url.scheme.as_slice().starts_with("http") {
+        if self.cookie.httponly && source == CookieSource::NonHTTP {
             return false;
         }
 
@@ -171,77 +168,67 @@ fn test_domain_match() {
     assert!(!Cookie::domain_match("bar.foo.com", "bar.com"));
     assert!(!Cookie::domain_match("bar.com", "baz.bar.com"));
     assert!(!Cookie::domain_match("foo.com", "bar.com"));
+
+    assert!(!Cookie::domain_match("bar.com", "bbar.com"));
+    assert!(Cookie::domain_match("235.132.2.3", "235.132.2.3"));
+    assert!(!Cookie::domain_match("235.132.2.3", "1.1.1.1"));
+    assert!(!Cookie::domain_match("235.132.2.3", ".2.3"));
 }
 
 #[test]
 fn test_default_path() {
     assert!(Cookie::default_path("/foo/bar/baz/").as_slice() == "/foo/bar/baz");
-    assert!(Cookie::default_path("/foo").as_slice() == "/foo");
+    assert!(Cookie::default_path("/foo/").as_slice() == "/foo");
+    assert!(Cookie::default_path("/foo").as_slice() == "/");
     assert!(Cookie::default_path("/").as_slice() == "/");
     assert!(Cookie::default_path("").as_slice() == "/");
+    assert!(Cookie::default_path("foo").as_slice() == "/");
 }
 
 #[test]
 fn fn_cookie_constructor() {
+    use cookie_storage::CookieSource;
+
     let url = &Url::parse("http://example.com/foo").unwrap();
 
     let gov_url = &Url::parse("http://gov.ac/foo").unwrap();
     // cookie name/value test
-    assert!(Cookie::new(" baz ".to_string(), url).is_none());
-    assert!(Cookie::new(" = bar  ".to_string(), url).is_none());
-    assert!(Cookie::new(" baz = ".to_string(), url).is_some());
+    assert!(cookie_rs::Cookie::parse(" baz ").is_err());
+    assert!(cookie_rs::Cookie::parse(" = bar  ").is_err());
+    assert!(cookie_rs::Cookie::parse(" baz = ").is_ok());
 
     // cookie domains test
-    assert!(Cookie::new(" baz = bar; Domain =  ".to_string(), url).is_some());
-    assert!(Cookie::new(" baz = bar; Domain =  ".to_string(), url).unwrap().domain.as_slice() == "example.com");
+    let cookie = cookie_rs::Cookie::parse(" baz = bar; Domain =  ").unwrap();
+    assert!(Cookie::new_wrapped(cookie, url, CookieSource::HTTP).is_some());
+    let cookie = cookie_rs::Cookie::parse(" baz = bar; Domain =  ").unwrap();
+    let cookie = Cookie::new_wrapped(cookie, url, CookieSource::HTTP).unwrap();
+    assert!(cookie.cookie.domain.as_ref().unwrap().as_slice() == "example.com");
 
     // cookie public domains test
-    assert!(Cookie::new(" baz = bar; Domain =  gov.ac".to_string(), url).is_none());
-    assert!(Cookie::new(" baz = bar; Domain =  gov.ac".to_string(), gov_url).is_some());
+    let cookie = cookie_rs::Cookie::parse(" baz = bar; Domain =  gov.ac").unwrap();
+    assert!(Cookie::new_wrapped(cookie.clone(), url, CookieSource::HTTP).is_none());
+    assert!(Cookie::new_wrapped(cookie, gov_url, CookieSource::HTTP).is_some());
 
     // cookie domain matching test
-    assert!(Cookie::new(" baz = bar ; Secure; Domain = bazample.com".to_string(), url).is_none());
+    let cookie = cookie_rs::Cookie::parse(" baz = bar ; Secure; Domain = bazample.com").unwrap();
+    assert!(Cookie::new_wrapped(cookie, url, CookieSource::HTTP).is_none());
 
-    assert!(Cookie::new(" baz = bar ; Secure; Path = /foo/bar/".to_string(), url).is_some());
+    let cookie = cookie_rs::Cookie::parse(" baz = bar ; Secure; Path = /foo/bar/").unwrap();
+    assert!(Cookie::new_wrapped(cookie, url, CookieSource::HTTP).is_some());
 
-    let cookie = Cookie::new(" baz = bar ; Secure; Path = /foo/bar/".to_string(), url).unwrap();
-    assert!(cookie.value.as_slice() == "bar");
-    assert!(cookie.name.as_slice() == "baz");
-    assert!(cookie.secure);
-    assert!(cookie.path.as_slice() == "/foo/bar/");
-    assert!(cookie.domain.as_slice() == "example.com");
+    let cookie = cookie_rs::Cookie::parse(" baz = bar ; HttpOnly").unwrap();
+    assert!(Cookie::new_wrapped(cookie, url, CookieSource::NonHTTP).is_none());
+
+    let cookie = cookie_rs::Cookie::parse(" baz = bar ; Secure; Path = /foo/bar/").unwrap();
+    let cookie = Cookie::new_wrapped(cookie, url, CookieSource::HTTP).unwrap();
+    assert!(cookie.cookie.value.as_slice() == "bar");
+    assert!(cookie.cookie.name.as_slice() == "baz");
+    assert!(cookie.cookie.secure);
+    assert!(cookie.cookie.path.as_ref().unwrap().as_slice() == "/foo/bar/");
+    assert!(cookie.cookie.domain.as_ref().unwrap().as_slice() == "example.com");
     assert!(cookie.host_only);
 
     let u = &Url::parse("http://example.com/foobar").unwrap();
-    assert!(Cookie::new("foobar=value;path=/".to_string(), u).is_some());
-}
-
-#[deriving(Clone)]
-pub struct CookieManager {
-    cookies: Vec<Cookie>,
-}
-
-impl CookieManager {
-    pub fn new() -> CookieManager {
-        CookieManager {
-            cookies: Vec::new()
-        }
-    }
-
-    pub fn add(&mut self, cookie: &Cookie) -> bool {
-        match self.cookies.iter().find(|x| {
-            x.cookie.domain == cookie.cookie.domain &&
-            x.cookie.name == cookie.cookie.name &&
-            x.cookie.path == cookie.cookie.path
-        }) {
-            Some(c) => {
-                if c.cookie.httponly && !cookie.scheme.as_slice().starts_with("http") {
-                    return false
-                }
-            }
-            None => {}
-        }
-        self.cookies.push(cookie.clone());
-        true
-    }
+    let cookie = cookie_rs::Cookie::parse("foobar=value;path=/").unwrap();
+    assert!(Cookie::new_wrapped(cookie, u, CookieSource::HTTP).is_some());
 }

components/net/cookie_storage.rs

@@ -7,6 +7,16 @@
 
 use url::Url;
 use cookie::Cookie;
+use std::cmp::Ordering;
+
+/// The creator of a given cookie
+#[derive(PartialEq, Copy)]
+pub enum CookieSource {
+    /// An HTTP API
+    HTTP,
+    /// A non-HTTP API
+    NonHTTP,
+}
 
 pub struct CookieStorage {
     cookies: Vec<Cookie>
@@ -20,7 +30,7 @@ impl CookieStorage {
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
-    pub fn remove(&mut self, cookie: &Cookie) -> Option<Cookie> {
+    pub fn remove(&mut self, cookie: &Cookie, source: CookieSource) -> Result<Option<Cookie>, ()> {
         // Step 1
         let position = self.cookies.iter().position(|c| {
             c.cookie.domain == cookie.cookie.domain &&
@@ -29,42 +39,68 @@ impl CookieStorage {
         });
 
         if let Some(ind) = position {
-            Some(self.cookies.remove(ind))
+            let c = self.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 {
-            None
+                // Undo the removal.
+                self.cookies.push(c);
+                Err(())
+            }
+        } else {
+            Ok(None)
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
-    pub fn push(&mut self, mut cookie: Cookie, request: &Url) {
+    pub fn push(&mut self, mut cookie: Cookie, source: CookieSource) {
+        let old_cookie = self.remove(&cookie, source);
+        if old_cookie.is_err() {
+            // This new cookie is not allowed to overwrite an existing one.
+            return;
+        }
+
+        if cookie.cookie.value.is_empty() {
+            return;
+        }
+
         // Step 11
-        if let Some(old_cookie) = self.remove(&cookie) {
-            // Step 11.2
-            if old_cookie.cookie.httponly && !request.scheme.starts_with("http") {
-                self.cookies.push(old_cookie);
-            } else {
+        if let Some(old_cookie) = old_cookie.unwrap() {
             // Step 11.3
-                cookie.created_at = old_cookie.created_at;
+            cookie.creation_time = old_cookie.creation_time;
+        }
+
         // Step 12
         self.cookies.push(cookie);
     }
-        }
-    }
 
     // http://tools.ietf.org/html/rfc6265#section-5.4
-    pub fn cookies_for_url(&mut self, url: Url) -> Option<String> {
+    pub fn cookies_for_url(&mut self, url: &Url, 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.clone()));
+            info!(" === SENT COOKIE RESULT {}", c.appropriate_for_url(url, source));
             // Step 1
-            c.appropriate_for_url(url.clone())
+            c.appropriate_for_url(url, source)
         };
 
-        let mut url_cookies = self.cookies.iter_mut().filter(filterer);
+        // Step 2
+        let mut url_cookies: Vec<&mut Cookie> = self.cookies.iter_mut().filter(filterer).collect();
+        url_cookies.sort_by(|a, b| {
+            let a_path_len = a.cookie.path.as_ref().map(|p| p.len()).unwrap_or(0);
+            let b_path_len = b.cookie.path.as_ref().map(|p| p.len()).unwrap_or(0);
+            match a_path_len.cmp(&b_path_len) {
+                Ordering::Equal => {
+                    let a_creation_time = a.creation_time.to_timespec();
+                    let b_creation_time = b.creation_time.to_timespec();
+                    a_creation_time.cmp(&b_creation_time)
+                }
+                result => result
+            }
+        });
 
-        // TODO Step 2
-
-        let reducer = |&:acc: String, c: &mut Cookie| -> String {
+        let reducer = |&:acc: String, c: &mut &mut Cookie| -> String {
             // Step 3
             c.touch();
 
@@ -74,7 +110,7 @@ impl CookieStorage {
                 _ => acc + ";"
             }) + c.cookie.name.as_slice() + "=" + c.cookie.value.as_slice()
         };
-        let result = url_cookies.fold("".to_string(), reducer);
+        let result = url_cookies.iter_mut().fold("".to_string(), reducer);
 
         info!(" === COOKIES SENT: {}", result);
         match result.len() {

components/net/http_loader.rs

@@ -2,6 +2,7 @@
  * 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_storage::CookieSource;
 use resource_task::{Metadata, TargetedLoadResponse, LoadData, start_sending_opt, ResponseSenders};
 use resource_task::ControlMsg;
 use resource_task::ProgressMsg::{Payload, Done};
@@ -108,7 +109,7 @@ reason: \"certificate verify failed\" }]";
         };
 
         let (tx, rx) = channel();
-        cookies_chan.send(ControlMsg::GetCookiesForUrl(url.clone(), tx));
+        cookies_chan.send(ControlMsg::GetCookiesForUrl(url.clone(), tx, CookieSource::HTTP));
         if let Some(cookies) = rx.recv().unwrap() {
             let mut v = Vec::new();
             v.push(cookies.into_bytes());
@@ -189,7 +190,8 @@ reason: \"certificate verify failed\" }]";
         }
 
         if let Some(&SetCookie(ref cookies)) = response.headers.get::<SetCookie>() {
-            cookies_chan.send(ControlMsg::SetCookies(cookies.clone(), url.clone()));
+            cookies_chan.send(ControlMsg::SetCookies(cookies.clone(), url.clone(),
+                                                     CookieSource::HTTP));
         }
 
         if response.status.class() == StatusClass::Redirection {
@@ -219,6 +221,8 @@ reason: \"certificate verify failed\" }]";
                     info!("redirecting to {}", new_url);
                     url = new_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 load_data.method == Method::Post &&
                         (response.status == StatusCode::MovedPermanently ||
                          response.status == StatusCode::Found) {

components/net/image_cache_task.rs

@@ -555,13 +555,6 @@ mod tests {
     }
 
     fn mock_resource_task<T: Closure+Send>(on_load: Box<T>) -> ResourceTask {
-        let cookie_chan = spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
-            loop {
-                match port.recv() {
-                    _ => { continue }
-                }
-            }
-        });
         spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
             loop {
                 match port.recv().unwrap() {
@@ -572,11 +565,11 @@ mod tests {
                             eventual_consumer: response.consumer.clone(),
                         };
                         let chan = start_sending(senders, Metadata::default(
-                            Url::parse("file:///fake", cookie_chan.clone()).unwrap()));
+                            Url::parse("file:///fake").unwrap()));
                         on_load.invoke(chan);
                     }
-                    resource_task::ControlMsg::Cookies(_) => {}
-                    resource_task::ControlMsg::Exit => break
+                    resource_task::ControlMsg::Exit => break,
+                    _ => {}
                 }
             }
         })
@@ -718,13 +711,6 @@ mod tests {
         let (image_bin_sent_chan, image_bin_sent) = channel();
 
         let (resource_task_exited_chan, resource_task_exited) = channel();
-        let cookie_chan = spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
-            loop {
-                match port.recv() {
-                    _ => { continue }
-                }
-            }
-        });
 
         let mock_resource_task = spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
             loop {
@@ -736,16 +722,16 @@ mod tests {
                             eventual_consumer: response.consumer.clone(),
                         };
                         let chan = start_sending(senders, Metadata::default(
-                            Url::parse("file:///fake").unwrap()), cookie_chan.clone());
+                            Url::parse("file:///fake").unwrap()));
                         chan.send(Payload(test_image_bin()));
                         chan.send(Done(Ok(())));
                         image_bin_sent_chan.send(());
                     }
-                    resource_task::ControlMsg::Cookies(_) => {}
                     resource_task::ControlMsg::Exit => {
                         resource_task_exited_chan.send(());
                         break
                     }
+                    _ => {}
                 }
             }
         });
@@ -778,13 +764,6 @@ mod tests {
         let (image_bin_sent_chan, image_bin_sent) = channel();
 
         let (resource_task_exited_chan, resource_task_exited) = channel();
-        let cookie_chan = spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
-            loop {
-                match port.recv() {
-                    _ => { continue }
-                }
-            }
-        });
         let mock_resource_task = spawn_listener(move |port: Receiver<resource_task::ControlMsg>| {
             loop {
                 match port.recv().unwrap() {
@@ -795,16 +774,16 @@ mod tests {
                             eventual_consumer: response.consumer.clone(),
                         };
                         let chan = start_sending(senders, Metadata::default(
-                            Url::parse("file:///fake").unwrap()), cookie_chan.clone());
+                            Url::parse("file:///fake").unwrap()));
                         chan.send(Payload(test_image_bin()));
                         chan.send(Done(Err("".to_string())));
                         image_bin_sent_chan.send(());
                     }
-                    resource_task::ControlMsg::Cookies(_) => {}
                     resource_task::ControlMsg::Exit => {
                         resource_task_exited_chan.send(());
                         break
                     }
+                    _ => {}
                 }
             }
         });

components/net/resource_task.rs

@@ -11,7 +11,7 @@ use http_loader;
 use sniffer_task;
 use sniffer_task::SnifferTask;
 use cookie_rs::Cookie;
-use cookie_storage::CookieStorage;
+use cookie_storage::{CookieStorage, CookieSource};
 use cookie;
 
 use util::task::spawn_named;
@@ -31,9 +31,9 @@ pub enum ControlMsg {
     /// Request the data associated with a particular URL
     Load(LoadData),
     /// Store a set of cookies for a given originating URL
-    SetCookies(Vec<Cookie>, Url),
+    SetCookies(Vec<Cookie>, Url, CookieSource),
     /// Retrieve the stored cookies for a given URL
-    GetCookiesForUrl(Url, Sender<Option<String>>),
+    GetCookiesForUrl(Url, Sender<Option<String>>, CookieSource),
     Exit
 }
 
@@ -205,17 +205,18 @@ struct ResourceManager {
     user_agent: Option<String>,
     sniffer_task: SnifferTask,
     cookie_storage: CookieStorage,
-    cookies_chan: Sender<ControlMsg>,
+    resource_task: Sender<ControlMsg>,
 }
 
 impl ResourceManager {
-    fn new(from_client: Receiver<ControlMsg>, user_agent: Option<String>, sniffer_task: SnifferTask, cookies_chan: Sender<ControlMsg>) -> ResourceManager {
+    fn new(from_client: Receiver<ControlMsg>, user_agent: Option<String>, sniffer_task: SnifferTask,
+           resource_task: Sender<ControlMsg>) -> ResourceManager {
         ResourceManager {
             from_client: from_client,
             user_agent: user_agent,
             sniffer_task: sniffer_task,
             cookie_storage: CookieStorage::new(),
-            cookies_chan: cookies_chan,
+            resource_task: resource_task,
         }
     }
 }
@@ -228,19 +229,15 @@ impl ResourceManager {
               ControlMsg::Load(load_data) => {
                 self.load(load_data)
               }
-              ControlMsg::SetCookies(vector, request) => {
+              ControlMsg::SetCookies(vector, request, source) => {
                 for cookie in vector.into_iter() {
-                    if let Some(cookie) = cookie::Cookie::new_wrapped(cookie, &request) {
-                        if cookie.cookie.value.is_empty() {
-                            self.cookie_storage.remove(&cookie);
-                        } else {
-                            self.cookie_storage.push(cookie, &request);
+                    if let Some(cookie) = cookie::Cookie::new_wrapped(cookie, &request, source) {
+                        self.cookie_storage.push(cookie, source);
                     }
                 }
               }
-              }
-              ControlMsg::GetCookiesForUrl(url, consumer) => {
-                consumer.send(self.cookie_storage.cookies_for_url(url));
+              ControlMsg::GetCookiesForUrl(url, consumer, source) => {
+                consumer.send(self.cookie_storage.cookies_for_url(&url, source));
               }
               ControlMsg::Exit => {
                 break
@@ -266,7 +263,7 @@ impl ResourceManager {
 
         let loader = match load_data.url.scheme.as_slice() {
             "file" => from_factory(file_loader::factory),
-            "http" | "https" => http_loader::factory(self.cookies_chan.clone()),
+            "http" | "https" => http_loader::factory(self.resource_task.clone()),
             "data" => from_factory(data_loader::factory),
             "about" => from_factory(about_loader::factory),
             _ => {

ports/cef/Cargo.lock

@@ -153,6 +153,7 @@ name = "devtools_traits"
 version = "0.0.1"
 dependencies = [
  "msg 0.0.1",
+ "util 0.0.1",
 ]
 
 [[package]]

ports/gonk/Cargo.lock

@@ -124,6 +124,7 @@ name = "devtools_traits"
 version = "0.0.1"
 dependencies = [
  "msg 0.0.1",
+ "util 0.0.1",
 ]
 
 [[package]]