Skip to content

Commit ae4e0e4

Browse files
authored
fix(builtins): harden curl redirect against credential leaks (#1020)
## Summary - Strip `Authorization`, `Cookie`, `Proxy-Authorization` headers on cross-origin redirects - Convert POST to GET and drop body on 301/302/303 redirects (per HTTP spec) - Preserve port in `resolve_redirect_url` for absolute-path redirects Closes #998 ## Test plan - [x] New unit tests: port preservation, same_origin, sensitive header stripping (8 tests) - [x] Existing curl tests still pass (20 total) - [x] `cargo clippy -- -D warnings` clean - [x] `cargo fmt --check` clean
1 parent fed036d commit ae4e0e4

File tree

1 file changed

+119
-15
lines changed

1 file changed

+119
-15
lines changed

crates/bashkit/src/builtins/curl.rs

Lines changed: 119 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -410,30 +410,33 @@ async fn execute_curl_request(
410410
};
411411

412412
// Make the request
413-
let body = if multipart_body.is_some() {
414-
multipart_body.as_deref()
413+
let initial_body = if multipart_body.is_some() {
414+
multipart_body.as_deref().map(|b| b.to_vec())
415415
} else {
416-
data.map(|d| d.as_bytes())
416+
data.map(|d| d.as_bytes().to_vec())
417417
};
418+
let mut current_body = initial_body;
419+
let mut current_method = http_method;
420+
let mut current_headers = header_pairs.clone();
418421
let mut current_url = url.to_string();
419422
let mut redirect_count = 0;
420423
const MAX_REDIRECTS: u32 = 10;
421424

422425
loop {
423426
if verbose {
424427
verbose_output.push_str(&format!("> {} {} HTTP/1.1\r\n", method, current_url));
425-
for (name, value) in &header_pairs {
428+
for (name, value) in &current_headers {
426429
verbose_output.push_str(&format!("> {}: {}\r\n", name, value));
427430
}
428431
verbose_output.push_str(">\r\n");
429432
}
430433

431434
let result = http_client
432435
.request_with_timeouts(
433-
http_method,
436+
current_method,
434437
&current_url,
435-
body,
436-
&header_pairs,
438+
current_body.as_deref(),
439+
&current_headers,
437440
max_time,
438441
connect_timeout,
439442
)
@@ -471,7 +474,28 @@ async fn execute_curl_request(
471474
.iter()
472475
.find(|(k, _)| k.eq_ignore_ascii_case("location"))
473476
{
474-
current_url = resolve_redirect_url(&current_url, location);
477+
let prev_url = current_url.clone();
478+
current_url = resolve_redirect_url(&prev_url, location);
479+
480+
// THREAT[TM-NET]: Strip sensitive headers on cross-origin
481+
// redirect to prevent credential leakage (issue #998).
482+
if !same_origin(&prev_url, &current_url) {
483+
current_headers.retain(|(name, _)| {
484+
!SENSITIVE_HEADERS
485+
.iter()
486+
.any(|s| name.eq_ignore_ascii_case(s))
487+
});
488+
}
489+
490+
// THREAT[TM-NET]: Convert POST to GET on 301/302/303
491+
// per HTTP spec — drop body (issue #998).
492+
if matches!(response.status, 301..=303)
493+
&& matches!(current_method, Method::Post)
494+
{
495+
current_method = Method::Get;
496+
current_body = None;
497+
}
498+
475499
continue;
476500
}
477501
}
@@ -602,14 +626,14 @@ fn resolve_redirect_url(base: &str, location: &str) -> String {
602626
if location.starts_with("http://") || location.starts_with("https://") {
603627
location.to_string()
604628
} else if location.starts_with('/') {
605-
// Absolute path - combine with base scheme and host
629+
// Absolute path - combine with base scheme, host, and port
606630
if let Ok(base_url) = url::Url::parse(base) {
607-
format!(
608-
"{}://{}{}",
609-
base_url.scheme(),
610-
base_url.host_str().unwrap_or(""),
611-
location
612-
)
631+
let host = base_url.host_str().unwrap_or("");
632+
if let Some(port) = base_url.port() {
633+
format!("{}://{}:{}{}", base_url.scheme(), host, port, location)
634+
} else {
635+
format!("{}://{}{}", base_url.scheme(), host, location)
636+
}
613637
} else {
614638
location.to_string()
615639
}
@@ -624,6 +648,19 @@ fn resolve_redirect_url(base: &str, location: &str) -> String {
624648
}
625649
}
626650

651+
/// Check if two URLs have the same origin (scheme + host + port).
652+
fn same_origin(a: &str, b: &str) -> bool {
653+
let (Ok(a_url), Ok(b_url)) = (url::Url::parse(a), url::Url::parse(b)) else {
654+
return false;
655+
};
656+
a_url.scheme() == b_url.scheme()
657+
&& a_url.host_str() == b_url.host_str()
658+
&& a_url.port_or_known_default() == b_url.port_or_known_default()
659+
}
660+
661+
/// Sensitive headers that must not be forwarded cross-origin on redirect.
662+
const SENSITIVE_HEADERS: &[&str] = &["authorization", "cookie", "proxy-authorization"];
663+
627664
/// Format the -w/--write-out output.
628665
#[cfg(feature = "http_client")]
629666
fn format_write_out(fmt: &str, response: &crate::network::Response, size: usize) -> String {
@@ -1254,5 +1291,72 @@ mod tests {
12541291
"https://example.com/original/relative"
12551292
);
12561293
}
1294+
1295+
#[test]
1296+
fn test_resolve_redirect_url_preserves_port() {
1297+
let base = "http://localhost:8080/original";
1298+
assert_eq!(
1299+
resolve_redirect_url(base, "/new/path"),
1300+
"http://localhost:8080/new/path"
1301+
);
1302+
}
1303+
1304+
#[test]
1305+
fn test_resolve_redirect_url_no_port() {
1306+
let base = "https://example.com/original";
1307+
assert_eq!(
1308+
resolve_redirect_url(base, "/new"),
1309+
"https://example.com/new"
1310+
);
1311+
}
1312+
1313+
#[test]
1314+
fn test_same_origin_true() {
1315+
assert!(same_origin(
1316+
"https://example.com/path1",
1317+
"https://example.com/path2"
1318+
));
1319+
}
1320+
1321+
#[test]
1322+
fn test_same_origin_false_different_host() {
1323+
assert!(!same_origin(
1324+
"https://example.com/path",
1325+
"https://other.com/path"
1326+
));
1327+
}
1328+
1329+
#[test]
1330+
fn test_same_origin_false_different_port() {
1331+
assert!(!same_origin(
1332+
"http://localhost:8080/path",
1333+
"http://localhost:9090/path"
1334+
));
1335+
}
1336+
1337+
#[test]
1338+
fn test_same_origin_false_different_scheme() {
1339+
assert!(!same_origin(
1340+
"http://example.com/path",
1341+
"https://example.com/path"
1342+
));
1343+
}
1344+
1345+
#[test]
1346+
fn test_sensitive_headers_stripped_cross_origin() {
1347+
let headers = vec![
1348+
("Authorization".to_string(), "Bearer secret".to_string()),
1349+
("Content-Type".to_string(), "application/json".to_string()),
1350+
("Cookie".to_string(), "session=abc".to_string()),
1351+
];
1352+
let mut filtered = headers.clone();
1353+
filtered.retain(|(name, _)| {
1354+
!SENSITIVE_HEADERS
1355+
.iter()
1356+
.any(|s| name.eq_ignore_ascii_case(s))
1357+
});
1358+
assert_eq!(filtered.len(), 1);
1359+
assert_eq!(filtered[0].0, "Content-Type");
1360+
}
12571361
}
12581362
}

0 commit comments

Comments
 (0)