Send headers within the redirect requests. (#2310)

awc/CHANGES.md

@@ -1,7 +1,10 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+### Changed
+* Send headers within the redirect requests. [#2310]
 
+[#2310]: https://github.com/actix/actix-web/pull/2310
 
 ## 3.0.0-beta.7 - 2021-06-26
 ### Changed

awc/src/middleware/redirect.rs

@@ -85,10 +85,12 @@ where
                 let max_redirect_times = self.max_redirect_times;
 
                 // backup the uri and method for reuse schema and authority.
-                let (uri, method) = match head {
+                let (uri, method, headers) = match head {
-                    RequestHeadType::Owned(ref head) => (head.uri.clone(), head.method.clone()),
+                    RequestHeadType::Owned(ref head) => {
+                        (head.uri.clone(), head.method.clone(), head.headers.clone())
+                    }
                     RequestHeadType::Rc(ref head, ..) => {
-                        (head.uri.clone(), head.method.clone())
+                        (head.uri.clone(), head.method.clone(), head.headers.clone())
                     }
                 };
 
@@ -104,6 +106,7 @@ where
                     max_redirect_times,
                     uri: Some(uri),
                     method: Some(method),
+                    headers: Some(headers),
                     body: body_opt,
                     addr,
                     connector: Some(connector),
@@ -127,9 +130,10 @@ pin_project_lite::pin_project! {
             max_redirect_times: u8,
             uri: Option<Uri>,
             method: Option<Method>,
+            headers: Option<header::HeaderMap>,
             body: Option<Bytes>,
             addr: Option<SocketAddr>,
-            connector: Option<Rc<S>>
+            connector: Option<Rc<S>>,
         }
     }
 }
@@ -148,6 +152,7 @@ where
                 max_redirect_times,
                 uri,
                 method,
+                headers,
                 body,
                 addr,
                 connector,
@@ -156,79 +161,60 @@ where
                     StatusCode::MOVED_PERMANENTLY
                     | StatusCode::FOUND
                     | StatusCode::SEE_OTHER
+                    | StatusCode::TEMPORARY_REDIRECT
+                    | StatusCode::PERMANENT_REDIRECT
                         if *max_redirect_times > 0 =>
                     {
-                        let org_uri = uri.take().unwrap();
+                        let is_redirect = res.head().status == StatusCode::TEMPORARY_REDIRECT
-                        // rebuild uri from the location header value.
+                            || res.head().status == StatusCode::PERMANENT_REDIRECT;
-                        let uri = rebuild_uri(&res, org_uri)?;
 
-                        // reset method
+                        let prev_uri = uri.take().unwrap();
-                        let method = method.take().unwrap();
+
-                        let method = match method {
+                        // rebuild uri from the location header value.
-                            Method::GET | Method::HEAD => method,
+                        let next_uri = build_next_uri(&res, &prev_uri)?;
-                            _ => Method::GET,
-                        };
 
                         // take ownership of states that could be reused
                         let addr = addr.take();
                         let connector = connector.take();
-                        let mut max_redirect_times = *max_redirect_times;
 
-                        // use a new request head.
+                        // reset method
-                        let mut head = RequestHead::default();
+                        let method = if is_redirect {
-                        head.uri = uri.clone();
+                            method.take().unwrap()
-                        head.method = method.clone();
+                        } else {
-
+                            let method = method.take().unwrap();
-                        let head = RequestHeadType::Owned(head);
+                            match method {
-
+                                Method::GET | Method::HEAD => method,
-                        max_redirect_times -= 1;
+                                _ => Method::GET,
-
-                        let fut = connector
-                            .as_ref()
-                            .unwrap()
-                            // remove body
-                            .call(ConnectRequest::Client(head, Body::None, addr));
-
-                        self.set(RedirectServiceFuture::Client {
-                            fut,
-                            max_redirect_times,
-                            uri: Some(uri),
-                            method: Some(method),
-                            // body is dropped on 301,302,303
-                            body: None,
-                            addr,
-                            connector,
-                        });
-
-                        self.poll(cx)
                             }
-                    StatusCode::TEMPORARY_REDIRECT | StatusCode::PERMANENT_REDIRECT
+                        };
-                        if *max_redirect_times > 0 =>
-                    {
-                        let org_uri = uri.take().unwrap();
-                        // rebuild uri from the location header value.
-                        let uri = rebuild_uri(&res, org_uri)?;
 
+                        let mut body = body.take();
+                        let body_new = if is_redirect {
                             // try to reuse body
-                        let body = body.take();
+                            match body {
-                        let body_new = match body {
                                 Some(ref bytes) => Body::Bytes(bytes.clone()),
                                 // TODO: should this be Body::Empty or Body::None.
                                 _ => Body::Empty,
+                            }
+                        } else {
+                            body = None;
+                            // remove body
+                            Body::None
                         };
 
-                        let addr = addr.take();
+                        let mut headers = headers.take().unwrap();
-                        let method = method.take().unwrap();
+
-                        let connector = connector.take();
+                        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
-                        let mut max_redirect_times = *max_redirect_times;
 
                         // use a new request head.
                         let mut head = RequestHead::default();
-                        head.uri = uri.clone();
+                        head.uri = next_uri.clone();
                         head.method = method.clone();
+                        head.headers = headers.clone();
 
                         let head = RequestHeadType::Owned(head);
 
+                        let mut max_redirect_times = *max_redirect_times;
                         max_redirect_times -= 1;
 
                         let fut = connector
@@ -239,8 +225,9 @@ where
                         self.set(RedirectServiceFuture::Client {
                             fut,
                             max_redirect_times,
-                            uri: Some(uri),
+                            uri: Some(next_uri),
                             method: Some(method),
+                            headers: Some(headers),
                             body,
                             addr,
                             connector,
@@ -256,7 +243,7 @@ where
     }
 }
 
-fn rebuild_uri(res: &ClientResponse, org_uri: Uri) -> Result<Uri, SendRequestError> {
+fn build_next_uri(res: &ClientResponse, prev_uri: &Uri) -> Result<Uri, SendRequestError> {
     let uri = res
         .headers()
         .get(header::LOCATION)
@@ -266,8 +253,8 @@ fn rebuild_uri(res: &ClientResponse, org_uri: Uri) -> Result<Uri, SendRequestErr
                 .map_err(|e| SendRequestError::Url(InvalidUrl::HttpError(e.into())))?;
             if uri.scheme().is_none() || uri.authority().is_none() {
                 let uri = Uri::builder()
-                    .scheme(org_uri.scheme().cloned().unwrap())
+                    .scheme(prev_uri.scheme().cloned().unwrap())
-                    .authority(org_uri.authority().cloned().unwrap())
+                    .authority(prev_uri.authority().cloned().unwrap())
                     .path_and_query(value.as_bytes())
                     .build()?;
                 Ok::<_, SendRequestError>(uri)
@@ -281,12 +268,25 @@ fn rebuild_uri(res: &ClientResponse, org_uri: Uri) -> Result<Uri, SendRequestErr
     Ok(uri)
 }
 
+fn remove_sensitive_headers(headers: &mut header::HeaderMap, prev_uri: &Uri, next_uri: &Uri) {
+    if next_uri.host() != prev_uri.host()
+        || next_uri.port() != prev_uri.port()
+        || next_uri.scheme() != prev_uri.scheme()
+    {
+        headers.remove(header::COOKIE);
+        headers.remove(header::AUTHORIZATION);
+        headers.remove(header::PROXY_AUTHORIZATION);
+    }
+}
+
 #[cfg(test)]
 mod tests {
-    use actix_web::{web, App, Error, HttpResponse};
+    use actix_web::{web, App, Error, HttpRequest, HttpResponse};
 
     use super::*;
+    use crate::http::HeaderValue;
     use crate::ClientBuilder;
+    use std::str::FromStr;
 
     #[actix_rt::test]
     async fn test_basic_redirect() {
@@ -347,4 +347,239 @@ mod tests {
 
         assert_eq!(res.status().as_u16(), 302);
     }
+
+    #[actix_rt::test]
+    async fn test_redirect_status_kind_307_308() {
+        let srv = actix_test::start(|| {
+            async fn root() -> HttpResponse {
+                HttpResponse::TemporaryRedirect()
+                    .append_header(("location", "/test"))
+                    .finish()
+            }
+
+            async fn test(req: HttpRequest, body: Bytes) -> HttpResponse {
+                if req.method() == Method::POST && !body.is_empty() {
+                    HttpResponse::Ok().finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            App::new()
+                .service(web::resource("/").route(web::to(root)))
+                .service(web::resource("/test").route(web::to(test)))
+        });
+
+        let res = srv.post("/").send_body("Hello").await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+    }
+
+    #[actix_rt::test]
+    async fn test_redirect_status_kind_301_302_303() {
+        let srv = actix_test::start(|| {
+            async fn root() -> HttpResponse {
+                HttpResponse::Found()
+                    .append_header(("location", "/test"))
+                    .finish()
+            }
+
+            async fn test(req: HttpRequest, body: Bytes) -> HttpResponse {
+                if (req.method() == Method::GET || req.method() == Method::HEAD)
+                    && body.is_empty()
+                {
+                    HttpResponse::Ok().finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            App::new()
+                .service(web::resource("/").route(web::to(root)))
+                .service(web::resource("/test").route(web::to(test)))
+        });
+
+        let res = srv.post("/").send_body("Hello").await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+
+        let res = srv.post("/").send().await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+    }
+
+    #[actix_rt::test]
+    async fn test_redirect_headers() {
+        let srv = actix_test::start(|| {
+            async fn root(req: HttpRequest) -> HttpResponse {
+                if req
+                    .headers()
+                    .get("custom")
+                    .unwrap_or(&HeaderValue::from_str("").unwrap())
+                    == "value"
+                {
+                    HttpResponse::Found()
+                        .append_header(("location", "/test"))
+                        .finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            async fn test(req: HttpRequest) -> HttpResponse {
+                if req
+                    .headers()
+                    .get("custom")
+                    .unwrap_or(&HeaderValue::from_str("").unwrap())
+                    == "value"
+                {
+                    HttpResponse::Ok().finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            App::new()
+                .service(web::resource("/").route(web::to(root)))
+                .service(web::resource("/test").route(web::to(test)))
+        });
+
+        let client = ClientBuilder::new()
+            .header("custom", "value")
+            .disable_redirects()
+            .finish();
+        let res = client.get(srv.url("/")).send().await.unwrap();
+        assert_eq!(res.status().as_u16(), 302);
+
+        let client = ClientBuilder::new().header("custom", "value").finish();
+        let res = client.get(srv.url("/")).send().await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+
+        let client = ClientBuilder::new().finish();
+        let res = client
+            .get(srv.url("/"))
+            .insert_header(("custom", "value"))
+            .send()
+            .await
+            .unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+    }
+
+    #[actix_rt::test]
+    async fn test_redirect_cross_origin_headers() {
+        // defining two services to have two different origins
+        let srv2 = actix_test::start(|| {
+            async fn root(req: HttpRequest) -> HttpResponse {
+                if req.headers().get(header::AUTHORIZATION).is_none() {
+                    HttpResponse::Ok().finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            App::new().service(web::resource("/").route(web::to(root)))
+        });
+        let srv2_port: u16 = srv2.addr().port();
+
+        let srv1 = actix_test::start(move || {
+            async fn root(req: HttpRequest) -> HttpResponse {
+                let port = *req.app_data::<u16>().unwrap();
+                if req.headers().get(header::AUTHORIZATION).is_some() {
+                    HttpResponse::Found()
+                        .append_header((
+                            "location",
+                            format!("http://localhost:{}/", port).as_str(),
+                        ))
+                        .finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            async fn test1(req: HttpRequest) -> HttpResponse {
+                if req.headers().get(header::AUTHORIZATION).is_some() {
+                    HttpResponse::Found()
+                        .append_header(("location", "/test2"))
+                        .finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            async fn test2(req: HttpRequest) -> HttpResponse {
+                if req.headers().get(header::AUTHORIZATION).is_some() {
+                    HttpResponse::Ok().finish()
+                } else {
+                    HttpResponse::InternalServerError().finish()
+                }
+            }
+
+            App::new()
+                .app_data(srv2_port)
+                .service(web::resource("/").route(web::to(root)))
+                .service(web::resource("/test1").route(web::to(test1)))
+                .service(web::resource("/test2").route(web::to(test2)))
+        });
+
+        // send a request to different origins, http://srv1/ then http://srv2/. So it should remove the header
+        let client = ClientBuilder::new()
+            .header(header::AUTHORIZATION, "auth_key_value")
+            .finish();
+        let res = client.get(srv1.url("/")).send().await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+
+        // send a request to same origin, http://srv1/test1 then http://srv1/test2. So it should NOT remove any header
+        let res = client.get(srv1.url("/test1")).send().await.unwrap();
+        assert_eq!(res.status().as_u16(), 200);
+    }
+
+    #[actix_rt::test]
+    async fn test_remove_sensitive_headers() {
+        fn gen_headers() -> header::HeaderMap {
+            let mut headers = header::HeaderMap::new();
+            headers.insert(header::USER_AGENT, HeaderValue::from_str("value").unwrap());
+            headers.insert(
+                header::AUTHORIZATION,
+                HeaderValue::from_str("value").unwrap(),
+            );
+            headers.insert(
+                header::PROXY_AUTHORIZATION,
+                HeaderValue::from_str("value").unwrap(),
+            );
+            headers.insert(header::COOKIE, HeaderValue::from_str("value").unwrap());
+            headers
+        }
+
+        // Same origin
+        let prev_uri = Uri::from_str("https://host/path1").unwrap();
+        let next_uri = Uri::from_str("https://host/path2").unwrap();
+        let mut headers = gen_headers();
+        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
+        assert_eq!(headers.len(), 4);
+
+        // different schema
+        let prev_uri = Uri::from_str("http://host/").unwrap();
+        let next_uri = Uri::from_str("https://host/").unwrap();
+        let mut headers = gen_headers();
+        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
+        assert_eq!(headers.len(), 1);
+
+        // different host
+        let prev_uri = Uri::from_str("https://host1/").unwrap();
+        let next_uri = Uri::from_str("https://host2/").unwrap();
+        let mut headers = gen_headers();
+        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
+        assert_eq!(headers.len(), 1);
+
+        // different port
+        let prev_uri = Uri::from_str("https://host:12/").unwrap();
+        let next_uri = Uri::from_str("https://host:23/").unwrap();
+        let mut headers = gen_headers();
+        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
+        assert_eq!(headers.len(), 1);
+
+        // different everything!
+        let prev_uri = Uri::from_str("http://host1:12/path1").unwrap();
+        let next_uri = Uri::from_str("https://host2:23/path2").unwrap();
+        let mut headers = gen_headers();
+        remove_sensitive_headers(&mut headers, &prev_uri, &next_uri);
+        assert_eq!(headers.len(), 1);
+    }
 }