From 2ed42f7b9e8dc1457ee6068361b23c5e5fae7501 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 1 Jul 2026 07:21:15 +0200 Subject: [PATCH] feat(crawler): re-validate redirect hops to close SSRF via 3xx Co-Authored-By: Claude Opus 4.8 --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/app.rs | 14 +++++ backend/src/bin/crawler.rs | 77 ++++++++++++++--------- backend/src/crawler/safety.rs | 115 ++++++++++++++++++++++++++++++++++ frontend/package.json | 2 +- 6 files changed, 181 insertions(+), 31 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index b1e86c3..b905376 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.90.9" +version = "0.91.0" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index a4b3cff..c5a1d88 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.90.9" +version = "0.91.0" edition = "2021" default-run = "mangalord" diff --git a/backend/src/app.rs b/backend/src/app.rs index fd2afa9..1c1a3b3 100644 --- a/backend/src/app.rs +++ b/backend/src/app.rs @@ -469,6 +469,12 @@ async fn spawn_analysis_daemon( // both. Mirrors the crawler client's `.no_proxy()` (see // `spawn_crawler_daemon`). .no_proxy() + // Re-validate redirect hops so a hostile/compromised vision + // endpoint can't 302 the bearer token + page bytes into the + // deployment's internal network. No allowlist here (the + // endpoint is a single admin-configured URL), so the policy + // enforces scheme + private-IP only. + .redirect(crate::crawler::safety::public_redirect_policy()) .build() .context("build analysis http client")?; let vision = crate::analysis::vision::VisionClient::new(http, cfg); @@ -494,6 +500,7 @@ async fn spawn_analysis_daemon( // still gets a useful side-channel on backend // uptime + vision health. .no_proxy() + .redirect(crate::crawler::safety::public_redirect_policy()) .build() .context("build vision readiness http client")?; Some(Arc::new(crate::analysis::daemon::HttpVisionReadiness { @@ -571,6 +578,13 @@ async fn spawn_crawler_daemon( let mut http_builder = reqwest::Client::builder() .timeout(std::time::Duration::from_secs(30)) .no_proxy() + // Re-validate every redirect hop against the download allowlist: + // reqwest's default policy follows up to 10 redirects, and + // `is_safe_url` only guards the initial URL, so an allowlisted CDN + // 302ing to a private IP would otherwise be followed (SSRF). + .redirect(crate::crawler::safety::safe_redirect_policy( + cfg.download_allowlist.clone(), + )) .cookie_provider(Arc::clone(&cookie_jar)); if let Some(ua) = &cfg.user_agent { http_builder = http_builder.user_agent(ua); diff --git a/backend/src/bin/crawler.rs b/backend/src/bin/crawler.rs index 0128788..e9a15e9 100644 --- a/backend/src/bin/crawler.rs +++ b/backend/src/bin/crawler.rs @@ -112,9 +112,20 @@ async fn main() -> anyhow::Result<()> { cookie_jar.add_cookie_str(&cookie_str, &seed_url); tracing::info!(domain, "seeded PHPSESSID into reqwest cookie jar"); } + // SSRF defence: only download from the catalog host + CDN host (plus + // optional CRAWLER_DOWNLOAD_ALLOWLIST extras). Built here so the same + // allowlist guards both the redirect policy and the per-image check. + let allowlist = Arc::new(build_download_allowlist(&start_url, cdn_host.as_deref())); let mut http_builder = reqwest::Client::builder() .timeout(Duration::from_secs(30)) .no_proxy() + // Re-validate every redirect hop against the allowlist — reqwest's + // default follows up to 10 redirects and `is_safe_url` only guards + // the initial URL, so a 302 to a private IP would otherwise pivot + // inside the deployment (SSRF). + .redirect(mangalord::crawler::safety::safe_redirect_policy( + (*allowlist).clone(), + )) .cookie_provider(cookie_jar); if let Some(ua) = &user_agent { http_builder = http_builder.user_agent(ua); @@ -216,6 +227,7 @@ async fn main() -> anyhow::Result<()> { rate_ms, cdn_host.as_deref(), cdn_rate_ms, + Arc::clone(&allowlist), limit, skip_chapters, skip_chapter_content || !session_ready, @@ -246,6 +258,7 @@ async fn run( rate_ms: u64, cdn_host: Option<&str>, cdn_rate_ms: u64, + allowlist: Arc, limit: usize, skip_chapters: bool, skip_chapter_content: bool, @@ -259,38 +272,12 @@ async fn run( } let rate = Arc::new(rate); - // SSRF defence: only download from the catalog host + CDN host - // (plus optional CRAWLER_DOWNLOAD_ALLOWLIST extras), and cap - // single-image downloads at CRAWLER_MAX_IMAGE_BYTES bytes. - // CRAWLER_ALLOW_ANY_HOST=true short-circuits the host check for - // sharded-CDN sources; private-IP and scheme guards still apply. - let allowlist = if env_bool("CRAWLER_ALLOW_ANY_HOST", false) { - mangalord::crawler::safety::DownloadAllowlist::allow_any() - } else { - let mut allow = mangalord::crawler::safety::DownloadAllowlist::new(); - if let Ok(parsed) = reqwest::Url::parse(start_url) { - if let Some(h) = parsed.host_str() { - allow = allow.allow(h); - } - } - if let Some(host) = cdn_host { - allow = allow.allow(host); - } - if let Ok(extras) = std::env::var("CRAWLER_DOWNLOAD_ALLOWLIST") { - for piece in extras.split(',') { - let trimmed = piece.trim(); - if !trimmed.is_empty() { - allow = allow.allow(trimmed); - } - } - } - allow - }; + // Per-image download cap (the allowlist is built in `main` and passed in + // so the HTTP client's redirect policy and this check share one source). let max_image_bytes: usize = std::env::var("CRAWLER_MAX_IMAGE_BYTES") .ok() .and_then(|s| s.parse().ok()) .unwrap_or(mangalord::crawler::safety::DEFAULT_MAX_IMAGE_BYTES); - let allowlist = Arc::new(allowlist); let stats = pipeline::run_metadata_pass( manager.as_ref(), @@ -497,3 +484,37 @@ fn env_bool(name: &str, default: bool) -> bool { } } +/// Build the crawler download allowlist from env + the catalog/CDN hosts. +/// Shared by the HTTP client's redirect policy and the per-image safety +/// check so both agree on which hosts are reachable. +/// +/// `CRAWLER_ALLOW_ANY_HOST=true` short-circuits the host check for +/// sharded-CDN sources; private-IP and scheme guards still apply. +fn build_download_allowlist( + start_url: &str, + cdn_host: Option<&str>, +) -> mangalord::crawler::safety::DownloadAllowlist { + use mangalord::crawler::safety::DownloadAllowlist; + if env_bool("CRAWLER_ALLOW_ANY_HOST", false) { + return DownloadAllowlist::allow_any(); + } + let mut allow = DownloadAllowlist::new(); + if let Ok(parsed) = reqwest::Url::parse(start_url) { + if let Some(h) = parsed.host_str() { + allow = allow.allow(h); + } + } + if let Some(host) = cdn_host { + allow = allow.allow(host); + } + if let Ok(extras) = std::env::var("CRAWLER_DOWNLOAD_ALLOWLIST") { + for piece in extras.split(',') { + let trimmed = piece.trim(); + if !trimmed.is_empty() { + allow = allow.allow(trimmed); + } + } + } + allow +} + diff --git a/backend/src/crawler/safety.rs b/backend/src/crawler/safety.rs index 2ed3c48..8eb928d 100644 --- a/backend/src/crawler/safety.rs +++ b/backend/src/crawler/safety.rs @@ -226,6 +226,73 @@ pub enum UrlSafetyError { HostNotAllowed(String), } +/// Maximum number of redirects the crawler will follow before giving up. +/// Matches reqwest's historical default; every hop is re-validated by +/// [`check_redirect_hop`], so the cap is a belt-and-braces stop against a +/// redirect loop rather than the primary SSRF defence. +pub const MAX_REDIRECTS: usize = 10; + +/// Why a redirect hop was refused. +#[derive(Debug, thiserror::Error)] +pub enum RedirectError { + #[error("redirect chain exceeded {0} hops")] + TooManyHops(usize), + #[error("redirect target rejected: {0}")] + Unsafe(#[from] UrlSafetyError), +} + +/// Decide whether a single redirect hop is safe to follow. +/// +/// `is_safe_url` only inspects the *initial* URL a caller hands to reqwest; +/// without re-validation an allowlisted CDN that answers `302 -> +/// http://169.254.169.254/...` or `-> http://127.0.0.1:5432/` would be +/// followed transparently (reqwest's default policy follows up to 10 +/// redirects). This re-runs the full allowlist + private-IP + scheme check on +/// each hop and enforces [`MAX_REDIRECTS`]. `completed_hops` is the number of +/// URLs already visited in the chain (reqwest's `attempt.previous().len()`). +pub fn check_redirect_hop( + next_url: &str, + completed_hops: usize, + allow: &DownloadAllowlist, +) -> Result<(), RedirectError> { + if completed_hops >= MAX_REDIRECTS { + return Err(RedirectError::TooManyHops(completed_hops)); + } + is_safe_url(next_url, allow)?; + Ok(()) +} + +/// Build a reqwest redirect policy that re-validates every hop against the +/// download allowlist (see [`check_redirect_hop`]). Use for the crawler image +/// clients, which fetch attacker-influenced URLs. +pub fn safe_redirect_policy(allow: DownloadAllowlist) -> reqwest::redirect::Policy { + reqwest::redirect::Policy::custom(move |attempt| { + let hops = attempt.previous().len(); + match check_redirect_hop(attempt.url().as_str(), hops, &allow) { + Ok(()) => attempt.follow(), + Err(e) => attempt.error(e), + } + }) +} + +/// Build a reqwest redirect policy that re-validates every hop with +/// [`ensure_public_target`] (scheme + private-IP, no allowlist). Use for +/// single-endpoint clients (the analysis vision endpoint / its probe) where +/// there is no per-deployment allowlist but a redirect into the deployment's +/// internal network must still be refused. +pub fn public_redirect_policy() -> reqwest::redirect::Policy { + reqwest::redirect::Policy::custom(move |attempt| { + let hops = attempt.previous().len(); + if hops >= MAX_REDIRECTS { + return attempt.error(RedirectError::TooManyHops(hops)); + } + match ensure_public_target(attempt.url().as_str()) { + Ok(()) => attempt.follow(), + Err(e) => attempt.error(RedirectError::Unsafe(e)), + } + }) +} + /// Drain a byte stream into a single buffer, bailing out as soon as /// the running total exceeds `max_bytes`. Generic over the stream so /// it's testable without a live HTTP response. @@ -578,6 +645,54 @@ mod tests { assert!(is_safe_url("https://CDN.EXAMPLE.com/x.jpg", &allow).is_ok()); } + // --- redirect-hop re-validation (SSRF via 3xx) --- + + #[test] + fn redirect_hop_allows_listed_public_target() { + let allow = allow_just("cdn.example.com"); + assert!(check_redirect_hop("https://cdn.example.com/next.jpg", 1, &allow).is_ok()); + } + + #[test] + fn redirect_hop_blocks_private_ip_target() { + // The core SSRF case: an allowlisted CDN 302s to the cloud metadata + // service / an intra-compose port. Must be refused mid-chain. + let allow = allow_just("cdn.example.com"); + for url in ["http://169.254.169.254/", "http://127.0.0.1:5432/", "http://10.0.0.1/"] { + let err = check_redirect_hop(url, 1, &allow).unwrap_err(); + assert!( + matches!(err, RedirectError::Unsafe(UrlSafetyError::PrivateIp(_))), + "expected PrivateIp for {url}, got {err:?}" + ); + } + } + + #[test] + fn redirect_hop_blocks_off_allowlist_public_host() { + // Per the strict policy: a redirect to an unlisted *public* host is + // also refused (allow_any covers the numbered-CDN case instead). + let allow = allow_just("cdn.example.com"); + let err = check_redirect_hop("https://evil.example.org/x", 1, &allow).unwrap_err(); + assert!(matches!(err, RedirectError::Unsafe(UrlSafetyError::HostNotAllowed(_)))); + } + + #[test] + fn redirect_hop_blocks_bad_scheme_target() { + let allow = DownloadAllowlist::allow_any(); + let err = check_redirect_hop("file:///etc/passwd", 1, &allow).unwrap_err(); + assert!(matches!(err, RedirectError::Unsafe(UrlSafetyError::BadScheme(_)))); + } + + #[test] + fn redirect_hop_caps_chain_length() { + let allow = allow_just("cdn.example.com"); + // A safe target is still refused once the hop cap is reached, so a + // redirect loop can't spin forever. + let err = check_redirect_hop("https://cdn.example.com/x", MAX_REDIRECTS, &allow) + .unwrap_err(); + assert!(matches!(err, RedirectError::TooManyHops(n) if n == MAX_REDIRECTS)); + } + #[tokio::test] async fn accumulate_capped_returns_full_body_under_cap() { let chunks: Vec> = vec![ diff --git a/frontend/package.json b/frontend/package.json index 8c00b98..59dd17b 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.90.9", + "version": "0.91.0", "private": true, "type": "module", "scripts": {