Move auth header on redirect to unit construction

The auth header stripping was in the wrong place (when serializing the request),
rather than in the construction of the Unit, where it ought to be.

This also makes redirect header retention testable.

src/request.rs

@@ -106,7 +106,7 @@ impl Request {
             &self.agent,
             &self.method,
             &url,
-            &self.headers,
+            self.headers,
             &reader,
             deadline,
         );

src/test/redirect.rs

@@ -220,3 +220,103 @@ fn too_many_redirects() {
     let resp = req.get("test://host/malicious_redirect_0").call().unwrap();
     assert_eq!(resp.get_url(), "test://host/malicious_redirect_10000");
 }
+
+#[test]
+fn redirect_no_keep_authorization() {
+    test::set_handler("/redir_no_keep_auth1", |unit| {
+        assert!(unit.has("authorization"));
+        test::make_response(
+            302,
+            "Go here",
+            vec!["Location: /redir_no_keep_auth2"],
+            vec![],
+        )
+    });
+    test::set_handler("/redir_no_keep_auth2", |unit| {
+        assert!(
+            !unit.has("authorization"),
+            "'Authorization' should not be kept on redirect"
+        );
+        test::make_response(200, "OK", vec!["x-foo: bar"], vec![])
+    });
+
+    let resp = get("test://host/redir_no_keep_auth1")
+        // should not be kept in second handler.
+        .set("authorization", "Bearer abc123")
+        .call()
+        .unwrap();
+
+    // ensure second handler runs
+    assert!(resp.has("x-foo"));
+    assert_eq!(resp.header("x-foo").unwrap(), "bar");
+}
+
+#[test]
+fn redirect_keep_auth_same_host() {
+    test::set_handler("/redirect_keep_auth_same_host1", |unit| {
+        assert!(unit.has("authorization"));
+        test::make_response(
+            302,
+            "Go here",
+            vec!["Location: /redirect_keep_auth_same_host2"],
+            vec![],
+        )
+    });
+    test::set_handler("/redirect_keep_auth_same_host2", |unit| {
+        assert!(
+            unit.has("authorization"),
+            "'Authorization' should have been kept on redirect"
+        );
+        test::make_response(200, "OK", vec!["x-foo: bar"], vec![])
+    });
+
+    let agent = builder()
+        .redirect_auth_headers(RedirectAuthHeaders::SameHost)
+        .build();
+
+    let resp = agent
+        .get("test://host/redirect_keep_auth_same_host1")
+        // should not be kept in second handler.
+        .set("authorization", "Bearer abc123")
+        .call()
+        .unwrap();
+
+    // ensure second handler runs
+    assert!(resp.has("x-foo"));
+    assert_eq!(resp.header("x-foo").unwrap(), "bar");
+}
+
+#[test]
+fn redirect_no_keep_auth_different_host() {
+    test::set_handler("/redirect_no_keep_auth_different_host1", |unit| {
+        assert!(unit.has("authorization"));
+        test::make_response(
+            302,
+            "Go here",
+            vec!["Location: test://host_different/redirect_no_keep_auth_different_host2"],
+            vec![],
+        )
+    });
+    test::set_handler("/redirect_no_keep_auth_different_host2", |unit| {
+        assert!(
+            !unit.has("authorization"),
+            "'Authorization' should not have been kept on redirect"
+        );
+        test::make_response(200, "OK", vec!["x-foo: bar"], vec![])
+    });
+
+    let agent = builder()
+        .redirect_auth_headers(RedirectAuthHeaders::SameHost)
+        .build();
+
+    let resp = agent
+        .get("test://host/redirect_no_keep_auth_different_host1")
+        // should not be kept in second handler.
+        .set("authorization", "Bearer abc123")
+        .call()
+        .unwrap();
+
+    // ensure second handler runs
+    assert!(resp.has("x-foo"));
+    assert_eq!(resp.header("x-foo").unwrap(), "bar");
+}

src/unit.rs

@@ -39,13 +39,13 @@ impl Unit {
         agent: &Agent,
         method: &str,
         url: &Url,
-        headers: &[Header],
+        mut headers: Vec<Header>,
         body: &SizedReader,
         deadline: Option<time::Instant>,
     ) -> Self {
         //
 
-        let (is_transfer_encoding_set, mut is_chunked) = get_header(headers, "transfer-encoding")
+        let (is_transfer_encoding_set, mut is_chunked) = get_header(&headers, "transfer-encoding")
             // if the user has set an encoding header, obey that.
             .map(|enc| {
                 let is_transfer_encoding_set = !enc.is_empty();
@@ -58,12 +58,12 @@ impl Unit {
             // otherwise, no chunking.
             .unwrap_or((false, false));
 
-        let extra_headers = {
+        let mut extra_headers = {
             let mut extra = vec![];
 
             // chunking and Content-Length headers are mutually exclusive
             // also don't write this if the user has set it themselves
-            if !is_chunked && get_header(headers, "content-length").is_none() {
+            if !is_chunked && get_header(&headers, "content-length").is_none() {
                 // if the payload is of known size (everything beside an unsized reader), set
                 // Content-Length,
                 // otherwise, use the chunked Transfer-Encoding (only if no other Transfer-Encoding
@@ -85,7 +85,7 @@ impl Unit {
             let username = url.username();
             let password = url.password().unwrap_or("");
             if (!username.is_empty() || !password.is_empty())
-                && get_header(headers, "authorization").is_none()
+                && get_header(&headers, "authorization").is_none()
             {
                 let encoded = base64::encode(&format!("{}:{}", username, password));
                 extra.push(Header::new("Authorization", &format!("Basic {}", encoded)));
@@ -97,11 +97,7 @@ impl Unit {
             extra
         };
 
-        let headers: Vec<_> = headers
+        headers.append(&mut extra_headers);
-            .iter()
-            .chain(extra_headers.iter())
-            .cloned()
-            .collect();
 
         Unit {
             agent: agent.clone(),
@@ -205,17 +201,32 @@ pub(crate) fn connect(
             }
             _ => break resp,
         };
+
+        let keep_auth_header = can_propagate_authorization_on_redirect(
+            &unit.agent.config.redirect_auth_headers,
+            url,
+            &new_url,
+        );
+
         debug!("redirect {} {} -> {}", resp.status(), url, new_url);
         history.push(unit.url);
         body = Payload::Empty.into_read();
-        unit.headers.retain(|h| h.name() != "Content-Length");
+
+        // reuse the previous header vec on redirects.
+        let mut headers = unit.headers;
+
+        // on redirects we don't want to keep "content-length". we also might want to
+        // strip away "authorization" to ensure credentials are not leaked.
+        headers.retain(|h| {
+            !h.is_name("content-length") && (!h.is_name("authorization") || keep_auth_header)
+        });
 
         // recreate the unit to get a new hostname and cookies for the new host.
         unit = Unit::new(
             &unit.agent,
             &new_method,
             &new_url,
-            &unit.headers,
+            headers,
             &body,
             unit.deadline,
         );
@@ -247,7 +258,7 @@ fn connect_inner(
         debug!("sending request {} {}", method, url);
     }
 
-    let send_result = send_prelude(unit, &mut stream, history);
+    let send_result = send_prelude(unit, &mut stream);
 
     if let Err(err) = send_result {
         if is_recycled {
@@ -354,19 +365,23 @@ fn connect_socket(unit: &Unit, hostname: &str, use_pooled: bool) -> Result<(Stre
     Ok((stream?, false))
 }
 
-fn can_propagate_authorization_on_redirect(unit: &Unit, history: &[Url]) -> bool {
+fn can_propagate_authorization_on_redirect(
-    match unit.agent.config.redirect_auth_headers {
+    redirect_auth_headers: &RedirectAuthHeaders,
+    prev_url: &Url,
+    url: &Url,
+) -> bool {
+    fn scheme_is_https(url: &Url) -> bool {
+        url.scheme() == "https" || (cfg!(test) && url.scheme() == "test")
+    }
+
+    match redirect_auth_headers {
         RedirectAuthHeaders::Never => false,
         RedirectAuthHeaders::SameHost => {
-            let host = unit.url.host_str();
+            let host = url.host_str();
-            let is_https = unit.url.scheme() == "https";
+            let is_https = scheme_is_https(url);
-
-            // This is fine because we only do this check on redirects where there
-            // must be a previous host.
-            let prev_url = &history[0];
 
             let prev_host = prev_url.host_str();
-            let prev_is_https = prev_url.scheme() == "https";
+            let prev_is_https = scheme_is_https(prev_url);
 
             host == prev_host && prev_is_https && is_https
         }
@@ -375,7 +390,7 @@ fn can_propagate_authorization_on_redirect(unit: &Unit, history: &[Url]) -> bool
 
 /// Send request line + headers (all up until the body).
 #[allow(clippy::write_with_newline)]
-fn send_prelude(unit: &Unit, stream: &mut Stream, history: &[Url]) -> io::Result<()> {
+fn send_prelude(unit: &Unit, stream: &mut Stream) -> io::Result<()> {
     // build into a buffer and send in one go.
     let mut prelude = PreludeBuilder::new();
 
@@ -414,15 +429,8 @@ fn send_prelude(unit: &Unit, stream: &mut Stream, history: &[Url]) -> io::Result
         prelude.write_header("Accept", "*/*")?;
     }
 
-    let preserve_auth = if !history.is_empty() {
-        can_propagate_authorization_on_redirect(unit, history)
-    } else {
-        true //Not in redirection
-    };
-
     // other headers
     for header in &unit.headers {
-        if preserve_auth || !header.is_name("Authorization") {
         if let Some(v) = header.value() {
             if is_header_sensitive(header) {
                 prelude.write_sensitive_header(header.name(), v)?;
@@ -431,7 +439,6 @@ fn send_prelude(unit: &Unit, stream: &mut Stream, history: &[Url]) -> io::Result
             }
         }
     }
-    }
 
     // finish
     prelude.finish()?;