Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions crates/crw-cli/src/commands/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
73 changes: 60 additions & 13 deletions crates/crw-cli/src/commands/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand All @@ -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" => {
Expand Down Expand Up @@ -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}")),
Expand All @@ -340,10 +385,12 @@ pub(crate) async fn proxy_call_tool(

async fn parse_response(resp: reqwest::Response) -> Result<Value, String> {
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)));
Expand Down
42 changes: 42 additions & 0 deletions crates/crw-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,45 @@ impl CrwError {
}

pub type CrwResult<T> = Result<T, CrwError>;

/// Display string for a `reqwest::Error` with the request URL stripped.
///
/// `reqwest::Error`'s `Display` appends `" for url (<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}");
}
}
2 changes: 1 addition & 1 deletion crates/crw-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
77 changes: 74 additions & 3 deletions crates/crw-core/src/proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down
20 changes: 15 additions & 5 deletions crates/crw-crawl/src/crawl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -841,15 +845,21 @@ pub async fn discover_urls(opts: DiscoverOptions<'_>) -> CrwResult<DiscoverResul
.redirect(crw_core::url_safety::safe_redirect_policy());
if let Some(ref proxy_url) = discover_proxy {
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);
crw_core::error::CrwError::InvalidRequest(format!(
"invalid proxy URL '{proxy_url}': {e}"
"invalid proxy URL '{redacted}': {}",
crw_core::error::reqwest_message(e)
))
})?;
discover_client_builder = discover_client_builder.proxy(p);
}
let client = discover_client_builder
.build()
.map_err(|e| crw_core::error::CrwError::Internal(format!("http client build: {e}")))?;
let client = discover_client_builder.build().map_err(|e| {
crw_core::error::CrwError::Internal(format!(
"http client build: {}",
crw_core::error::reqwest_message(e)
))
})?;

// The base URL is always part of the result, so seed it up front and let it
// count against `max_urls`. Appending it at the very end instead would push the
Expand Down
4 changes: 2 additions & 2 deletions crates/crw-crawl/src/robots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ impl RobotsTxt {
.get(&url)
.send()
.await
.map_err(|e| CrwError::HttpError(e.to_string()))?;
.map_err(|e| CrwError::HttpError(crw_core::error::reqwest_message(e)))?;

if !resp.status().is_success() {
return Ok(Self {
Expand All @@ -34,7 +34,7 @@ impl RobotsTxt {
let text = resp
.text()
.await
.map_err(|e| CrwError::HttpError(e.to_string()))?;
.map_err(|e| CrwError::HttpError(crw_core::error::reqwest_message(e)))?;

Ok(Self::parse(&text))
}
Expand Down
Loading
Loading