Repository: algesten/ureq
Pull request #177 (2020-10-04 17:17:58)

Remove convenience methods for CONNECT verb.
CONNECT, specified at https://tools.ietf.org/html/rfc7231#section-4.3.6,
says:

> if successful, thereafter restrict its behavior to blind forwarding
> of packets, in both directions, until the tunnel is closed.

ureq doesn't actually support the semantics of CONNECT, since it doesn't
offer a bidirectional channel on a Response. So I'm fairly confident no
one is using these methods.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9779d63..175fa46 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 1.5.1
+ * Use cookie_store crate for correct cookie handling (#169).
+ * Fix bug in picking wrong host for redirects introduced in 1.5.0 (#180).
+ * Allow proxy settings on Agent (#178).
+
 # 1.5.0
  * Add pluggable name resolution. Users can now override the IP addresses for
    hostnames of their choice (#148).
diff --git a/Cargo.toml b/Cargo.toml
index d71fb83..ac9318a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "ureq"
-version = "1.5.0"
+version = "1.5.1"
 authors = ["Martin Algesten <martin@algesten.se>"]
 description = "Minimal HTTP request library"
 license = "MIT/Apache-2.0"
@@ -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]
diff --git a/src/agent.rs b/src/agent.rs
index 1aa5929..554d4db 100644
--- a/src/agent.rs
+++ b/src/agent.rs
@@ -1,10 +1,15 @@
 #[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;
+use crate::proxy::Proxy;
 use crate::request::Request;
 use crate::resolve::ArcResolver;
 
@@ -51,9 +56,11 @@ pub struct Agent {
 pub(crate) struct AgentState {
     /// Reused connections between requests.
     pub(crate) pool: ConnectionPool,
+    pub(crate) proxy: Option<Proxy>,
     /// 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,
 }
 
@@ -215,10 +222,29 @@ impl Agent {
         self
     }
 
+    /// Set the proxy server to use for all connections from this Agent.
+    ///
+    /// Example:
+    /// ```
+    /// let proxy = ureq::Proxy::new("user:password@cool.proxy:9090").unwrap();
+    /// let agent = ureq::agent()
+    ///     .set_proxy(proxy)
+    ///     .build();
+    /// ```
+    pub fn set_proxy(&mut self, proxy: Proxy) -> &mut Agent {
+        let mut state = self.state.lock().unwrap();
+        state.proxy = Some(proxy);
+        drop(state);
+        self
+    }
+
     /// Gets a cookie in this agent by name. Cookies are available
     /// 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 +255,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.
@@ -281,11 +339,6 @@ impl Agent {
         self.request("OPTIONS", path)
     }
 
-    /// Make a CONNECT request from this agent.
-    pub fn connect(&self, path: &str) -> Request {
-        self.request("CONNECT", path)
-    }
-
     /// Make a PATCH request from this agent.
     pub fn patch(&self, path: &str) -> Request {
         self.request("PATCH", path)
diff --git a/src/lib.rs b/src/lib.rs
index 829657e..54092d5 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -199,11 +199,6 @@ pub fn options(path: &str) -> Request {
     request("OPTIONS", path)
 }
 
-/// Make an CONNECT request.
-pub fn connect(path: &str) -> Request {
-    request("CONNECT", path)
-}
-
 /// Make an PATCH request.
 pub fn patch(path: &str) -> Request {
     request("PATCH", path)
diff --git a/src/request.rs b/src/request.rs
index 85bce0c..daee345 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -12,6 +12,7 @@ use crate::body::BodySize;
 use crate::body::{Payload, SizedReader};
 use crate::error::Error;
 use crate::header::{self, Header};
+use crate::proxy::Proxy;
 use crate::unit::{self, Unit};
 use crate::Response;
 
@@ -43,7 +44,7 @@ pub struct Request {
     pub(crate) timeout_write: u64,
     pub(crate) timeout: Option<time::Duration>,
     pub(crate) redirects: u32,
-    pub(crate) proxy: Option<crate::proxy::Proxy>,
+    pub(crate) proxy: Option<Proxy>,
     #[cfg(feature = "tls")]
     pub(crate) tls_config: Option<TLSClientConfig>,
     #[cfg(all(feature = "native-tls", not(feature = "tls")))]
@@ -559,12 +560,22 @@ impl Request {
     ///     .set_proxy(proxy)
     ///     .build();
     /// ```
-    pub fn set_proxy(&mut self, proxy: crate::proxy::Proxy) -> &mut Request {
+    pub fn set_proxy(&mut self, proxy: Proxy) -> &mut Request {
         self.proxy = Some(proxy);
         self
     }
 
-    /// Set the TLS client config to use for the connection.
+    pub(crate) fn proxy(&self) -> Option<Proxy> {
+        if let Some(proxy) = &self.proxy {
+            Some(proxy.clone())
+        } else if let Some(proxy) = &self.agent.lock().unwrap().proxy {
+            Some(proxy.clone())
+        } else {
+            None
+        }
+    }
+
+    /// Set the TLS client config to use for the connection. See [`ClientConfig`](https://docs.rs/rustls/latest/rustls/struct.ClientConfig.html).
     ///
     /// See [`ClientConfig`](https://docs.rs/rustls/latest/rustls/struct.ClientConfig.html).
     ///
diff --git a/src/stream.rs b/src/stream.rs
index 84ac940..f14a3ae 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -384,7 +384,8 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
     } else {
         unit.deadline
     };
-    let netloc = match unit.req.proxy {
+    let proxy: Option<Proxy> = unit.req.proxy();
+    let netloc = match proxy {
         Some(ref proxy) => format!("{}:{}", proxy.server, proxy.port),
         None => format!("{}:{}", hostname, port),
     };
@@ -399,7 +400,7 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
         return Err(Error::DnsFailed(format!("No ip address for {}", hostname)));
     }
 
-    let proto = if let Some(ref proxy) = unit.req.proxy {
+    let proto = if let Some(ref proxy) = proxy {
         Some(proxy.proto)
     } else {
         None
@@ -419,7 +420,7 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
         let stream = if Some(Proto::SOCKS5) == proto {
             connect_socks5(
                 &unit,
-                unit.req.proxy.to_owned().unwrap(),
+                proxy.clone().unwrap(),
                 deadline,
                 sock_addr,
                 hostname,
@@ -473,7 +474,7 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
     }
 
     if proto == Some(Proto::HTTPConnect) {
-        if let Some(ref proxy) = unit.req.proxy {
+        if let Some(ref proxy) = proxy {
             write!(stream, "{}", proxy.connect(hostname, port)).unwrap();
             stream.flush()?;
 
diff --git a/src/unit.rs b/src/unit.rs
index 94953d6..3ec4794 100644
--- a/src/unit.rs
+++ b/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();
@@ -145,7 +145,10 @@ pub(crate) fn connect(
 ) -> Result<Response, Error> {
     //
 
-    let host = req.get_host()?;
+    let host = unit
+        .url
+        .host_str()
+        .ok_or(Error::BadUrl("no host".to_string()))?;
     let url = &unit.url;
     let method = &unit.req.method;
     // open socket
@@ -198,6 +201,7 @@ pub(crate) fn connect(
     }
 
     // squirrel away cookies
+    #[cfg(feature = "cookie")]
     save_cookies(&unit, &resp);
 
     // handle redirects
@@ -250,62 +254,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.
@@ -404,42 +364,31 @@ fn send_prelude(unit: &Unit, stream: &mut Stream, redir: bool) -> io::Result<()>
     // finish
     write!(prelude, "\r\n")?;
 
+    debug!("writing prelude: {}", String::from_utf8_lossy(&prelude));
     // write all to the wire
     stream.write_all(&prelude[..])?;
 
     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)]
@@ -447,27 +396,25 @@ fn save_cookies(unit: &Unit, resp: &Response) {
 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";
 


Test patch:
diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs
index 97568b3..3d83175 100644
--- a/src/test/agent_test.rs
+++ b/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<()> {
diff --git a/src/test/redirect.rs b/src/test/redirect.rs
index 03639e2..5ec1a41 100644
--- a/src/test/redirect.rs
+++ b/src/test/redirect.rs
@@ -1,3 +1,9 @@
+use std::{
+    io::{self, Write},
+    net::TcpStream,
+};
+use test::testserver::{self, TestServer};
+
 use crate::test;
 
 use super::super::*;
@@ -76,6 +82,25 @@ fn redirect_get() {
     assert_eq!(resp.header("x-foo").unwrap(), "bar");
 }
 
+#[test]
+fn redirect_host() {
+    // Set up a redirect to a host that doesn't exist; it should fail.
+    let srv = TestServer::new(|mut stream: TcpStream| -> io::Result<()> {
+        testserver::read_headers(&stream);
+        write!(stream, "HTTP/1.1 302 Found\r\n")?;
+        write!(stream, "Location: http://example.invalid/\r\n")?;
+        write!(stream, "\r\n")?;
+        Ok(())
+    });
+    let url = format!("http://localhost:{}/", srv.port);
+    let resp = crate::get(&url).call();
+    assert!(
+        matches!(resp.synthetic_error(), Some(Error::DnsFailed(_))),
+        "{:?}",
+        resp.synthetic_error()
+    );
+}
+
 #[test]
 fn redirect_post() {
     test::set_handler("/redirect_post1", |_| {