diff --git a/crates/tinytools-std/src/network/http_request.rs b/crates/tinytools-std/src/network/http_request.rs index 39aac65..e91b8bc 100644 --- a/crates/tinytools-std/src/network/http_request.rs +++ b/crates/tinytools-std/src/network/http_request.rs @@ -174,6 +174,18 @@ impl HttpRequestTool { .collect() } + fn is_safe_response_header(name: &str) -> bool { + matches!( + name.to_ascii_lowercase().as_str(), + "content-type" + | "content-length" + | "retry-after" + | "x-ratelimit-limit" + | "x-ratelimit-remaining" + | "x-ratelimit-reset" + ) + } + async fn execute_request( &self, url: &str, @@ -242,14 +254,28 @@ impl HttpRequestTool { let status = response.status(); let status_code = status.as_u16(); - let response_headers = response.headers().iter(); - let headers_text = response_headers - .map(|(k, _)| { - let is_sensitive = k.as_str().to_lowercase().contains("set-cookie"); - if is_sensitive { - format!("{}: ***REDACTED***", k.as_str()) + // Name *and* value. This printed the name twice — `{}: {:?}` over + // `(k.as_str(), k.as_str())` — so every line read + // `x-ratelimit-remaining: "x-ratelimit-remaining"` and the header block + // carried no information at all. The redaction beside it only makes + // sense if values were meant to be shown, which is the reading taken + // here: rate-limit and retry-after headers are exactly what a caller + // needs on the failures this block renders. + let headers_text = response + .headers() + .iter() + .map(|(name, value)| { + // Response headers are untrusted and service-specific headers + // can carry credentials under arbitrary names. Only expose + // values from this small set of useful diagnostic headers. + if Self::is_safe_response_header(name.as_str()) { + format!( + "{}: {}", + name.as_str(), + value.to_str().unwrap_or("") + ) } else { - format!("{}: {:?}", k.as_str(), k.as_str()) + format!("{}: ***REDACTED***", name.as_str()) } }) .collect::>() @@ -271,7 +297,13 @@ impl HttpRequestTool { if status.is_success() { Ok(ToolResult::success(output)) } else { - Ok(ToolResult::error(format!("HTTP {status_code}"))) + // The same `output` on both arms. This returned the bare string + // `HTTP 403` — twenty characters — having already built the status + // line, headers and body and then dropped them, so the one part of + // the response that said *why* never reached the caller. A real + // case: GitHub's 403 names the missing `User-Agent` header outright, + // with a documentation link, and all of it was discarded. + Ok(ToolResult::error(output)) } } diff --git a/crates/tinytools-std/src/network/http_request_tests.rs b/crates/tinytools-std/src/network/http_request_tests.rs index 520cc33..e064ace 100644 --- a/crates/tinytools-std/src/network/http_request_tests.rs +++ b/crates/tinytools-std/src/network/http_request_tests.rs @@ -228,6 +228,35 @@ fn redact_headers_does_not_alter_original() { assert_eq!(headers[0].1, "Bearer real-token"); } +#[test] +fn only_explicitly_safe_response_header_values_are_displayed() { + for safe in [ + "content-type", + "content-length", + "retry-after", + "x-ratelimit-remaining", + ] { + assert!(HttpRequestTool::is_safe_response_header(safe), "{safe}"); + assert!(HttpRequestTool::is_safe_response_header( + &safe.to_ascii_uppercase() + )); + } + + for sensitive in [ + "set-cookie", + "www-authenticate", + "proxy-authenticate", + "authentication-info", + "x-api-key", + "x-service-token", + ] { + assert!( + !HttpRequestTool::is_safe_response_header(sensitive), + "{sensitive} must be redacted" + ); + } +} + #[test] fn redirect_policy_is_none() { let tool = test_tool(vec!["example.com"]); @@ -452,7 +481,7 @@ async fn a_hook_failure_is_returned_as_the_error_without_a_retry() { #[tokio::test] async fn a_body_is_sent_and_a_success_response_is_formatted_with_redacted_cookies() { - let ok = "HTTP/1.1 200 OK\r\nSet-Cookie: session=abc\r\nX-Other: 1\r\nContent-Length: 5\r\nConnection: close\r\n\r\nhello"; + let ok = "HTTP/1.1 200 OK\r\nSet-Cookie: session=abc\r\nWWW-Authenticate: Bearer secret-challenge\r\nX-Api-Key: api-secret\r\nX-Service-Token: service-secret\r\nX-Other: 1\r\nContent-Length: 5\r\nConnection: close\r\n\r\nhello"; let (addr, seen) = serve(vec![ok.to_string()]).await; let tool = test_tool(vec![]); let response = tool @@ -470,15 +499,34 @@ async fn a_body_is_sent_and_a_success_response_is_formatted_with_redacted_cookie assert!(text.contains("hello"), "{text}"); assert!(text.contains("set-cookie: ***REDACTED***"), "{text}"); assert!(!text.contains("session=abc"), "{text}"); + for secret in ["secret-challenge", "api-secret", "service-secret"] { + assert!(!text.contains(secret), "response leaked {secret}: {text}"); + } + for header in ["www-authenticate", "x-api-key", "x-service-token"] { + assert!( + text.to_ascii_lowercase() + .contains(&format!("{header}: ***redacted***")), + "{text}" + ); + } assert!(seen.lock().unwrap()[0].ends_with("payload")); } #[tokio::test] async fn a_non_success_response_is_reported_as_an_error() { - let (addr, _) = serve(vec![ - "HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" - .to_string(), - ]) + // The failure now carries the whole response rather than just its code. + // + // It used to answer `HTTP 500` —, and in a measured case `HTTP 403`, + // twenty characters, after building the status line, headers and body and + // then dropping them. The discarded body was the only thing that said what + // was wrong: GitHub's 403 names the missing `User-Agent` header outright. + let body = "Request forbidden by administrative rules. \ + Please make sure your request has a User-Agent header"; + let (addr, _) = serve(vec![format!( + "HTTP/1.1 403 Forbidden\r\nContent-Length: {}\r\nX-RateLimit-Remaining: 59\r\nConnection: close\r\n\r\n{}", + body.len(), + body + )]) .await; let tool = test_tool(vec![]); let response = tool @@ -491,8 +539,17 @@ async fn a_non_success_response_is_reported_as_an_error() { .await .unwrap(); let result = tool.format_response(response).await.unwrap(); - assert!(result.is_error); - assert!(result.text().contains("HTTP 500")); + assert!(result.is_error, "a 403 is still a failure"); + let text = result.text(); + // Anchored: a caller classifying the failure reads the status from the head + // of the text, and an unreadable one gets guessed at. + assert!(text.starts_with("Status: 403"), "{text}"); + // The reason the server gave. + assert!(text.contains("User-Agent header"), "{text}"); + // Header *values*, not the name printed twice. `x-ratelimit-remaining` is + // exactly what a caller wants on the failures this block renders. + let lower = text.to_ascii_lowercase(); + assert!(lower.contains("x-ratelimit-remaining: 59"), "{text}"); } #[tokio::test]