diff --git a/crates/crw-cli/src/commands/doctor.rs b/crates/crw-cli/src/commands/doctor.rs index 8056468b..574b1bbc 100644 --- a/crates/crw-cli/src/commands/doctor.rs +++ b/crates/crw-cli/src/commands/doctor.rs @@ -459,10 +459,9 @@ fn proxy_parse_check(config: &AppConfig) -> CheckResult { ), Ok(None) => CheckResult::pass("proxy.parse", "no proxy configured (direct egress)"), Err(_e) => { - // `crw_core::ProxyEntry::parse`'s error string interpolates the - // raw configured value (it can carry userinfo), so it must never - // reach a report meant to be pasted into a support thread. Report - // the shape of the problem, not the value. + // `crw_core::ProxyEntry::parse` redacts userinfo, but this report + // gets pasted into support threads: withhold the value entirely and + // report the shape of the problem instead. let count = if config.crawler.proxy_list.is_empty() { 1 } else { diff --git a/crates/crw-cli/src/commands/mcp.rs b/crates/crw-cli/src/commands/mcp.rs index 62623a35..c4b7c712 100644 --- a/crates/crw-cli/src/commands/mcp.rs +++ b/crates/crw-cli/src/commands/mcp.rs @@ -197,7 +197,12 @@ pub(crate) async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_crawl" => { @@ -208,7 +213,12 @@ pub(crate) async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_check_crawl_status" => { @@ -222,7 +232,12 @@ pub(crate) async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_map" => { @@ -233,7 +248,12 @@ pub(crate) async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_search" => { @@ -244,7 +264,12 @@ pub(crate) async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_extract" => { @@ -255,7 +280,12 @@ pub(crate) async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_check_extract_status" => { @@ -269,7 +299,12 @@ pub(crate) async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_cancel_extract" => { @@ -283,7 +318,12 @@ pub(crate) async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_parse_file" => { @@ -331,7 +371,12 @@ pub(crate) async fn proxy_call_tool( .multipart(form) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } _ => Err(format!("unknown tool: {tool_name}")), @@ -340,10 +385,12 @@ pub(crate) async fn proxy_call_tool( async fn parse_response(resp: reqwest::Response) -> Result { let status = resp.status(); - let body = resp - .text() - .await - .map_err(|e| format!("failed to read response: {e}"))?; + let body = resp.text().await.map_err(|e| { + format!( + "failed to read response: {}", + crw_core::error::reqwest_message(e) + ) + })?; if !status.is_success() { return Err(format!("API error ({}): {}", status, truncate(&body, 500))); diff --git a/crates/crw-core/src/error.rs b/crates/crw-core/src/error.rs index 7fceddc8..7bb49d2b 100644 --- a/crates/crw-core/src/error.rs +++ b/crates/crw-core/src/error.rs @@ -81,3 +81,45 @@ impl CrwError { } pub type CrwResult = Result; + +/// Display string for a `reqwest::Error` with the request URL stripped. +/// +/// `reqwest::Error`'s `Display` appends `" for url ()"`, and these strings +/// reach API callers verbatim in the scrape `error` field, a crawl document's +/// `block.reason`, and the `/v2` error envelopes. That URL is frequently +/// internal infrastructure (a CDP endpoint, a sidecar host, the managed LLM +/// provider) or carries credentials, none of which a caller may see. Log the +/// full error with `tracing` at the call site when the operator needs the URL. +pub fn reqwest_message(e: reqwest::Error) -> String { + e.without_url().to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn reqwest_message_drops_the_request_url() { + // Loopback port 1 is privileged, so no test process can be listening + // on it: the connect is refused at once, nothing leaves the machine, + // no proxy is consulted, and the resulting error still carries the + // request URL. + let err = reqwest::Client::builder() + .no_proxy() + .build() + .unwrap() + .get("http://127.0.0.1:1/internal-secret-path") + .timeout(std::time::Duration::from_secs(5)) + .send() + .await + .unwrap_err(); + assert!( + err.to_string().contains("for url"), + "precondition: reqwest still appends the URL, got {err}" + ); + + let msg = reqwest_message(err); + assert!(!msg.contains("for url"), "URL not stripped: {msg}"); + assert!(!msg.contains("internal-secret-path"), "URL leaked: {msg}"); + } +} diff --git a/crates/crw-core/src/lib.rs b/crates/crw-core/src/lib.rs index b014a349..b28fc25b 100644 --- a/crates/crw-core/src/lib.rs +++ b/crates/crw-core/src/lib.rs @@ -32,6 +32,6 @@ pub mod url_safety; pub use config::AppConfig; pub use deadline::Deadline; pub use error::{CrwError, CrwResult}; -pub use proxy::{ProxyEntry, ProxyRotation, ProxyRotator}; +pub use proxy::{ProxyEntry, ProxyRotation, ProxyRotator, redact_proxy_url}; pub use reserved_sem::{BatchGate, LanePermit, ReservedSemaphore}; pub use scrape_class::{REQUEST_CLASS, ScrapeClass, current_scrape_class}; diff --git a/crates/crw-core/src/proxy.rs b/crates/crw-core/src/proxy.rs index 1daee448..8ff83443 100644 --- a/crates/crw-core/src/proxy.rs +++ b/crates/crw-core/src/proxy.rs @@ -50,6 +50,33 @@ pub struct ProxyEntry { const ALLOWED_SCHEMES: [&str; 4] = ["http", "https", "socks5", "socks5h"]; +/// `raw` with any `user:pass@` userinfo replaced by `***@`. +/// +/// Proxy URLs carry credentials and end up in `ConfigError` messages that are +/// returned to API callers verbatim, so no error string may ever quote a proxy +/// URL directly. Purely textual on purpose: it must also redact a value that +/// failed to parse as a URL, which is exactly when these errors fire. +pub fn redact_proxy_url(raw: &str) -> String { + let trimmed = raw.trim(); + // Keep the scheme only when it is one (RFC 3986: a letter followed by + // letters, digits, `+`, `-`, `.`). Anything else before `://` may itself be + // the credentials of a mangled value, so it is masked with the rest. + let is_scheme = |s: &str| { + let mut chars = s.chars(); + chars.next().is_some_and(|c| c.is_ascii_alphabetic()) + && chars.all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')) + }; + let (prefix, rest) = match trimmed.split_once("://") { + Some((scheme, rest)) if is_scheme(scheme) => (format!("{scheme}://"), rest), + _ => (String::new(), trimmed), + }; + // Last `@` wins: a host never contains one, an unencoded password can. + match rest.rsplit_once('@') { + Some((_, host)) => format!("{prefix}***@{host}"), + None => format!("{prefix}{rest}"), + } +} + impl ProxyEntry { /// Parse and validate a proxy URL. Returns an error string (no silent /// fallback) when the scheme is unsupported or the host is missing. @@ -58,19 +85,20 @@ impl ProxyEntry { if trimmed.is_empty() { return Err("empty proxy URL".to_string()); } + let redacted = redact_proxy_url(trimmed); let url = - url::Url::parse(trimmed).map_err(|e| format!("invalid proxy URL '{trimmed}': {e}"))?; + url::Url::parse(trimmed).map_err(|e| format!("invalid proxy URL '{redacted}': {e}"))?; let scheme = url.scheme().to_ascii_lowercase(); if !ALLOWED_SCHEMES.contains(&scheme.as_str()) { return Err(format!( - "unsupported proxy scheme '{scheme}' in '{trimmed}' (allowed: http, https, socks5, socks5h)" + "unsupported proxy scheme '{scheme}' in '{redacted}' (allowed: http, https, socks5, socks5h)" )); } let host = url .host_str() - .ok_or_else(|| format!("proxy URL '{trimmed}' has no host"))?; + .ok_or_else(|| format!("proxy URL '{redacted}' has no host"))?; // Chrome's `proxyServer` only understands `socks5` (which already does // remote DNS) — it does not recognize the `socks5h` scheme. Normalize so @@ -265,6 +293,49 @@ fn fnv1a(s: &str) -> u64 { mod tests { use super::*; + #[test] + fn redact_proxy_url_masks_userinfo() { + assert_eq!( + redact_proxy_url("http://user:s3cret@gw.example.com:823"), + "http://***@gw.example.com:823" + ); + // No credentials: unchanged. + assert_eq!( + redact_proxy_url("socks5h://gw.example.com:1080"), + "socks5h://gw.example.com:1080" + ); + // A typo'd scheme still parses out, which is the common `--proxy htp://` + // case an operator needs to see. + assert_eq!(redact_proxy_url("htp://user:pw@host"), "htp://***@host"); + // A value too mangled to split on `://` loses the prefix rather than + // risking the credentials: masking wins over fidelity here. + assert_eq!(redact_proxy_url("htp:/user:pw@host"), "***@host"); + // A "scheme" that is not one is credentials in disguise: masked too. + assert_eq!( + redact_proxy_url("user:s3cret://tail@host:8080"), + "***@host:8080" + ); + assert_eq!(redact_proxy_url(""), ""); + // An `@` in the path of a credential-free value is masked too: masking + // wins over fidelity, and this documents that it is on purpose. + assert_eq!( + redact_proxy_url("http://gw.example.com:823/a@b"), + "http://***@b" + ); + // A literal `@` in the password does not fool the split. + assert_eq!( + redact_proxy_url("http://user:p@ss@host:8080"), + "http://***@host:8080" + ); + } + + #[test] + fn parse_error_never_quotes_credentials() { + let err = ProxyEntry::parse("ftp://user:s3cret@gw.example.com:823").unwrap_err(); + assert!(!err.contains("s3cret"), "credentials leaked: {err}"); + assert!(err.contains("***@gw.example.com:823"), "got {err}"); + } + #[test] fn parse_http_with_auth() { let e = ProxyEntry::parse("http://user:pass@host.example:8080").unwrap(); diff --git a/crates/crw-crawl/src/crawl.rs b/crates/crw-crawl/src/crawl.rs index 9d5c6bb6..7d39de55 100644 --- a/crates/crw-crawl/src/crawl.rs +++ b/crates/crw-crawl/src/crawl.rs @@ -260,7 +260,11 @@ async fn run_crawl_inner(opts: CrawlOptions<'_>) { send_failed( id, &state_tx, - format!("invalid crawl proxy URL '{proxy_url}': {e}"), + format!( + "invalid crawl proxy URL '{}': {}", + crw_core::redact_proxy_url(proxy_url), + crw_core::error::reqwest_message(e) + ), ); return; } @@ -841,15 +845,21 @@ pub async fn discover_urls(opts: DiscoverOptions<'_>) -> CrwResult return Ok(resp), Err(e) => e, }; - match retry { + let out = match retry { Some(retry) if request_never_reached_provider(&err) => retry.send().await, _ => Err(err), + }; + if let Err(ref e) = out { + // The endpoint names the managed provider, so callers get a stripped + // message (`crw_core::error::reqwest_message`). Operators need the URL, + // and this is the one place every provider POST passes through. + tracing::warn!("provider request failed: {e}"); } + out } /// True only when the request provably never got to the provider, so replaying @@ -445,13 +452,20 @@ async fn call_anthropic( .json(&body), ) .await - .map_err(|e| CrwError::Internal(format!("LLM request failed: {e}")))?; + .map_err(|e| { + CrwError::Internal(format!( + "LLM request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); - let text = resp - .text() - .await - .map_err(|e| CrwError::Internal(format!("LLM response read failed: {e}")))?; + let text = resp.text().await.map_err(|e| { + CrwError::Internal(format!( + "LLM response read failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; if !status.is_success() { // NOTE: body may contain the request echoed back by some gateways. // The HTTP status code is enough — do not leak the body. @@ -544,7 +558,12 @@ async fn call_openai( .json(&body), ) .await - .map_err(|e| CrwError::Internal(format!("LLM request failed: {e}")))?; + .map_err(|e| { + CrwError::Internal(format!( + "LLM request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); let is_retryable = status == reqwest::StatusCode::TOO_MANY_REQUESTS @@ -558,10 +577,12 @@ async fn call_openai( continue; } - let text = resp - .text() - .await - .map_err(|e| CrwError::Internal(format!("LLM response read failed: {e}")))?; + let text = resp.text().await.map_err(|e| { + CrwError::Internal(format!( + "LLM response read failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; break (status, text); }; if !status.is_success() { @@ -622,13 +643,20 @@ async fn call_azure( .json(&body), ) .await - .map_err(|e| CrwError::Internal(format!("LLM request failed: {e}")))?; + .map_err(|e| { + CrwError::Internal(format!( + "LLM request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); - let text = resp - .text() - .await - .map_err(|e| CrwError::Internal(format!("LLM response read failed: {e}")))?; + let text = resp.text().await.map_err(|e| { + CrwError::Internal(format!( + "LLM response read failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; if !status.is_success() { return Err(CrwError::Internal(format!("LLM HTTP {status} from azure"))); } diff --git a/crates/crw-extract/src/responses.rs b/crates/crw-extract/src/responses.rs index 5df7ded8..1dc25adc 100644 --- a/crates/crw-extract/src/responses.rs +++ b/crates/crw-extract/src/responses.rs @@ -60,11 +60,19 @@ async fn post( .json(body), ) .await - .map_err(|e| CrwError::ExtractionError(format!("Responses API request failed: {e}")))?; + .map_err(|e| { + CrwError::ExtractionError(format!( + "Responses API request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); let text = resp.text().await.map_err(|e| { - CrwError::ExtractionError(format!("Failed to read Responses API response: {e}")) + CrwError::ExtractionError(format!( + "Failed to read Responses API response: {}", + crw_core::error::reqwest_message(e) + )) })?; if !status.is_success() { // The HTTP status code is enough — do not leak the body. A gateway that diff --git a/crates/crw-extract/src/structured.rs b/crates/crw-extract/src/structured.rs index e3224d57..3cada0da 100644 --- a/crates/crw-extract/src/structured.rs +++ b/crates/crw-extract/src/structured.rs @@ -549,11 +549,19 @@ pub(crate) async fn call_anthropic( .json(&body), ) .await - .map_err(|e| CrwError::ExtractionError(format!("Anthropic API request failed: {e}")))?; + .map_err(|e| { + CrwError::ExtractionError(format!( + "Anthropic API request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); let text = resp.text().await.map_err(|e| { - CrwError::ExtractionError(format!("Failed to read Anthropic response: {e}")) + CrwError::ExtractionError(format!( + "Failed to read Anthropic response: {}", + crw_core::error::reqwest_message(e) + )) })?; if forcing && status.is_client_error() { @@ -838,11 +846,19 @@ pub(crate) async fn call_openai( .json(&body), ) .await - .map_err(|e| CrwError::ExtractionError(format!("OpenAI API request failed: {e}")))?; + .map_err(|e| { + CrwError::ExtractionError(format!( + "OpenAI API request failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); let text = resp.text().await.map_err(|e| { - CrwError::ExtractionError(format!("Failed to read OpenAI response: {e}")) + CrwError::ExtractionError(format!( + "Failed to read OpenAI response: {}", + crw_core::error::reqwest_message(e) + )) })?; if forcing && status.is_client_error() { diff --git a/crates/crw-mcp/src/main.rs b/crates/crw-mcp/src/main.rs index 8e4ac070..bc977318 100644 --- a/crates/crw-mcp/src/main.rs +++ b/crates/crw-mcp/src/main.rs @@ -238,7 +238,12 @@ async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_crawl" => { @@ -249,7 +254,12 @@ async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_check_crawl_status" => { @@ -263,7 +273,12 @@ async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_map" => { @@ -274,7 +289,12 @@ async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_search" => { @@ -285,7 +305,12 @@ async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_extract" => { @@ -298,7 +323,12 @@ async fn proxy_call_tool( .json(&args) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_check_extract_status" => { @@ -312,7 +342,12 @@ async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_cancel_extract" => { @@ -326,7 +361,12 @@ async fn proxy_call_tool( .timeout(TIMEOUT_CRAWL_STATUS) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } "crw_parse_file" => { @@ -374,7 +414,12 @@ async fn proxy_call_tool( .multipart(form) .send() .await - .map_err(|e| format!("HTTP request failed: {e}"))?; + .map_err(|e| { + format!( + "HTTP request failed: {}", + crw_core::error::reqwest_message(e) + ) + })?; parse_response(resp).await } _ => Err(format!("unknown tool: {tool_name}")), @@ -383,10 +428,12 @@ async fn proxy_call_tool( async fn parse_response(resp: reqwest::Response) -> Result { let status = resp.status(); - let body = resp - .text() - .await - .map_err(|e| format!("failed to read response: {e}"))?; + let body = resp.text().await.map_err(|e| { + format!( + "failed to read response: {}", + crw_core::error::reqwest_message(e) + ) + })?; if !status.is_success() { return Err(format!("API error ({}): {}", status, truncate(&body, 500))); diff --git a/crates/crw-renderer/src/camoufox.rs b/crates/crw-renderer/src/camoufox.rs index eb6dc0bd..eabab3dd 100644 --- a/crates/crw-renderer/src/camoufox.rs +++ b/crates/crw-renderer/src/camoufox.rs @@ -104,12 +104,22 @@ impl CamoufoxRenderer { let resp = tokio::time::timeout(budget, fut) .await .map_err(|_| CrwError::Timeout(budget.as_millis() as u64))? - .map_err(|e| CrwError::RendererError(format!("camoufox POST {path}: {e}")))?; + .map_err(|e| { + // The sidecar base URL is internal infrastructure: operators get it + // from the log, callers only get the failure shape. + tracing::warn!(base_url = %self.base_url, path, "camoufox request failed: {e}"); + CrwError::RendererError(format!( + "camoufox POST {path}: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status(); - let value: serde_json::Value = resp - .json() - .await - .map_err(|e| CrwError::RendererError(format!("camoufox POST {path} body: {e}")))?; + let value: serde_json::Value = resp.json().await.map_err(|e| { + CrwError::RendererError(format!( + "camoufox POST {path} body: {}", + crw_core::error::reqwest_message(e) + )) + })?; if !status.is_success() { let msg = value .get("error") diff --git a/crates/crw-renderer/src/cdp.rs b/crates/crw-renderer/src/cdp.rs index f64adc51..de1eceae 100644 --- a/crates/crw-renderer/src/cdp.rs +++ b/crates/crw-renderer/src/cdp.rs @@ -689,12 +689,21 @@ async fn resolve_ws_url_with_cache( .timeout(Duration::from_secs(5)) .send() .await - .map_err(|e| CrwError::RendererError(format!("CDP discovery failed: {e}")))?; + .map_err(|e| { + // The endpoint is already in the `Discovering browser WS URL` line + // above; it is internal infrastructure and must not reach the caller. + CrwError::RendererError(format!( + "CDP discovery failed: {}", + crw_core::error::reqwest_message(e) + )) + })?; - let body: serde_json::Value = resp - .json() - .await - .map_err(|e| CrwError::RendererError(format!("CDP discovery parse error: {e}")))?; + let body: serde_json::Value = resp.json().await.map_err(|e| { + CrwError::RendererError(format!( + "CDP discovery parse error: {}", + crw_core::error::reqwest_message(e) + )) + })?; let ws_url = body .get("webSocketDebuggerUrl") diff --git a/crates/crw-renderer/src/cloak.rs b/crates/crw-renderer/src/cloak.rs index 1ebc24dc..c2bf21d8 100644 --- a/crates/crw-renderer/src/cloak.rs +++ b/crates/crw-renderer/src/cloak.rs @@ -168,10 +168,21 @@ impl CloakRenderer { let resp = tokio::time::timeout(budget, rb.send()) .await .map_err(|_| CrwError::Timeout(budget.as_millis() as u64))? - .map_err(|e| CrwError::RendererError(format!("cloak GET {path_and_query}: {e}")))?; + .map_err(|e| { + // Same rule as camoufox: the mirror base URL is ours, not the + // caller's. Log it, do not return it. + tracing::warn!(base_url = %self.base_url, "cloak request failed: {e}"); + CrwError::RendererError(format!( + "cloak GET {path_and_query}: {}", + crw_core::error::reqwest_message(e) + )) + })?; let status = resp.status().as_u16(); let body = resp.text().await.map_err(|e| { - CrwError::RendererError(format!("cloak GET {path_and_query} body: {e}")) + CrwError::RendererError(format!( + "cloak GET {path_and_query} body: {}", + crw_core::error::reqwest_message(e) + )) })?; Ok((status, body)) } diff --git a/crates/crw-renderer/src/http_only.rs b/crates/crw-renderer/src/http_only.rs index bf886b20..83a847e5 100644 --- a/crates/crw-renderer/src/http_only.rs +++ b/crates/crw-renderer/src/http_only.rs @@ -244,7 +244,10 @@ fn ratelimit_proxy_url() -> Option { /// fallback should react to. Detected by message (rustls/openssl surface these /// as opaque connect errors, so there is no typed predicate to match on). fn is_cert_error(e: &reqwest::Error) -> bool { - let mut src: Option<&(dyn std::error::Error + 'static)> = Some(e); + // Start at the source: the reqwest error's own Display is the kind string + // plus the request URL, so matching it could only ever false-positive on a + // target path that happens to mention certificates. + let mut src: Option<&(dyn std::error::Error + 'static)> = std::error::Error::source(e); while let Some(s) = src { let m = s.to_string().to_ascii_lowercase(); if m.contains("certificate") @@ -301,14 +304,23 @@ fn build_client( } if let Some(proxy_url) = proxy { - let p = reqwest::Proxy::all(proxy_url) - .map_err(|e| CrwError::ConfigError(format!("invalid proxy URL '{proxy_url}': {e}")))?; + let p = reqwest::Proxy::all(proxy_url).map_err(|e| { + // NEVER interpolate `proxy_url` itself: it carries `user:pass@`. + let redacted = crw_core::redact_proxy_url(proxy_url); + CrwError::ConfigError(format!( + "invalid proxy URL '{redacted}': {}", + crw_core::error::reqwest_message(e) + )) + })?; builder = builder.proxy(p); } - builder - .build() - .map_err(|e| CrwError::ConfigError(format!("failed to build HTTP client: {e}"))) + builder.build().map_err(|e| { + CrwError::ConfigError(format!( + "failed to build HTTP client: {}", + crw_core::error::reqwest_message(e) + )) + }) } /// Simple HTTP fetcher using reqwest. No JS rendering. @@ -889,10 +901,16 @@ impl PageFetcher for HttpFetcher { reporting target_unreachable (was: http_error)" ); } - return Err(if e.is_connect() && (!use_proxy || direct_connect_failed) { - CrwError::TargetUnreachable(format!("Could not reach {url}: {e}")) + let unreachable = e.is_connect() && (!use_proxy || direct_connect_failed); + // reqwest's " for url (...)" tail carries the request target, not + // the proxy, so dropping it here is for uniformity with every other + // site. `url` is the caller's own target and safe to echo, so both + // arms name it themselves. + let msg = crw_core::error::reqwest_message(e); + return Err(if unreachable { + CrwError::TargetUnreachable(format!("Could not reach {url}: {msg}")) } else { - CrwError::HttpError(e.to_string()) + CrwError::HttpError(format!("{url}: {msg}")) }); } } @@ -953,7 +971,7 @@ impl PageFetcher for HttpFetcher { ) .await { - Ok(r) => r.map_err(|e| CrwError::HttpError(e.to_string()))?, + Ok(r) => r.map_err(|e| CrwError::HttpError(crw_core::error::reqwest_message(e)))?, Err(_) => { return Err(CrwError::Timeout( (start.elapsed().as_millis().max(1)) as u64, diff --git a/crates/crw-search/src/client.rs b/crates/crw-search/src/client.rs index 729cdaa4..759fdd1b 100644 --- a/crates/crw-search/src/client.rs +++ b/crates/crw-search/src/client.rs @@ -44,7 +44,11 @@ async fn read_capped(response: reqwest::Response, cap: usize) -> Result, let mut buf: Vec = Vec::with_capacity(64 * 1024); let mut stream = response.bytes_stream(); while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|e: reqwest::Error| SearchError::Transport(e.to_string()))?; + let chunk = chunk.map_err(|e: reqwest::Error| { + // Same reason as the `send()` arm below (issue #90): the embedded + // request URL can carry the backend host and its credentials. + SearchError::Transport(crw_core::error::reqwest_message(e)) + })?; if buf.len() + chunk.len() > cap { return Err(SearchError::InvalidResponse(format!( "response too large: exceeded {cap}-byte cap" @@ -226,8 +230,8 @@ impl SearxngClient { } else { // `without_url()` strips reqwest's embedded request URL from // the Display string — that URL can carry credentials/tokens - // (issue #90). The route layer re-attaches a sanitized origin. - SearchError::Transport(e.without_url().to_string()) + // (issue #90). The route layer logs the sanitized origin instead. + SearchError::Transport(crw_core::error::reqwest_message(e)) } })?; diff --git a/crates/crw-server/src/routes/research.rs b/crates/crw-server/src/routes/research.rs index 6c2afe29..f665f29a 100644 --- a/crates/crw-server/src/routes/research.rs +++ b/crates/crw-server/src/routes/research.rs @@ -288,10 +288,9 @@ pub async fn github( // Curated-engine leg; a paid general-web tier cannot honour it. paid_rescue: false, }; - let resp = client - .fetch(¶ms) - .await - .map_err(|e| CrwError::HttpError(format!("github search failed: {e}")))?; + let resp = client.fetch(¶ms).await.map_err(|e| { + super::search::map_search_error(e, state.config.search.timeout_ms, client.base_url()) + })?; let results: Vec = resp .results .into_iter() diff --git a/crates/crw-server/src/routes/search.rs b/crates/crw-server/src/routes/search.rs index 06bc9f97..6d499a05 100644 --- a/crates/crw-server/src/routes/search.rs +++ b/crates/crw-server/src/routes/search.rs @@ -1303,24 +1303,33 @@ fn validate_request(req: &SearchRequest, max_limit: u32) -> Result<(), CrwError> } /// Map a transport/timeout/upstream `SearchError` onto the HTTP `CrwError`. -/// `base_url` is the configured SearXNG URL; the transport (`target_unreachable`) -/// arm names its **origin** (issue #90) so the operator sees *which* host failed -/// — sanitized, so a credentialed URL never reaches the response. Timeouts keep -/// `error_code: "timeout"`; the host is correlated via the startup log instead. -fn map_search_error(err: SearchError, timeout_ms: u64, base_url: &str) -> CrwError { +/// `base_url` is the configured search backend URL. The operator still learns +/// which host failed (issue #90), but through the log: the response names no +/// host, and an upstream error page is reduced to its status, because both are +/// internal infrastructure a caller may not see. Timeouts keep +/// `error_code: "timeout"`. +pub(crate) fn map_search_error(err: SearchError, timeout_ms: u64, base_url: &str) -> CrwError { match err { SearchError::Timeout => CrwError::Timeout(timeout_ms), - SearchError::Upstream { status, body } => CrwError::HttpError(format!( - "Search backend returned HTTP {status}: {}", - body.chars().take(200).collect::() - )), + SearchError::Upstream { status, body } => { + tracing::warn!( + search_backend = %crate::diagnostics::sanitize_url_origin(base_url), + status, + body = %body.chars().take(200).collect::(), + "search backend returned an error" + ); + CrwError::HttpError(format!("Search backend returned HTTP {status}")) + } SearchError::InvalidResponse(msg) => { CrwError::HttpError(format!("Search backend returned invalid JSON: {msg}")) } - SearchError::Transport(msg) => CrwError::TargetUnreachable(format!( - "Search backend ({}): {msg}", - crate::diagnostics::sanitize_url_origin(base_url) - )), + SearchError::Transport(msg) => { + tracing::warn!( + search_backend = %crate::diagnostics::sanitize_url_origin(base_url), + "search backend unreachable: {msg}" + ); + CrwError::TargetUnreachable(format!("Search backend unreachable: {msg}")) + } } } @@ -1892,14 +1901,20 @@ mod tests { } #[test] - fn map_search_error_transport_names_sanitized_host() { - // issue #90: the unreachable error must name the configured host so the - // operator knows *what* failed — but origin-only, never the raw URL. + fn map_search_error_transport_names_no_host() { + // The unreachable error keeps the transport reason for the caller and + // nothing about the backend: no host, no userinfo, no path token. The + // operator gets the sanitized origin from the log line instead. let err = SearchError::Transport("dns error: failed to lookup address".into()); let mapped = map_search_error(err, 5000, "https://user:pass@searxng:8080/tok?k=v"); match mapped { CrwError::TargetUnreachable(msg) => { - assert!(msg.contains("https://searxng:8080"), "{msg}"); + assert!(msg.contains("dns error"), "{msg}"); + assert!( + !msg.contains("://"), + "must not name the backend origin: {msg}" + ); + assert!(!msg.contains("8080"), "must not leak the port: {msg}"); assert!(!msg.contains("user"), "must not leak userinfo: {msg}"); assert!(!msg.contains("pass"), "must not leak credentials: {msg}"); assert!(!msg.contains("tok"), "must not leak path token: {msg}"); @@ -2216,28 +2231,28 @@ mod tests { } #[test] - fn map_search_error_upstream_body_truncated_to_200_chars() { - let long_body = "x".repeat(500); + fn map_search_error_upstream_body_never_reaches_the_caller() { + // An upstream error page is internal infrastructure output (an nginx + // or uwsgi page naming the upstream address); the caller gets the + // status alone. let err = SearchError::Upstream { - status: 500, - body: long_body, + status: 502, + body: "502 Bad Gateway upstream 10.0.0.7:8080".into(), }; match map_search_error(err, 5000, "http://searxng:8080") { CrwError::HttpError(msg) => { - // 200 chars of body plus the "HTTP {status}: " prefix. - let x_count = msg.chars().filter(|c| *c == 'x').count(); - assert_eq!(x_count, 200, "expected body truncated to 200 chars: {msg}"); + assert_eq!(msg, "Search backend returned HTTP 502"); } other => panic!("expected HttpError, got {other:?}"), } } #[test] - fn map_search_error_transport_plain_host_no_credentials() { + fn map_search_error_transport_plain_host_not_named() { let err = SearchError::Transport("connection refused".into()); match map_search_error(err, 5000, "http://searxng-internal:8080") { CrwError::TargetUnreachable(msg) => { - assert!(msg.contains("http://searxng-internal:8080"), "{msg}"); + assert!(!msg.contains("searxng-internal"), "{msg}"); assert!(msg.contains("connection refused"), "{msg}"); } other => panic!("expected TargetUnreachable, got {other:?}"),