From 2b0eca98277235781d1473ccd41317b6096f6196 Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sat, 11 Dec 2021 09:03:49 +0100 Subject: [PATCH] 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 | 2 +- src/test/redirect.rs | 100 +++++++++++++++++++++++++++++++++++++++++++ src/unit.rs | 79 ++++++++++++++++++---------------- 3 files changed, 144 insertions(+), 37 deletions(-) diff --git a/src/request.rs b/src/request.rs index e98a6ed..cfb7b16 100644 --- a/src/request.rs +++ b/src/request.rs @@ -106,7 +106,7 @@ impl Request { &self.agent, &self.method, &url, - &self.headers, + self.headers, &reader, deadline, ); diff --git a/src/test/redirect.rs b/src/test/redirect.rs index 2d3dc66..ed50008 100644 --- a/src/test/redirect.rs +++ b/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"); +} diff --git a/src/unit.rs b/src/unit.rs index a78d154..d06e183 100644 --- a/src/unit.rs +++ b/src/unit.rs @@ -39,13 +39,13 @@ impl Unit { agent: &Agent, method: &str, url: &Url, - headers: &[Header], + mut headers: Vec
, body: &SizedReader, deadline: Option, ) -> 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 - .iter() - .chain(extra_headers.iter()) - .cloned() - .collect(); + headers.append(&mut extra_headers); 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 { - match unit.agent.config.redirect_auth_headers { +fn can_propagate_authorization_on_redirect( + 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 is_https = unit.url.scheme() == "https"; - - // This is fine because we only do this check on redirects where there - // must be a previous host. - let prev_url = &history[0]; + let host = url.host_str(); + let is_https = scheme_is_https(url); 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,21 +429,13 @@ 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)?; - } else { - prelude.write_header(header.name(), v)?; - } + if let Some(v) = header.value() { + if is_header_sensitive(header) { + prelude.write_sensitive_header(header.name(), v)?; + } else { + prelude.write_header(header.name(), v)?; } } }