Check redirect locations are valid Uris (seanmonstar#486)

Closes seanmonstar#484
prfss · Apr 1, 2019 · 5c3494b · 5c3494b
1 parent d62f8c2
commit 5c3494b
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 8 deletions.
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
@@ -30,7 +30,7 @@ use native_tls::TlsConnector;
 use super::request::{Request, RequestBuilder};
 use super::response::Response;
 use connect::Connector;
-use into_url::to_uri;
+use into_url::{expect_uri, try_uri};
 use redirect::{self, RedirectPolicy, remove_sensitive_headers};
 use {IntoUrl, Method, Proxy, StatusCode, Url};
 #[cfg(feature = "tls")]
@@ -520,7 +520,7 @@ impl Client {
             headers.insert(ACCEPT_ENCODING, HeaderValue::from_static("gzip"));
         }
 
-        let uri = to_uri(&url);
+        let uri = expect_uri(&url);
 
         let (reusable, body) = match body {
             Some(body) => {
@@ -709,6 +709,17 @@ impl Future for PendingRequest {
                             self.url.join(str::from_utf8(val.as_bytes()).ok()?).ok()
                         })();
 
+                        // Check that the `url` is also a valid `http::Uri`.
+                        //
+                        // If not, just log it and skip the redirect.
+                        let loc = loc.and_then(|url| {
+                            if try_uri(&url).is_some() {
+                                Some(url)
+                            } else {
+                                None
+                            }
+                        });
+
                         if loc.is_none() {
                             debug!("Location header had invalid URI: {:?}", val);
                         }
@@ -733,7 +744,7 @@ impl Future for PendingRequest {
 
                             remove_sensitive_headers(&mut self.headers, &self.url, &self.urls);
                             debug!("redirecting to {:?} '{}'", self.method, self.url);
-                            let uri = to_uri(&self.url);
+                            let uri = expect_uri(&self.url);
                             let body = match self.body {
                                 Some(Some(ref body)) => ::hyper::Body::from(body.clone()),
                                 _ => ::hyper::Body::empty(),

diff --git a/src/into_url.rs b/src/into_url.rs
@@ -38,10 +38,14 @@ impl<'a> PolyfillTryInto for &'a String {
     }
 }
 
-pub(crate) fn to_uri(url: &Url) -> ::hyper::Uri {
+pub(crate) fn expect_uri(url: &Url) -> ::hyper::Uri {
     url.as_str().parse().expect("a parsed Url should always be a valid Uri")
 }
 
+pub(crate) fn try_uri(url: &Url) -> Option<::hyper::Uri> {
+    url.as_str().parse().ok()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/src/proxy.rs b/src/proxy.rs
@@ -56,7 +56,7 @@ impl Proxy {
     /// # fn main() {}
     /// ```
     pub fn http<U: IntoUrl>(url: U) -> ::Result<Proxy> {
-        let uri = ::into_url::to_uri(&url.into_url()?);
+        let uri = ::into_url::expect_uri(&url.into_url()?);
         Ok(Proxy::new(Intercept::Http(uri)))
     }
 
@@ -75,7 +75,7 @@ impl Proxy {
     /// # fn main() {}
     /// ```
     pub fn https<U: IntoUrl>(url: U) -> ::Result<Proxy> {
-        let uri = ::into_url::to_uri(&url.into_url()?);
+        let uri = ::into_url::expect_uri(&url.into_url()?);
         Ok(Proxy::new(Intercept::Https(uri)))
     }
 
@@ -94,7 +94,7 @@ impl Proxy {
     /// # fn main() {}
     /// ```
     pub fn all<U: IntoUrl>(url: U) -> ::Result<Proxy> {
-        let uri = ::into_url::to_uri(&url.into_url()?);
+        let uri = ::into_url::expect_uri(&url.into_url()?);
         Ok(Proxy::new(Intercept::All(uri)))
     }
 
@@ -200,7 +200,7 @@ impl Proxy {
                         .parse()
                         .expect("should be valid Url")
                 )
-                    .map(|u| into_url::to_uri(&u) )
+                    .map(|u| into_url::expect_uri(&u) )
             },
         }
     }

diff --git a/tests/redirect.rs b/tests/redirect.rs
@@ -358,3 +358,32 @@ fn test_referer_is_not_set_if_disabled() {
         .send()
         .unwrap();
 }
+
+#[test]
+fn test_invalid_location_stops_redirect_gh484() {
+    let server = server! {
+        request: b"\
+            GET /yikes HTTP/1.1\r\n\
+            user-agent: $USERAGENT\r\n\
+            accept: */*\r\n\
+            accept-encoding: gzip\r\n\
+            host: $HOST\r\n\
+            \r\n\
+            ",
+        response: b"\
+            HTTP/1.1 302 Found\r\n\
+            Server: test-yikes\r\n\
+            Location: http://www.yikes{KABOOM}\r\n\
+            Content-Length: 0\r\n\
+            \r\n\
+            "
+    };
+
+    let url = format!("http://{}/yikes", server.addr());
+
+    let res = reqwest::get(&url).unwrap();
+
+    assert_eq!(res.url().as_str(), url);
+    assert_eq!(res.status(), reqwest::StatusCode::FOUND);
+    assert_eq!(res.headers().get(reqwest::header::SERVER).unwrap(), &"test-yikes");
+}