Repository: algesten/ureq
Pull request #351 (2021-03-24 03:52:21)

Add size limits on responses
This prevents a malicious server from running the client out of memory
by returning a very large body.

Limit into_string() to 1 megabyte.
Limit individual header fields to 100 kilobytes.
Limit number of header fields to 100.

Part of #267


Patch:
diff --git a/Cargo.toml b/Cargo.toml
index b755086..fe3f255 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -26,7 +26,7 @@ socks-proxy = ["socks"]
 [dependencies]
 base64 = "0.13"
 chunked_transfer = "1.2.0"
-cookie = { version = "0.15", features = ["percent-encode"], optional = true}
+cookie = { version = "0.15", deafult-features = false, optional = true}
 once_cell = "1"
 url = "2"
 socks = { version = "0.3.2", optional = true }
diff --git a/src/error.rs b/src/error.rs
index 881a04c..0fd37b6 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -288,6 +288,8 @@ pub enum ErrorKind {
 }
 
 impl ErrorKind {
+    #[allow(clippy::wrong_self_convention)]
+    #[allow(clippy::new_ret_no_self)]
     pub(crate) fn new(self) -> Error {
         Error::new(self, None)
     }
diff --git a/src/header.rs b/src/header.rs
index 342f1c1..6a2116d 100644
--- a/src/header.rs
+++ b/src/header.rs
@@ -190,7 +190,7 @@ fn valid_name(name: &[u8]) -> bool {
 }
 
 #[inline]
-fn is_tchar(b: &u8) -> bool {
+pub(crate) fn is_tchar(b: &u8) -> bool {
     match b {
         b'!' | b'#' | b'$' | b'%' | b'&' => true,
         b'\'' | b'*' | b'+' | b'-' | b'.' => true,
diff --git a/src/lib.rs b/src/lib.rs
index 2ef48a1..850121f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,5 +1,10 @@
 #![forbid(unsafe_code)]
 #![warn(clippy::all)]
+// new is just more readable than ..Default::default().
+#![allow(clippy::new_without_default)]
+// the matches! macro is obscure and not widely known.
+#![allow(clippy::match_like_matches_macro)]
+
 //! A simple, safe HTTP client.
 //!
 //! Ureq's first priority is being easy for you to use. It's great for
@@ -296,7 +301,7 @@ pub use crate::agent::AgentBuilder;
 pub use crate::error::{Error, ErrorKind, OrAnyStatus, Transport};
 pub use crate::header::Header;
 pub use crate::proxy::Proxy;
-pub use crate::request::Request;
+pub use crate::request::{Request, RequestUrl};
 pub use crate::resolve::Resolver;
 pub use crate::response::Response;
 
@@ -327,8 +332,7 @@ pub fn is_test(is: bool) -> bool {
     if is {
         IS_TEST.store(true, Ordering::SeqCst);
     }
-    let x = IS_TEST.load(Ordering::SeqCst);
-    x
+    IS_TEST.load(Ordering::SeqCst)
 }
 
 /// Agents are used to hold configuration and keep state between requests.
diff --git a/src/proxy.rs b/src/proxy.rs
index 0aed467..36a4086 100644
--- a/src/proxy.rs
+++ b/src/proxy.rs
@@ -46,7 +46,9 @@ impl Proxy {
         match host {
             Some(host) => {
                 let mut parts = host.as_ref().split(':').collect::<Vec<&str>>().into_iter();
-                let host = parts.next().ok_or(ErrorKind::InvalidProxyUrl.new())?;
+                let host = parts
+                    .next()
+                    .ok_or_else(|| ErrorKind::InvalidProxyUrl.new())?;
                 let port = parts.next();
                 Ok((
                     String::from(host),
@@ -155,11 +157,11 @@ Proxy-Connection: Keep-Alive\r\n\
         let top_line = response_string
             .lines()
             .next()
-            .ok_or(ErrorKind::ProxyConnect.new())?;
+            .ok_or_else(|| ErrorKind::ProxyConnect.new())?;
         let status_code = top_line
             .split_whitespace()
             .nth(1)
-            .ok_or(ErrorKind::ProxyConnect.new())?;
+            .ok_or_else(|| ErrorKind::ProxyConnect.new())?;
 
         match status_code {
             "200" => Ok(()),
diff --git a/src/request.rs b/src/request.rs
index 0113990..79de5c3 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -1,7 +1,7 @@
 use std::io::Read;
 use std::{fmt, time};
 
-use url::{form_urlencoded, Url};
+use url::{form_urlencoded, ParseError, Url};
 
 use crate::body::Payload;
 use crate::header::{self, Header};
@@ -14,31 +14,6 @@ use super::SerdeValue;
 
 pub type Result<T> = std::result::Result<T, Error>;
 
-#[derive(Clone)]
-struct ParsedUrl(std::result::Result<Url, url::ParseError>);
-
-impl fmt::Display for ParsedUrl {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        if let Ok(url) = &self.0 {
-            write!(f, "{}", url.as_str())
-        } else {
-            write!(f, "{:?}", self.0)
-        }
-    }
-}
-
-impl From<String> for ParsedUrl {
-    fn from(s: String) -> Self {
-        ParsedUrl(s.parse())
-    }
-}
-
-impl From<Url> for ParsedUrl {
-    fn from(url: Url) -> Self {
-        ParsedUrl(Ok(url))
-    }
-}
-
 /// Request instances are builders that creates a request.
 ///
 /// ```
@@ -54,7 +29,7 @@ impl From<Url> for ParsedUrl {
 pub struct Request {
     agent: Agent,
     method: String,
-    parsed_url: ParsedUrl,
+    url: String,
     error_on_non_2xx: bool,
     headers: Vec<Header>,
     timeout: Option<time::Duration>,
@@ -65,25 +40,25 @@ impl fmt::Debug for Request {
         write!(
             f,
             "Request({} {}, {:?})",
-            self.method, self.parsed_url, self.headers
+            self.method, self.url, self.headers
         )
     }
 }
 
 impl Request {
     pub(crate) fn new(agent: Agent, method: String, url: String) -> Request {
-        Self::_new(agent, method, url.into())
+        Self::_new(agent, method, url)
     }
 
     pub(crate) fn with_url(agent: Agent, method: String, url: Url) -> Request {
-        Self::_new(agent, method, url.into())
+        Self::_new(agent, method, url.to_string())
     }
 
-    fn _new(agent: Agent, method: String, parsed_url: ParsedUrl) -> Request {
+    fn _new(agent: Agent, method: String, url: String) -> Request {
         Request {
             agent,
             method,
-            parsed_url,
+            url,
             headers: vec![],
             error_on_non_2xx: true,
             timeout: None,
@@ -114,11 +89,22 @@ impl Request {
         self.do_call(Payload::Empty)
     }
 
+    fn parse_url(&self) -> Result<Url> {
+        Ok(self.url.parse().and_then(|url: Url|
+            // No hostname is fine for urls in general, but not for website urls.
+            if url.host_str().is_none() {
+                Err(ParseError::EmptyHost)
+            } else {
+                Ok(url)
+            }
+        )?)
+    }
+
     fn do_call(self, payload: Payload) -> Result<Response> {
         for h in &self.headers {
             h.validate()?;
         }
-        let url = self.parsed_url.0?;
+        let url = self.parse_url()?;
 
         let deadline = match self.timeout.or(self.agent.config.timeout) {
             None => None,
@@ -351,8 +337,11 @@ impl Request {
     /// # }
     /// ```
     pub fn query(mut self, param: &str, value: &str) -> Self {
-        if let Ok(url) = &mut self.parsed_url.0 {
+        if let Ok(mut url) = self.parse_url() {
             url.query_pairs_mut().append_pair(param, value);
+
+            // replace url
+            self.url = url.to_string();
         }
         self
     }
@@ -368,6 +357,115 @@ impl Request {
         &self.method
     }
 
+    /// Get the url str that will be used for this request.
+    ///
+    /// The url might differ from that originally provided when constructing the
+    /// request if additional query parameters have been added using [`Request::query()`].
+    ///
+    /// In case the original url provided to build the request is not possible to
+    /// parse to a Url, this function returns the original, and it will error once the
+    /// Request object is used.
+    ///
+    /// ```
+    /// # fn main() -> Result<(), ureq::Error> {
+    /// # ureq::is_test(true);
+    /// let req = ureq::get("http://httpbin.org/get")
+    ///     .query("foo", "bar");
+    ///
+    /// assert_eq!(req.url(), "http://httpbin.org/get?foo=bar");
+    /// # Ok(())
+    /// # }
+    /// ```
+    ///
+    /// ```
+    /// # fn main() -> Result<(), ureq::Error> {
+    /// # ureq::is_test(true);
+    /// let req = ureq::get("SO WRONG")
+    ///     .query("foo", "bar"); // does nothing
+    ///
+    /// assert_eq!(req.url(), "SO WRONG");
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn url(&self) -> &str {
+        &self.url
+    }
+
+    /// Get the parsed url that will be used for this request. The parsed url
+    /// has functions to inspect the parts of the url further.
+    ///
+    /// The url might differ from that originally provided when constructing the
+    /// request if additional query parameters have been added using [`Request::query()`].
+    ///
+    /// Returns a `Result` since a common use case is to construct
+    /// the [`Request`] using a `&str` in which case the url needs to be parsed
+    /// to inspect the parts. If the Request url is not possible to parse, this
+    /// function produces the same error that would otherwise happen when
+    /// `call` or `send_*` is called.
+    ///
+    /// ```
+    /// # fn main() -> Result<(), ureq::Error> {
+    /// # ureq::is_test(true);
+    /// let req = ureq::get("http://httpbin.org/get")
+    ///     .query("foo", "bar");
+    ///
+    /// assert_eq!(req.request_url().unwrap().host(), "httpbin.org");
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn request_url(&self) -> Result<RequestUrl> {
+        Ok(RequestUrl::new(self.parse_url()?))
+    }
+}
+
+/// Parsed result of a request url with handy inspection methods.
+#[derive(Debug, Clone)]
+pub struct RequestUrl {
+    url: Url,
+    query_pairs: Vec<(String, String)>,
+}
+
+impl RequestUrl {
+    fn new(url: Url) -> Self {
+        // This is needed to avoid url::Url Cow<str>. We want ureq API to work with &str.
+        let query_pairs = url
+            .query_pairs()
+            .map(|(k, v)| (k.to_string(), v.to_string()))
+            .collect();
+
+        RequestUrl { url, query_pairs }
+    }
+
+    /// Handle the request url as a standard [`url::Url`].
+    pub fn as_url(&self) -> &Url {
+        &self.url
+    }
+
+    /// Get the scheme of the request url, i.e. "https" or "http".
+    pub fn scheme(&self) -> &str {
+        self.url.scheme()
+    }
+
+    /// Host of the request url.
+    pub fn host(&self) -> &str {
+        // this unwrap() is ok, because RequestUrl is tested for empty host
+        // urls in Request::parse_url().
+        self.url.host_str().unwrap()
+    }
+
+    /// Port of the request url, if available. Ports are only available if they
+    /// are present in the original url. Specifically the scheme default ports,
+    /// 443 for `https` and and 80 for `http` are `None` unless explicitly
+    /// set in the url, i.e. `https://my-host.com:443/some/path`.
+    pub fn port(&self) -> Option<u16> {
+        self.url.port()
+    }
+
+    /// Path of the request url.
+    pub fn path(&self) -> &str {
+        self.url.path()
+    }
+
     /// Returns all query parameters as a vector of key-value pairs.
     ///
     /// ```
@@ -377,23 +475,18 @@ impl Request {
     ///     .query("foo", "42")
     ///     .query("foo", "43");
     ///
-    /// assert_eq!(req.query_params(), vec![
-    ///     ("foo".to_string(), "42".to_string()),
-    ///     ("foo".to_string(), "43".to_string())
+    /// assert_eq!(req.request_url().unwrap().query_pairs(), vec![
+    ///     ("foo", "42"),
+    ///     ("foo", "43")
     /// ]);
     /// # Ok(())
     /// # }
     /// ```
-    pub fn query_params(&self) -> Vec<(String, String)> {
-        let mut ret = vec![];
-
-        if let Ok(url) = &self.parsed_url.0 {
-            for (k, v) in url.query_pairs() {
-                ret.push((k.into(), v.into()));
-            }
-        }
-
-        ret
+    pub fn query_pairs(&self) -> Vec<(&str, &str)> {
+        self.query_pairs
+            .iter()
+            .map(|(k, v)| (k.as_str(), v.as_str()))
+            .collect()
     }
 }
 
@@ -423,4 +516,17 @@ mod tests {
             .send(&bytes[1..2])
             .ok();
     }
+
+    #[test]
+    fn disallow_empty_host() {
+        let req = crate::agent().get("file:///some/path");
+
+        // Both request_url and call() must surface the same error.
+        assert_eq!(
+            req.request_url().unwrap_err().kind(),
+            crate::ErrorKind::InvalidUrl
+        );
+
+        assert_eq!(req.call().unwrap_err().kind(), crate::ErrorKind::InvalidUrl);
+    }
 }
diff --git a/src/response.rs b/src/response.rs
index fc6ca2f..9287627 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -20,6 +20,11 @@ use encoding_rs::Encoding;
 
 pub const DEFAULT_CONTENT_TYPE: &str = "text/plain";
 pub const DEFAULT_CHARACTER_SET: &str = "utf-8";
+const INTO_STRING_LIMIT: usize = 10 * 1_024 * 1_024;
+// Follow the example of curl and limit a single header to 100kB:
+// https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html
+const MAX_HEADER_SIZE: usize = 100 * 1_024;
+const MAX_HEADER_COUNT: usize = 100;
 
 /// Response instances are created as results of firing off requests.
 ///
@@ -211,6 +216,11 @@ impl Response {
     ///    length regardless of how many bytes the server sends.
     /// 3. If no length header, the reader is until server stream end.
     ///
+    /// Note: If you use `read_to_end()` on the resulting reader, a malicious
+    /// server might return enough bytes to exhaust available memroy. If you're
+    /// making requests to untrusted servers, you should use `.take()` to
+    /// limit the response bytes read.
+    ///
     /// Example:
     ///
     /// ```
@@ -226,6 +236,7 @@ impl Response {
     ///
     /// let mut bytes: Vec<u8> = Vec::with_capacity(len);
     /// resp.into_reader()
+    ///     .take(10_000_000)
     ///     .read_to_end(&mut bytes)?;
     ///
     /// assert_eq!(bytes.len(), len);
@@ -291,6 +302,8 @@ impl Response {
     /// implementation first reads the reader to end into a `Vec<u8>` and then
     /// attempts to decode it using the charset.
     ///
+    /// If the response is larger than 10 megabytes, this will return an error.
+    ///
     /// Example:
     ///
     /// ```
@@ -321,14 +334,30 @@ impl Response {
                 .or_else(|| Encoding::for_label(DEFAULT_CHARACTER_SET.as_bytes()))
                 .unwrap();
             let mut buf: Vec<u8> = vec![];
-            self.into_reader().read_to_end(&mut buf)?;
+            self.into_reader()
+                .take((INTO_STRING_LIMIT + 1) as u64)
+                .read_to_end(&mut buf)?;
+            if buf.len() > INTO_STRING_LIMIT {
+                return Err(io::Error::new(
+                    io::ErrorKind::Other,
+                    "response too big for into_string",
+                ));
+            }
             let (text, _, _) = encoding.decode(&buf);
             Ok(text.into_owned())
         }
         #[cfg(not(feature = "charset"))]
         {
             let mut buf: Vec<u8> = vec![];
-            self.into_reader().read_to_end(&mut buf)?;
+            self.into_reader()
+                .take((INTO_STRING_LIMIT + 1) as u64)
+                .read_to_end(&mut buf)?;
+            if buf.len() > INTO_STRING_LIMIT {
+                return Err(io::Error::new(
+                    io::ErrorKind::Other,
+                    "response too big for into_string",
+                ));
+            }
             Ok(String::from_utf8_lossy(&buf).to_string())
         }
     }
@@ -427,7 +456,7 @@ impl Response {
         let (index, status) = parse_status_line(status_line.as_str())?;
 
         let mut headers: Vec<Header> = Vec::new();
-        loop {
+        while headers.len() <= MAX_HEADER_COUNT {
             let line = read_next_line(&mut stream, "a header")?;
             if line.is_empty() {
                 break;
@@ -437,6 +466,12 @@ impl Response {
             }
         }
 
+        if headers.len() > MAX_HEADER_COUNT {
+            return Err(ErrorKind::BadHeader.msg(
+                format!("more than {} header fields in response", MAX_HEADER_COUNT).as_str(),
+            ));
+        }
+
         Ok(Response {
             url: None,
             status_line,
@@ -549,27 +584,33 @@ impl FromStr for Response {
 
 fn read_next_line(reader: &mut impl BufRead, context: &str) -> io::Result<HeaderLine> {
     let mut buf = Vec::new();
-    let result = reader.read_until(b'\n', &mut buf);
-
-    if let Err(e) = result {
-        // Provide context to errors encountered while reading the line.
-        let reason = format!("Error encountered in {}", context);
-
-        let kind = e.kind();
+    let result = reader
+        .take((MAX_HEADER_SIZE + 1) as u64)
+        .read_until(b'\n', &mut buf);
 
-        // Use an intermediate wrapper type which carries the error message
-        // as well as a .source() reference to the original error.
-        let wrapper = Error::new(ErrorKind::Io, Some(reason)).src(e);
-
-        return Err(io::Error::new(kind, wrapper));
-    }
-
-    if result? == 0 {
-        return Err(io::Error::new(
+    match result {
+        Ok(0) => Err(io::Error::new(
             io::ErrorKind::ConnectionAborted,
             "Unexpected EOF",
-        ));
-    }
+        )),
+        Ok(n) if n > MAX_HEADER_SIZE => Err(io::Error::new(
+            io::ErrorKind::Other,
+            format!("header field longer than {} bytes", MAX_HEADER_SIZE),
+        )),
+        Ok(_) => Ok(()),
+        Err(e) => {
+            // Provide context to errors encountered while reading the line.
+            let reason = format!("Error encountered in {}", context);
+
+            let kind = e.kind();
+
+            // Use an intermediate wrapper type which carries the error message
+            // as well as a .source() reference to the original error.
+            let wrapper = Error::new(ErrorKind::Io, Some(reason)).src(e);
+
+            Err(io::Error::new(kind, wrapper))
+        }
+    }?;
 
     if !buf.ends_with(b"\n") {
         return Err(io::Error::new(
@@ -672,6 +713,8 @@ impl Read for ErrorReader {
 
 #[cfg(test)]
 mod tests {
+    use std::io::Cursor;
+
     use super::*;
 
     #[test]
@@ -755,6 +798,25 @@ mod tests {
         assert_eq!("hello world!!!", resp.into_string().unwrap());
     }
 
+    #[test]
+    fn into_string_large() {
+        const LEN: usize = INTO_STRING_LIMIT + 1;
+        let s = format!(
+            "HTTP/1.1 200 OK\r\n\
+                 Content-Length: {}\r\n
+                 \r\n
+                 {}",
+            LEN,
+            "A".repeat(LEN),
+        );
+        let result = s.parse::<Response>().unwrap();
+        let err = result
+            .into_string()
+            .expect_err("didn't error with too-long body");
+        assert_eq!(err.to_string(), "response too big for into_string");
+        assert_eq!(err.kind(), io::ErrorKind::Other);
+    }
+
     #[test]
     #[cfg(feature = "json")]
     fn parse_simple_json() {
@@ -801,6 +863,43 @@ mod tests {
         assert_eq!(resp.status_text(), "");
     }
 
+    #[test]
+    fn read_next_line_large() {
+        const LEN: usize = MAX_HEADER_SIZE + 1;
+        let s = format!("Long-Header: {}\r\n", "A".repeat(LEN),);
+        let mut cursor = Cursor::new(s);
+        let result = read_next_line(&mut cursor, "some context");
+        let err = result.expect_err("did not error on too-large header");
+        assert_eq!(err.kind(), io::ErrorKind::Other);
+        assert_eq!(
+            err.to_string(),
+            format!("header field longer than {} bytes", MAX_HEADER_SIZE)
+        );
+    }
+
+    #[test]
+    fn too_many_headers() {
+        const LEN: usize = MAX_HEADER_COUNT + 1;
+        let s = format!(
+            "HTTP/1.1 200 OK\r\n\
+                 {}
+                 \r\n
+                 hi",
+            "Header: value\r\n".repeat(LEN),
+        );
+        let err = s
+            .parse::<Response>()
+            .expect_err("did not error on too many headers");
+        assert_eq!(err.kind(), ErrorKind::BadHeader);
+        assert_eq!(
+            err.to_string(),
+            format!(
+                "Bad Header: more than {} header fields in response",
+                MAX_HEADER_COUNT
+            )
+        );
+    }
+
     #[test]
     #[cfg(feature = "charset")]
     fn read_next_line_non_ascii_reason() {
diff --git a/src/stream.rs b/src/stream.rs
index 84d3933..5ed48ca 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -25,6 +25,7 @@ pub(crate) struct Stream {
     inner: BufReader<Inner>,
 }
 
+#[allow(clippy::large_enum_variant)]
 enum Inner {
     Http(TcpStream),
     #[cfg(feature = "tls")]
diff --git a/src/unit.rs b/src/unit.rs
index 99f63c7..a50d346 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -36,7 +36,7 @@ impl Unit {
         agent: &Agent,
         method: &str,
         url: &Url,
-        headers: &Vec<Header>,
+        headers: &[Header],
         body: &SizedReader,
         deadline: Option<time::Instant>,
     ) -> Self {
@@ -229,7 +229,8 @@ fn connect_inner(
     let host = unit
         .url
         .host_str()
-        .ok_or(ErrorKind::InvalidUrl.msg("no host in URL"))?;
+        // This unwrap is ok because Request::parse_url() ensure there is always a host present.
+        .unwrap();
     let url = &unit.url;
     let method = &unit.method;
     // open socket
@@ -301,7 +302,16 @@ fn extract_cookies(agent: &Agent, url: &Url) -> Option<Header> {
         .cookie_tin
         .get_request_cookies(url)
         .iter()
-        .map(|c| c.encoded().to_string())
+        // This guards against sending rfc non-compliant cookies, even if the user has
+        // "prepped" their local cookie store with such cookies.
+        .filter(|c| {
+            let is_ok = is_cookie_rfc_compliant(c);
+            if !is_ok {
+                debug!("do not send non compliant cookie: {:?}", c);
+            }
+            is_ok
+        })
+        .map(|c| c.to_string())
         .collect::<Vec<_>>()
         .join(";");
     match header_value.as_str() {
@@ -419,7 +429,15 @@ fn save_cookies(unit: &Unit, resp: &Response) {
         );
         match Cookie::parse(header_value.to_string()) {
             Err(_) => None,
-            Ok(c) => Some(c),
+            Ok(c) => {
+                // This guards against accepting rfc non-compliant cookies from a host.
+                if is_cookie_rfc_compliant(&c) {
+                    Some(c)
+                } else {
+                    debug!("ignore incoming non compliant cookie: {:?}", c);
+                    None
+                }
+            }
         }
     });
     unit.agent
@@ -428,9 +446,73 @@ fn save_cookies(unit: &Unit, resp: &Response) {
         .store_response_cookies(cookies, &unit.url.clone());
 }
 
+#[cfg(feature = "cookies")]
+fn is_cookie_rfc_compliant(cookie: &Cookie) -> bool {
+    // https://tools.ietf.org/html/rfc6265#page-9
+    // set-cookie-header = "Set-Cookie:" SP set-cookie-string
+    // set-cookie-string = cookie-pair *( ";" SP cookie-av )
+    // cookie-pair       = cookie-name "=" cookie-value
+    // cookie-name       = token
+    // cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
+    // cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
+    //                       ; US-ASCII characters excluding CTLs,
+    //                       ; whitespace DQUOTE, comma, semicolon,
+    //                       ; and backslash
+    // token             = <token, defined in [RFC2616], Section 2.2>
+
+    // https://tools.ietf.org/html/rfc2616#page-17
+    // CHAR           = <any US-ASCII character (octets 0 - 127)>
+    // ...
+    //        CTL            = <any US-ASCII control character
+    //                         (octets 0 - 31) and DEL (127)>
+    // ...
+    //        token          = 1*<any CHAR except CTLs or separators>
+    //        separators     = "(" | ")" | "<" | ">" | "@"
+    //                       | "," | ";" | ":" | "\" | <">
+    //                       | "/" | "[" | "]" | "?" | "="
+    //                       | "{" | "}" | SP | HT
+
+    fn is_valid_name(b: &u8) -> bool {
+        header::is_tchar(b)
+    }
+
+    fn is_valid_value(b: &u8) -> bool {
+        b.is_ascii()
+            && !b.is_ascii_control()
+            && !b.is_ascii_whitespace()
+            && *b != b'"'
+            && *b != b','
+            && *b != b';'
+            && *b != b'\\'
+    }
+
+    let name = cookie.name().as_bytes();
+
+    let valid_name = name.iter().all(is_valid_name);
+
+    if !valid_name {
+        log::trace!("cookie name is not valid: {:?}", cookie.name());
+        return false;
+    }
+
+    let value = cookie.value().as_bytes();
+
+    let valid_value = value.iter().all(is_valid_value);
+
+    if !valid_value {
+        log::trace!("cookie value is not valid: {:?}", cookie.value());
+        return false;
+    }
+
+    true
+}
+
 #[cfg(test)]
 #[cfg(feature = "cookies")]
 mod tests {
+    use cookie::Cookie;
+    use cookie_store::CookieStore;
+
     use super::*;
 
     use crate::Agent;
@@ -458,4 +540,51 @@ mod tests {
                 || result == Some(Header::new("Cookie", order2))
         );
     }
+
+    #[test]
+    fn not_send_illegal_cookies() {
+        // This prepares a cookie store with a cookie that isn't legal
+        // according to the relevant rfcs. ureq should not send this.
+        let empty = b"";
+        let mut store = CookieStore::load_json(&empty[..]).unwrap();
+        let url = Url::parse("https://mydomain.com").unwrap();
+        let cookie = Cookie::new("borked///", "illegal<>//");
+        store.insert_raw(&cookie, &url).unwrap();
+
+        let agent = crate::builder().cookie_store(store).build();
+        let cookies = extract_cookies(&agent, &url);
+        assert_eq!(cookies, None);
+    }
+
+    #[test]
+    fn check_cookie_crate_allows_illegal() {
+        // This test is there to see whether the cookie crate enforces
+        // https://tools.ietf.org/html/rfc6265#page-9
+        // https://tools.ietf.org/html/rfc2616#page-17
+        // for cookie name or cookie value.
+        // As long as it doesn't, we do additional filtering in ureq
+        // to not let non-compliant cookies through.
+        let cookie = Cookie::parse("borked///=illegal\\,").unwrap();
+        // these should not be allowed according to the RFCs.
+        assert_eq!(cookie.name(), "borked///");
+        assert_eq!(cookie.value(), "illegal\\,");
+    }
+
+    #[test]
+    fn illegal_cookie_name() {
+        let cookie = Cookie::parse("borked/=value").unwrap();
+        assert!(!is_cookie_rfc_compliant(&cookie));
+    }
+
+    #[test]
+    fn illegal_cookie_value() {
+        let cookie = Cookie::parse("name=borked,").unwrap();
+        assert!(!is_cookie_rfc_compliant(&cookie));
+    }
+
+    #[test]
+    fn legal_cookie_name_value() {
+        let cookie = Cookie::parse("name=value").unwrap();
+        assert!(is_cookie_rfc_compliant(&cookie));
+    }
 }


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index fcbdcf2..b886f0a 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,7 +10,8 @@ jobs:
     - uses: actions/checkout@v2
     - name: Run Rustfmt
       run: cargo fmt -- --check
-
+    - name: Run Clippy
+      run: RUSTFLAGS="-D warnings" cargo clippy      
   doc:
     name: Docs
     runs-on: ubuntu-latest
diff --git a/src/testserver.rs b/src/testserver.rs
index 8261fd8..930a559 100644
--- a/src/testserver.rs
+++ b/src/testserver.rs
@@ -113,13 +113,8 @@ pub fn read_request(stream: &TcpStream) -> TestHeaders {
     // There's a risk stream is ended here, and fill_buf() would block.
     stream.set_nonblocking(true).ok();
     let mut reader = BufReader::new(stream);
-    loop {
-        let amount = match reader.fill_buf() {
-            Ok(buf) => buf.len(),
-            Err(_) => {
-                break;
-            }
-        };
+    while let Ok(buf) = reader.fill_buf() {
+        let amount = buf.len();
         if amount == 0 {
             break;
         }
@@ -148,17 +143,13 @@ impl TestServer {
             }
         });
         // before returning from new(), ensure the server is ready to accept connections
-        loop {
-            if let Err(e) = TcpStream::connect(format!("127.0.0.1:{}", port)) {
-                match e.kind() {
-                    io::ErrorKind::ConnectionRefused => {
-                        std::thread::sleep(Duration::from_millis(100));
-                        continue;
-                    }
-                    _ => eprintln!("testserver: pre-connect with error {}", e),
+        while let Err(e) = TcpStream::connect(format!("127.0.0.1:{}", port)) {
+            match e.kind() {
+                io::ErrorKind::ConnectionRefused => {
+                    std::thread::sleep(Duration::from_millis(100));
+                    continue;
                 }
-            } else {
-                break;
+                _ => eprintln!("testserver: pre-connect with error {}", e),
             }
         }
         TestServer {
@@ -172,9 +163,8 @@ impl Drop for TestServer {
     fn drop(&mut self) {
         self.done.store(true, Ordering::SeqCst);
         // Connect once to unblock the listen loop.
-        match TcpStream::connect(format!("localhost:{}", self.port)) {
-            Err(e) => eprintln!("error dropping testserver: {}", e),
-            _ => {}
+        if let Err(e) = TcpStream::connect(format!("localhost:{}", self.port)) {
+            eprintln!("error dropping testserver: {}", e);
         }
     }
 }