Skip to content

Commit

Permalink
Fix cookie leak on redirect (#608)
Browse files Browse the repository at this point in the history
This removes the cookie header on redirect, otherwise if there are
multiple redirects, a new cookie header gets appended each time, while
the existing one remains. This could result in the cookies being leaked
to a third party.
  • Loading branch information
amoskvin committed Jun 14, 2023
1 parent 329e1f4 commit fbae139
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 2 deletions.
44 changes: 44 additions & 0 deletions src/test/redirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,50 @@ fn redirect_post_with_data() {
assert_eq!(resp.status(), 200);
}

#[cfg(feature = "cookies")]
#[test]
fn redirect_post_with_cookies() {
test::set_handler("/secure/login", |_| {
test::make_response(
302,
"Go here",
vec![
"Set-cookie: USER=ferris; Path=/; SameSite=Strict",
"Set-cookie: SECRET=123456; Path=/secure; SameSite=Strict",
"Location: /secure/login-handler",
],
vec![],
)
});
test::set_handler("/secure/login-handler", |unit| {
let cookie_headers = unit.all("cookie");
assert_eq!(cookie_headers.len(), 1);
assert!(cookie_headers.first().unwrap().contains("USER"));
assert!(cookie_headers.first().unwrap().contains("SECRET"));
test::make_response(302, "Go here", vec!["Location: /out"], vec![])
});
test::set_handler("/out", |unit| {
let cookie_headers = unit.all("cookie");
assert_eq!(unit.all("cookie").len(), 1);
assert!(cookie_headers.first().unwrap().contains("USER"));
assert!(!cookie_headers.first().unwrap().contains("SECRET"));
test::make_response(
302,
"Go away",
vec!["Location: test://external.example/external"],
vec![],
)
});
test::set_handler("/external", |unit| {
assert_eq!(unit.method, "GET");
assert_eq!(unit.all("cookie").len(), 0);
test::make_response(200, "OK", vec![], vec![])
});
let resp = post("test://host.example/secure/login").call().unwrap();
assert_eq!(resp.status(), 200);
assert_eq!(resp.get_url(), "test://external.example/external");
}

#[test]
fn redirect_308() {
test::set_handler("/redirect_get3", |_| {
Expand Down
6 changes: 4 additions & 2 deletions src/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,11 @@ pub(crate) fn connect(
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.
// strip away "authorization" and "cookie" to ensure credentials are not leaked.
headers.retain(|h| {
!h.is_name("content-length") && (!h.is_name("authorization") || keep_auth_header)
!h.is_name("content-length")
&& !h.is_name("cookie")
&& (!h.is_name("authorization") || keep_auth_header)
});

// recreate the unit to get a new hostname and cookies for the new host.
Expand Down

0 comments on commit fbae139

Please sign in to comment.