Return the response on a failed http_request, not just its status code

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("<binary>")
+                    )
                 } else {
-                    format!("{}: {:?}", k.as_str(), k.as_str())
+                    format!("{}: ***REDACTED***", name.as_str())
                 }
             })
             .collect::<Vec<_>>()
@@ -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))
         }
     }
 

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]