From fcdfe01dcf329abd713e4e043f5044277a7775d8 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 24 Jul 2026 01:11:16 -0500 Subject: [PATCH] fix(fetchers): enforce policy on PubMed API requests --- crates/fetchkit/src/fetchers/mod.rs | 57 +++++++++++++++----------- crates/fetchkit/src/fetchers/pubmed.rs | 54 +++++++++++++++++++++++- specs/fetchers.md | 10 +++-- 3 files changed, 93 insertions(+), 28 deletions(-) diff --git a/crates/fetchkit/src/fetchers/mod.rs b/crates/fetchkit/src/fetchers/mod.rs index 2d1d02e..0fd15af 100644 --- a/crates/fetchkit/src/fetchers/mod.rs +++ b/crates/fetchkit/src/fetchers/mod.rs @@ -210,30 +210,7 @@ impl FetcherRegistry { let parsed_url = Url::parse(&request.url).map_err(|_| FetchError::InvalidUrlScheme)?; - options.validate_url(&parsed_url)?; - - // THREAT[TM-INPUT-002]: Normalize URL before prefix matching to prevent - // encoding-based bypasses (case, trailing dots, default ports) - // THREAT[TM-INPUT-007]: URL-aware prefix matching prevents subdomain tricks - if !options.allow_prefixes.is_empty() { - let allowed = options - .allow_prefixes - .iter() - .any(|prefix| url_matches_policy_prefix(&parsed_url, prefix)); - if !allowed { - debug!(url = %request.url, "URL not in allow list"); - return Err(FetchError::BlockedUrl); - } - } - - if options - .block_prefixes - .iter() - .any(|prefix| url_matches_policy_prefix(&parsed_url, prefix)) - { - debug!(url = %request.url, "URL matched block list"); - return Err(FetchError::BlockedUrl); - } + validate_url_policy(&parsed_url, options)?; for fetcher in &self.fetchers { if fetcher.matches(&parsed_url) { @@ -277,6 +254,38 @@ impl FetcherRegistry { } } +// THREAT[TM-SSRF-006]: Specialized fetchers can rewrite user-facing URLs to +// secondary API URLs. Apply the same host, port, and prefix policy to every +// synthesized outbound URL before transport dispatch. +pub(crate) fn validate_url_policy(url: &Url, options: &FetchOptions) -> Result<(), FetchError> { + options.validate_url(url)?; + + // THREAT[TM-INPUT-002]: Normalize URL before prefix matching to prevent + // encoding-based bypasses (case, trailing dots, default ports) + // THREAT[TM-INPUT-007]: URL-aware prefix matching prevents subdomain tricks + if !options.allow_prefixes.is_empty() { + let allowed = options + .allow_prefixes + .iter() + .any(|prefix| url_matches_policy_prefix(url, prefix)); + if !allowed { + debug!(url = %url, "URL not in allow list"); + return Err(FetchError::BlockedUrl); + } + } + + if options + .block_prefixes + .iter() + .any(|prefix| url_matches_policy_prefix(url, prefix)) + { + debug!(url = %url, "URL matched block list"); + return Err(FetchError::BlockedUrl); + } + + Ok(()) +} + // THREAT[TM-INPUT-010]: Invalid file destinations must fail before any outbound request. // Mitigation: reject blank paths and invoke the adapter's preflight validation first. async fn preflight_save_path<'a>( diff --git a/crates/fetchkit/src/fetchers/pubmed.rs b/crates/fetchkit/src/fetchers/pubmed.rs index e9a7320..a5678cf 100644 --- a/crates/fetchkit/src/fetchers/pubmed.rs +++ b/crates/fetchkit/src/fetchers/pubmed.rs @@ -3,7 +3,7 @@ use crate::client::FetchOptions; use crate::error::FetchError; use crate::fetchers::default::{read_full_body, transport_request}; -use crate::fetchers::Fetcher; +use crate::fetchers::{validate_url_policy, Fetcher}; use crate::types::{FetchRequest, FetchResponse}; use crate::DEFAULT_USER_AGENT; use async_trait::async_trait; @@ -77,6 +77,7 @@ impl Fetcher for PubMedFetcher { let article = Self::parse_url(&page_url) .ok_or_else(|| FetchError::FetcherError("Not a supported PubMed URL".into()))?; let (api_url, host, format) = api_url(&article)?; + validate_url_policy(&api_url, options)?; let mut headers = HeaderMap::new(); headers.insert( USER_AGENT, @@ -287,6 +288,30 @@ fn truncate(mut value: String, max: usize) -> (String, bool) { #[cfg(test)] mod tests { use super::*; + use crate::dns::DnsPolicy; + use crate::transport::{HttpTransport, TransportError, TransportRequest, TransportResponse}; + use async_trait::async_trait; + use std::sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }; + + struct CountingTransport { + calls: AtomicUsize, + } + + #[async_trait] + impl HttpTransport for CountingTransport { + async fn execute( + &self, + _req: TransportRequest, + ) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + Err(TransportError::Other( + "transport should not be called".into(), + )) + } + } #[test] fn parses_pubmed_and_pmc_urls() { @@ -319,6 +344,33 @@ mod tests { } } + #[tokio::test] + async fn blocks_pubmed_api_url_when_outbound_policy_disallows_it() { + let transport = Arc::new(CountingTransport { + calls: AtomicUsize::new(0), + }); + let options = FetchOptions { + allow_prefixes: vec!["http://pubmed.ncbi.nlm.nih.gov".into()], + block_prefixes: vec!["https://www.ebi.ac.uk".into()], + blocked_hosts: vec!["www.ebi.ac.uk".into()], + allowed_ports: vec![80], + dns_policy: DnsPolicy::allow_all(), + transport: Some(transport.clone()), + ..Default::default() + }; + + let error = PubMedFetcher::new() + .fetch( + &FetchRequest::new("https://pubmed.ncbi.nlm.nih.gov/12345/"), + &options, + ) + .await + .unwrap_err(); + + assert!(matches!(error, FetchError::BlockedUrl)); + assert_eq!(transport.calls.load(Ordering::SeqCst), 0); + } + #[test] fn renders_pubmed_metadata_abstract_and_keywords() { let response = serde_json::json!({"resultList":{"result":[{ diff --git a/specs/fetchers.md b/specs/fetchers.md index 1034836..09bd619 100644 --- a/specs/fetchers.md +++ b/specs/fetchers.md @@ -23,8 +23,9 @@ Central dispatcher that: 2. Iterates fetchers, uses first matching one 3. Falls back to default fetcher if none match 4. Provides `register()` for adding custom fetchers -5. Validates URL scheme and allow/block lists before dispatching -6. Provides `fetch_to_file()` that dispatches to matched fetcher's `fetch_to_file()` +5. Validates URL scheme and host/port/allow/block URL policy before dispatching +6. Exposes the same URL policy validation for specialized fetchers that synthesize secondary API URLs +7. Provides `fetch_to_file()` that dispatches to matched fetcher's `fetch_to_file()` ### Built-in Fetchers @@ -219,7 +220,10 @@ Central dispatcher that: ### HTTP Transport All fetchers perform their outbound HTTP exclusively through a pluggable -`HttpTransport` (see `transport.rs`). The transport is a single-hop socket adapter: +`HttpTransport` (see `transport.rs`). Specialized fetchers that rewrite a matched +URL to a secondary API URL MUST apply the configured host, port, allow-prefix, and +block-prefix policy to the rewritten URL before handing it to transport. The +transport is a single-hop socket adapter: it never follows redirects and never performs DNS policy resolution. fetchkit owns URL validation, DNS policy (resolve-then-check, producing `TransportRequest.pinned_addrs`), manual per-hop redirect following, bot-auth signing, and body-size/timeout caps;