Use cookie_store crate instead of cookie::CookieJar (#169)

CookieJar doesn't support the path-match and domain-match algorithms from [RFC 6265](https://tools.ietf.org/html/rfc6265#section-5.1.3), while cookie_store does.

This fixes some issues with the cookie matching algorithm currently in ureq. For instance,
the domain-match uses substring matching rather than the RFC 6265 algorithm.

This deletes two tests:

match_cookies_returns_nothing_when_no_cookies didn't test much
agent_cookies was failing because cookie_store rejects cookies on the `test:` scheme.
  The way around this is to set up a testserver - but it turns out cookies_on_redirect already
  does that, and covers the same cases and more.

This changes some cookie-related behavior:

 - Cookies could previously be sent to a wrong domain - e.g. a cookie set on `example.com`
  could go to `example.com.evil.com` or `evilexample.com`. Probably no one was relying on
  this, since it's quite broken.
 - A cookie with a path of `/foo` could be sent on a request to `/foobar`, but now it can't.
 - Cookies could previously be set on IP addresses, but now they can't.
 - Cookies could previously be set for domains other than the one on the request (or its
  parents), but now they can't.
 - When a cookie had no domain attribute, it would previously get the domain from the
  request, and subsequently be sent to that domain and all subdomains. Now, it will only
  be sent to that exact domain (host-only).

That last one is probably the most likely to break people, since someone could depend
on it without realizing it was broken behavior.

Cargo.toml

@@ -20,7 +20,7 @@ json = ["serde", "serde_json"]
 charset = ["encoding"]
 tls = ["rustls", "webpki", "webpki-roots"]
 native-certs = ["rustls-native-certs"]
-cookies = ["cookie"]
+cookies = ["cookie", "cookie_store"]
 socks-proxy = ["socks"]
 
 [dependencies]
@@ -39,6 +39,7 @@ serde = { version = "1", optional = true }
 serde_json = { version = "1", optional = true }
 encoding = { version = "0.2", optional = true }
 native-tls = { version = "0.2", optional = true }
+cookie_store = { version = "0.12.0", optional = true }
 log = "0.4.11"
 
 [dev-dependencies]

src/agent.rs

@@ -1,7 +1,11 @@
 #[cfg(feature = "cookie")]
-use cookie::{Cookie, CookieJar};
+use cookie::Cookie;
+#[cfg(feature = "cookie")]
+use cookie_store::CookieStore;
 use std::sync::Arc;
 use std::sync::Mutex;
+#[cfg(feature = "cookie")]
+use url::Url;
 
 use crate::header::{self, Header};
 use crate::pool::ConnectionPool;
@@ -52,8 +56,9 @@ pub(crate) struct AgentState {
     /// Reused connections between requests.
     pub(crate) pool: ConnectionPool,
     /// Cookies saved between requests.
+    /// Invariant: All cookies must have a nonempty domain and path.
     #[cfg(feature = "cookie")]
-    pub(crate) jar: CookieJar,
+    pub(crate) jar: CookieStore,
     pub(crate) resolver: ArcResolver,
 }
 
@@ -219,6 +224,9 @@ impl Agent {
     /// either by setting it in the agent, or by making requests
     /// that `Set-Cookie` in the agent.
     ///
+    /// Note that this will return any cookie for the given name,
+    /// regardless of which host and path that cookie was set on.
+    ///
     /// ```
     /// let agent = ureq::agent();
     ///
@@ -229,21 +237,53 @@ impl Agent {
     #[cfg(feature = "cookie")]
     pub fn cookie(&self, name: &str) -> Option<Cookie<'static>> {
         let state = self.state.lock().unwrap();
-        state.jar.get(name).cloned()
+        let first_found = state.jar.iter_any().find(|c| c.name() == name);
+        if let Some(first_found) = first_found {
+            let c: &Cookie = &*first_found;
+            Some(c.clone())
+        } else {
+            None
+        }
     }
 
     /// Set a cookie in this agent.
     ///
+    /// Cookies without a domain, or with a malformed domain or path,
+    /// will be silently ignored.
+    ///
     /// ```
     /// let agent = ureq::agent();
     ///
-    /// let cookie = ureq::Cookie::new("name", "value");
+    /// let cookie = ureq::Cookie::build("name", "value")
+    ///   .domain("example.com")
+    ///   .path("/")
+    ///   .secure(true)
+    ///   .finish();
     /// agent.set_cookie(cookie);
     /// ```
     #[cfg(feature = "cookie")]
     pub fn set_cookie(&self, cookie: Cookie<'static>) {
+        let mut cookie = cookie.clone();
+        if cookie.domain().is_none() {
+            return;
+        }
+
+        if cookie.path().is_none() {
+            cookie.set_path("/");
+        }
+        let path = cookie.path().unwrap();
+        let domain = cookie.domain().unwrap();
+
+        let fake_url: Url = match format!("http://{}{}", domain, path).parse() {
+            Ok(u) => u,
+            Err(_) => return,
+        };
         let mut state = self.state.lock().unwrap();
-        state.jar.add_original(cookie);
+        let cs_cookie = match cookie_store::Cookie::try_from_raw_cookie(&cookie, &fake_url) {
+            Ok(c) => c,
+            Err(_) => return,
+        };
+        state.jar.insert(cs_cookie, &fake_url).ok();
     }
 
     /// Make a GET request from this agent.

src/test/agent_test.rs

@@ -31,34 +31,6 @@ fn agent_reuse_headers() {
     assert_eq!(resp.header("X-Call").unwrap(), "2");
 }
 
-#[cfg(feature = "cookie")]
-#[test]
-fn agent_cookies() {
-    let agent = agent();
-
-    test::set_handler("/agent_cookies", |_unit| {
-        test::make_response(
-            200,
-            "OK",
-            vec!["Set-Cookie: foo=bar%20baz; Path=/; HttpOnly"],
-            vec![],
-        )
-    });
-
-    agent.get("test://host/agent_cookies").call();
-
-    assert!(agent.cookie("foo").is_some());
-    assert_eq!(agent.cookie("foo").unwrap().value(), "bar baz");
-
-    test::set_handler("/agent_cookies", |unit| {
-        assert!(unit.has("cookie"));
-        assert_eq!(unit.header("cookie").unwrap(), "foo=bar%20baz");
-        test::make_response(200, "OK", vec![], vec![])
-    });
-
-    agent.get("test://host/agent_cookies").call();
-}
-
 // Handler that answers with a simple HTTP response, and times
 // out idle connections after 2 seconds.
 fn idle_timeout_handler(mut stream: TcpStream) -> io::Result<()> {

src/unit.rs

@@ -6,8 +6,9 @@ use qstring::QString;
 use url::Url;
 
 #[cfg(feature = "cookie")]
-use cookie::{Cookie, CookieJar};
+use cookie::Cookie;
 
+#[cfg(feature = "cookie")]
 use crate::agent::AgentState;
 use crate::body::{self, BodySize, Payload, SizedReader};
 use crate::header;
@@ -49,10 +50,6 @@ impl Unit {
 
         let query_string = combine_query(&url, &req.query, mix_queries);
 
-        let cookie_header: Option<Header> = url
-            .host_str()
-            .and_then(|host_str| extract_cookies(&req.agent, &url.scheme(), host_str, &url.path()));
-
         let extra_headers = {
             let mut extra = vec![];
 
@@ -84,12 +81,15 @@ impl Unit {
                 extra.push(Header::new("Authorization", &format!("Basic {}", encoded)));
             }
 
+            #[cfg(feature = "cookie")]
+            extra.extend(extract_cookies(&req.agent, &url).into_iter());
+
             extra
         };
+
         let headers: Vec<_> = req
             .headers
             .iter()
-            .chain(cookie_header.iter())
             .chain(extra_headers.iter())
             .cloned()
             .collect();
@@ -198,6 +198,7 @@ pub(crate) fn connect(
     }
 
     // squirrel away cookies
+    #[cfg(feature = "cookie")]
     save_cookies(&unit, &resp);
 
     // handle redirects
@@ -250,62 +251,18 @@ pub(crate) fn connect(
 }
 
 #[cfg(feature = "cookie")]
-fn extract_cookies(
-    state: &std::sync::Mutex<AgentState>,
-    scheme: &str,
-    host: &str,
-    path: &str,
-) -> Option<Header> {
+fn extract_cookies(state: &std::sync::Mutex<AgentState>, url: &Url) -> Option<Header> {
     let state = state.lock().unwrap();
-    let is_secure = scheme.eq_ignore_ascii_case("https");
-
-    match_cookies(&state.jar, host, path, is_secure)
-}
-
-#[cfg(not(feature = "cookie"))]
-fn extract_cookies(
-    _state: &std::sync::Mutex<AgentState>,
-    _scheme: &str,
-    _host: &str,
-    _path: &str,
-) -> Option<Header> {
-    None
-}
-
-// TODO check so cookies can't be set for tld:s
-#[cfg(feature = "cookie")]
-fn match_cookies(jar: &CookieJar, domain: &str, path: &str, is_secure: bool) -> Option<Header> {
-    Some(
-        jar.iter()
-            .filter(|c| {
-                // if there is a domain, it must be matched.
-                // if there is no domain, then ignore cookie
-                let domain_ok = c
-                    .domain()
-                    .map(|cdom| domain.contains(cdom))
-                    .unwrap_or(false);
-                // a path must match the beginning of request path.
-                // no cookie path, we say is ok. is it?!
-                let path_ok = c
-                    .path()
-                    .map(|cpath| path.find(cpath).map(|pos| pos == 0).unwrap_or(false))
-                    .unwrap_or(true);
-                // either the cookie isnt secure, or we're not doing a secure request.
-                let secure_ok = !c.secure().unwrap_or(false) || is_secure;
-
-                domain_ok && path_ok && secure_ok
-            })
-            .map(|c| {
-                let name = c.name().to_string();
-                let value = c.value().to_string();
-                let nameval = Cookie::new(name, value).encoded().to_string();
-                nameval
-            })
-            .collect::<Vec<_>>()
-            .join(";"),
-    )
-    .filter(|x| !x.is_empty())
-    .map(|s| Header::new("Cookie", &s))
+    let header_value = state
+        .jar
+        .get_request_cookies(url)
+        .map(|c| Cookie::new(c.name(), c.value()).encoded().to_string())
+        .collect::<Vec<_>>()
+        .join(";");
+    match header_value.as_str() {
+        "" => None,
+        val => Some(Header::new("Cookie", val)),
+    }
 }
 
 /// Combine the query of the url and the query options set on the request object.
@@ -410,64 +367,50 @@ fn send_prelude(unit: &Unit, stream: &mut Stream, redir: bool) -> io::Result<()>
     Ok(())
 }
 
-#[cfg(not(feature = "cookie"))]
-fn save_cookies(_unit: &Unit, _resp: &Response) {}
-
 /// Investigate a response for "Set-Cookie" headers.
 #[cfg(feature = "cookie")]
 fn save_cookies(unit: &Unit, resp: &Response) {
     //
 
-    let cookies = resp.all("set-cookie");
-    if cookies.is_empty() {
+    let headers = resp.all("set-cookie");
+    // Avoid locking if there are no cookie headers
+    if headers.is_empty() {
         return;
     }
-
-    // only lock if we know there is something to process
-    let state = &mut unit.req.agent.lock().unwrap();
-    for raw_cookie in cookies.iter() {
-        let to_parse = if raw_cookie.to_lowercase().contains("domain=") {
-            (*raw_cookie).to_string()
-        } else {
-            let host = &unit.url.host_str().unwrap().to_string();
-            format!("{}; Domain={}", raw_cookie, host)
-        };
-        match Cookie::parse_encoded(&to_parse[..]) {
-            Err(_) => (), // ignore unparseable cookies
-            Ok(cookie) => {
-                let cookie = cookie.into_owned();
-                state.jar.add(cookie)
-            }
+    let cookies = headers.into_iter().flat_map(|header_value| {
+        match Cookie::parse(header_value.to_string()) {
+            Err(_) => None,
+            Ok(c) => Some(c),
         }
-    }
+    });
+    let state = &mut unit.req.agent.lock().unwrap();
+    state.jar.store_response_cookies(cookies, &unit.url.clone());
 }
 
 #[cfg(test)]
 #[cfg(feature = "cookies")]
 mod tests {
     use super::*;
 
+    use crate::Agent;
     ///////////////////// COOKIE TESTS //////////////////////////////
 
-    #[test]
-    fn match_cookies_returns_nothing_when_no_cookies() {
-        let jar = CookieJar::new();
-
-        let result = match_cookies(&jar, "crates.io", "/", false);
-        assert_eq!(result, None);
-    }
-
     #[test]
     fn match_cookies_returns_one_header() {
-        let mut jar = CookieJar::new();
-        let cookie1 = Cookie::parse("cookie1=value1; Domain=crates.io").unwrap();
-        let cookie2 = Cookie::parse("cookie2=value2; Domain=crates.io").unwrap();
-        jar.add(cookie1);
-        jar.add(cookie2);
+        let agent = Agent::default();
+        let url: Url = "https://crates.io/".parse().unwrap();
+        let cookie1: Cookie = "cookie1=value1; Domain=crates.io; Path=/".parse().unwrap();
+        let cookie2: Cookie = "cookie2=value2; Domain=crates.io; Path=/".parse().unwrap();
+        agent
+            .state
+            .lock()
+            .unwrap()
+            .jar
+            .store_response_cookies(vec![cookie1, cookie2].into_iter(), &url);
 
         // There's no guarantee to the order in which cookies are defined.
         // Ensure that they're either in one order or the other.
-        let result = match_cookies(&jar, "crates.io", "/", false);
+        let result = extract_cookies(&agent.state, &url);
         let order1 = "cookie1=value1;cookie2=value2";
         let order2 = "cookie2=value2;cookie1=value1";