From 91d6a50dbaae45f6e0ceda20a43335276ab73d6f Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 11 Jul 2026 13:44:30 +0200 Subject: [PATCH] fix: guard all browser subresources against SSRF and fail closed The CDP Fetch interceptor only paused main-frame Document requests, so a scraped page's /fetch()/XHR subresources to internal targets (169.254.169.254, postgres:5432, RFC1918) reached those hosts unguarded. Drop the ResourceType::Document constraint so every request runs through is_blocked. Also make open_page fail-closed: if the guard can't install, close the blank page and return the error instead of navigating unguarded. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/crawler/intercept.rs | 70 +++++++++++++++++++++++--------- frontend/package.json | 2 +- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 24f86b0..39b07ef 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.124.13" +version = "0.124.14" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 74f1d50..1c5331e 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.124.13" +version = "0.124.14" edition = "2021" default-run = "mangalord" diff --git a/backend/src/crawler/intercept.rs b/backend/src/crawler/intercept.rs index 6db78ee..474d103 100644 --- a/backend/src/crawler/intercept.rs +++ b/backend/src/crawler/intercept.rs @@ -9,10 +9,16 @@ //! attacker-owned hostname that resolves to `169.254.169.254`) would be //! loaded and parsed as if it were catalog content. //! -//! This module installs CDP `Fetch` interception on a page so every -//! **Document** (main-frame) request and redirect is re-validated through the -//! same [`ensure_public_target`] + resolved-IP check the reqwest paths use, -//! failing any that target an internal address. +//! This module installs CDP `Fetch` interception on a page so **every** request +//! it issues — the main-frame Document, its redirects, *and every subresource* +//! (``, `fetch()`/XHR, media, …) — is re-validated through the same +//! [`ensure_public_target`] + resolved-IP check the reqwest paths use, failing +//! any that target an internal address. Intercepting only the Document would +//! leave a scraped page free to pull `` or +//! `fetch('http://postgres:5432')` straight past the guard, so no resource type +//! is exempt. (WebSocket handshakes are not surfaced by CDP `Fetch`, so `ws://` +//! internal targets remain out of this hook's reach — the reqwest-layer +//! resolver does not see them either; documented as a known gap.) //! //! **Opt-in / default-off.** Enabling `Fetch` means every intercepted request //! *must* be resolved by a live handler or the navigation hangs, so this is a @@ -20,9 +26,9 @@ //! `CRAWLER_SSRF_INTERCEPT` (default `false`) and the wiring has **not** been //! exercised against a real Chromium in CI — validate with a manual crawl //! before enabling in production. When disabled, [`open_page`] is byte-for-byte -//! the previous `browser.new_page(url)` behavior. Even when enabled, a failure -//! to install the guard falls back to an unguarded navigation rather than -//! failing the crawl. +//! the previous `browser.new_page(url)` behavior. When enabled, the guard is +//! **fail-closed**: if interception can't be installed, [`open_page`] closes the +//! blank page and returns the error rather than navigating unguarded. use std::net::IpAddr; use std::sync::atomic::{AtomicBool, Ordering}; @@ -32,7 +38,7 @@ use chromiumoxide::cdp::browser_protocol::fetch::{ ContinueRequestParams, EnableParams, EventRequestPaused, FailRequestParams, RequestPattern, RequestStage, }; -use chromiumoxide::cdp::browser_protocol::network::{ErrorReason, ResourceType}; +use chromiumoxide::cdp::browser_protocol::network::ErrorReason; use chromiumoxide::error::Result as CdpResult; use chromiumoxide::Page; use futures_util::StreamExt; @@ -129,26 +135,35 @@ pub async fn open_page(browser: &Browser, url: &str) -> CdpResult { } let page = browser.new_page("about:blank").await?; if let Err(e) = install_navigation_guard(&page).await { - // Fail open on install error: navigate unguarded rather than wedge the - // crawl. The reqwest-layer resolver still covers image downloads. - tracing::warn!(url = %url, error = %e, "SSRF navigation guard failed to install; navigating unguarded"); + // Fail closed: an unguarded page could be redirected (or pull a + // subresource) to an internal target, so refuse to navigate. Close the + // blank page and surface the error so the caller aborts this fetch. + tracing::warn!(url = %url, error = %e, "SSRF navigation guard failed to install; aborting navigation (fail-closed)"); + let _ = page.close().await; + return Err(e); } page.goto(url).await?; Ok(page) } -/// Enable `Fetch` for Document requests on `page` and spawn a task that +/// The `Fetch` interception patterns to register. A single pattern with **no** +/// `resource_type` constraint matches every request the page makes — Document, +/// Image, Fetch/XHR, media, everything — so subresources to internal targets are +/// re-validated too, not only the main-frame navigation. Restricting this to +/// `ResourceType::Document` (the previous behavior) left every ``/`fetch()` +/// subresource unguarded, which is the SSRF hole this closes. Pinned to the +/// request stage so each request is paused once, before it leaves the browser. +fn interception_patterns() -> Vec { + vec![RequestPattern::builder() + .request_stage(RequestStage::Request) + .build()] +} + +/// Enable `Fetch` for all requests on `page` and spawn a task that /// continues/fails each paused request per [`is_blocked`]. async fn install_navigation_guard(page: &Page) -> CdpResult<()> { - // Intercept only main-frame Document requests at the request stage — a - // navigation plus its redirects, nothing else. Keeps the paused-request - // volume tiny so the handler can't become a page-load bottleneck. - let pattern = RequestPattern::builder() - .resource_type(ResourceType::Document) - .request_stage(RequestStage::Request) - .build(); page.execute(EnableParams { - patterns: Some(vec![pattern]), + patterns: Some(interception_patterns()), handle_auth_requests: None, }) .await?; @@ -244,6 +259,21 @@ mod tests { assert!(!is_blocked("about:blank").await); } + #[test] + fn interception_covers_all_resource_types() { + // Regression guard for the SSRF subresource hole: the CDP Fetch pattern + // must NOT be constrained to Document, or ``/`fetch()`/XHR + // subresources to internal targets slip past `is_blocked`. A pattern + // with no `resource_type` set intercepts every request type. + let patterns = interception_patterns(); + assert_eq!(patterns.len(), 1); + assert!( + patterns[0].resource_type.is_none(), + "interception must cover all resource types (subresources included), not just Document" + ); + assert_eq!(patterns[0].request_stage, Some(RequestStage::Request)); + } + #[test] fn enabled_toggle_roundtrips() { // Global; restore afterwards so other tests see the default. diff --git a/frontend/package.json b/frontend/package.json index 5de39c0..63cf872 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.124.13", + "version": "0.124.14", "private": true, "type": "module", "scripts": {