From 3eb4444e468fdd5ec26c0651fc1c518ae4f01f28 Mon Sep 17 00:00:00 2001 From: Rafi Date: Sun, 16 Aug 2026 05:13:05 -0400 Subject: [PATCH 01/13] Add the imgproxy delivery layer imgforge processed images correctly and then said almost nothing about them. A response carried a content type and, on a cache hit, a cache status; everything a CDN or a browser uses to avoid asking again was missing, and one URL could only ever mean one format. Content negotiation reads Accept and can serve WebP or AVIF to clients that advertise them, either where the URL left the format open (detection) or over the top of an explicit one (enforcement). The chosen format is part of the cache key and the response carries Vary: Accept, because otherwise a shared cache hands an AVIF to a client that said it could not read one. Caching headers follow: a configured TTL or the origin's own Cache-Control, an ETag over the response bytes with If-None-Match answered as a 304, Last-Modified, and a canonical Link back to the source. Client hints let the browser size the image for a URL that did not. Alongside those, the settings a deployment needs around them: a base URL and an allow list for sources, a path prefix and a health endpoint, a CORS origin, truncated signatures, debug headers, and development error detail. --- Cargo.lock | 1 + Cargo.toml | 3 + src/app.rs | 51 +- src/caching/cache.rs | 22 +- src/config/env_vars.rs | 12 + src/config/mod.rs | 41 +- src/config/source.rs | 352 ++++++++ src/constants.rs | 4 + src/fetch.rs | 11 + src/handlers.rs | 159 +++- src/lib.rs | 8 +- src/negotiation.rs | 449 ++++++++++ src/processing/tests_support.rs | 26 +- src/response.rs | 319 +++++++ src/server.rs | 43 +- src/service/cache_key.rs | 40 +- src/service/mod.rs | 216 ++++- src/service/tests.rs | 73 ++ src/test_support.rs | 33 + src/url.rs | 69 +- tests/handlers_integration_tests_extended.rs | 848 ++++++++++++++++++- 21 files changed, 2671 insertions(+), 109 deletions(-) create mode 100644 src/config/source.rs create mode 100644 src/negotiation.rs create mode 100644 src/response.rs create mode 100644 src/test_support.rs diff --git a/Cargo.lock b/Cargo.lock index ad857be..77f68a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1454,6 +1454,7 @@ dependencies = [ "hex", "hmac", "http-body-util", + "httpdate", "image", "kamadak-exif", "lazy_static", diff --git a/Cargo.toml b/Cargo.toml index d5fcc65..5250bf5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,9 @@ lazy_static = "1.5.0" governor = "0.10.4" bytes = "1.12.1" metrics = "0.24.6" +# Already in the tree via hyper and axum-extra; naming it directly costs no +# extra compilation and replaces a hand-rolled HTTP-date parser. +httpdate = "1.0.3" [dev-dependencies] tempfile = "3.27.0" diff --git a/src/app.rs b/src/app.rs index 73f6214..cd8fb93 100644 --- a/src/app.rs +++ b/src/app.rs @@ -59,7 +59,7 @@ impl Imgforge { let cache = Cache::new(cache_config.clone()).await?; let metadata_cache = MetadataCache::new(cache_config).await?; let vips_app = Arc::new(init_vips()?); - let http_client = build_http_client(config.download_timeout)?; + let http_client = build_http_client(&config)?; let rate_limiter = build_rate_limiter(config.rate_limit_per_minute); let watermark_cache = OnceCell::new(); @@ -108,7 +108,11 @@ impl Imgforge { path: &str, bearer_token: Option<&str>, ) -> Result { - let request = crate::service::ProcessRequest { path, bearer_token }; + let request = crate::service::ProcessRequest { + path, + bearer_token, + hints: crate::negotiation::RequestHints::default(), + }; crate::service::process_path(self.state.clone(), request).await } @@ -123,7 +127,11 @@ impl Imgforge { path: &str, bearer_token: Option<&str>, ) -> Result { - let request = crate::service::ProcessRequest { path, bearer_token }; + let request = crate::service::ProcessRequest { + path, + bearer_token, + hints: crate::negotiation::RequestHints::default(), + }; crate::service::image_info(self.state.clone(), request).await } } @@ -158,9 +166,40 @@ fn init_vips() -> Result { VipsApp::new("imgforge", false).map_err(InitError::Libvips) } -fn build_http_client(timeout_secs: u64) -> Result { - let timeout = Duration::from_secs(timeout_secs); - reqwest::Client::builder().timeout(timeout).build() +/// Builds the outbound HTTP client, including the redirect policy that holds +/// every hop to the source allow list. +/// +/// Public so integration tests exercise the real policy rather than a +/// look-alike that would not catch a regression in it. +pub fn build_http_client(config: &Config) -> Result { + reqwest::Client::builder() + .timeout(Duration::from_secs(config.download_timeout)) + .user_agent(config.user_agent.clone()) + .redirect(redirect_policy(config)) + .build() +} + +/// Bounds the redirect chain, and holds it to the same allow list as the +/// original URL. +/// +/// Checking only the requested URL is checking the wrong thing: an allowed +/// origin that redirects to `http://169.254.169.254/` would be followed +/// straight past the restriction, which is the classic way an image proxy +/// becomes an SSRF gadget. Every destination is revalidated. +fn redirect_policy(config: &Config) -> reqwest::redirect::Policy { + let max_redirects = config.max_redirects; + let rules = config.source_rules.clone(); + + reqwest::redirect::Policy::custom(move |attempt| { + if attempt.previous().len() >= max_redirects { + return attempt.error("too many redirects"); + } + if !rules.permits(attempt.url().as_str()) { + warn!("Refusing a redirect to a source outside IMGFORGE_ALLOWED_SOURCES"); + return attempt.stop(); + } + attempt.follow() + }) } fn build_rate_limiter(limit_per_minute: Option) -> Option { diff --git a/src/caching/cache.rs b/src/caching/cache.rs index 8052338..344912f 100644 --- a/src/caching/cache.rs +++ b/src/caching/cache.rs @@ -35,6 +35,13 @@ fn block_size_for_capacity(capacity: usize) -> usize { pub struct CachedImage { pub bytes: Bytes, pub content_type: &'static str, + /// The URL these bytes were fetched from, after any redirects. + /// + /// Kept so a hit can be checked against the allow list as it stands now. + /// The request's own URL is validated before the lookup, but a redirect can + /// have moved the actual source somewhere that is no longer permitted, and + /// an entry outlives the policy that admitted it. + pub source_url: String, } impl Code for CachedImage { @@ -46,6 +53,10 @@ impl Code for CachedImage { let content_type_bytes = self.content_type.as_bytes(); content_type_bytes.len().encode(writer)?; writer.write_all(content_type_bytes).map_err(FoyerError::io_error)?; + + let source_bytes = self.source_url.as_bytes(); + source_bytes.len().encode(writer)?; + writer.write_all(source_bytes).map_err(FoyerError::io_error)?; Ok(()) } @@ -61,14 +72,21 @@ impl Code for CachedImage { .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in content type"))?; let content_type = format_to_content_type(content_str); + let source_len = usize::decode(reader)?; + let mut source_buf = vec![0u8; source_len]; + reader.read_exact(&mut source_buf).map_err(FoyerError::io_error)?; + let source_url = String::from_utf8(source_buf) + .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in source url"))?; + Ok(CachedImage { bytes: Bytes::from(data), content_type, + source_url, }) } fn estimated_size(&self) -> usize { - self.bytes.len() + self.content_type.len() + std::mem::size_of::() * 2 + self.bytes.len() + self.content_type.len() + self.source_url.len() + std::mem::size_of::() * 3 } } @@ -394,6 +412,7 @@ mod tests { let value = CachedImage { bytes: Bytes::from(vec![1, 2, 3]), content_type: "image/jpeg", + source_url: "https://example.test/cached.png".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); @@ -412,6 +431,7 @@ mod tests { let value = CachedImage { bytes: Bytes::from(vec![1, 2, 3]), content_type: "image/jpeg", + source_url: "https://example.test/cached.png".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); let retrieved = cache.get(&key).await.unwrap(); diff --git a/src/config/env_vars.rs b/src/config/env_vars.rs index a19f542..8d84e30 100644 --- a/src/config/env_vars.rs +++ b/src/config/env_vars.rs @@ -63,3 +63,15 @@ where .map(Some) .map_err(|source| ConfigError::InvalidSecurityLimit { name, value, source }) } + +/// Reads a comma-separated list, dropping empty entries. +pub(super) fn list_var(name: &'static str) -> Result>, ConfigError> { + Ok(optional_var(name)?.map(|value| { + value + .split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .map(str::to_string) + .collect() + })) +} diff --git a/src/config/mod.rs b/src/config/mod.rs index 170a017..9d60742 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,6 +1,9 @@ //! Server configuration, assembled from the environment at startup. mod env_vars; +pub mod source; + +pub use source::{SourcePattern, SourceRules}; use crate::constants::*; use crate::limits::{ @@ -9,12 +12,15 @@ use crate::limits::{ }; use crate::processing::options::{OptionDefaults, ProcessingOption}; use crate::processing::presets::{parse_options_string, PresetError}; -use env_vars::{bool_var, optional_var, parsed_var, security_limit_var}; +use env_vars::{bool_var, list_var, optional_var, parsed_var, security_limit_var}; use std::collections::HashMap; use std::env; use std::str::FromStr; use thiserror::Error; +/// Number of bytes in a full HMAC-SHA256 signature. +const FULL_SIGNATURE_SIZE: usize = 32; + #[derive(Debug, Error)] pub enum ConfigError { #[error("invalid IMGFORGE_KEY")] @@ -60,6 +66,8 @@ pub enum ConfigError { value: String, reason: String, }, + #[error("{name} must be between 1 and {max}")] + SignatureSizeOutOfRange { name: &'static str, max: usize }, } #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -157,6 +165,8 @@ pub struct Config { pub max_animation_frames: Option, /// Ceiling on the pixel count of a single animation frame. pub max_animation_frame_resolution: Option, + /// How many leading signature bytes a signed URL must match. + pub signature_size: usize, /// Starting values for the processing options a URL may override. pub option_defaults: OptionDefaults, @@ -181,6 +191,8 @@ pub struct Config { /// Emit `X-Origin-*` headers describing the source image. pub enable_debug_headers: bool, + /// How source URLs are resolved and restricted. + pub source_rules: SourceRules, /// `User-Agent` sent when fetching a source image. pub user_agent: String, /// How many redirects a source fetch may follow. @@ -311,6 +323,7 @@ impl Config { rate_limit_per_minute: None, max_animation_frames: None, max_animation_frame_resolution: None, + signature_size: FULL_SIGNATURE_SIZE, option_defaults: OptionDefaults::default(), ttl: None, cache_control_passthrough: false, @@ -322,6 +335,7 @@ impl Config { health_check_path: "/health".to_string(), development_errors_mode: false, enable_debug_headers: false, + source_rules: SourceRules::default(), user_agent: DEFAULT_USER_AGENT.to_string(), max_redirects: 10, enable_webp_detection: false, @@ -404,6 +418,7 @@ impl Config { config.max_animation_frames = security_limit_var(ENV_MAX_ANIMATION_FRAMES)?; config.max_animation_frame_resolution = security_limit_var(ENV_MAX_ANIMATION_FRAME_RESOLUTION)?; + config.signature_size = resolve_signature_size()?; config.option_defaults = OptionDefaults { auto_rotate: bool_var(ENV_AUTO_ROTATE, true)?, @@ -427,6 +442,14 @@ impl Config { config.development_errors_mode = bool_var(ENV_DEVELOPMENT_ERRORS_MODE, false)?; config.enable_debug_headers = bool_var(ENV_ENABLE_DEBUG_HEADERS, false)?; + config.source_rules = SourceRules { + base_url: optional_var(ENV_BASE_URL)?.filter(|value| !value.trim().is_empty()), + allowed: list_var(ENV_ALLOWED_SOURCES)? + .unwrap_or_default() + .iter() + .map(|pattern| SourcePattern::parse(pattern)) + .collect(), + }; config.user_agent = optional_var(ENV_USER_AGENT)? .filter(|value| !value.trim().is_empty()) .unwrap_or_else(|| DEFAULT_USER_AGENT.to_string()); @@ -442,6 +465,22 @@ impl Config { } } +/// A signature may be truncated to fewer bytes, which shortens the URL at the +/// cost of collision resistance. Zero would accept anything, and more than a +/// full HMAC-SHA256 could never match. +fn resolve_signature_size() -> Result { + let Some(size) = parsed_var::(ENV_SIGNATURE_SIZE)? else { + return Ok(FULL_SIGNATURE_SIZE); + }; + if size == 0 || size > FULL_SIGNATURE_SIZE { + return Err(ConfigError::SignatureSizeOutOfRange { + name: ENV_SIGNATURE_SIZE, + max: FULL_SIGNATURE_SIZE, + }); + } + Ok(size) +} + /// Normalises a mount prefix to either empty or `/segment` with no trailing /// slash, so routes can be built by concatenation without doubling separators. fn normalize_path_prefix(prefix: Option<&str>) -> String { diff --git a/src/config/source.rs b/src/config/source.rs new file mode 100644 index 0000000..f8b0d88 --- /dev/null +++ b/src/config/source.rs @@ -0,0 +1,352 @@ +//! Which source URLs a deployment will fetch, and how a bare path becomes one. + +use reqwest::Url; +use tracing::debug; + +/// One entry of `IMGFORGE_ALLOWED_SOURCES`. +/// +/// imgproxy matches a prefix, with `*` allowed as a wildcard for one host +/// label — `https://*.example.com/` permits `images.example.com` but not +/// `example.com` or `a.b.example.com`. +/// +/// The candidate is compared as a *parsed URL* rather than as a string. Three +/// separate bypasses came out of comparing text, each closed only where it was +/// found: `https://cdn.example.com` matched `cdn.example.com.evil.test` because +/// the string starts the same way; the `user@` form hid the real host behind +/// userinfo; and `/public/../private/x` satisfied a `/public/` prefix while the +/// URL parser resolved the dots and fetched `/private/x`. Every one of them is +/// a case where the bytes imgforge inspected and the address reqwest dialled +/// were not the same thing. Parsing first makes them the same thing by +/// construction, which is why this is a parse rather than a fourth check. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SourcePattern { + scheme: String, + host: HostMatch, + /// Port the entry named, when it named one. + port: Option, + /// The path the entry restricts to, already normalised. + path_prefix: String, +} + +/// How an entry's host half is compared against a URL's host. +#[derive(Debug, Clone, PartialEq, Eq)] +enum HostMatch { + /// The host must equal this exactly. + Exact(String), + /// The host must end with this after exactly one further label. + Suffix(String), + /// The entry could not be parsed as a URL prefix at all, so it is compared + /// as literal text. imgproxy documents entries with a scheme; this only + /// keeps a malformed one behaving as it did rather than silently matching + /// nothing, and it can never be more permissive than the string it names. + Unstructured, +} + +impl SourcePattern { + pub fn parse(pattern: &str) -> Self { + let unstructured = || Self { + scheme: pattern.to_string(), + host: HostMatch::Unstructured, + port: None, + path_prefix: String::new(), + }; + + // The wildcard is not a legal host, so the entry cannot be parsed as a + // URL directly. Substituting a placeholder label lets the same parser + // handle both forms, and keeps the port and path handling identical. + let (probe, wildcard) = match pattern.split_once("://*.") { + Some((scheme, rest)) => (format!("{scheme}://imgforge-wildcard.{rest}"), true), + None => (pattern.to_string(), false), + }; + + let Ok(parsed) = Url::parse(&probe) else { + return unstructured(); + }; + let Some(host) = parsed.host_str() else { + return unstructured(); + }; + + let host = if wildcard { + match host.split_once('.') { + Some((_, suffix)) => HostMatch::Suffix(format!(".{suffix}")), + None => return unstructured(), + } + } else { + HostMatch::Exact(host.to_string()) + }; + + Self { + scheme: parsed.scheme().to_string(), + host, + port: parsed.port(), + // `Url` has already resolved any dot segments here, so an entry and + // a candidate are compared in the same normalised terms. + path_prefix: normalise_prefix(parsed.path(), pattern), + } + } + + pub fn matches(&self, url: &str) -> bool { + if matches!(self.host, HostMatch::Unstructured) { + return url.starts_with(&self.scheme); + } + + let Ok(parsed) = Url::parse(url) else { + return false; + }; + if parsed.scheme() != self.scheme || parsed.port() != self.port { + return false; + } + let Some(host) = parsed.host_str() else { + return false; + }; + + let host_ok = match &self.host { + HostMatch::Exact(expected) => host.eq_ignore_ascii_case(expected), + HostMatch::Suffix(suffix) => { + let host = host.to_ascii_lowercase(); + match host.strip_suffix(suffix.as_str()) { + // The wildcard stands for exactly one label, so what + // precedes the suffix must be a single non-empty label. + Some(label) => !label.is_empty() && !label.contains('.'), + None => false, + } + } + HostMatch::Unstructured => unreachable!("handled above"), + }; + + // `Url::path()` is normalised, so `/public/../private/x` arrives here as + // `/private/x` — the path reqwest will actually request. + host_ok && parsed.path().starts_with(&self.path_prefix) + } +} + +/// The path an entry restricts to. +/// +/// `Url::parse` gives a bare host the path `/`, which as a prefix would permit +/// the whole host — correct when the entry named no path, wrong if it did. The +/// original text decides which of those the operator meant. +fn normalise_prefix(path: &str, pattern: &str) -> String { + let named_a_path = pattern + .split_once("://") + .is_some_and(|(_, rest)| rest.contains('/') && !rest.ends_with("://")); + if !named_a_path && path == "/" { + return String::new(); + } + path.to_string() +} + +/// How source URLs are resolved and restricted. +#[derive(Debug, Clone, Default)] +pub struct SourceRules { + /// Prepended to every source URL, so URLs can carry only a path. + pub base_url: Option, + /// When non-empty, a source URL must match one of these to be fetched. + pub allowed: Vec, +} + +impl SourceRules { + /// Applies the base URL to a decoded source reference. + pub fn resolve(&self, url: &str) -> String { + let Some(base) = self.base_url.as_deref() else { + return url.to_string(); + }; + // A URL that already names a scheme is complete; the base is for the + // shorthand form where the URL carries only a path. + if url.contains("://") { + return url.to_string(); + } + format!("{}{}", base.trim_end_matches('/'), ensure_leading_slash(url)) + } + + /// Whether a resolved source URL may be fetched. + pub fn permits(&self, url: &str) -> bool { + if self.allowed.is_empty() { + return true; + } + let permitted = self.allowed.iter().any(|pattern| pattern.matches(url)); + if !permitted { + debug!("Source URL rejected by IMGFORGE_ALLOWED_SOURCES"); + } + permitted + } +} + +fn ensure_leading_slash(path: &str) -> String { + if path.starts_with('/') { + path.to_string() + } else { + format!("/{path}") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_wildcard_stands_for_exactly_one_label() { + let pattern = SourcePattern::parse("https://*.example.com/"); + + assert!(pattern.matches("https://images.example.com/cat.jpg")); + // Bare domain and multi-label subdomains are both outside the pattern, + // which is what keeps `*.example.com` from being read as "anything + // ending in example.com". + assert!(!pattern.matches("https://example.com/cat.jpg")); + assert!(!pattern.matches("https://a.b.example.com/cat.jpg")); + assert!(!pattern.matches("http://images.example.com/cat.jpg")); + // An attacker-controlled host must not be able to smuggle the suffix + // into the path. + assert!(!pattern.matches("https://evil.com/.example.com/cat.jpg")); + } + + /// The suffix has to end the authority, not merely appear in it. Without + /// that anchor an attacker registers `example.com.evil.test`, prefixes a + /// label, and is fetched from — which is the whole of the SSRF the allow + /// list exists to prevent. + #[test] + fn a_wildcard_cannot_be_extended_into_another_domain() { + // Both spellings of the pattern have to hold: only the trailing slash + // was ever incidentally anchored. + for spelling in ["https://*.example.com", "https://*.example.com/"] { + let pattern = SourcePattern::parse(spelling); + + assert!(pattern.matches("https://img.example.com/cat.jpg"), "{spelling}"); + assert!( + !pattern.matches("https://img.example.com.evil.test/cat.jpg"), + "{spelling} must not admit a longer domain that merely starts the same way" + ); + assert!( + !pattern.matches("https://img.example.com.evil.test/.example.com/x"), + "{spelling} must not be satisfied by a suffix hidden in the path" + ); + // `user@host` puts the real host after the authority's `@`, which + // is the same trick spelled with credentials. + assert!( + !pattern.matches("https://img.example.com@evil.test/cat.jpg"), + "{spelling} must not admit a host smuggled past userinfo" + ); + // A query or fragment ends the authority just as a slash does. + assert!( + !pattern.matches("https://img.example.com.evil.test?a=.example.com"), + "{spelling} must not be satisfied from the query string" + ); + } + } + + /// The literal form needs the same anchoring as the wildcard. Fixing only + /// the wildcard branch left `https://cdn.example.com` — the spelling most + /// operators reach for — matching `cdn.example.com.evil.test`, which is the + /// whole of the SSRF the allow list exists to prevent. + #[test] + fn a_literal_entry_cannot_be_extended_into_another_domain() { + for spelling in ["https://cdn.example.com", "https://cdn.example.com/"] { + let pattern = SourcePattern::parse(spelling); + + assert!(pattern.matches("https://cdn.example.com/cat.jpg"), "{spelling}"); + assert!( + !pattern.matches("https://cdn.example.com.evil.test/cat.jpg"), + "{spelling} must not admit a longer domain that starts the same way" + ); + assert!( + !pattern.matches("https://cdn.example.com@evil.test/cat.jpg"), + "{spelling} must not admit a host smuggled past userinfo" + ); + assert!( + !pattern.matches("https://cdn.example.com.evil.test?a=cdn.example.com"), + "{spelling} must not be satisfied from the query string" + ); + // A different scheme is a different origin. + assert!(!pattern.matches("http://cdn.example.com/cat.jpg"), "{spelling}"); + // A port is part of the authority, so an entry naming none does not + // match a URL that does. + assert!(!pattern.matches("https://cdn.example.com:8443/cat.jpg"), "{spelling}"); + } + + // An entry that names a port matches that port. + let ported = SourcePattern::parse("https://cdn.example.com:8443/"); + assert!(ported.matches("https://cdn.example.com:8443/cat.jpg")); + assert!(!ported.matches("https://cdn.example.com/cat.jpg")); + } + + /// A path prefix has to survive dot segments. `/public/../private/x` + /// satisfies a naive `/public/` prefix while the URL parser resolves the + /// dots and fetches `/private/x` — the check and the request disagreeing + /// about what the URL says, which is the whole shape of this bug class. + #[test] + fn a_path_prefix_cannot_be_escaped_by_traversal() { + for spelling in ["https://cdn.example.com/public/", "https://*.example.com/public/"] { + let pattern = SourcePattern::parse(spelling); + let host = if spelling.contains('*') { + "img.example.com" + } else { + "cdn.example.com" + }; + + assert!(pattern.matches(&format!("https://{host}/public/cat.jpg")), "{spelling}"); + assert!( + !pattern.matches(&format!("https://{host}/public/../private/secret.png")), + "{spelling} must not admit a dot-segment escape" + ); + assert!( + !pattern.matches(&format!("https://{host}/public/../../etc/passwd")), + "{spelling} must not admit a deeper escape" + ); + // Percent-encoded dots resolve the same way once parsed. + assert!( + !pattern.matches(&format!("https://{host}/public/%2e%2e/private/secret.png")), + "{spelling} must not admit an encoded escape" + ); + // A path that merely contains "..'" in a segment name is not a + // traversal and stays permitted. + assert!( + pattern.matches(&format!("https://{host}/public/a..b/cat.jpg")), + "{spelling}" + ); + } + } + + /// Hosts are compared case-insensitively, as DNS does. + #[test] + fn host_matching_ignores_case() { + assert!(SourcePattern::parse("https://cdn.example.com/").matches("https://CDN.Example.COM/cat.jpg")); + assert!(SourcePattern::parse("https://*.example.com/").matches("https://IMG.Example.com/cat.jpg")); + } + + /// A wildcard may also constrain the path, and that half stays a prefix. + #[test] + fn a_wildcard_can_carry_a_path_prefix() { + let pattern = SourcePattern::parse("https://*.example.com/assets/"); + + assert!(pattern.matches("https://img.example.com/assets/cat.jpg")); + assert!(!pattern.matches("https://img.example.com/private/cat.jpg")); + assert!(!pattern.matches("https://img.example.com.evil.test/assets/cat.jpg")); + } + + #[test] + fn a_plain_pattern_matches_by_prefix() { + let pattern = SourcePattern::parse("https://cdn.example.com/assets/"); + assert!(pattern.matches("https://cdn.example.com/assets/cat.jpg")); + assert!(!pattern.matches("https://cdn.example.com/private/cat.jpg")); + } + + #[test] + fn an_empty_allow_list_permits_everything() { + let rules = SourceRules::default(); + assert!(rules.permits("https://anywhere.example/cat.jpg")); + } + + #[test] + fn the_base_url_only_applies_to_relative_references() { + let rules = SourceRules { + base_url: Some("https://cdn.example.com/".to_string()), + allowed: Vec::new(), + }; + + assert_eq!(rules.resolve("cat.jpg"), "https://cdn.example.com/cat.jpg"); + assert_eq!(rules.resolve("/cat.jpg"), "https://cdn.example.com/cat.jpg"); + assert_eq!( + rules.resolve("https://other.example/cat.jpg"), + "https://other.example/cat.jpg" + ); + } +} diff --git a/src/constants.rs b/src/constants.rs index 838b3ee..28de0eb 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -3,6 +3,8 @@ pub const ENV_KEY: &str = "IMGFORGE_KEY"; pub const ENV_SALT: &str = "IMGFORGE_SALT"; pub const ENV_SECRET: &str = "IMGFORGE_SECRET"; pub const ENV_ALLOW_UNSIGNED: &str = "IMGFORGE_ALLOW_UNSIGNED"; +/// Number of leading signature bytes that must match, for truncated signatures. +pub const ENV_SIGNATURE_SIZE: &str = "IMGFORGE_SIGNATURE_SIZE"; pub const ENV_MAX_SRC_FILE_SIZE: &str = "IMGFORGE_MAX_SRC_FILE_SIZE"; pub const ENV_ALLOWED_MIME_TYPES: &str = "IMGFORGE_ALLOWED_MIME_TYPES"; pub const ENV_MAX_SRC_RESOLUTION: &str = "IMGFORGE_MAX_SRC_RESOLUTION"; @@ -52,6 +54,8 @@ pub const ENV_DEVELOPMENT_ERRORS_MODE: &str = "IMGFORGE_DEVELOPMENT_ERRORS_MODE" pub const ENV_ENABLE_DEBUG_HEADERS: &str = "IMGFORGE_ENABLE_DEBUG_HEADERS"; // Source resolution. +pub const ENV_BASE_URL: &str = "IMGFORGE_BASE_URL"; +pub const ENV_ALLOWED_SOURCES: &str = "IMGFORGE_ALLOWED_SOURCES"; pub const ENV_USER_AGENT: &str = "IMGFORGE_USER_AGENT"; pub const ENV_MAX_REDIRECTS: &str = "IMGFORGE_MAX_REDIRECTS"; diff --git a/src/fetch.rs b/src/fetch.rs index 9dbb830..a8e1f6a 100644 --- a/src/fetch.rs +++ b/src/fetch.rs @@ -29,6 +29,14 @@ pub enum FetchError { #[derive(Debug, Clone, Default)] pub struct FetchedImage { pub bytes: Bytes, + /// The URL the bytes actually came from, after any redirects. + /// + /// Not the same question as the URL that was requested: the allow list is + /// checked against what a request *asks* for, and a redirect can move the + /// answer somewhere else. The redirect policy revalidates each hop as it + /// happens, which leaves only the cache — an entry outlives the fetch, so + /// it has to remember where its bytes came from. + pub final_url: String, pub content_type: Option, pub cache_control: Option, pub last_modified: Option, @@ -74,6 +82,8 @@ pub async fn fetch_image( // An error page is not an image. Returning its bytes meant the failure // surfaced later as "failed to decode source image", which told the caller // nothing about the 404 that actually happened. + let final_url = response.url().to_string(); + if !response.status().is_success() { record_fetch_metrics(fetch_start, "error"); return Err(FetchError::UpstreamStatus { @@ -127,6 +137,7 @@ pub async fn fetch_image( record_fetch_metrics(fetch_start, "success"); Ok(FetchedImage { bytes: image_bytes.freeze(), + final_url, content_type, cache_control, last_modified, diff --git a/src/handlers.rs b/src/handlers.rs index 6dd55dc..3351350 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -1,8 +1,10 @@ use crate::app::AppState; -use crate::service::{self, CacheStatus, ProcessRequest}; +use crate::negotiation::RequestHints; +use crate::response::{matches_if_modified_since, matches_if_none_match}; +use crate::service::{self, CacheStatus, DebugInfo, ProcessRequest, ProcessedImage}; use axum::extract::{Path, State}; -use axum::http::{header, HeaderValue, StatusCode}; -use axum::response::{IntoResponse, Json}; +use axum::http::{header, HeaderMap, HeaderName, HeaderValue, StatusCode}; +use axum::response::{IntoResponse, Json, Response}; use axum_extra::headers::{authorization::Bearer, Authorization}; use axum_extra::TypedHeader; use serde_json::json; @@ -27,6 +29,7 @@ pub async fn info_handler( ProcessRequest { path: &path, bearer_token: bearer.as_deref(), + hints: RequestHints::default(), }, ) .await @@ -47,7 +50,7 @@ pub async fn info_handler( } Err(err) => { error!(path, error = ?err, "Info handler error"); - (err.status(), err.message().into_owned()).into_response() + error_response(state.as_ref(), &err) } } } @@ -56,44 +59,144 @@ pub async fn info_handler( pub async fn image_forge_handler( State(state): State>, Path(path): Path, + request_headers: HeaderMap, auth_header: Option>>, ) -> impl IntoResponse { let bearer = auth_header.map(|TypedHeader(auth)| auth.token().to_string()); + let hints = RequestHints::from_headers(&request_headers, state.config.enable_client_hints); match service::process_path( - state, + state.clone(), ProcessRequest { path: &path, bearer_token: bearer.as_deref(), + hints, }, ) .await { - Ok(result) => { - let mut headers = header::HeaderMap::new(); - headers.insert(header::CONTENT_TYPE, HeaderValue::from_static(result.content_type)); - if result.cache_status == CacheStatus::Hit { - headers.insert( - header::CACHE_STATUS, - HeaderValue::from_static(CacheStatus::Hit.as_header_value()), - ); - } - if let Some(content_disposition) = result.content_disposition { - match HeaderValue::from_str(&content_disposition) { - Ok(value) => { - headers.insert(header::CONTENT_DISPOSITION, value); - } - Err(err) => { - error!("Invalid Content-Disposition header value: {}", err); - } - } - } - - (StatusCode::OK, headers, result.bytes).into_response() - } + Ok(result) => image_response(state.as_ref(), &request_headers, result), Err(err) => { error!(path, error = ?err, "Image handler error"); - (err.status(), err.message().into_owned()).into_response() + error_response(state.as_ref(), &err) + } + } +} + +/// Builds the response for a processed image, including the conditional-request +/// short circuit. +fn image_response(state: &AppState, request_headers: &HeaderMap, result: ProcessedImage) -> Response { + let mut headers = header::HeaderMap::new(); + headers.insert(header::CONTENT_TYPE, HeaderValue::from_static(result.content_type)); + + if result.cache_status == CacheStatus::Hit { + headers.insert( + header::CACHE_STATUS, + HeaderValue::from_static(CacheStatus::Hit.as_header_value()), + ); + } + + if let Some(content_disposition) = result.content_disposition.as_deref() { + insert_header(&mut headers, header::CONTENT_DISPOSITION, content_disposition); + } + + let delivery = &result.headers; + if let Some(cache_control) = delivery.cache_control.as_deref() { + insert_header(&mut headers, header::CACHE_CONTROL, cache_control); + } + if let Some(last_modified) = delivery.last_modified.as_deref() { + insert_header(&mut headers, header::LAST_MODIFIED, last_modified); + } + if let Some(canonical) = delivery.canonical.as_deref() { + insert_header(&mut headers, header::LINK, canonical); + } + if !delivery.vary.is_empty() { + insert_header(&mut headers, header::VARY, &delivery.vary.join(", ")); + } + if let Some(origin) = state.config.allow_origin.as_deref() { + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); + } + if let Some(debug) = result.debug { + insert_debug_headers(&mut headers, debug); + } + + // The body was produced either way — the saving is bandwidth, not work — + // but for a large image over a slow link that is the saving that matters. + // + // RFC 9110 makes `If-None-Match` take precedence over `If-Modified-Since` + // only when the request *carries* one. Keying that on whether an ETag was + // emitted instead meant a deployment with both features on never evaluated + // `If-Modified-Since` at all, and answered a conditional request that sent + // only that header with the whole body. + let mut not_modified = false; + if let Some(etag) = delivery.etag.as_deref() { + insert_header(&mut headers, header::ETAG, etag); + not_modified = matches_if_none_match(request_headers, etag); + } + if !not_modified && !request_headers.contains_key(header::IF_NONE_MATCH) { + if let Some(last_modified) = delivery.last_modified.as_deref() { + not_modified = matches_if_modified_since(request_headers, last_modified); } } + + if not_modified { + return (StatusCode::NOT_MODIFIED, headers).into_response(); + } + + (StatusCode::OK, headers, result.bytes).into_response() +} + +/// Emits the sizes the response actually knows. +/// +/// A zero is "not measured" rather than a measurement: a cache hit never made +/// the source request, so it can report the result it is holding but nothing +/// about the origin. Sending `x-origin-width: 0` there would be a false +/// statement dressed as a diagnostic, so the unknown fields are simply omitted. +fn insert_debug_headers(headers: &mut header::HeaderMap, debug: DebugInfo) { + let values = [ + ("x-origin-content-length", debug.origin_bytes as u64), + ("x-origin-width", u64::from(debug.origin_width)), + ("x-origin-height", u64::from(debug.origin_height)), + ("x-result-width", u64::from(debug.result_width)), + ("x-result-height", u64::from(debug.result_height)), + ]; + + for (name, value) in values { + if value == 0 { + continue; + } + if let Ok(name) = HeaderName::from_bytes(name.as_bytes()) { + headers.insert(name, HeaderValue::from(value)); + } + } +} + +/// A header value that cannot be represented is dropped rather than failing the +/// response: the image is still correct without it. +fn insert_header(headers: &mut header::HeaderMap, name: HeaderName, value: &str) { + match HeaderValue::from_str(value) { + Ok(value) => { + headers.insert(name, value); + } + Err(err) => error!("Invalid value for the {} header: {}", name, err), + } +} + +/// Turns a service failure into a response. +/// +/// The client normally sees only the curated message; development-errors mode +/// adds the underlying cause, which is what makes a misconfigured deployment +/// diagnosable without reading the server's logs. +fn error_response(state: &AppState, err: &service::ServiceError) -> Response { + let mut body = err.message().into_owned(); + if state.config.development_errors_mode { + body.push_str(&format!("\n\n{err:?}")); + } + + let mut headers = header::HeaderMap::new(); + if let Some(origin) = state.config.allow_origin.as_deref() { + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); + } + + (err.status(), headers, body).into_response() } diff --git a/src/lib.rs b/src/lib.rs index 2abc00d..96305cb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,9 +7,13 @@ pub mod handlers; pub mod limits; pub mod middleware; pub mod monitoring; +pub mod negotiation; pub mod processing; +pub mod response; pub mod server; pub mod service; +#[cfg(test)] +mod test_support; pub mod url; pub mod utils; @@ -17,6 +21,7 @@ pub use app::{AppState, Imgforge, InitError}; pub use config::{DefaultOutputFormat, DefaultOutputFormatParseError}; pub use fetch::FetchError; pub use limits::{MaxSourceFileSize, MaxSourceResolution, SecurityLimitError}; +pub use negotiation::RequestHints; pub use processing::options::OptionParseError; pub use processing::presets::PresetError; pub use processing::save::SaveError; @@ -24,6 +29,7 @@ pub use processing::transform::TransformError; pub use processing::utils::ColorParseError; pub use processing::watermark::WatermarkError; pub use processing::ProcessingError; +pub use response::DeliveryHeaders; pub use server::ServerError; -pub use service::{CacheStatus, ImageInfo, ProcessRequest, ProcessedImage, ServiceError}; +pub use service::{CacheStatus, DebugInfo, ImageInfo, ProcessRequest, ProcessedImage, ServiceError}; pub use url::SourceUrlDecodeError; diff --git a/src/negotiation.rs b/src/negotiation.rs new file mode 100644 index 0000000..7389ae8 --- /dev/null +++ b/src/negotiation.rs @@ -0,0 +1,449 @@ +//! Choosing what to send based on what the client said it can take. +//! +//! Two independent mechanisms, both from imgproxy. Format negotiation reads +//! `Accept` and upgrades the output to WebP or AVIF when the client advertises +//! it, which is how one URL can serve a modern format to browsers that support +//! it and JPEG to everything else. Client hints read `Width` and `DPR`, letting +//! the browser rather than the URL decide how large the image needs to be. + +use crate::config::Config; +use crate::processing::options::ParsedOptions; +use crate::processing::save::is_format_supported; +use axum::http::HeaderMap; +use tracing::debug; + +/// What the request told us about the client. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct RequestHints { + /// The raw `Accept` header, used for format negotiation. + pub accept: Option, + /// Device pixel ratio the client reported. + pub dpr: Option, + /// Layout width in CSS pixels the client reported. + pub width: Option, +} + +impl RequestHints { + /// Reads the hints a request carries. + /// + /// Client hints are only read when the server opted in: they let the client + /// change the response for a URL that does not mention them, which is fine + /// when a deployment expects it and surprising when it does not. + pub fn from_headers(headers: &HeaderMap, enable_client_hints: bool) -> Self { + let header = |name: &str| headers.get(name).and_then(|value| value.to_str().ok()); + + let accept = header("accept").map(str::to_string); + if !enable_client_hints { + return Self { + accept, + ..Self::default() + }; + } + + // The `Sec-CH-` spellings are the current ones; the bare names are what + // older clients send, and imgproxy accepts both. + let dpr = header("sec-ch-dpr") + .or_else(|| header("dpr")) + .and_then(|value| value.trim().parse::().ok()) + .filter(|dpr| dpr.is_finite() && *dpr > 0.0); + let width = header("sec-ch-width") + .or_else(|| header("width")) + .and_then(|value| value.trim().parse::().ok()) + .filter(|width| *width > 0); + + Self { accept, dpr, width } + } + + /// How strongly the `Accept` header advertises a media type. + /// + /// `None` means the type is not offered at all, which includes an explicit + /// `q=0` refusal. Otherwise the weight the client attached to it, defaulting + /// to 1 when it named none. + fn quality_for(&self, media_type: &str) -> Option { + let accept = self.accept.as_deref()?; + + accept + .split(',') + .filter_map(|range| { + let mut parts = range.split(';').map(str::trim); + let name = parts.next()?; + if !name.eq_ignore_ascii_case(media_type) { + return None; + } + + // Anything unparseable, or absent, is the default of 1. + Some( + parts + .filter_map(|parameter| parameter.strip_prefix("q=")) + .next() + .and_then(|q| q.parse::().ok()) + .unwrap_or(1.0), + ) + }) + // A repeated type is the client's own contradiction; taking the + // strongest offer is the reading that serves it something. + .fold(None::, |best, q| Some(best.map_or(q, |best| best.max(q)))) + .filter(|q| *q > 0.0) + } +} + +/// Formats that can be negotiated, best compression first. +/// +/// The order breaks ties: a client that offers both at the same weight gets +/// AVIF, which is smaller than WebP at equal quality. It does not override the +/// client, which gets to rank them itself with `q`. +const NEGOTIABLE: &[(&str, &str)] = &[("avif", "image/avif"), ("webp", "image/webp")]; + +/// Picks the output format for a request, or `None` to leave it alone. +/// +/// A format named in the URL normally wins — that is the point of naming it — +/// but `enforce` overrides even that, which is what lets a deployment move its +/// whole catalogue to a modern format without rewriting the URLs. +pub fn negotiate_format(config: &Config, hints: &RequestHints, has_explicit_format: bool) -> Option<&'static str> { + let mut best: Option<(&'static str, f32)> = None; + + for (format, media_type) in NEGOTIABLE { + let (detect, enforce) = match *format { + "avif" => (config.enable_avif_detection, config.enforce_avif), + "webp" => (config.enable_webp_detection, config.enforce_webp), + _ => continue, + }; + + if !detect && !enforce { + continue; + } + if has_explicit_format && !enforce { + continue; + } + let Some(quality) = hints.quality_for(media_type) else { + continue; + }; + // A build without the encoder would turn negotiation into a 400 for + // every modern browser. + if !is_format_supported(format) { + debug!("{} was negotiated but this libvips build cannot encode it", format); + continue; + } + + // The client ranked these, so serving the first one imgforge happens to + // prefer would ignore what it said: `image/avif;q=0.1, image/webp` asks + // for WebP. The strict comparison keeps NEGOTIABLE's order as the + // tie-break, so an equal-weight offer still resolves to AVIF. + if best.is_none_or(|(_, best_quality)| quality > best_quality) { + best = Some((format, quality)); + } + } + + let (format, quality) = best?; + debug!("Negotiated {} from the client's Accept header (q={})", format, quality); + Some(format) +} + +/// The request headers a response can differ by, for `Vary`. +/// +/// A shared cache has to be told, or it will hand an AVIF to a client that +/// cannot read one — or, with client hints on, hand one client's dimensions to +/// another. +pub fn vary_headers(config: &Config) -> Vec<&'static str> { + let mut headers = Vec::new(); + + if config.enable_webp_detection || config.enforce_webp || config.enable_avif_detection || config.enforce_avif { + headers.push("Accept"); + } + + if config.enable_client_hints { + headers.extend_from_slice(&["Sec-CH-Width", "Width", "Sec-CH-DPR", "DPR"]); + } + + headers +} + +/// Folds the client's own size hints into the processing options. +/// +/// The URL still wins: a request that already names a width is asking for that +/// width, and a hint is the client's suggestion for a URL that left the choice +/// open. +pub fn apply_client_hints(options: &mut ParsedOptions, hints: &RequestHints) { + if let Some(dpr) = hints.dpr { + if options.dpr.is_none_or(|configured| configured <= 1.0) { + debug!("Applying client DPR hint: {}", dpr); + options.dpr = Some(dpr.clamp(1.0, 5.0)); + } + } + + let Some(width) = hints.width else { + return; + }; + + // `Width` is already in physical pixels — that is what the client hint + // means — while `dpr` is multiplied back onto the resize target later in + // the pipeline. Taking the hint at face value therefore applied the ratio + // twice: `Width: 640` with `DPR: 2` produced a 1280px image for a client + // that asked for 640. Dividing here is what imgproxy does for the same + // reason, as `imath.Shrink(features.ClientHintsWidth, dpr)`. + // + // The divisor is the *client's* reported ratio, not the effective one. The + // hint describes that client's own screen, so it is the only ratio the + // number was expressed against; a `dpr:` in the URL is a separate + // instruction that multiplies afterwards. imgproxy reads the hints before + // URL options are applied, which produces exactly this ordering. + let hinted_dpr = hints.dpr.map_or(1.0, |dpr| dpr.clamp(1.0, 5.0)); + let width = if hinted_dpr > 1.0 { + let shrunk = (f64::from(width) / f64::from(hinted_dpr)).round() as u32; + debug!("Client width hint {} is {} before DPR {}", width, shrunk, hinted_dpr); + shrunk.max(1) + } else { + width + }; + + match options.resize.as_mut() { + Some(resize) if resize.width == 0 => { + debug!("Applying client width hint: {}", width); + resize.width = width; + } + Some(_) => {} + None => { + debug!("Applying client width hint as the resize target: {}", width); + options.width = Some(width); + options.resize = Some(crate::processing::options::Resize { + resizing_type: crate::processing::options::ResizingType::Fit, + width, + height: 0, + }); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::processing::options::ResizingType; + use crate::test_support::init_vips; + + fn config_with(detection: (bool, bool), enforcement: (bool, bool)) -> Config { + let mut config = Config::new(vec![0u8; 32], vec![0u8; 32]); + config.enable_webp_detection = detection.0; + config.enable_avif_detection = detection.1; + config.enforce_webp = enforcement.0; + config.enforce_avif = enforcement.1; + config + } + + fn accepting(accept: &str) -> RequestHints { + RequestHints { + accept: Some(accept.to_string()), + ..RequestHints::default() + } + } + + #[test] + fn detection_only_applies_when_the_url_left_the_format_open() { + init_vips(); + let config = config_with((true, false), (false, false)); + let hints = accepting("image/webp,image/*,*/*"); + + assert_eq!(negotiate_format(&config, &hints, false), Some("webp")); + // A URL that names a format is asking for it. + assert_eq!(negotiate_format(&config, &hints, true), None); + // A client that does not advertise WebP never gets one. + assert_eq!(negotiate_format(&config, &accepting("image/*"), false), None); + } + + #[test] + fn enforcement_overrides_an_explicit_format() { + init_vips(); + let config = config_with((false, false), (true, false)); + let hints = accepting("image/webp"); + + assert_eq!(negotiate_format(&config, &hints, true), Some("webp")); + } + + #[test] + fn avif_is_preferred_when_the_client_takes_both() { + init_vips(); + let config = config_with((true, true), (false, false)); + let hints = accepting("image/avif,image/webp,*/*"); + + assert_eq!(negotiate_format(&config, &hints, false), Some("avif")); + + // With only WebP detection enabled, an AVIF-capable client still gets + // WebP: negotiation offers what the deployment turned on. + let config = config_with((true, false), (false, false)); + assert_eq!(negotiate_format(&config, &hints, false), Some("webp")); + } + + #[test] + fn a_zero_quality_is_a_refusal_rather_than_an_offer() { + init_vips(); + let config = config_with((true, true), (false, false)); + + // The name is present, and explicitly refused. + assert_eq!( + negotiate_format(&config, &accepting("image/avif;q=0, image/webp"), false), + Some("webp") + ); + assert_eq!(negotiate_format(&config, &accepting("image/webp;q=0.0"), false), None); + // A positive quality still counts, and matching is case-insensitive. + assert_eq!( + negotiate_format(&config, &accepting("IMAGE/WEBP;q=0.5"), false), + Some("webp") + ); + // A substring of an unrelated range must not match. + assert_eq!( + negotiate_format(&config, &accepting("application/image/webp+xml"), false), + None + ); + } + + /// `q` ranks the offers; it does not merely gate them. Reducing it to + /// "greater than zero" made the fixed AVIF-first order override a client + /// that had explicitly said it would rather have WebP. + #[test] + fn the_clients_quality_weights_decide_between_two_offers() { + init_vips(); + let config = config_with((true, true), (false, false)); + + assert_eq!( + negotiate_format(&config, &accepting("image/avif;q=0.1, image/webp;q=1"), false), + Some("webp"), + "a client that prefers WebP must be given WebP" + ); + assert_eq!( + negotiate_format(&config, &accepting("image/avif;q=1, image/webp;q=0.1"), false), + Some("avif") + ); + // An unweighted offer is q=1, so it outranks a weighted one below it. + assert_eq!( + negotiate_format(&config, &accepting("image/avif;q=0.5, image/webp"), false), + Some("webp") + ); + // Equal weights fall back to imgforge's own order, which prefers the + // format that compresses better. + assert_eq!( + negotiate_format(&config, &accepting("image/avif;q=0.8, image/webp;q=0.8"), false), + Some("avif") + ); + assert_eq!( + negotiate_format(&config, &accepting("image/avif,image/webp,*/*"), false), + Some("avif") + ); + } + + #[test] + fn client_hints_are_ignored_unless_enabled() { + let mut headers = HeaderMap::new(); + headers.insert("dpr", "2".parse().unwrap()); + headers.insert("width", "800".parse().unwrap()); + headers.insert("accept", "image/webp".parse().unwrap()); + + let ignored = RequestHints::from_headers(&headers, false); + assert_eq!(ignored.dpr, None); + assert_eq!(ignored.width, None); + // Accept is not a client hint and is always read. + assert_eq!(ignored.accept.as_deref(), Some("image/webp")); + + let honoured = RequestHints::from_headers(&headers, true); + assert_eq!(honoured.dpr, Some(2.0)); + assert_eq!(honoured.width, Some(800)); + } + + /// `Width` is defined in physical pixels, and `dpr` is multiplied back onto + /// the resize target further down the pipeline. Taking the hint at face + /// value applied the ratio twice, so a client asking for 640 was sent 1280. + #[test] + fn the_width_hint_is_not_multiplied_by_dpr_twice() { + use crate::processing::options::Resize; + + let hints = RequestHints { + width: Some(640), + dpr: Some(2.0), + ..RequestHints::default() + }; + + let mut options = ParsedOptions::default(); + apply_client_hints(&mut options, &hints); + + // Stored at 320 so that the later DPR pass brings it back to the 640 + // physical pixels the client actually asked for. + assert_eq!(options.resize.expect("the hint supplies a width").width, 320); + assert_eq!(options.dpr, Some(2.0)); + + // Without a DPR hint the width is already what it should be. + let mut options = ParsedOptions::default(); + apply_client_hints( + &mut options, + &RequestHints { + width: Some(640), + ..RequestHints::default() + }, + ); + assert_eq!(options.resize.unwrap().width, 640); + + // A width that divides to nothing still has to describe an image. + let mut options = ParsedOptions::default(); + apply_client_hints( + &mut options, + &RequestHints { + width: Some(1), + dpr: Some(5.0), + ..RequestHints::default() + }, + ); + assert_eq!(options.resize.unwrap().width, 1); + + // The divisor is the client's own ratio. A `dpr:` in the URL is a + // separate instruction applied afterwards, so it must not change how the + // hint is read — imgproxy reads the hints before URL options for exactly + // this reason. + let mut options = ParsedOptions { + dpr: Some(3.0), + ..ParsedOptions::default() + }; + apply_client_hints(&mut options, &hints); + assert_eq!( + options.resize.unwrap().width, + 320, + "the URL's own dpr must not become the divisor for the client's hint" + ); + assert_eq!(options.dpr, Some(3.0), "and the URL's dpr still wins for scaling"); + + // The same division applies when the hint fills a zero-width resize the + // URL already named. + let mut options = ParsedOptions { + resize: Some(Resize { + resizing_type: ResizingType::Fill, + width: 0, + height: 480, + }), + ..ParsedOptions::default() + }; + apply_client_hints(&mut options, &hints); + assert_eq!(options.resize.unwrap().width, 320); + } + + #[test] + fn a_width_in_the_url_wins_over_the_hint() { + use crate::processing::options::{Resize, ResizingType}; + + let hints = RequestHints { + width: Some(800), + ..RequestHints::default() + }; + + let mut options = ParsedOptions { + resize: Some(Resize { + resizing_type: ResizingType::Fit, + width: 300, + height: 0, + }), + ..ParsedOptions::default() + }; + apply_client_hints(&mut options, &hints); + assert_eq!(options.resize.unwrap().width, 300); + + // With no width of its own, the hint supplies one. + let mut options = ParsedOptions::default(); + apply_client_hints(&mut options, &hints); + assert_eq!(options.resize.unwrap().width, 800); + } +} diff --git a/src/processing/tests_support.rs b/src/processing/tests_support.rs index a601e2d..4a88499 100644 --- a/src/processing/tests_support.rs +++ b/src/processing/tests_support.rs @@ -2,31 +2,9 @@ use crate::processing::save; use crate::processing::watermark; use bytes::Bytes; use image::{ImageBuffer, Rgba, RgbaImage}; -use lazy_static::lazy_static; -use libvips::{ops, VipsApp, VipsImage}; - -lazy_static! { - static ref APP: VipsApp = { - let app = VipsApp::new("Test", false).expect("Cannot initialize libvips"); - app.concurrency_set(1); - app - }; -} - -pub fn init_vips() { - let _ = &*APP; -} +use libvips::{ops, VipsImage}; -/// libvips reports the useful part of a failure in a global buffer that the -/// crate's error type does not carry, so tests that need to tell one failure -/// from another have to read it directly. It is sticky — clear it first. -pub fn clear_vips_error() { - APP.error_clear(); -} - -pub fn vips_error_buffer() -> String { - APP.error_buffer().unwrap_or_default().to_string() -} +pub use crate::test_support::{clear_vips_error, init_vips, vips_error_buffer}; /// Decodes encoded bytes into a `VipsImage`, keeping the buffer alive as long /// as the image needs it. diff --git a/src/response.rs b/src/response.rs new file mode 100644 index 0000000..ea869ef --- /dev/null +++ b/src/response.rs @@ -0,0 +1,319 @@ +//! The caching and provenance headers a processed response carries. + +use crate::config::Config; +use crate::fetch::FetchedImage; +use axum::http::HeaderMap; +use sha2::{Digest, Sha256}; + +/// Headers derived from the configuration and the source response. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct DeliveryHeaders { + pub cache_control: Option, + pub etag: Option, + pub last_modified: Option, + /// `Link: ; rel="canonical"`, pointing at the original image. + pub canonical: Option, + /// Request headers the response could differ by. + pub vary: Vec<&'static str>, +} + +impl DeliveryHeaders { + /// Builds the headers for a response produced from `source`. + pub fn build(config: &Config, source: &SourceMetadata, body: &[u8], vary: &[&'static str]) -> Self { + Self { + cache_control: cache_control(config, source.cache_control.as_deref()), + etag: config.use_etag.then(|| entity_tag(body)), + last_modified: config + .last_modified_enabled + .then(|| source.last_modified.clone()) + .flatten(), + canonical: config + .set_canonical_header + .then(|| source.url.clone()) + .flatten() + .map(|url| format!("<{}>; rel=\"canonical\"", link_uri_reference(&url))), + vary: vary.to_vec(), + } + } +} + +/// The part of a source response that shapes the delivery headers. +#[derive(Debug, Clone, Default)] +pub struct SourceMetadata { + pub cache_control: Option, + pub last_modified: Option, + pub url: Option, +} + +impl SourceMetadata { + pub fn from_fetch(fetched: &FetchedImage, url: &str) -> Self { + Self { + cache_control: fetched.cache_control.clone(), + last_modified: fetched.last_modified.clone(), + url: Some(url.to_string()), + } + } +} + +/// The `Cache-Control` value to send. +/// +/// Passthrough hands the origin's own policy to the client, which is what a +/// deployment wants when the origin already expresses one; otherwise a +/// configured TTL becomes a `max-age`. With neither, no header is sent and the +/// client falls back to its own heuristics, which is the behaviour imgforge +/// has always had. +fn cache_control(config: &Config, source_cache_control: Option<&str>) -> Option { + if config.cache_control_passthrough { + if let Some(value) = source_cache_control.filter(|value| !value.trim().is_empty()) { + return Some(sanitise_header_value(value)); + } + } + + config.ttl.map(|ttl| format!("max-age={ttl}, public")) +} + +/// A strong entity tag for a response body. +/// +/// Derived from the bytes rather than from the source's own `ETag` and the +/// processing options: the same URL against the same source can still produce +/// different bytes once content negotiation is in play, and hashing the result +/// is the only derivation that cannot disagree with what was actually sent. +fn entity_tag(body: &[u8]) -> String { + let digest = Sha256::digest(body); + let mut tag = String::with_capacity(2 + 32); + tag.push('"'); + for byte in digest.iter().take(16) { + tag.push_str(&format!("{byte:02x}")); + } + tag.push('"'); + tag +} + +/// Header values may not contain control characters; a source URL or an origin +/// header is attacker-influenced input, so it is stripped rather than trusted. +/// Escapes a URL for the `<...>` URI-reference slot of a `Link` header. +/// +/// RFC 8288 ends the reference at the first `>`, so a source URL carrying one +/// closes the brackets early and everything after it is read as further link +/// parameters — a source could append its own `rel=` and have imgforge state it +/// as fact. Stripping control characters does not help: `<` and `>` are neither. +/// Percent-encoding them keeps the URL both valid and inert, and matches what +/// the URL parser does with the same characters before the fetch, so the header +/// still names the address that was actually requested. +fn link_uri_reference(url: &str) -> String { + sanitise_header_value(url).replace('<', "%3C").replace('>', "%3E") +} + +fn sanitise_header_value(value: &str) -> String { + value + .chars() + .filter(|character| !character.is_control()) + .take(1024) + .collect() +} + +/// Whether a request's `If-Modified-Since` covers the timestamp we would send. +/// +/// Whether the representation is unmodified since the date the request names. +/// +/// RFC 9110 asks the question chronologically — "not modified *since*" — so any +/// date at or after the one we would send answers 304. Comparing the strings +/// instead only recognised a client echoing our value back byte for byte, which +/// silently sent the whole body to a client whose cached copy was provably +/// current: a caching proxy that normalises the date's spelling, or one holding +/// a copy it fetched later than the origin's own timestamp, got a 200 every +/// time. The parse falls back to the string comparison rather than to `false`, +/// so a date in a form `httpdate` will not read still behaves as it used to. +pub fn matches_if_modified_since(headers: &HeaderMap, last_modified: &str) -> bool { + let Some(requested) = headers + .get("if-modified-since") + .and_then(|value| value.to_str().ok()) + .map(str::trim) + else { + return false; + }; + let last_modified = last_modified.trim(); + + match ( + httpdate::parse_http_date(requested), + httpdate::parse_http_date(last_modified), + ) { + (Ok(requested), Ok(modified)) => modified <= requested, + _ => requested == last_modified, + } +} + +/// Whether a request's `If-None-Match` matches the tag we are about to send. +/// +/// Handles the comma-separated list form and the weak-comparison prefix, both +/// of which real clients send. +pub fn matches_if_none_match(headers: &HeaderMap, etag: &str) -> bool { + let Some(value) = headers.get("if-none-match").and_then(|value| value.to_str().ok()) else { + return false; + }; + + value + .split(',') + .map(str::trim) + .any(|candidate| candidate == "*" || candidate.trim_start_matches("W/") == etag.trim_start_matches("W/")) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn config() -> Config { + Config::new(vec![0u8; 32], vec![0u8; 32]) + } + + /// "Not modified *since*" is a chronological question. Comparing the strings + /// only recognised a client echoing our exact value back, so a proxy holding + /// a provably current copy under a later or differently spelled date was + /// sent the whole body. + #[test] + fn if_modified_since_is_compared_chronologically() { + let last_modified = "Wed, 21 Oct 2015 07:28:00 GMT"; + let asking = |value: &str| { + let mut headers = HeaderMap::new(); + headers.insert("if-modified-since", value.parse().unwrap()); + matches_if_modified_since(&headers, last_modified) + }; + + // The exact echo, which is what a well-behaved client sends. + assert!(asking(last_modified)); + // Later than ours: the copy is still current, so 304. + assert!(asking("Thu, 22 Oct 2015 07:28:00 GMT")); + assert!(asking("Wed, 21 Oct 2015 07:28:01 GMT")); + // Earlier: the client's copy predates the representation, so send it. + assert!(!asking("Tue, 20 Oct 2015 07:28:00 GMT")); + assert!(!asking("Wed, 21 Oct 2015 07:27:59 GMT")); + + // The other two formats RFC 9110 requires a recipient to accept. + assert!(asking("Thursday, 22-Oct-15 07:28:00 GMT")); + assert!(asking("Thu Oct 22 07:28:00 2015")); + + // A date neither side can parse falls back to the old exact match + // rather than to a blanket 304. + assert!(!asking("not a date")); + + // No header at all is not a conditional request. + assert!(!matches_if_modified_since(&HeaderMap::new(), last_modified)); + } + + /// RFC 8288 ends the `<...>` URI reference at the first `>`, so a source URL + /// carrying one closes the brackets early and everything after it is read as + /// further link parameters — letting a source append its own `rel=` and have + /// imgforge state it as fact. Control-character stripping does not catch it: + /// `<` and `>` are neither. + #[test] + fn the_canonical_link_cannot_be_broken_out_of() { + let mut config = Config::new(vec![0u8; 32], vec![0u8; 32]); + config.set_canonical_header = true; + + let canonical_for = |url: &str| { + DeliveryHeaders::build( + &config, + &SourceMetadata { + url: Some(url.to_string()), + ..SourceMetadata::default() + }, + b"body", + &[], + ) + .canonical + .expect("the canonical header is enabled") + }; + + let injected = canonical_for("https://cdn.example.com/a>;rel=\"stylesheet\";x='), + "the URI reference must contain no bare '>': {injected}" + ); + assert!(injected.contains("%3E"), "the bracket should be encoded: {injected}"); + assert!(injected.contains("%3C"), "and so should its opening partner"); + assert!( + injected.ends_with("; rel=\"canonical\""), + "exactly one link-value should be emitted: {injected}" + ); + + // An ordinary URL is untouched. + let plain = canonical_for("https://cdn.example.com/cat.jpg"); + assert_eq!(plain, "; rel=\"canonical\""); + } + + #[test] + fn no_ttl_and_no_passthrough_sends_no_cache_control() { + let config = config(); + assert_eq!(cache_control(&config, Some("max-age=60")), None); + } + + #[test] + fn passthrough_prefers_the_origin_and_falls_back_to_the_ttl() { + let mut config = config(); + config.cache_control_passthrough = true; + config.ttl = Some(3600); + + assert_eq!( + cache_control(&config, Some("public, max-age=60")).as_deref(), + Some("public, max-age=60") + ); + // An origin that says nothing leaves the configured policy in charge. + assert_eq!(cache_control(&config, None).as_deref(), Some("max-age=3600, public")); + assert_eq!( + cache_control(&config, Some(" ")).as_deref(), + Some("max-age=3600, public") + ); + } + + #[test] + fn a_ttl_alone_becomes_a_max_age() { + let mut config = config(); + config.ttl = Some(86_400); + assert_eq!( + cache_control(&config, Some("no-store")).as_deref(), + Some("max-age=86400, public") + ); + } + + #[test] + fn entity_tags_track_the_bytes_that_were_sent() { + let one = entity_tag(b"first"); + let two = entity_tag(b"second"); + + assert_ne!(one, two); + assert_eq!(one, entity_tag(b"first")); + assert!(one.starts_with('"') && one.ends_with('"')); + } + + #[test] + fn conditional_requests_match_a_list_and_the_weak_prefix() { + let etag = entity_tag(b"body"); + + let mut headers = HeaderMap::new(); + headers.insert("if-none-match", etag.parse().unwrap()); + assert!(matches_if_none_match(&headers, &etag)); + + let mut headers = HeaderMap::new(); + headers.insert("if-none-match", format!("\"other\", W/{etag}").parse().unwrap()); + assert!(matches_if_none_match(&headers, &etag)); + + let mut headers = HeaderMap::new(); + headers.insert("if-none-match", "*".parse().unwrap()); + assert!(matches_if_none_match(&headers, &etag)); + + let mut headers = HeaderMap::new(); + headers.insert("if-none-match", "\"nope\"".parse().unwrap()); + assert!(!matches_if_none_match(&headers, &etag)); + + // No header at all is not a match, or every first request would 304. + assert!(!matches_if_none_match(&HeaderMap::new(), &etag)); + } + + #[test] + fn control_characters_never_reach_a_header_value() { + let mut config = config(); + config.cache_control_passthrough = true; + let smuggled = cache_control(&config, Some("max-age=60\r\nX-Injected: yes")); + assert_eq!(smuggled.as_deref(), Some("max-age=60X-Injected: yes")); + } +} diff --git a/src/server.rs b/src/server.rs index 8bb511b..8ee13b1 100644 --- a/src/server.rs +++ b/src/server.rs @@ -17,6 +17,14 @@ use tower_http::trace::TraceLayer; use tracing::{info, info_span, warn}; use tracing_subscriber::{EnvFilter, FmtSubscriber}; +/// Fixed routes imgforge registers itself, which the health path cannot take +/// over. +/// +/// `/status` is deliberately absent: it serves the health handler already, so +/// pointing the health path at it is a synonym rather than a conflict, and the +/// router simply skips the second registration. +const RESERVED_ROUTES: &[&str] = &["/metrics", "/info"]; + #[derive(Debug, Error)] pub enum ServerError { #[error("failed to install tracing subscriber: {0}")] @@ -39,6 +47,8 @@ pub enum ServerError { #[source] source: std::io::Error, }, + #[error("IMGFORGE_HEALTH_CHECK_PATH is {path}, which is already served by imgforge itself")] + ReservedHealthCheckPath { path: String }, } /// Configure and run the imgforge HTTP servers. @@ -68,11 +78,34 @@ pub async fn start() -> Result<(), ServerError> { let main_metric_handle = metric_handle.clone(); let main_state = state.clone(); - let app = Router::new() - .route("/status", get(status_handler)) - .route("/info/{*path}", get(info_handler)) + let prefix = state.config.path_prefix.clone(); + let health_path = state.config.health_check_path.clone(); + let route = |path: &str| format!("{prefix}{path}"); + + // Registering two GET handlers on one path makes axum panic while the + // router is being built, so a health path that lands on a route imgforge + // already owns takes the whole server down with a message about routing + // rather than about configuration. Refusing it here says what is actually + // wrong. `/status` is exempt: it is the same handler under its other name, + // so it is a synonym rather than a collision. + if RESERVED_ROUTES.contains(&health_path.as_str()) { + return Err(ServerError::ReservedHealthCheckPath { path: health_path }); + } + + // `/status` is imgforge's own name for the liveness endpoint and `/health` + // is imgproxy's, so a deployment can be moved either way without touching + // its orchestration; IMGFORGE_HEALTH_CHECK_PATH renames the latter. + let mut app = Router::new() + .route(&route("/status"), get(status_handler)) + .route(&route("/info/{*path}"), get(info_handler)); + + if health_path != "/status" { + app = app.route(&route(&health_path), get(status_handler)); + } + + let app = app .route( - "/{*path}", + &route("/{*path}"), get(image_forge_handler) .layer(axum::middleware::from_fn_with_state( state.clone(), @@ -81,7 +114,7 @@ pub async fn start() -> Result<(), ServerError> { .layer(axum::middleware::from_fn(middleware::status_code_metric_middleware)), ) .route( - "/metrics", + &route("/metrics"), get(move || async move { monitoring::update_vips_metrics(&main_state.vips_app); main_metric_handle.render() diff --git a/src/service/cache_key.rs b/src/service/cache_key.rs index b7f1958..0df1116 100644 --- a/src/service/cache_key.rs +++ b/src/service/cache_key.rs @@ -28,6 +28,15 @@ use std::borrow::Cow; #[derive(Debug, Clone, Copy)] pub struct CacheKeyParts<'a> { pub path: &'a str, + /// The URL the request actually resolves to, after `IMGFORGE_BASE_URL`. + /// + /// The path alone does not identify the bytes: with a base URL configured, + /// the same relative reference points at a different origin the moment that + /// setting changes, and the first request after a migration would otherwise + /// be answered from the old origin's entry without ever fetching the new + /// one. Including it also means an entry is tied to the address it was + /// fetched from rather than to the shorthand that named it. + pub source_url: &'a str, pub default_format: DefaultOutputFormat, pub has_explicit_format: bool, pub is_raw: bool, @@ -57,6 +66,12 @@ pub struct CacheKeyParts<'a> { pub option_defaults: Option, /// Format chosen from the request's `Accept` header, when one was. pub negotiated_format: Option<&'static str>, + /// Dimensions the client's own hints contributed, when they were honoured. + /// + /// A `Width: 320` request and a `Width: 1280` request are the same URL and + /// different images, so without this the first one's output is handed to + /// the second. + pub client_hints: Option<(u32, u32)>, } pub fn processed_cache_key<'a>(parts: CacheKeyParts<'a>) -> Cow<'a, str> { @@ -65,26 +80,31 @@ pub fn processed_cache_key<'a>(parts: CacheKeyParts<'a>) -> Cow<'a, str> { // very much can, and they are the only thing standing between a tightened // policy and the bytes already in the cache. if parts.is_raw { - return source_limits(parts, Cow::Borrowed(parts.path)); + return source_limits(parts, Cow::Owned(source_scoped(parts))); } let base = if parts.has_explicit_format { - Cow::Borrowed(parts.path) + Cow::Owned(source_scoped(parts)) } else { match parts.negotiated_format { // Content negotiation makes one URL produce different bytes for // different clients, so the chosen format is part of the identity // of the entry. Without this a Chrome request would poison the // cache for a client that cannot read AVIF. - Some(format) => Cow::Owned(format!("accept-format={format}:{}", parts.path)), + Some(format) => Cow::Owned(format!("accept-format={format}:{}", source_scoped(parts))), None => Cow::Owned(format!( "default-format={}:{}", parts.default_format.as_str(), - parts.path + source_scoped(parts) )), } }; + let base = match parts.client_hints { + Some((width, dpr_thousandths)) => Cow::Owned(format!("hint={width}x{dpr_thousandths}:{base}")), + None => base, + }; + // The ceilings that describe the *result*. Changing any of them retires the // entries stored under the previous setting, including the change from // "unset" to a value: an unset limit contributes nothing to the key and a @@ -109,6 +129,18 @@ pub fn processed_cache_key<'a>(parts: CacheKeyParts<'a>) -> Cow<'a, str> { source_limits(parts, base) } +/// The request path, scoped to the source it actually resolves to. +/// +/// Only prefixed when the two differ — a URL that carries its own absolute +/// source resolves to itself, so the vast majority of keys stay exactly as they +/// were and only a `IMGFORGE_BASE_URL` deployment pays for the distinction. +fn source_scoped(parts: CacheKeyParts<'_>) -> String { + if parts.source_url == parts.path { + return parts.path.to_string(); + } + format!("src={}:{}", parts.source_url, parts.path) +} + /// Namespaces a key by the limits that describe the source rather than the /// result. /// diff --git a/src/service/mod.rs b/src/service/mod.rs index 962c10e..4f87d06 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -13,12 +13,14 @@ use crate::config::{Config, DefaultOutputFormat}; use crate::fetch::{fetch_image, FetchedImage}; use crate::limits::MaxSourceFileSize; use crate::monitoring::{ImageOperation, ImageOperationActivityGuard, ImageOperationPhase, ImageOperationTimer}; +use crate::negotiation::{apply_client_hints, negotiate_format, vary_headers, RequestHints}; use crate::processing::metadata; use crate::processing::options::{parse_all_options_with_defaults, OptionDefaults, ParsedOptions}; use crate::processing::presets::expand_presets; use crate::processing::watermark::{self, CachedWatermark}; use crate::processing::{process_image, ProcessingError}; -use crate::url::{parse_path, validate_signature, ImgforgeUrl}; +use crate::response::{DeliveryHeaders, SourceMetadata}; +use crate::url::{parse_path, validate_signature_of_size, ImgforgeUrl}; use crate::utils::{format_to_content_type, read_exif_orientation}; use axum::http::StatusCode; use bytes::Bytes; @@ -58,6 +60,23 @@ pub struct ProcessedImage { pub content_type: &'static str, pub cache_status: CacheStatus, pub content_disposition: Option, + /// Caching and provenance headers derived from the configuration. + pub headers: DeliveryHeaders, + /// What the source was and what it became, when debug headers are on. + pub debug: Option, +} + +/// Sizes worth reporting back when `IMGFORGE_ENABLE_DEBUG_HEADERS` is set. +/// +/// Answers the question a cache-efficiency investigation always starts with: +/// how big was the thing we downloaded, and how big is the thing we sent. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct DebugInfo { + pub origin_bytes: usize, + pub origin_width: u32, + pub origin_height: u32, + pub result_width: u32, + pub result_height: u32, } /// Result of fetching image metadata. @@ -77,6 +96,20 @@ pub struct ImageInfo { pub struct ProcessRequest<'a> { pub path: &'a str, pub bearer_token: Option<&'a str>, + /// What the client's headers said it can accept and how large it needs it. + pub hints: RequestHints, +} + +impl<'a> ProcessRequest<'a> { + /// A request carrying nothing but a path, for callers using imgforge as a + /// library rather than over HTTP. + pub fn new(path: &'a str) -> Self { + Self { + path, + bearer_token: None, + hints: RequestHints::default(), + } + } } /// Default output format when the URL requests none (#45): the source @@ -114,10 +147,36 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // Resolved before the cache lookup: the key depends on the ceilings. apply_effective_limits(config, &mut parsed_options); + // Recorded before the hints are folded in, so the key can name exactly what + // they contributed rather than what the URL already asked for. + let client_hints = config.enable_client_hints.then(|| { + apply_client_hints(&mut parsed_options, &request.hints); + ( + parsed_options.resize.map(|resize| resize.width).unwrap_or(0), + (parsed_options.dpr_factor() * 1000.0).round() as u32, + ) + }); + + // Negotiation can replace the output format, so the key has to record which + // format this response was actually built for. + let has_explicit_format = parsed_options.format.is_some(); + let negotiated_format = negotiate_format(config, &request.hints, has_explicit_format); + if let Some(format) = negotiated_format { + parsed_options.format = Some(format.to_string()); + } + let vary = vary_headers(config); + + // Resolved before the cache lookup: a source the deployment no longer + // permits must stop being served, and a persistent cache would otherwise + // keep answering for it long after `IMGFORGE_ALLOWED_SOURCES` was tightened + // — the request never reaches the check because it never reaches the fetch. + let decoded_url = resolve_source_url(config, &url_parts)?; + let cache_key = cache_key::processed_cache_key(CacheKeyParts { path, + source_url: &decoded_url, default_format: config.default_format, - has_explicit_format: parsed_options.format.is_some(), + has_explicit_format: has_explicit_format && negotiated_format.is_none(), is_raw: parsed_options.raw, max_result_dimension: parsed_options.max_result_dimension, max_animation_frames: parsed_options.max_animation_frames, @@ -138,28 +197,67 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // Only when the deployment changes them, so a default configuration // keeps the keys it already had. option_defaults: Some(config.option_defaults()).filter(|defaults| *defaults != OptionDefaults::default()), - negotiated_format: None, + negotiated_format, + client_hints, + }); + + // A hit is only usable if the source it came from is still permitted. The + // request's own URL was checked above, but a redirect can have moved the + // actual source elsewhere, and an entry outlives the policy that admitted + // it. Treated as a miss rather than a refusal: the re-fetch follows the + // redirect again and the redirect policy gives the accurate answer, which + // is right whether the destination moved somewhere permitted or nowhere. + let cached_image = state.cache.get(cache_key.as_ref()).await.filter(|cached| { + let permitted = cached.source_url.is_empty() || config.source_rules.permits(&cached.source_url); + if !permitted { + debug!("Ignoring a cached entry fetched from a source that is no longer allowed"); + } + permitted }); - if let Some(cached_image) = state.cache.get(cache_key.as_ref()).await { + if let Some(cached_image) = cached_image { debug!("Image found in cache for path={}", path); + // A cache hit has no source response to draw on, so the origin's own + // caching headers are not available; the configured policy still is, + // and the entity tag comes from the bytes either way. + let headers = DeliveryHeaders::build(config, &SourceMetadata::default(), &cached_image.bytes, &vary); + + // Enabling the debug headers should not make them appear and disappear + // with the cache. A hit never made the source request, so nothing about + // the origin can be reported — but the result is right here, and its + // header is cheap to read. + let debug = config.enable_debug_headers.then(|| { + let mut debug = DebugInfo::default(); + if let Ok(result) = VipsImage::new_from_buffer(&cached_image.bytes, "") { + debug.result_width = result.get_width().max(0) as u32; + debug.result_height = result.get_height().max(0) as u32; + } + debug + }); + return Ok(ProcessedImage { bytes: cached_image.bytes, content_type: cached_image.content_type, cache_status: CacheStatus::Hit, content_disposition: None, + headers, + debug, }); } let content_disposition = content_disposition_for(&parsed_options); - let decoded_url = url_parts.source_url.decode()?; - debug!("Processing image forge request for URL: {}", decoded_url); let max_src_file_size = resolve_max_src_file_size(config, &parsed_options).map(MaxSourceFileSize::get); let fetched = fetch_image(&state.http_client, &decoded_url, max_src_file_size).await?; + let source_metadata = SourceMetadata::from_fetch(&fetched, &decoded_url); + // Where the bytes came from, which a redirect can move away from what was + // asked for. The canonical header keeps naming the requested URL — that is + // the address callers should use — but the cache entry has to remember the + // one it actually fetched, so a hit can be rechecked against the allow list. + let fetched_from = fetched.final_url.clone(); let FetchedImage { bytes: image_bytes, content_type: source_content_type, @@ -197,6 +295,9 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> image_bytes, source_content_type, content_disposition, + &source_metadata, + &fetched_from, + &vary, ) .await; } @@ -220,7 +321,8 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> let blocking_state = state.clone(); let span = tracing::Span::current(); let blocking_queue = ImageOperationTimer::start(ImageOperation::Process, ImageOperationPhase::BlockingQueue); - let (processed_image_bytes, output_format) = tokio::task::spawn_blocking(move || { + let want_debug = config.enable_debug_headers; + let (processed_image_bytes, output_format, debug) = tokio::task::spawn_blocking(move || { drop(blocking_queue); drop(waiting); let _active = ImageOperationActivityGuard::active(ImageOperation::Process); @@ -249,6 +351,17 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> parsed_options.format = Some(output_format.clone()); let opened = open_source(&image_bytes, &parsed_options, &output_format)?; + // The origin the debug headers describe is the source as fetched, so + // they measure it even when a thumbnail stands in for decoding. + let mut debug = want_debug.then(|| { + let origin = opened.constraint_image(); + DebugInfo { + origin_bytes: opened.metadata_bytes.len(), + origin_width: origin.get_width().max(0) as u32, + origin_height: origin.get_height().max(0) as u32, + ..DebugInfo::default() + } + }); // Measured on the source as fetched, never on a substituted stand-in: // the ceilings say what this deployment will accept, and a 10000px @@ -286,7 +399,17 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> ); let processed_image_bytes = process_image(source_image, parsed_options, &metadata_bytes, watermark.as_ref())?; - Ok::<_, ServiceError>((processed_image_bytes, output_format)) + + // Reading the result's dimensions means decoding its header, which is + // only worth doing when someone asked to see them. + if let Some(debug) = debug.as_mut() { + if let Ok(result) = VipsImage::new_from_buffer(&processed_image_bytes, "") { + debug.result_width = result.get_width().max(0) as u32; + debug.result_height = result.get_height().max(0) as u32; + } + } + + Ok::<_, ServiceError>((processed_image_bytes, output_format, debug)) }) .await .map_err(|source| ServiceError::BlockingTask { @@ -295,12 +418,15 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> })??; let content_type = format_to_content_type(&output_format); + let headers = DeliveryHeaders::build(config, &source_metadata, &processed_image_bytes, &vary); + if content_disposition.is_none() && !matches!(state.cache, ImgforgeCache::None) { if let Err(err) = state.cache.insert( cache_key.into_owned(), CachedImage { bytes: processed_image_bytes.clone(), content_type, + source_url: fetched_from.clone(), }, ) { error!("Failed to cache image: {}", err); @@ -319,6 +445,8 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> content_type, cache_status: CacheStatus::Miss, content_disposition, + headers, + debug, }) } @@ -349,6 +477,38 @@ impl OpenedSource { } } +/// Resolves the source URL a request names, applying the base URL and the +/// allow list. +/// +/// The allow list is checked after the base URL is applied, because that is the +/// URL that will actually be fetched — checking the shorthand form would let a +/// relative reference sidestep the restriction entirely. +fn resolve_source_url(config: &Config, url_parts: &ImgforgeUrl) -> Result { + let decoded = url_parts.source_url.decode()?; + permitted_url(config, &decoded) +} + +/// Applies the base URL to a reference and checks the result against the allow +/// list, returning the URL that may actually be fetched. +/// +/// Every outbound fetch a request can cause goes through here, not just the one +/// for the main image. `watermark_url` names an arbitrary URL that imgforge +/// fetches server-side, so leaving it out made the allow list trivially +/// sidesteppable: the option pointed at `169.254.169.254` or any internal host +/// and imgforge fetched it. The redirect policy guards the hops *after* the +/// first request, which is no help when the first request is already the +/// attack. +fn permitted_url(config: &Config, url: &str) -> Result { + let resolved = config.source_rules.resolve(url); + + if !config.source_rules.permits(&resolved) { + error!("Source URL is not in IMGFORGE_ALLOWED_SOURCES"); + return Err(ServiceError::Fetch(crate::fetch::FetchError::SourceNotAllowed)); + } + + Ok(resolved) +} + /// Opens the source, honouring `enforce_thumbnail` and the multi-page plan. fn open_source( image_bytes: &Bytes, @@ -440,7 +600,18 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re debug!("Info path captured: {}", path); let url_parts = parse_and_authorize(config, path, request.bearer_token)?; - if let Some(cached_metadata) = state.metadata_cache.get(path).await { + // Resolved before the cache lookup, for the same reason the processed path + // does it: a source the deployment no longer permits must stop being + // described, and a persistent metadata cache would otherwise keep answering + // for it long after `IMGFORGE_ALLOWED_SOURCES` was tightened. + let decoded_url = resolve_source_url(config, &url_parts)?; + + // Keyed by the resolved source rather than the path, for the same reason + // the processed cache is: a relative reference means a different image the + // moment `IMGFORGE_BASE_URL` changes. + let metadata_key = format!("{decoded_url}\u{0}{path}"); + + if let Some(cached_metadata) = state.metadata_cache.get(&metadata_key).await { debug!("Metadata found in cache for path={}", path); return Ok(ImageInfo { width: cached_metadata.width, @@ -455,8 +626,6 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re }); } - let decoded_url = url_parts.source_url.decode()?; - let fetched = fetch_image(&state.http_client, &decoded_url, None).await?; let image_bytes = fetched.bytes; let content_type = fetched.content_type; @@ -529,7 +698,7 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re })?; if cacheable && !matches!(state.metadata_cache, MetadataCache::None) { - if let Err(err) = state.metadata_cache.insert(path.to_string(), metadata.clone()) { + if let Err(err) = state.metadata_cache.insert(metadata_key.clone(), metadata.clone()) { error!("Failed to cache metadata: {}", err); } } @@ -594,7 +763,13 @@ fn parse_and_authorize(config: &Config, path: &str, bearer_token: Option<&str>) error!("Invalid URL format: {}", path); ServiceError::new(StatusCode::BAD_REQUEST, "Invalid URL format") })?; - if !validate_signature(&config.key, &config.salt, &url_parts.signature, &path_to_sign) { + if !validate_signature_of_size( + &config.key, + &config.salt, + &url_parts.signature, + &path_to_sign, + config.signature_size, + ) { error!("Invalid signature for path: {}", path_to_sign); return Err(ServiceError::new(StatusCode::FORBIDDEN, "Invalid signature")); } @@ -616,8 +791,12 @@ async fn resolve_watermark( parsed_options: &ParsedOptions, ) -> Result, ServiceError> { if let Some(url) = &parsed_options.watermark_url { + // A watermark is a source like any other: the deployment decided which + // hosts it will fetch from, and that decision cannot depend on which + // option named the URL. + let url = permitted_url(&state.config, url)?; debug!("Fetching watermark from URL: {}", url); - match fetch_image(&state.http_client, url, None).await { + match fetch_image(&state.http_client, &url, None).await { Ok(fetched) => Ok(Some(CachedWatermark::from_bytes(fetched.bytes))), Err(source) => Err(ServiceError::WatermarkFetch { source }), } @@ -673,6 +852,7 @@ async fn resolve_watermark( } /// Returns the source bytes as they arrived, for `raw` and `skip_processing`. +#[allow(clippy::too_many_arguments)] async fn serve_source_response( state: &AppState, path: &str, @@ -680,6 +860,9 @@ async fn serve_source_response( image_bytes: Bytes, source_content_type: Option, content_disposition: Option, + source_metadata: &SourceMetadata, + fetched_from: &str, + vary: &[&'static str], ) -> Result { let content_type = source_content_type .as_deref() @@ -692,6 +875,7 @@ async fn serve_source_response( CachedImage { bytes: image_bytes.clone(), content_type, + source_url: fetched_from.to_string(), }, ) { error!("Failed to cache raw image: {}", err); @@ -700,11 +884,15 @@ async fn serve_source_response( info!("Imgforge served source path={} bytes={}", path, image_bytes.len()); + let headers = DeliveryHeaders::build(&state.config, source_metadata, &image_bytes, vary); + Ok(ProcessedImage { bytes: image_bytes, content_type, cache_status: CacheStatus::Miss, content_disposition, + headers, + debug: None, }) } diff --git a/src/service/tests.rs b/src/service/tests.rs index 1d72a57..1fa73f5 100644 --- a/src/service/tests.rs +++ b/src/service/tests.rs @@ -17,6 +17,7 @@ use std::error::Error as _; fn key_parts(path: &str) -> CacheKeyParts<'_> { CacheKeyParts { path, + source_url: path, default_format: DefaultOutputFormat::Source, has_explicit_format: false, is_raw: false, @@ -29,6 +30,7 @@ fn key_parts(path: &str) -> CacheKeyParts<'_> { watermark_path: None, option_defaults: None, negotiated_format: None, + client_hints: None, } } @@ -94,6 +96,35 @@ fn cache_keys_are_namespaced_by_the_effective_animation_limits() { assert_eq!(unlimited, processed_cache_key(key_parts(path))); } +/// A `Width: 320` request and a `Width: 1280` request are the same URL and +/// different images, so they cannot share an entry. +#[test] +fn client_hint_dimensions_get_their_own_cache_entries() { + let path = "/unsafe/example"; + + let narrow = processed_cache_key(CacheKeyParts { + client_hints: Some((320, 1000)), + ..key_parts(path) + }); + let wide = processed_cache_key(CacheKeyParts { + client_hints: Some((1280, 1000)), + ..key_parts(path) + }); + let retina = processed_cache_key(CacheKeyParts { + client_hints: Some((320, 2000)), + ..key_parts(path) + }); + + assert_ne!(narrow, wide); + assert_ne!(narrow, retina, "the device pixel ratio changes the bytes too"); + // With hints off, the key is untouched, so enabling the feature does not + // invalidate a cache that never used it. + assert_eq!( + processed_cache_key(key_parts(path)), + processed_cache_key(key_parts(path)) + ); +} + #[test] fn negotiated_formats_get_their_own_cache_entries() { let path = "/unsafe/resize:fit:100:100/example"; @@ -1084,3 +1115,45 @@ fn skip_processing_understands_every_format_alias() { DefaultOutputFormat::Source )); } + +/// A relative source reference names a different image the moment +/// `IMGFORGE_BASE_URL` changes, so the path alone cannot identify the entry: +/// the first request after an origin migration would be answered from the old +/// origin's bytes without ever fetching the new one. +#[test] +fn cache_keys_follow_the_resolved_source_url() { + let path = "/unsafe/resize:fit:100:100/cat.jpg"; + + let old_origin = processed_cache_key(CacheKeyParts { + source_url: "https://old.example.com/cat.jpg", + ..key_parts(path) + }); + let new_origin = processed_cache_key(CacheKeyParts { + source_url: "https://new.example.com/cat.jpg", + ..key_parts(path) + }); + assert_ne!(old_origin, new_origin); + + // A raw response resolves through the same base URL, so it needs the same + // distinction — it is the one shape that hands back origin bytes directly. + let raw_old = processed_cache_key(CacheKeyParts { + is_raw: true, + source_url: "https://old.example.com/cat.jpg", + ..key_parts(path) + }); + let raw_new = processed_cache_key(CacheKeyParts { + is_raw: true, + source_url: "https://new.example.com/cat.jpg", + ..key_parts(path) + }); + assert_ne!(raw_old, raw_new); + + // A URL that carries its own absolute source resolves to itself, so the + // overwhelmingly common case pays nothing: no `src=` scope appears at all. + let unscoped = processed_cache_key(key_parts(path)); + assert!( + !unscoped.contains("src="), + "a self-resolving URL should not be scoped: {unscoped}" + ); + assert!(old_origin.contains("src=") && raw_old.contains("src=")); +} diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 0000000..b068724 --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,33 @@ +//! libvips initialisation shared by every test module. +//! +//! libvips is a process-global library: it is initialised once and shut down +//! once. Two modules each holding their own `VipsApp` would initialise it +//! twice and, worse, shut it down while the other still holds images. Any test +//! that touches a vips operation — including the format probe behind +//! `is_format_supported` — has to go through this. + +use lazy_static::lazy_static; +use libvips::VipsApp; + +lazy_static! { + static ref APP: VipsApp = { + let app = VipsApp::new("imgforge-tests", false).expect("Cannot initialize libvips"); + app.concurrency_set(1); + app + }; +} + +pub fn init_vips() { + let _ = &*APP; +} + +/// libvips reports the useful part of a failure in a global buffer that the +/// crate's error type does not carry, so tests that need to tell one failure +/// from another have to read it directly. It is sticky — clear it first. +pub fn clear_vips_error() { + APP.error_clear(); +} + +pub fn vips_error_buffer() -> String { + APP.error_buffer().unwrap_or_default().to_string() +} diff --git a/src/url.rs b/src/url.rs index 243a67e..d9dcc5e 100644 --- a/src/url.rs +++ b/src/url.rs @@ -56,19 +56,46 @@ pub struct ImgforgeUrl { pub source_url: SourceUrlInfo, } +/// Number of bytes in a full HMAC-SHA256 digest. +pub const FULL_SIGNATURE_SIZE: usize = 32; + /// Validates the URL signature using HMAC-SHA256. pub fn validate_signature(key: &[u8], salt: &[u8], signature: &str, path: &str) -> bool { + validate_signature_of_size(key, salt, signature, path, FULL_SIGNATURE_SIZE) +} + +/// Validates a signature that may have been truncated to `signature_size` bytes. +/// +/// imgproxy lets a deployment shorten signatures to keep URLs manageable, at +/// the cost of collision resistance. The comparison is still constant time and +/// still covers every byte the URL claims — a short signature is checked in +/// full against the same prefix of the real digest, so truncation weakens the +/// signature only by the bytes it drops. +pub fn validate_signature_of_size(key: &[u8], salt: &[u8], signature: &str, path: &str, signature_size: usize) -> bool { type HmacSha256 = Hmac; + let Ok(decoded_signature) = URL_SAFE_NO_PAD.decode(signature) else { + return false; + }; + + let signature_size = signature_size.clamp(1, FULL_SIGNATURE_SIZE); + + // A signature of the wrong length is rejected outright. Without this a + // single correct byte would pass whenever the configuration allowed a + // one-byte signature and the URL carried one. + if decoded_signature.len() != signature_size { + return false; + } + let mut mac = HmacSha256::new_from_slice(key).expect("HMAC can take key of any size"); mac.update(salt); mac.update(path.as_bytes()); - let decoded_signature = match URL_SAFE_NO_PAD.decode(signature) { - Ok(s) => s, - Err(_) => return false, - }; - mac.verify_slice(&decoded_signature).is_ok() + if signature_size == FULL_SIGNATURE_SIZE { + return mac.verify_slice(&decoded_signature).is_ok(); + } + + mac.verify_truncated_left(&decoded_signature).is_ok() } /// Parses the incoming URL path into its imgforge components. @@ -226,6 +253,38 @@ mod tests { .collect() } + #[test] + fn test_truncated_signatures_match_the_digest_prefix() { + let key = b"test_key"; + let salt = b"test_salt"; + let path = "/resize:fill:300:200/plain/https://example.com/image.jpg"; + + type HmacSha256 = Hmac; + let mut mac = HmacSha256::new_from_slice(key).unwrap(); + mac.update(salt); + mac.update(path.as_bytes()); + let full = mac.finalize().into_bytes(); + + let short = URL_SAFE_NO_PAD.encode(&full[..8]); + assert!(validate_signature_of_size(key, salt, &short, path, 8)); + + // A signature of a different length than the deployment expects is not + // a valid signature, however many of its bytes happen to be right. + assert!(!validate_signature_of_size(key, salt, &short, path, 16)); + assert!(!validate_signature_of_size(key, salt, &short, path, 32)); + + // And a wrong prefix still fails at the shortened length. + let mut wrong = full[..8].to_vec(); + wrong[0] ^= 0xff; + assert!(!validate_signature_of_size( + key, + salt, + &URL_SAFE_NO_PAD.encode(&wrong), + path, + 8 + )); + } + #[test] fn test_validate_signature_invalid() { let key = b"test_key"; diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index f7b89be..90fd23b 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -3,12 +3,14 @@ use axum::{ http::{Request, StatusCode}, }; use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; +use bytes::Bytes; use http_body_util::BodyExt; use image::{ImageBuffer, Rgba}; use imgforge::app::AppState; use imgforge::caching::cache::ImgforgeCache; use imgforge::caching::config::CacheConfig; use imgforge::config::Config; +use imgforge::config::{SourcePattern, SourceRules}; use imgforge::handlers::image_forge_handler; use imgforge::middleware::request_id_middleware; use imgforge::MaxSourceFileSize; @@ -647,6 +649,665 @@ async fn format_aliases_are_described_by_their_own_media_type() { ); } } +// --------------------------------------------------------------------------- +// Delivery-layer coverage: content negotiation, caching headers, source +// restrictions and the pass-through paths. +// --------------------------------------------------------------------------- + +fn delivery_png(width: u32, height: u32) -> Vec { + let img: ImageBuffer, Vec> = ImageBuffer::from_pixel(width, height, Rgba([12, 34, 56, 255])); + let mut bytes: Vec = Vec::new(); + img.write_to(&mut std::io::Cursor::new(&mut bytes), image::ImageFormat::Png) + .unwrap(); + bytes +} + +fn delivery_config() -> Config { + let mut config = Config::new(vec![0u8; 32], vec![0u8; 32]); + config.workers = 2; + config.allow_unsigned = true; + config +} + +async fn delivery_state(config: Config) -> Arc { + let http_client = imgforge::app::build_http_client(&config).expect("client builds"); + + Arc::new(AppState { + semaphore: Arc::new(Semaphore::new(config.workers)), + cache: ImgforgeCache::None, + metadata_cache: imgforge::caching::cache::MetadataCache::None, + rate_limiter: None, + config, + vips_app: VIPS_APP.clone(), + http_client, + watermark_cache: OnceCell::new(), + }) +} + +fn delivery_router(state: Arc) -> axum::Router { + axum::Router::new() + .route("/{*path}", axum::routing::get(image_forge_handler)) + .with_state(state) +} + +struct DeliveryResponse { + status: StatusCode, + headers: axum::http::HeaderMap, + body: Bytes, +} + +/// A delivery request against a caller-supplied cache, so two requests under +/// different configurations can share one. +async fn delivery_request_with_cache( + config: Config, + cache: ImgforgeCache, + uri: &str, + headers: &[(&str, &str)], +) -> DeliveryResponse { + let http_client = imgforge::app::build_http_client(&config).expect("client builds"); + let state = Arc::new(AppState { + semaphore: Arc::new(Semaphore::new(config.workers)), + cache, + metadata_cache: imgforge::caching::cache::MetadataCache::None, + rate_limiter: None, + config, + vips_app: VIPS_APP.clone(), + http_client, + watermark_cache: OnceCell::new(), + }); + delivery_request(state, uri, headers).await +} + +async fn delivery_request(state: Arc, uri: &str, headers: &[(&str, &str)]) -> DeliveryResponse { + let mut builder = Request::builder().uri(uri); + for (name, value) in headers { + builder = builder.header(*name, *value); + } + let response = delivery_router(state) + .oneshot(builder.body(Body::empty()).unwrap()) + .await + .unwrap(); + + DeliveryResponse { + status: response.status(), + headers: response.headers().clone(), + body: response.into_body().collect().await.unwrap().to_bytes(), + } +} + +fn delivery_header(response: &DeliveryResponse, name: &str) -> Option { + response + .headers + .get(name) + .and_then(|value| value.to_str().ok()) + .map(str::to_string) +} + +/// Serves a PNG and returns the imgforge path that fetches it. +async fn delivery_source(server: &MockServer, extra_headers: &[(&str, &str)]) -> String { + let mut template = ResponseTemplate::new(200) + .set_body_bytes(delivery_png(40, 30)) + .insert_header("Content-Type", "image/png"); + for (name, value) in extra_headers { + template = template.insert_header(*name, *value); + } + + Mock::given(method("GET")) + .and(path("/image.png")) + .respond_with(template) + .mount(server) + .await; + + URL_SAFE_NO_PAD.encode(format!("{}/image.png", server.uri())) +} + +#[tokio::test] +async fn accept_negotiation_upgrades_the_format_and_marks_the_response_as_varying() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.enable_webp_detection = true; + let state = delivery_state(config).await; + + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + let response = delivery_request(state.clone(), &uri, &[("accept", "image/webp,image/*,*/*")]).await; + + assert_eq!(response.status, StatusCode::OK); + assert_eq!( + delivery_header(&response, "content-type").as_deref(), + Some("image/webp") + ); + // Without this a shared cache would serve the WebP to a client that asked + // for anything but. + assert_eq!(delivery_header(&response, "vary").as_deref(), Some("Accept")); + + // A client that does not advertise WebP keeps the source's own format. + let response = delivery_request(state, &uri, &[("accept", "image/png,*/*")]).await; + assert_eq!(delivery_header(&response, "content-type").as_deref(), Some("image/png")); +} + +#[tokio::test] +async fn an_explicit_format_is_only_overridden_by_enforcement() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + let uri = format!("/unsafe/format:png/{encoded}"); + let accept = [("accept", "image/webp")]; + + let mut detecting = delivery_config(); + detecting.enable_webp_detection = true; + let response = delivery_request(delivery_state(detecting).await, &uri, &accept).await; + assert_eq!(delivery_header(&response, "content-type").as_deref(), Some("image/png")); + + let mut enforcing = delivery_config(); + enforcing.enforce_webp = true; + let response = delivery_request(delivery_state(enforcing).await, &uri, &accept).await; + assert_eq!( + delivery_header(&response, "content-type").as_deref(), + Some("image/webp") + ); +} + +#[tokio::test] +async fn an_entity_tag_turns_a_repeat_request_into_a_304() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.use_etag = true; + let state = delivery_state(config).await; + + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + let first = delivery_request(state.clone(), &uri, &[]).await; + let etag = delivery_header(&first, "etag").expect("an etag should be sent"); + assert!(!first.body.is_empty()); + + let second = delivery_request(state.clone(), &uri, &[("if-none-match", &etag)]).await; + assert_eq!(second.status, StatusCode::NOT_MODIFIED); + assert!(second.body.is_empty(), "a 304 carries no body"); + assert_eq!(delivery_header(&second, "etag").as_deref(), Some(etag.as_str())); + + // A stale tag still gets the image. + let stale = delivery_request(state, &uri, &[("if-none-match", "\"stale\"")]).await; + assert_eq!(stale.status, StatusCode::OK); + assert!(!stale.body.is_empty()); +} + +#[tokio::test] +async fn cache_control_comes_from_the_ttl_or_the_origin() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Cache-Control", "public, max-age=99")]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + // Nothing configured: imgforge stays out of the client's caching decisions. + let response = delivery_request(delivery_state(delivery_config()).await, &uri, &[]).await; + assert_eq!(delivery_header(&response, "cache-control"), None); + + let mut with_ttl = delivery_config(); + with_ttl.ttl = Some(600); + let response = delivery_request(delivery_state(with_ttl).await, &uri, &[]).await; + assert_eq!( + delivery_header(&response, "cache-control").as_deref(), + Some("max-age=600, public") + ); + + let mut passthrough = delivery_config(); + passthrough.ttl = Some(600); + passthrough.cache_control_passthrough = true; + let response = delivery_request(delivery_state(passthrough).await, &uri, &[]).await; + assert_eq!( + delivery_header(&response, "cache-control").as_deref(), + Some("public, max-age=99") + ); +} + +#[tokio::test] +async fn last_modified_and_canonical_headers_describe_the_source() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let mut config = delivery_config(); + config.last_modified_enabled = true; + config.set_canonical_header = true; + let response = delivery_request(delivery_state(config).await, &uri, &[]).await; + + assert_eq!( + delivery_header(&response, "last-modified").as_deref(), + Some("Wed, 21 Oct 2015 07:28:00 GMT") + ); + let link = delivery_header(&response, "link").expect("a canonical link should be sent"); + assert!(link.starts_with('<') && link.ends_with("; rel=\"canonical\""), "{link}"); + assert!(link.contains("/image.png")); +} + +#[tokio::test] +async fn a_source_outside_the_allow_list_is_refused() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let mut config = delivery_config(); + config.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse("https://images.example.com/")], + }; + + let response = delivery_request(delivery_state(config).await, &uri, &[]).await; + assert_eq!(response.status, StatusCode::BAD_REQUEST); + assert!(String::from_utf8_lossy(&response.body).contains("not allowed")); +} + +#[tokio::test] +async fn a_base_url_lets_a_url_carry_only_the_path() { + let server = MockServer::start().await; + delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.source_rules = SourceRules { + base_url: Some(server.uri()), + allowed: Vec::new(), + }; + + let encoded = URL_SAFE_NO_PAD.encode("image.png"); + let response = delivery_request( + delivery_state(config).await, + &format!("/unsafe/rs:fit:20:20/{encoded}"), + &[], + ) + .await; + + assert_eq!(response.status, StatusCode::OK); + assert!(!response.body.is_empty()); +} + +#[tokio::test] +async fn cross_origin_and_debug_headers_are_emitted_when_configured() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.allow_origin = Some("https://app.example.com".to_string()); + config.enable_debug_headers = true; + + let response = delivery_request( + delivery_state(config).await, + &format!("/unsafe/rs:fit:20:20/{encoded}"), + &[], + ) + .await; + + assert_eq!( + delivery_header(&response, "access-control-allow-origin").as_deref(), + Some("https://app.example.com") + ); + assert_eq!(delivery_header(&response, "x-origin-width").as_deref(), Some("40")); + assert_eq!(delivery_header(&response, "x-origin-height").as_deref(), Some("30")); + assert_eq!(delivery_header(&response, "x-result-width").as_deref(), Some("20")); + assert_eq!(delivery_header(&response, "x-result-height").as_deref(), Some("15")); +} + +#[tokio::test] +async fn skip_processing_returns_the_source_bytes_untouched() { + let server = MockServer::start().await; + let source = delivery_png(40, 30); + Mock::given(method("GET")) + .and(path("/image.png")) + .respond_with( + ResponseTemplate::new(200) + .set_body_bytes(source.clone()) + .insert_header("Content-Type", "image/png"), + ) + .mount(&server) + .await; + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/image.png", server.uri())); + + // A resize is requested, and skipped, because the source format is listed. + let response = delivery_request( + delivery_state(delivery_config()).await, + &format!("/unsafe/skp:png/rs:fit:10:10/{encoded}"), + &[], + ) + .await; + + assert_eq!(response.status, StatusCode::OK); + assert_eq!(response.body.as_ref(), source.as_slice()); + + // Asking for a different format is a conversion, which cannot be skipped. + let response = delivery_request( + delivery_state(delivery_config()).await, + &format!("/unsafe/skp:png/format:jpeg/rs:fit:10:10/{encoded}"), + &[], + ) + .await; + assert_eq!( + delivery_header(&response, "content-type").as_deref(), + Some("image/jpeg") + ); + assert_ne!(response.body.as_ref(), source.as_slice()); +} + +#[tokio::test] +async fn client_hints_size_a_url_that_left_the_width_open() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.enable_client_hints = true; + let state = delivery_state(config).await; + + let response = delivery_request(state.clone(), &format!("/unsafe/{encoded}"), &[("width", "20")]).await; + let decoded = image::load_from_memory(&response.body).expect("result decodes"); + assert_eq!(decoded.width(), 20); + + // A URL that names its own width is not overridden by the hint. + let response = delivery_request(state, &format!("/unsafe/rs:fit:10:10/{encoded}"), &[("width", "20")]).await; + let decoded = image::load_from_memory(&response.body).expect("result decodes"); + assert_eq!(decoded.width(), 10); +} + +#[tokio::test] +async fn an_upstream_failure_names_the_upstream_status() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/missing.png")) + .respond_with(ResponseTemplate::new(404)) + .mount(&server) + .await; + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/missing.png", server.uri())); + + let response = delivery_request( + delivery_state(delivery_config()).await, + &format!("/unsafe/{encoded}"), + &[], + ) + .await; + + assert_eq!(response.status, StatusCode::BAD_REQUEST); + assert!( + String::from_utf8_lossy(&response.body).contains("404"), + "the response should name the upstream status, got: {}", + String::from_utf8_lossy(&response.body) + ); +} + +/// An allowed origin that redirects elsewhere must not carry the request past +/// the allow list. Checking only the URL the caller supplied is checking the +/// wrong thing — it is the classic way an image proxy becomes an SSRF gadget. +#[tokio::test] +async fn a_redirect_out_of_the_allow_list_is_refused() { + let origin = MockServer::start().await; + let elsewhere = MockServer::start().await; + + Mock::given(method("GET")) + .and(path("/internal.png")) + .respond_with( + ResponseTemplate::new(200) + .set_body_bytes(delivery_png(40, 30)) + .insert_header("Content-Type", "image/png"), + ) + .mount(&elsewhere) + .await; + + Mock::given(method("GET")) + .and(path("/image.png")) + .respond_with(ResponseTemplate::new(302).insert_header("Location", format!("{}/internal.png", elsewhere.uri()))) + .mount(&origin) + .await; + + let mut config = delivery_config(); + // Only the first server is permitted; the redirect points at the second. + config.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&origin.uri())], + }; + + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/image.png", origin.uri())); + let response = delivery_request( + delivery_state(config).await, + &format!("/unsafe/rs:fit:20:20/{encoded}"), + &[], + ) + .await; + + assert_ne!( + response.status, + StatusCode::OK, + "the redirect target was outside the allow list and must not have been fetched" + ); +} + +/// With no allow list configured, redirects are followed as before. +#[tokio::test] +async fn a_redirect_is_followed_when_no_allow_list_is_configured() { + let origin = MockServer::start().await; + let elsewhere = MockServer::start().await; + + Mock::given(method("GET")) + .and(path("/final.png")) + .respond_with( + ResponseTemplate::new(200) + .set_body_bytes(delivery_png(40, 30)) + .insert_header("Content-Type", "image/png"), + ) + .mount(&elsewhere) + .await; + Mock::given(method("GET")) + .and(path("/start.png")) + .respond_with(ResponseTemplate::new(302).insert_header("Location", format!("{}/final.png", elsewhere.uri()))) + .mount(&origin) + .await; + + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/start.png", origin.uri())); + let response = delivery_request( + delivery_state(delivery_config()).await, + &format!("/unsafe/rs:fit:20:20/{encoded}"), + &[], + ) + .await; + + assert_eq!(response.status, StatusCode::OK); + assert!(!response.body.is_empty()); +} + +/// The response has to tell shared caches which request headers it varies by, +/// or a CDN reuses one client's dimensions for another. +#[tokio::test] +async fn client_hints_are_named_in_vary() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + let mut config = delivery_config(); + config.enable_client_hints = true; + config.enable_webp_detection = true; + + let response = delivery_request( + delivery_state(config).await, + &format!("/unsafe/rs:fit:20:20/{encoded}"), + &[], + ) + .await; + + let vary = delivery_header(&response, "vary").expect("a vary header should be sent"); + for expected in ["Accept", "Width", "DPR"] { + assert!(vary.contains(expected), "expected {expected} in Vary, got: {vary}"); + } +} + +/// `Last-Modified` is a validator, so a client returning it should get a 304 +/// rather than the whole image again. +#[tokio::test] +async fn if_modified_since_is_honoured() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")]).await; + + let mut config = delivery_config(); + config.last_modified_enabled = true; + + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + let state = delivery_state(config).await; + + let first = delivery_request(state.clone(), &uri, &[]).await; + let last_modified = delivery_header(&first, "last-modified").expect("a validator should be sent"); + + let second = delivery_request(state, &uri, &[("if-modified-since", &last_modified)]).await; + assert_eq!(second.status, StatusCode::NOT_MODIFIED); + assert!(second.body.is_empty()); +} + +/// RFC 9110 gives `If-None-Match` precedence over `If-Modified-Since` when the +/// request carries both. Keying that on whether imgforge *emitted* an ETag +/// instead meant a deployment with both validators enabled never looked at +/// `If-Modified-Since` at all, and sent the whole body to a client that had +/// asked, correctly, whether it needed one. +#[tokio::test] +async fn if_modified_since_still_works_when_etags_are_enabled() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")]).await; + + let mut config = delivery_config(); + config.last_modified_enabled = true; + config.use_etag = true; + + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + let state = delivery_state(config).await; + + let first = delivery_request(state.clone(), &uri, &[]).await; + let last_modified = delivery_header(&first, "last-modified").expect("a validator should be sent"); + let etag = delivery_header(&first, "etag").expect("an entity tag should be sent too"); + + // The date alone, with no entity tag to fall back on. + let by_date = delivery_request(state.clone(), &uri, &[("if-modified-since", &last_modified)]).await; + assert_eq!( + by_date.status, + StatusCode::NOT_MODIFIED, + "an ETag being available must not stop the date from being read" + ); + assert!(by_date.body.is_empty()); + + // The tag still wins when both are present. + let by_tag = delivery_request( + state.clone(), + &uri, + &[("if-none-match", &etag), ("if-modified-since", &last_modified)], + ) + .await; + assert_eq!(by_tag.status, StatusCode::NOT_MODIFIED); + + // A stale tag is a mismatch, and a mismatch means send the body — even + // though the date would have said otherwise. That is the precedence rule, + // and it is why the date cannot simply be checked as well. + let stale_tag = delivery_request( + state, + &uri, + &[ + ("if-none-match", "\"not-the-current-tag\""), + ("if-modified-since", &last_modified), + ], + ) + .await; + assert_eq!(stale_tag.status, StatusCode::OK); + assert!(!stale_tag.body.is_empty()); +} + +/// A watermark is a source like any other. `watermark_url` names a URL that +/// imgforge fetches server-side, and it went straight to the HTTP client with no +/// allow-list check — so the option pointed at an internal address and imgforge +/// fetched it, which is exactly the SSRF `IMGFORGE_ALLOWED_SOURCES` exists to +/// prevent. The redirect policy only guards the hops after the first request. +#[tokio::test] +async fn a_watermark_url_outside_the_allow_list_is_refused() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + + // The watermark lives on the same mock server as the image, so the only + // thing deciding the outcome is whether the allow list is consulted. + let watermark_url = format!("{}/image.png", server.uri()); + let encoded_watermark = URL_SAFE_NO_PAD.encode(watermark_url.as_bytes()); + let uri = format!("/unsafe/rs:fit:20:20/wm:0.5/wmu:{encoded_watermark}/{encoded}"); + + // With the mock server allowed, the watermark is fetched and the request + // succeeds — proving the URL itself is good. + let mut permissive = delivery_config(); + permissive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + }; + let allowed = delivery_request(delivery_state(permissive).await, &uri, &[]).await; + assert_eq!( + allowed.status, + StatusCode::OK, + "the watermark URL is inside the allow list and should be fetched" + ); + + // Now allow only an unrelated host. The main image is refused, which is the + // established behaviour... + let mut restrictive = delivery_config(); + restrictive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse("https://images.example.com/")], + }; + let refused = delivery_request(delivery_state(restrictive).await, &uri, &[]).await; + assert_eq!(refused.status, StatusCode::BAD_REQUEST); + assert!(String::from_utf8_lossy(&refused.body).contains("not allowed")); + + // ...and the watermark has to be refused on its own account too, not merely + // because the image beside it was. Here the image is permitted and only the + // watermark is out of bounds. + let mut image_only = delivery_config(); + image_only.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/image.png", server.uri()))], + }; + let watermark_elsewhere = URL_SAFE_NO_PAD.encode(b"http://169.254.169.254/latest/meta-data/"); + let smuggled = format!("/unsafe/rs:fit:20:20/wm:0.5/wmu:{watermark_elsewhere}/{encoded}"); + let response = delivery_request(delivery_state(image_only).await, &smuggled, &[]).await; + assert_eq!( + response.status, + StatusCode::BAD_REQUEST, + "a watermark URL outside the allow list must not be fetched" + ); + assert!(String::from_utf8_lossy(&response.body).contains("not allowed")); +} + +/// The allow list has to be consulted before the cache answers. A persistent +/// cache outlives the configuration that filled it, so a source that is no +/// longer permitted would otherwise keep being served from the entry it left +/// behind — the request never reaches the check because it never reaches the +/// fetch. +#[tokio::test] +async fn a_cache_hit_does_not_outlive_the_source_allow_list() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let cache = ImgforgeCache::new(Some(CacheConfig::Memory { capacity: 1024 * 1024 })) + .await + .unwrap(); + + // Fill the cache while the source is permitted. + let mut permissive = delivery_config(); + permissive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + }; + let warm = delivery_request_with_cache(permissive, cache.clone(), &uri, &[]).await; + assert_eq!(warm.status, StatusCode::OK); + + // Tighten the rules over the same cache. The entry is still there, and must + // not be reachable. + let mut restrictive = delivery_config(); + restrictive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse("https://images.example.com/")], + }; + let response = delivery_request_with_cache(restrictive, cache, &uri, &[]).await; + assert_eq!( + response.status, + StatusCode::BAD_REQUEST, + "a cached entry must not survive the source that produced it being disallowed" + ); +} /// A `raw` response returns origin bytes with nothing between them and the /// client, and every source limit is checked after the cache lookup. Without the @@ -676,45 +1337,192 @@ async fn a_cached_passthrough_does_not_outlive_the_source_limits() { .await .unwrap(); + let respond = |config: Config, cache: ImgforgeCache, uri: String| async move { + let state = create_test_state_with_cache(config, cache).await; + let app = axum::Router::new() + .route("/{*path}", axum::routing::get(image_forge_handler)) + .with_state(state); + make_request(app, &uri).await.0 + }; + // Warm the cache while nothing restricts the source. - let permissive = create_test_config(vec![], vec![], true); - let state = create_test_state_with_cache(permissive, cache.clone()).await; - let app = axum::Router::new() - .route("/{*path}", axum::routing::get(image_forge_handler)) - .with_state(state); - let (warm, _) = make_request(app, &uri).await; + let warm = respond(create_test_config(vec![], vec![], true), cache.clone(), uri.clone()).await; assert_eq!(warm, StatusCode::OK, "the passthrough should succeed while permitted"); - // Now forbid the source's resolution, over the same cache. 400x400 is + // Now forbid the source's resolution over the same cache. 400x400 is // 160,000 pixels, well over a 0.05 MP ceiling. let mut restrictive = create_test_config(vec![], vec![], true); restrictive.max_src_resolution = Some("0.05".parse().unwrap()); - let state = create_test_state_with_cache(restrictive, cache.clone()).await; - let app = axum::Router::new() - .route("/{*path}", axum::routing::get(image_forge_handler)) - .with_state(state); - let (status, _) = make_request(app, &uri).await; assert_eq!( - status, + respond(restrictive, cache.clone(), uri.clone()).await, StatusCode::BAD_REQUEST, "a cached passthrough must not survive the limit that now forbids it" ); - // The same for a MIME restriction that the source does not satisfy. + // The same for a MIME restriction the source does not satisfy. let mut mime_restricted = create_test_config(vec![], vec![], true); mime_restricted.allowed_mime_types = Some(vec!["image/jpeg".to_string()]); - let state = create_test_state_with_cache(mime_restricted, cache).await; - let app = axum::Router::new() - .route("/{*path}", axum::routing::get(image_forge_handler)) - .with_state(state); - let (status, _) = make_request(app, &uri).await; assert_eq!( - status, + respond(mime_restricted, cache, uri).await, StatusCode::BAD_REQUEST, "a cached passthrough must not survive a MIME policy that now forbids it" ); } +/// `/info` has to consult the allow list before its cache, exactly as the image +/// path does. A persistent metadata cache outlives the configuration that filled +/// it, so a source removed from `IMGFORGE_ALLOWED_SOURCES` kept being described +/// from the entry it left behind — the endpoint answered before reaching the +/// check that should have refused it. +#[tokio::test] +async fn info_does_not_describe_a_source_the_allow_list_now_forbids() { + let server = MockServer::start().await; + let image = create_test_image(64, 48, [10, 20, 30, 255]); + + Mock::given(method("GET")) + .and(path("/described.png")) + .respond_with( + ResponseTemplate::new(200) + .set_body_bytes(image) + .insert_header("Content-Type", "image/png"), + ) + .mount(&server) + .await; + + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/described.png", server.uri()).as_bytes()); + let uri = format!("/info/unsafe/{encoded}"); + + let metadata_cache = + imgforge::caching::cache::MetadataCache::new(Some(CacheConfig::Memory { capacity: 1024 * 1024 })) + .await + .unwrap(); + + let respond = |config: Config, cache: imgforge::caching::cache::MetadataCache, uri: String| async move { + let http_client = imgforge::app::build_http_client(&config).expect("client builds"); + let state = Arc::new(AppState { + semaphore: Arc::new(Semaphore::new(config.workers)), + cache: ImgforgeCache::None, + metadata_cache: cache, + rate_limiter: None, + config, + vips_app: VIPS_APP.clone(), + http_client, + watermark_cache: OnceCell::new(), + }); + let app = axum::Router::new() + .route("/info/{*path}", axum::routing::get(imgforge::handlers::info_handler)) + .with_state(state); + make_request(app, &uri).await.0 + }; + + // Populate the metadata cache while the source is permitted. + let mut permissive = create_test_config(vec![], vec![], true); + permissive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + }; + assert_eq!( + respond(permissive, metadata_cache.clone(), uri.clone()).await, + StatusCode::OK, + "the source is permitted, so /info should describe it" + ); + + // Tighten the allow list over the same cache. + let mut restrictive = create_test_config(vec![], vec![], true); + restrictive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse("https://images.example.com/")], + }; + assert_eq!( + respond(restrictive, metadata_cache, uri).await, + StatusCode::BAD_REQUEST, + "cached metadata must not outlive the source being disallowed" + ); +} + +/// The allow list is checked against the URL a request *asks* for, and a +/// redirect can move the answer somewhere else. The redirect policy catches that +/// as it happens, which leaves the cache: an entry outlives the policy that +/// admitted it, so a hit has to be rechecked against where its bytes came from. +#[tokio::test] +async fn a_cached_redirect_destination_is_revalidated_on_a_hit() { + let origin = MockServer::start().await; + let image = create_test_image(48, 48, [20, 90, 140, 255]); + + // /entry redirects to /final on the same server, so both are reachable and + // only the allow list decides the outcome. + Mock::given(method("GET")) + .and(path("/final.png")) + .respond_with( + ResponseTemplate::new(200) + .set_body_bytes(image) + .insert_header("Content-Type", "image/png"), + ) + .mount(&origin) + .await; + Mock::given(method("GET")) + .and(path("/entry.png")) + .respond_with( + ResponseTemplate::new(302).insert_header("Location", format!("{}/final.png", origin.uri()).as_str()), + ) + .mount(&origin) + .await; + + let encoded = URL_SAFE_NO_PAD.encode(format!("{}/entry.png", origin.uri()).as_bytes()); + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let cache = ImgforgeCache::new(Some(CacheConfig::Memory { capacity: 1024 * 1024 })) + .await + .unwrap(); + + // Built through the real client factory so the redirect policy is active — + // the point of the test is what happens across a redirect, and a plain + // reqwest client follows them without consulting the allow list at all. + let respond = |config: Config, cache: ImgforgeCache, uri: String| async move { + let http_client = imgforge::app::build_http_client(&config).expect("client builds"); + let state = Arc::new(AppState { + semaphore: Arc::new(Semaphore::new(config.workers)), + cache, + metadata_cache: imgforge::caching::cache::MetadataCache::None, + rate_limiter: None, + config, + vips_app: VIPS_APP.clone(), + http_client, + watermark_cache: OnceCell::new(), + }); + let app = axum::Router::new() + .route("/{*path}", axum::routing::get(image_forge_handler)) + .with_state(state); + make_request(app, &uri).await.0 + }; + + // Both entry and destination permitted: the redirect is followed and cached. + let mut permissive = create_test_config(vec![], vec![], true); + permissive.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/", origin.uri()))], + }; + assert_eq!( + respond(permissive, cache.clone(), uri.clone()).await, + StatusCode::OK, + "both hops are allowed, so the fetch should succeed" + ); + + // Now permit only the entry point. The cached bytes came from /final.png, + // which is no longer allowed, so the entry must not be served — and the + // re-fetch fails at the redirect for the same reason. + let mut entry_only = create_test_config(vec![], vec![], true); + entry_only.source_rules = SourceRules { + base_url: None, + allowed: vec![SourcePattern::parse(&format!("{}/entry.png", origin.uri()))], + }; + assert_eq!( + respond(entry_only, cache, uri).await, + StatusCode::BAD_REQUEST, + "a cached entry must not outlive the destination it was fetched from" + ); +} + /// Both of these settings change the bytes of a response whose URL never /// changes, and neither carries a version bump to retire what it invalidates. /// The cache key has to carry them, and the request path has to actually pass From 6bbb7613398252e0193986250fc81c79515d0dd4 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:18:11 -0400 Subject: [PATCH 02/13] Preserve query constraints in source patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An allow-list entry is a prefix of the whole URL, but parsing kept only its path, so https://api.example/render?tenant=public also admitted ?tenant=private and no query at all — wider than the boundary the operator wrote down. The query is now kept and prefix-matched, and an entry that names one pins its path exactly: as a prefix of the raw URL, more path would have to come before the ?, and nothing can. Co-Authored-By: Claude Fable 5 --- src/config/source.rs | 56 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/src/config/source.rs b/src/config/source.rs index f8b0d88..04b73ff 100644 --- a/src/config/source.rs +++ b/src/config/source.rs @@ -26,6 +26,13 @@ pub struct SourcePattern { port: Option, /// The path the entry restricts to, already normalised. path_prefix: String, + /// The query the entry restricts to, when it named one. + /// + /// As a prefix of the raw URL, an entry with a query pins everything up to + /// it: `?tenant=public` permits `?tenant=public&x=1` but not + /// `?tenant=private`, and not a URL with no query at all. Dropping it + /// widened the boundary the operator wrote down. + query_prefix: Option, } /// How an entry's host half is compared against a URL's host. @@ -49,6 +56,7 @@ impl SourcePattern { host: HostMatch::Unstructured, port: None, path_prefix: String::new(), + query_prefix: None, }; // The wildcard is not a legal host, so the entry cannot be parsed as a @@ -75,13 +83,23 @@ impl SourcePattern { HostMatch::Exact(host.to_string()) }; + // A query pins the path with it: as a prefix of the raw URL, anything + // more path would have to come before the `?`, and nothing can. So the + // path is kept exactly as parsed for equality rather than run through + // the prefix normalisation. + let (path_prefix, query_prefix) = match parsed.query() { + Some(query) => (parsed.path().to_string(), Some(query.to_string())), + // `Url` has already resolved any dot segments here, so an entry and + // a candidate are compared in the same normalised terms. + None => (normalise_prefix(parsed.path(), pattern), None), + }; + Self { scheme: parsed.scheme().to_string(), host, port: parsed.port(), - // `Url` has already resolved any dot segments here, so an entry and - // a candidate are compared in the same normalised terms. - path_prefix: normalise_prefix(parsed.path(), pattern), + path_prefix, + query_prefix, } } @@ -116,7 +134,13 @@ impl SourcePattern { // `Url::path()` is normalised, so `/public/../private/x` arrives here as // `/private/x` — the path reqwest will actually request. - host_ok && parsed.path().starts_with(&self.path_prefix) + let path_ok = match &self.query_prefix { + Some(query) => { + parsed.path() == self.path_prefix && parsed.query().unwrap_or("").starts_with(query.as_str()) + } + None => parsed.path().starts_with(&self.path_prefix), + }; + host_ok && path_ok } } @@ -233,6 +257,30 @@ mod tests { } } + /// An entry with a query is a prefix of the whole URL, query included. + /// Discarding it read `?tenant=public` as "any query or none", which + /// widened the boundary the operator wrote down. + #[test] + fn a_query_in_the_entry_restricts_the_match() { + let pattern = SourcePattern::parse("https://api.example.test/render?tenant=public"); + + assert!(pattern.matches("https://api.example.test/render?tenant=public")); + // More query after the prefix is more URL after the prefix, which a + // prefix permits. + assert!(pattern.matches("https://api.example.test/render?tenant=public&size=2")); + assert!(!pattern.matches("https://api.example.test/render?tenant=private")); + assert!(!pattern.matches("https://api.example.test/render")); + // A query pins the path with it: as a prefix of the raw URL, anything + // more path would have to come before the `?`, and nothing can. + assert!(!pattern.matches("https://api.example.test/render/extra?tenant=public")); + assert!(!pattern.matches("https://api.example.test/other?tenant=public")); + + // An entry without a query keeps its prefix-of-the-path reading and + // says nothing about the candidate's query. + let open = SourcePattern::parse("https://api.example.test/render"); + assert!(open.matches("https://api.example.test/render?tenant=private")); + } + /// The literal form needs the same anchoring as the wildcard. Fixing only /// the wildcard branch left `https://cdn.example.com` — the spelling most /// operators reach for — matching `cdn.example.com.evil.test`, which is the From f7f41b130a28a3f3a1dcd3021b0918c9f486c055 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:18:11 -0400 Subject: [PATCH 03/13] Recheck every cached source against the allow list as it stands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cached composite validated only the main image on a hit, so a watermark_url host removed from IMGFORGE_ALLOWED_SOURCES kept serving its pixels out of the cache. The watermark URL is now checked before the lookup alongside the main URL, and the entry remembers where the watermark was actually fetched from — redirects included — so the hit filter rechecks both sources. Metadata entries had the same gap one layer down: CachedMetadata carried no final fetch URL, so an /info answer that had followed a redirect outlived the policy that admitted its destination. The entry now records it and a hit revalidates it, the same rule the image cache applies. Co-Authored-By: Claude Fable 5 --- src/caching/cache.rs | 66 ++++++++++++++++++++++++++++++++++++++++++-- src/service/mod.rs | 51 ++++++++++++++++++++++++++++------ 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/caching/cache.rs b/src/caching/cache.rs index 344912f..13b399e 100644 --- a/src/caching/cache.rs +++ b/src/caching/cache.rs @@ -42,6 +42,10 @@ pub struct CachedImage { /// have moved the actual source somewhere that is no longer permitted, and /// an entry outlives the policy that admitted it. pub source_url: String, + /// Where a `watermark_url` watermark was fetched from, after any redirects, + /// or empty when the entry composites none. Its pixels are in the bytes + /// above, so its source is rechecked on a hit exactly as the image's own. + pub watermark_source_url: String, } impl Code for CachedImage { @@ -57,6 +61,10 @@ impl Code for CachedImage { let source_bytes = self.source_url.as_bytes(); source_bytes.len().encode(writer)?; writer.write_all(source_bytes).map_err(FoyerError::io_error)?; + + let watermark_bytes = self.watermark_source_url.as_bytes(); + watermark_bytes.len().encode(writer)?; + writer.write_all(watermark_bytes).map_err(FoyerError::io_error)?; Ok(()) } @@ -78,15 +86,26 @@ impl Code for CachedImage { let source_url = String::from_utf8(source_buf) .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in source url"))?; + let watermark_len = usize::decode(reader)?; + let mut watermark_buf = vec![0u8; watermark_len]; + reader.read_exact(&mut watermark_buf).map_err(FoyerError::io_error)?; + let watermark_source_url = String::from_utf8(watermark_buf) + .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in watermark source url"))?; + Ok(CachedImage { bytes: Bytes::from(data), content_type, source_url, + watermark_source_url, }) } fn estimated_size(&self) -> usize { - self.bytes.len() + self.content_type.len() + self.source_url.len() + std::mem::size_of::() * 3 + self.bytes.len() + + self.content_type.len() + + self.source_url.len() + + self.watermark_source_url.len() + + std::mem::size_of::() * 4 } } @@ -102,6 +121,10 @@ pub struct CachedMetadata { pub orientation: u32, /// Frames or pages the source carries; 1 for a still image. pub pages: u32, + /// The URL the description was read from, after any redirects, so a hit + /// can be rechecked against the allow list as it stands now — the same + /// reason `CachedImage` remembers its own. + pub source_url: String, } impl Code for CachedMetadata { @@ -122,6 +145,10 @@ impl Code for CachedMetadata { self.has_alpha.encode(writer)?; self.orientation.encode(writer)?; self.pages.encode(writer)?; + + let source_bytes = self.source_url.as_bytes(); + source_bytes.len().encode(writer)?; + writer.write_all(source_bytes).map_err(FoyerError::io_error)?; Ok(()) } @@ -149,6 +176,12 @@ impl Code for CachedMetadata { let orientation = u32::decode(reader)?; let pages = u32::decode(reader)?; + let source_len = usize::decode(reader)?; + let mut source_buf = vec![0u8; source_len]; + reader.read_exact(&mut source_buf).map_err(FoyerError::io_error)?; + let source_url = String::from_utf8(source_buf) + .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in source url"))?; + Ok(CachedMetadata { width, height, @@ -159,15 +192,17 @@ impl Code for CachedMetadata { has_alpha, orientation, pages, + source_url, }) } fn estimated_size(&self) -> usize { std::mem::size_of::() * 5 - + std::mem::size_of::() * 2 + + std::mem::size_of::() * 3 + std::mem::size_of::() + self.format.len() + self.content_type.len() + + self.source_url.len() } } @@ -413,6 +448,7 @@ mod tests { bytes: Bytes::from(vec![1, 2, 3]), content_type: "image/jpeg", source_url: "https://example.test/cached.png".to_string(), + watermark_source_url: "https://cdn.example.test/mark.png".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); @@ -432,10 +468,36 @@ mod tests { bytes: Bytes::from(vec![1, 2, 3]), content_type: "image/jpeg", source_url: "https://example.test/cached.png".to_string(), + watermark_source_url: "https://cdn.example.test/mark.png".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); let retrieved = cache.get(&key).await.unwrap(); assert_eq!(retrieved.bytes, value.bytes); assert_eq!(retrieved.content_type, value.content_type); + // Both provenance URLs have to survive the disk round trip, or the + // hit-time allow-list check silently checks nothing. + assert_eq!(retrieved.source_url, value.source_url); + assert_eq!(retrieved.watermark_source_url, value.watermark_source_url); + } + + #[test] + fn cached_metadata_round_trips_through_its_encoding() { + let metadata = CachedMetadata { + width: 800, + height: 600, + format: "jpeg".to_string(), + content_type: "image/jpeg".to_string(), + size_bytes: 1234, + channels: 3, + has_alpha: false, + orientation: 6, + pages: 4, + source_url: "https://cdn.example.test/real.jpg".to_string(), + }; + + let mut buf = Vec::new(); + metadata.encode(&mut buf).unwrap(); + let decoded = CachedMetadata::decode(&mut buf.as_slice()).unwrap(); + assert_eq!(decoded, metadata); } } diff --git a/src/service/mod.rs b/src/service/mod.rs index 4f87d06..07cd72f 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -172,6 +172,14 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // — the request never reaches the check because it never reaches the fetch. let decoded_url = resolve_source_url(config, &url_parts)?; + // The watermark's URL is part of the request too, so it is checked where + // the main URL is checked — before the cache can answer. Validating it + // only on the miss path let a cached composite keep serving pixels from a + // host the allow list no longer names. + if let Some(url) = parsed_options.watermark_url.as_deref() { + permitted_url(config, url)?; + } + let cache_key = cache_key::processed_cache_key(CacheKeyParts { path, source_url: &decoded_url, @@ -208,7 +216,12 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // redirect again and the redirect policy gives the accurate answer, which // is right whether the destination moved somewhere permitted or nowhere. let cached_image = state.cache.get(cache_key.as_ref()).await.filter(|cached| { - let permitted = cached.source_url.is_empty() || config.source_rules.permits(&cached.source_url); + let source_ok = cached.source_url.is_empty() || config.source_rules.permits(&cached.source_url); + // The watermark's pixels are in the composite, so where they came from + // counts exactly as much as where the image's own did. + let watermark_ok = + cached.watermark_source_url.is_empty() || config.source_rules.permits(&cached.watermark_source_url); + let permitted = source_ok && watermark_ok; if !permitted { debug!("Ignoring a cached entry fetched from a source that is no longer allowed"); } @@ -302,10 +315,13 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> .await; } - let watermark = if needs_watermark(&parsed_options) { - resolve_watermark(state.as_ref(), &parsed_options).await? + let (watermark, watermark_fetched_from) = if needs_watermark(&parsed_options) { + match resolve_watermark(state.as_ref(), &parsed_options).await? { + Some((watermark, fetched_from)) => (Some(watermark), fetched_from), + None => (None, String::new()), + } } else { - None + (None, String::new()) }; let waiting = ImageOperationActivityGuard::waiting(ImageOperation::Process); @@ -427,6 +443,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> bytes: processed_image_bytes.clone(), content_type, source_url: fetched_from.clone(), + watermark_source_url: watermark_fetched_from.clone(), }, ) { error!("Failed to cache image: {}", err); @@ -611,7 +628,18 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re // moment `IMGFORGE_BASE_URL` changes. let metadata_key = format!("{decoded_url}\u{0}{path}"); - if let Some(cached_metadata) = state.metadata_cache.get(&metadata_key).await { + // Same rule as the image cache: the request's own URL was checked above, + // but a redirect can have moved the described source somewhere the allow + // list no longer names, and an entry outlives the policy that admitted it. + let cached_metadata = state.metadata_cache.get(&metadata_key).await.filter(|cached| { + let permitted = cached.source_url.is_empty() || config.source_rules.permits(&cached.source_url); + if !permitted { + debug!("Ignoring cached metadata read from a source that is no longer allowed"); + } + permitted + }); + + if let Some(cached_metadata) = cached_metadata { debug!("Metadata found in cache for path={}", path); return Ok(ImageInfo { width: cached_metadata.width, @@ -627,6 +655,7 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re } let fetched = fetch_image(&state.http_client, &decoded_url, None).await?; + let fetched_from = fetched.final_url; let image_bytes = fetched.bytes; let content_type = fetched.content_type; @@ -672,6 +701,7 @@ pub async fn image_info(state: Arc, request: ProcessRequest<'_>) -> Re has_alpha: image_has_alpha(channels), orientation: read_exif_orientation(&image_bytes).unwrap_or(0), pages: img.get_n_pages().max(1) as u32, + source_url: fetched_from, }, true, ) @@ -786,10 +816,13 @@ fn needs_watermark(parsed_options: &ParsedOptions) -> bool { parsed_options.watermark.is_some() || parsed_options.watermark_url.is_some() } +/// The watermark to composite, alongside the URL it was actually fetched +/// from — empty for the configured file watermark, which no allow list +/// governs. async fn resolve_watermark( state: &AppState, parsed_options: &ParsedOptions, -) -> Result, ServiceError> { +) -> Result, ServiceError> { if let Some(url) = &parsed_options.watermark_url { // A watermark is a source like any other: the deployment decided which // hosts it will fetch from, and that decision cannot depend on which @@ -797,7 +830,7 @@ async fn resolve_watermark( let url = permitted_url(&state.config, url)?; debug!("Fetching watermark from URL: {}", url); match fetch_image(&state.http_client, &url, None).await { - Ok(fetched) => Ok(Some(CachedWatermark::from_bytes(fetched.bytes))), + Ok(fetched) => Ok(Some((CachedWatermark::from_bytes(fetched.bytes), fetched.final_url))), Err(source) => Err(ServiceError::WatermarkFetch { source }), } } else if parsed_options.watermark.is_some() { @@ -842,7 +875,7 @@ async fn resolve_watermark( .map_err(ServiceError::from) }) .await?; - Ok(Some(watermark.clone())) + Ok(Some((watermark.clone(), String::new()))) } else { Ok(None) } @@ -876,6 +909,8 @@ async fn serve_source_response( bytes: image_bytes.clone(), content_type, source_url: fetched_from.to_string(), + // A passthrough composites nothing. + watermark_source_url: String::new(), }, ) { error!("Failed to cache raw image: {}", err); From e9e0ad571d821d7b53b9303c01454e77dd093ea9 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:18:11 -0400 Subject: [PATCH 04/13] Let an explicit dpr:1 refuse a larger client hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default ratio was stored as Some(1.0), identical to a URL that explicitly asked for dpr:1, so the hint gate told them apart by value and overwrote both. The default is now None — presence is the signal — and a URL that names any ratio, 1 included, keeps it. Co-Authored-By: Claude Fable 5 --- src/negotiation.rs | 22 +++++++++++++++++++++- src/processing/options/mod.rs | 6 +++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/negotiation.rs b/src/negotiation.rs index 7389ae8..f91e327 100644 --- a/src/negotiation.rs +++ b/src/negotiation.rs @@ -165,7 +165,10 @@ pub fn vary_headers(config: &Config) -> Vec<&'static str> { /// open. pub fn apply_client_hints(options: &mut ParsedOptions, hints: &RequestHints) { if let Some(dpr) = hints.dpr { - if options.dpr.is_none_or(|configured| configured <= 1.0) { + // Only when the URL left the choice open. `dpr:1` names a ratio just + // as `dpr:2` does, but the explicit form and the default both arrive + // here as 1.0 — so presence is the signal, not the value. + if options.dpr.is_none() { debug!("Applying client DPR hint: {}", dpr); options.dpr = Some(dpr.clamp(1.0, 5.0)); } @@ -407,6 +410,23 @@ mod tests { ); assert_eq!(options.dpr, Some(3.0), "and the URL's dpr still wins for scaling"); + // `dpr:1` in the URL is a choice, not an absence: it says "do not + // scale", and a larger hint must not overrule it. It used to, because + // the default was also stored as 1.0 and the two were told apart by + // value rather than by presence. + let mut options = ParsedOptions { + dpr: Some(1.0), + ..ParsedOptions::default() + }; + apply_client_hints( + &mut options, + &RequestHints { + dpr: Some(2.0), + ..RequestHints::default() + }, + ); + assert_eq!(options.dpr, Some(1.0), "an explicit dpr:1 refuses the hint"); + // The same division applies when the hint fills a zero-width resize the // URL already named. let mut options = ParsedOptions { diff --git a/src/processing/options/mod.rs b/src/processing/options/mod.rs index 685fe92..e2f10d5 100644 --- a/src/processing/options/mod.rs +++ b/src/processing/options/mod.rs @@ -199,7 +199,11 @@ impl ParsedOptions { expires: None, filename: None, return_attachment: defaults.return_attachment, - dpr: Some(1.0), + // `None` until the URL names one — presence is how "the URL said + // `dpr:1`" stays distinguishable from "the URL said nothing", which + // is what lets an explicit `dpr:1` refuse a larger client hint. + // `dpr_factor()` reads the absence as 1.0. + dpr: None, min_width: None, min_height: None, zoom: None, From bcd36aeb154c023abefe9cde07af074d36745926 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:26:06 -0400 Subject: [PATCH 05/13] Report debug diagnostics for passthrough responses too A raw or skip_processing miss always returned debug: None, so enabling IMGFORGE_ENABLE_DEBUG_HEADERS silently did nothing for exactly the requests whose passthrough nature the headers would have made visible. A passthrough returns the source as the result, so both halves of the diagnostics describe the same bytes. Co-Authored-By: Claude Fable 5 --- src/service/mod.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/service/mod.rs b/src/service/mod.rs index 07cd72f..ac37bef 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -921,13 +921,31 @@ async fn serve_source_response( let headers = DeliveryHeaders::build(&state.config, source_metadata, &image_bytes, vary); + // A passthrough returns the source as the result, so both halves of the + // diagnostics describe the same bytes. Leaving the whole struct out + // silently switched the feature off for raw and skip_processing requests. + let debug = state.config.enable_debug_headers.then(|| { + let mut debug = DebugInfo { + origin_bytes: image_bytes.len(), + ..DebugInfo::default() + }; + if let Ok(img) = VipsImage::new_from_buffer(&image_bytes, "") { + let (width, height) = (img.get_width().max(0) as u32, img.get_height().max(0) as u32); + debug.origin_width = width; + debug.origin_height = height; + debug.result_width = width; + debug.result_height = height; + } + debug + }); + Ok(ProcessedImage { bytes: image_bytes, content_type, cache_status: CacheStatus::Miss, content_disposition, headers, - debug: None, + debug, }) } From b17324f4a2ffeb4aca9089aa7b120a7f2a7df80f Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:36:08 -0400 Subject: [PATCH 06/13] Keep bearer-protected responses out of shared caches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With IMGFORGE_SECRET set, the TTL policy said public — an explicit invitation for a CDN to store the authorised answer and replay it to a request carrying no token. Bearer deployments now always say private: the TTL keeps its max-age, passthrough no longer forwards the origin's policy (the origin cannot know a token guards it now), and even with nothing configured the refusal is said out loud so heuristic caching cannot decide otherwise. Co-Authored-By: Claude Fable 5 --- src/response.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/response.rs b/src/response.rs index ea869ef..142122a 100644 --- a/src/response.rs +++ b/src/response.rs @@ -63,6 +63,19 @@ impl SourceMetadata { /// client falls back to its own heuristics, which is the behaviour imgforge /// has always had. fn cache_control(config: &Config, source_cache_control: Option<&str>) -> Option { + // A bearer-protected response must never be reusable from a shared cache: + // `public` expressly invites a CDN to store the authorised answer and + // replay it to a request carrying no token. The origin's own policy does + // not get a say either — it cannot know imgforge put a token in front of + // it — so this outranks passthrough, and it applies even with no TTL + // configured, because heuristic caching needs refusing too. + if config.secret.as_deref().is_some_and(|secret| !secret.is_empty()) { + return Some(match config.ttl { + Some(ttl) => format!("max-age={ttl}, private"), + None => "private".to_string(), + }); + } + if config.cache_control_passthrough { if let Some(value) = source_cache_control.filter(|value| !value.trim().is_empty()) { return Some(sanitise_header_value(value)); From 27cc355bc005a385bf6b53e6a2f55caf4c76f6dc Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:36:08 -0400 Subject: [PATCH 07/13] Answer CORS preflights on the image and info routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authorization is not a safelisted request header, so a browser holding a bearer token asks with OPTIONS before it will send the real request. The router knew only GET, so the preflight got a 405 — and no header on the eventual GET could repair that. With IMGFORGE_ALLOW_ORIGIN set, an OPTIONS handler now grants the origin, GET, and the Authorization header; without it, OPTIONS keeps meaning 405. Co-Authored-By: Claude Fable 5 --- src/handlers.rs | 20 +++++ src/server.rs | 5 +- tests/handlers_integration_tests_extended.rs | 78 +++++++++++++++++++- 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/src/handlers.rs b/src/handlers.rs index 3351350..d78a601 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -16,6 +16,26 @@ pub async fn status_handler() -> impl IntoResponse { (StatusCode::OK, Json(json!({"status": "ok"}))) } +/// Answers CORS preflights for the image and info routes. +/// +/// `Authorization` is not a safelisted request header, so a browser holding a +/// bearer token asks with `OPTIONS` before it will send the real request. +/// Putting `Access-Control-Allow-Origin` on the eventual GET could never make +/// that work: the preflight hit a router with only GET handlers and was told +/// 405 before any CORS header existed. +pub async fn preflight_handler(State(state): State>) -> Response { + let Some(origin) = state.config.allow_origin.as_deref() else { + return StatusCode::METHOD_NOT_ALLOWED.into_response(); + }; + + let mut headers = HeaderMap::new(); + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_METHODS, "GET, OPTIONS"); + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_HEADERS, "Authorization"); + insert_header(&mut headers, header::ACCESS_CONTROL_MAX_AGE, "86400"); + (StatusCode::NO_CONTENT, headers).into_response() +} + /// Handles the /info/{*path} endpoint, returning metadata about the source image. pub async fn info_handler( State(state): State>, diff --git a/src/server.rs b/src/server.rs index 8ee13b1..cbd462d 100644 --- a/src/server.rs +++ b/src/server.rs @@ -3,7 +3,7 @@ use crate::caching::config::CacheConfig; use crate::caching::error::CacheError; use crate::config::{Config, ConfigError}; use crate::constants::*; -use crate::handlers::{image_forge_handler, info_handler, status_handler}; +use crate::handlers::{image_forge_handler, info_handler, preflight_handler, status_handler}; use crate::middleware; use crate::monitoring; use axum::http::StatusCode; @@ -97,7 +97,7 @@ pub async fn start() -> Result<(), ServerError> { // its orchestration; IMGFORGE_HEALTH_CHECK_PATH renames the latter. let mut app = Router::new() .route(&route("/status"), get(status_handler)) - .route(&route("/info/{*path}"), get(info_handler)); + .route(&route("/info/{*path}"), get(info_handler).options(preflight_handler)); if health_path != "/status" { app = app.route(&route(&health_path), get(status_handler)); @@ -107,6 +107,7 @@ pub async fn start() -> Result<(), ServerError> { .route( &route("/{*path}"), get(image_forge_handler) + .options(preflight_handler) .layer(axum::middleware::from_fn_with_state( state.clone(), middleware::rate_limit_middleware, diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index 90fd23b..907a8e1 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -11,7 +11,7 @@ use imgforge::caching::cache::ImgforgeCache; use imgforge::caching::config::CacheConfig; use imgforge::config::Config; use imgforge::config::{SourcePattern, SourceRules}; -use imgforge::handlers::image_forge_handler; +use imgforge::handlers::{image_forge_handler, preflight_handler}; use imgforge::middleware::request_id_middleware; use imgforge::MaxSourceFileSize; use lazy_static::lazy_static; @@ -861,6 +861,82 @@ async fn cache_control_comes_from_the_ttl_or_the_origin() { ); } +/// A bearer-protected deployment must not mark responses shared-cacheable: +/// `public` invites a CDN to replay the authorised answer to a request that +/// carries no token. Neither the TTL default nor the origin's own policy gets +/// a say — the origin cannot know a token now guards it. +#[tokio::test] +async fn bearer_protected_responses_are_never_publicly_cacheable() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Cache-Control", "public, max-age=99")]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + let auth = [("authorization", "Bearer token")]; + + let mut config = delivery_config(); + config.secret = Some("token".to_string()); + config.ttl = Some(600); + config.cache_control_passthrough = true; + let response = delivery_request(delivery_state(config).await, &uri, &auth).await; + assert_eq!(response.status, StatusCode::OK); + assert_eq!( + delivery_header(&response, "cache-control").as_deref(), + Some("max-age=600, private"), + "passthrough must not forward the origin's public policy" + ); + + // With no TTL the refusal still has to be said out loud, or a heuristic + // cache decides for itself. + let mut config = delivery_config(); + config.secret = Some("token".to_string()); + let response = delivery_request(delivery_state(config).await, &uri, &auth).await; + assert_eq!(delivery_header(&response, "cache-control").as_deref(), Some("private")); +} + +/// `Authorization` is not a safelisted request header, so a browser holding a +/// bearer token sends OPTIONS before the real request. A router answering only +/// GET turned that preflight into a 405, and no header on the eventual GET +/// could repair it. +#[tokio::test] +async fn a_cors_preflight_is_answered_when_an_origin_is_allowed() { + let preflight = |state: Arc| async { + let app = axum::Router::new() + .route( + "/{*path}", + axum::routing::get(image_forge_handler).options(preflight_handler), + ) + .with_state(state); + app.oneshot( + Request::builder() + .method("OPTIONS") + .uri("/unsafe/rs:fit:20:20/whatever") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap() + }; + + let mut config = delivery_config(); + config.allow_origin = Some("https://app.example.test".to_string()); + let response = preflight(delivery_state(config).await).await; + assert_eq!(response.status(), StatusCode::NO_CONTENT); + assert_eq!( + response.headers().get("access-control-allow-origin").unwrap(), + "https://app.example.test" + ); + let allowed_headers = response + .headers() + .get("access-control-allow-headers") + .expect("the preflight names the headers it permits") + .to_str() + .unwrap(); + assert!(allowed_headers.contains("Authorization"), "got: {allowed_headers}"); + + // Without a configured origin there is nothing to grant. + let response = preflight(delivery_state(delivery_config()).await).await; + assert_eq!(response.status(), StatusCode::METHOD_NOT_ALLOWED); +} + #[tokio::test] async fn last_modified_and_canonical_headers_describe_the_source() { let server = MockServer::start().await; From 442336521b63be91614441ebfeec0d2c9ae6da44 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:42:23 -0400 Subject: [PATCH 08/13] Carry the CORS grant on info answers and read q= case-insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The successful /info JSON was the one response without Access-Control-Allow-Origin, so a cross-origin request could pass the preflight, complete at the server, and still be unreadable to the caller. And Accept parameter names are case-insensitive, so Q=0 is the same refusal as q=0 — reading only the lowercase spelling turned an explicit refusal into the default full weight and served the refused format. Co-Authored-By: Claude Fable 5 --- src/handlers.rs | 9 ++++++- src/negotiation.rs | 16 ++++++++++++- tests/handlers_integration_tests_extended.rs | 25 ++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/handlers.rs b/src/handlers.rs index d78a601..914501f 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -66,7 +66,14 @@ pub async fn info_handler( "orientation": info.orientation, "pages": info.pages, }); - (StatusCode::OK, Json(response)).into_response() + // The grant has to be on the answer, not just the preflight and + // the errors — without it a cross-origin request completes at the + // server and the caller still cannot read the JSON. + let mut headers = HeaderMap::new(); + if let Some(origin) = state.config.allow_origin.as_deref() { + insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); + } + (StatusCode::OK, headers, Json(response)).into_response() } Err(err) => { error!(path, error = ?err, "Info handler error"); diff --git a/src/negotiation.rs b/src/negotiation.rs index f91e327..a911ce6 100644 --- a/src/negotiation.rs +++ b/src/negotiation.rs @@ -72,9 +72,17 @@ impl RequestHints { } // Anything unparseable, or absent, is the default of 1. + // Parameter names are case-insensitive, so `Q=0` refuses a + // type exactly as `q=0` does — reading only the lowercase + // spelling turned that refusal into the default full weight. Some( parts - .filter_map(|parameter| parameter.strip_prefix("q=")) + .filter_map(|parameter| { + parameter + .get(..2) + .filter(|prefix| prefix.eq_ignore_ascii_case("q=")) + .map(|_| ¶meter[2..]) + }) .next() .and_then(|q| q.parse::().ok()) .unwrap_or(1.0), @@ -286,6 +294,12 @@ mod tests { Some("webp") ); assert_eq!(negotiate_format(&config, &accepting("image/webp;q=0.0"), false), None); + // Parameter names are case-insensitive too: `Q=0` is the same refusal, + // not an unrecognised parameter defaulting to full weight. + assert_eq!( + negotiate_format(&config, &accepting("image/avif;Q=0, image/webp"), false), + Some("webp") + ); // A positive quality still counts, and matching is case-insensitive. assert_eq!( negotiate_format(&config, &accepting("IMAGE/WEBP;q=0.5"), false), diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index 907a8e1..ad3691f 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -892,6 +892,31 @@ async fn bearer_protected_responses_are_never_publicly_cacheable() { assert_eq!(delivery_header(&response, "cache-control").as_deref(), Some("private")); } +/// The CORS grant has to be on the successful `/info` answer itself: a browser +/// that passed the preflight still cannot read JSON that arrives without it. +#[tokio::test] +async fn info_success_responses_carry_the_cors_grant() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + let uri = format!("/info/unsafe/{encoded}"); + + let mut config = delivery_config(); + config.allow_origin = Some("https://app.example.test".to_string()); + let app = axum::Router::new() + .route("/info/{*path}", axum::routing::get(imgforge::handlers::info_handler)) + .with_state(delivery_state(config).await); + let response = app + .oneshot(Request::builder().uri(&uri).body(Body::empty()).unwrap()) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.headers().get("access-control-allow-origin").unwrap(), + "https://app.example.test" + ); +} + /// `Authorization` is not a safelisted request header, so a browser holding a /// bearer token sends OPTIONS before the real request. A router answering only /// GET turned that preflight into a 405, and no header on the eventual GET From 377c79369fe2f6101085837329d313915dc2c8f5 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 20:51:43 -0400 Subject: [PATCH 09/13] Refuse malformed allow-list entries at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An entry that failed URL parsing fell back to raw-prefix text matching — the exact bug class the parsed matcher exists to end. A mistyped https://trusted.example:bad text-matched https://trusted.example:bad@evil.test/x, whose real host hides behind userinfo. Parsing is now fallible: a boundary that cannot be enforced as written is a configuration error naming the entry, not a different boundary. Co-Authored-By: Claude Fable 5 --- src/config/mod.rs | 13 ++- src/config/source.rs | 83 +++++++++++--------- tests/handlers_integration_tests_extended.rs | 22 +++--- 3 files changed, 67 insertions(+), 51 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index 9d60742..348a03c 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -58,6 +58,8 @@ pub enum ConfigError { }, #[error("image-processing worker count must be greater than zero")] ZeroWorkers, + #[error("invalid IMGFORGE_ALLOWED_SOURCES entry {entry:?}: {reason}")] + InvalidSourcePattern { entry: String, reason: String }, #[error("image-processing worker count {value} exceeds the supported maximum of {max}")] WorkerCountTooLarge { value: usize, max: usize }, #[error("invalid value for {name} ({value:?}): {reason}")] @@ -444,11 +446,18 @@ impl Config { config.source_rules = SourceRules { base_url: optional_var(ENV_BASE_URL)?.filter(|value| !value.trim().is_empty()), + // A malformed entry is a startup error: an allow list that cannot + // be enforced as written must not quietly enforce something else. allowed: list_var(ENV_ALLOWED_SOURCES)? .unwrap_or_default() .iter() - .map(|pattern| SourcePattern::parse(pattern)) - .collect(), + .map(|pattern| { + SourcePattern::parse(pattern).map_err(|reason| ConfigError::InvalidSourcePattern { + entry: pattern.clone(), + reason, + }) + }) + .collect::, _>>()?, }; config.user_agent = optional_var(ENV_USER_AGENT)? .filter(|value| !value.trim().is_empty()) diff --git a/src/config/source.rs b/src/config/source.rs index 04b73ff..e5481e0 100644 --- a/src/config/source.rs +++ b/src/config/source.rs @@ -42,23 +42,19 @@ enum HostMatch { Exact(String), /// The host must end with this after exactly one further label. Suffix(String), - /// The entry could not be parsed as a URL prefix at all, so it is compared - /// as literal text. imgproxy documents entries with a scheme; this only - /// keeps a malformed one behaving as it did rather than silently matching - /// nothing, and it can never be more permissive than the string it names. - Unstructured, } impl SourcePattern { - pub fn parse(pattern: &str) -> Self { - let unstructured = || Self { - scheme: pattern.to_string(), - host: HostMatch::Unstructured, - port: None, - path_prefix: String::new(), - query_prefix: None, - }; - + /// Parses one entry, or says why it cannot be one. + /// + /// A malformed entry is refused rather than matched as text. The fallback + /// used to compare raw prefixes, and raw prefixes are the bug class this + /// type exists to end: `https://trusted.example:bad` failed to parse, so + /// it text-matched `https://trusted.example:bad@evil.test/x` — a URL whose + /// real host hides behind userinfo. An entry that cannot be parsed cannot + /// be enforced, and a security boundary that cannot be enforced is a + /// startup error, not a matching strategy. + pub fn parse(pattern: &str) -> Result { // The wildcard is not a legal host, so the entry cannot be parsed as a // URL directly. Substituting a placeholder label lets the same parser // handle both forms, and keeps the port and path handling identical. @@ -67,17 +63,15 @@ impl SourcePattern { None => (pattern.to_string(), false), }; - let Ok(parsed) = Url::parse(&probe) else { - return unstructured(); - }; + let parsed = Url::parse(&probe).map_err(|err| format!("not a valid URL prefix: {err}"))?; let Some(host) = parsed.host_str() else { - return unstructured(); + return Err("the entry names no host".to_string()); }; let host = if wildcard { match host.split_once('.') { - Some((_, suffix)) => HostMatch::Suffix(format!(".{suffix}")), - None => return unstructured(), + Some((_, suffix)) if !suffix.is_empty() => HostMatch::Suffix(format!(".{suffix}")), + _ => return Err("the wildcard needs a domain after it".to_string()), } } else { HostMatch::Exact(host.to_string()) @@ -94,20 +88,16 @@ impl SourcePattern { None => (normalise_prefix(parsed.path(), pattern), None), }; - Self { + Ok(Self { scheme: parsed.scheme().to_string(), host, port: parsed.port(), path_prefix, query_prefix, - } + }) } pub fn matches(&self, url: &str) -> bool { - if matches!(self.host, HostMatch::Unstructured) { - return url.starts_with(&self.scheme); - } - let Ok(parsed) = Url::parse(url) else { return false; }; @@ -129,7 +119,6 @@ impl SourcePattern { None => false, } } - HostMatch::Unstructured => unreachable!("handled above"), }; // `Url::path()` is normalised, so `/public/../private/x` arrives here as @@ -209,7 +198,7 @@ mod tests { #[test] fn a_wildcard_stands_for_exactly_one_label() { - let pattern = SourcePattern::parse("https://*.example.com/"); + let pattern = SourcePattern::parse("https://*.example.com/").expect("pattern parses"); assert!(pattern.matches("https://images.example.com/cat.jpg")); // Bare domain and multi-label subdomains are both outside the pattern, @@ -232,7 +221,7 @@ mod tests { // Both spellings of the pattern have to hold: only the trailing slash // was ever incidentally anchored. for spelling in ["https://*.example.com", "https://*.example.com/"] { - let pattern = SourcePattern::parse(spelling); + let pattern = SourcePattern::parse(spelling).expect("pattern parses"); assert!(pattern.matches("https://img.example.com/cat.jpg"), "{spelling}"); assert!( @@ -257,12 +246,26 @@ mod tests { } } + /// A malformed entry is a startup error, not a matching strategy. The old + /// text fallback compared raw prefixes, so `https://trusted.example:bad` + /// matched `https://trusted.example:bad@evil.test/x` — a URL whose real + /// host hides behind userinfo, which is the bug class parsing exists to + /// end. + #[test] + fn a_malformed_entry_is_refused_rather_than_text_matched() { + assert!(SourcePattern::parse("https://trusted.example:bad").is_err()); + // No scheme is no URL prefix. + assert!(SourcePattern::parse("*.example.com").is_err()); + // A wildcard with nothing after it stands for no domain at all. + assert!(SourcePattern::parse("https://*.").is_err()); + } + /// An entry with a query is a prefix of the whole URL, query included. /// Discarding it read `?tenant=public` as "any query or none", which /// widened the boundary the operator wrote down. #[test] fn a_query_in_the_entry_restricts_the_match() { - let pattern = SourcePattern::parse("https://api.example.test/render?tenant=public"); + let pattern = SourcePattern::parse("https://api.example.test/render?tenant=public").expect("pattern parses"); assert!(pattern.matches("https://api.example.test/render?tenant=public")); // More query after the prefix is more URL after the prefix, which a @@ -277,7 +280,7 @@ mod tests { // An entry without a query keeps its prefix-of-the-path reading and // says nothing about the candidate's query. - let open = SourcePattern::parse("https://api.example.test/render"); + let open = SourcePattern::parse("https://api.example.test/render").expect("pattern parses"); assert!(open.matches("https://api.example.test/render?tenant=private")); } @@ -288,7 +291,7 @@ mod tests { #[test] fn a_literal_entry_cannot_be_extended_into_another_domain() { for spelling in ["https://cdn.example.com", "https://cdn.example.com/"] { - let pattern = SourcePattern::parse(spelling); + let pattern = SourcePattern::parse(spelling).expect("pattern parses"); assert!(pattern.matches("https://cdn.example.com/cat.jpg"), "{spelling}"); assert!( @@ -311,7 +314,7 @@ mod tests { } // An entry that names a port matches that port. - let ported = SourcePattern::parse("https://cdn.example.com:8443/"); + let ported = SourcePattern::parse("https://cdn.example.com:8443/").expect("pattern parses"); assert!(ported.matches("https://cdn.example.com:8443/cat.jpg")); assert!(!ported.matches("https://cdn.example.com/cat.jpg")); } @@ -323,7 +326,7 @@ mod tests { #[test] fn a_path_prefix_cannot_be_escaped_by_traversal() { for spelling in ["https://cdn.example.com/public/", "https://*.example.com/public/"] { - let pattern = SourcePattern::parse(spelling); + let pattern = SourcePattern::parse(spelling).expect("pattern parses"); let host = if spelling.contains('*') { "img.example.com" } else { @@ -356,14 +359,18 @@ mod tests { /// Hosts are compared case-insensitively, as DNS does. #[test] fn host_matching_ignores_case() { - assert!(SourcePattern::parse("https://cdn.example.com/").matches("https://CDN.Example.COM/cat.jpg")); - assert!(SourcePattern::parse("https://*.example.com/").matches("https://IMG.Example.com/cat.jpg")); + assert!(SourcePattern::parse("https://cdn.example.com/") + .expect("pattern parses") + .matches("https://CDN.Example.COM/cat.jpg")); + assert!(SourcePattern::parse("https://*.example.com/") + .expect("pattern parses") + .matches("https://IMG.Example.com/cat.jpg")); } /// A wildcard may also constrain the path, and that half stays a prefix. #[test] fn a_wildcard_can_carry_a_path_prefix() { - let pattern = SourcePattern::parse("https://*.example.com/assets/"); + let pattern = SourcePattern::parse("https://*.example.com/assets/").expect("pattern parses"); assert!(pattern.matches("https://img.example.com/assets/cat.jpg")); assert!(!pattern.matches("https://img.example.com/private/cat.jpg")); @@ -372,7 +379,7 @@ mod tests { #[test] fn a_plain_pattern_matches_by_prefix() { - let pattern = SourcePattern::parse("https://cdn.example.com/assets/"); + let pattern = SourcePattern::parse("https://cdn.example.com/assets/").expect("pattern parses"); assert!(pattern.matches("https://cdn.example.com/assets/cat.jpg")); assert!(!pattern.matches("https://cdn.example.com/private/cat.jpg")); } diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index ad3691f..cb69055 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -991,7 +991,7 @@ async fn a_source_outside_the_allow_list_is_refused() { let mut config = delivery_config(); config.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse("https://images.example.com/")], + allowed: vec![SourcePattern::parse("https://images.example.com/").expect("pattern parses")], }; let response = delivery_request(delivery_state(config).await, &uri, &[]).await; @@ -1160,7 +1160,7 @@ async fn a_redirect_out_of_the_allow_list_is_refused() { // Only the first server is permitted; the redirect points at the second. config.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&origin.uri())], + allowed: vec![SourcePattern::parse(&origin.uri()).expect("pattern parses")], }; let encoded = URL_SAFE_NO_PAD.encode(format!("{}/image.png", origin.uri())); @@ -1332,7 +1332,7 @@ async fn a_watermark_url_outside_the_allow_list_is_refused() { let mut permissive = delivery_config(); permissive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri())).expect("pattern parses")], }; let allowed = delivery_request(delivery_state(permissive).await, &uri, &[]).await; assert_eq!( @@ -1346,7 +1346,7 @@ async fn a_watermark_url_outside_the_allow_list_is_refused() { let mut restrictive = delivery_config(); restrictive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse("https://images.example.com/")], + allowed: vec![SourcePattern::parse("https://images.example.com/").expect("pattern parses")], }; let refused = delivery_request(delivery_state(restrictive).await, &uri, &[]).await; assert_eq!(refused.status, StatusCode::BAD_REQUEST); @@ -1358,7 +1358,7 @@ async fn a_watermark_url_outside_the_allow_list_is_refused() { let mut image_only = delivery_config(); image_only.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/image.png", server.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/image.png", server.uri())).expect("pattern parses")], }; let watermark_elsewhere = URL_SAFE_NO_PAD.encode(b"http://169.254.169.254/latest/meta-data/"); let smuggled = format!("/unsafe/rs:fit:20:20/wm:0.5/wmu:{watermark_elsewhere}/{encoded}"); @@ -1390,7 +1390,7 @@ async fn a_cache_hit_does_not_outlive_the_source_allow_list() { let mut permissive = delivery_config(); permissive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri())).expect("pattern parses")], }; let warm = delivery_request_with_cache(permissive, cache.clone(), &uri, &[]).await; assert_eq!(warm.status, StatusCode::OK); @@ -1400,7 +1400,7 @@ async fn a_cache_hit_does_not_outlive_the_source_allow_list() { let mut restrictive = delivery_config(); restrictive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse("https://images.example.com/")], + allowed: vec![SourcePattern::parse("https://images.example.com/").expect("pattern parses")], }; let response = delivery_request_with_cache(restrictive, cache, &uri, &[]).await; assert_eq!( @@ -1520,7 +1520,7 @@ async fn info_does_not_describe_a_source_the_allow_list_now_forbids() { let mut permissive = create_test_config(vec![], vec![], true); permissive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/", server.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/", server.uri())).expect("pattern parses")], }; assert_eq!( respond(permissive, metadata_cache.clone(), uri.clone()).await, @@ -1532,7 +1532,7 @@ async fn info_does_not_describe_a_source_the_allow_list_now_forbids() { let mut restrictive = create_test_config(vec![], vec![], true); restrictive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse("https://images.example.com/")], + allowed: vec![SourcePattern::parse("https://images.example.com/").expect("pattern parses")], }; assert_eq!( respond(restrictive, metadata_cache, uri).await, @@ -1601,7 +1601,7 @@ async fn a_cached_redirect_destination_is_revalidated_on_a_hit() { let mut permissive = create_test_config(vec![], vec![], true); permissive.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/", origin.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/", origin.uri())).expect("pattern parses")], }; assert_eq!( respond(permissive, cache.clone(), uri.clone()).await, @@ -1615,7 +1615,7 @@ async fn a_cached_redirect_destination_is_revalidated_on_a_hit() { let mut entry_only = create_test_config(vec![], vec![], true); entry_only.source_rules = SourceRules { base_url: None, - allowed: vec![SourcePattern::parse(&format!("{}/entry.png", origin.uri()))], + allowed: vec![SourcePattern::parse(&format!("{}/entry.png", origin.uri())).expect("pattern parses")], }; assert_eq!( respond(entry_only, cache, uri).await, From bf08442a8fc8854269f9fda047393c5d834c50f2 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 21:00:13 -0400 Subject: [PATCH 10/13] Store the entity tag with the entry and open preflights to validators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every ETag-enabled cache hit re-hashed the whole body on the async worker — a per-hit SHA-256 for exactly the requests the cache made cheap. The tag is now computed on the miss, which just produced the bytes anyway, stored with the entry regardless of the current setting (the entry outlives the configuration), and served back on hits. The CORS preflight also granted only Authorization, but If-None-Match and If-Modified-Since are not safelisted either — a preflight that blocked them blocked exactly the revalidation this change shipped. Co-Authored-By: Claude Fable 5 --- src/caching/cache.rs | 20 ++++++++++++++++- src/handlers.rs | 9 +++++++- src/response.rs | 19 ++++++++++++++-- src/service/mod.rs | 7 +++++- tests/handlers_integration_tests_extended.rs | 23 ++++++++++++++++++++ 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/caching/cache.rs b/src/caching/cache.rs index 13b399e..d20b8c2 100644 --- a/src/caching/cache.rs +++ b/src/caching/cache.rs @@ -46,6 +46,9 @@ pub struct CachedImage { /// or empty when the entry composites none. Its pixels are in the bytes /// above, so its source is rechecked on a hit exactly as the image's own. pub watermark_source_url: String, + /// The entity tag of `bytes`, computed when the entry was stored so a hit + /// does not hash the whole body again on the async worker. + pub etag: String, } impl Code for CachedImage { @@ -65,6 +68,10 @@ impl Code for CachedImage { let watermark_bytes = self.watermark_source_url.as_bytes(); watermark_bytes.len().encode(writer)?; writer.write_all(watermark_bytes).map_err(FoyerError::io_error)?; + + let etag_bytes = self.etag.as_bytes(); + etag_bytes.len().encode(writer)?; + writer.write_all(etag_bytes).map_err(FoyerError::io_error)?; Ok(()) } @@ -92,11 +99,18 @@ impl Code for CachedImage { let watermark_source_url = String::from_utf8(watermark_buf) .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in watermark source url"))?; + let etag_len = usize::decode(reader)?; + let mut etag_buf = vec![0u8; etag_len]; + reader.read_exact(&mut etag_buf).map_err(FoyerError::io_error)?; + let etag = + String::from_utf8(etag_buf).map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in etag"))?; + Ok(CachedImage { bytes: Bytes::from(data), content_type, source_url, watermark_source_url, + etag, }) } @@ -105,7 +119,8 @@ impl Code for CachedImage { + self.content_type.len() + self.source_url.len() + self.watermark_source_url.len() - + std::mem::size_of::() * 4 + + self.etag.len() + + std::mem::size_of::() * 5 } } @@ -449,6 +464,7 @@ mod tests { content_type: "image/jpeg", source_url: "https://example.test/cached.png".to_string(), watermark_source_url: "https://cdn.example.test/mark.png".to_string(), + etag: "\"abc123\"".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); @@ -469,6 +485,7 @@ mod tests { content_type: "image/jpeg", source_url: "https://example.test/cached.png".to_string(), watermark_source_url: "https://cdn.example.test/mark.png".to_string(), + etag: "\"abc123\"".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); let retrieved = cache.get(&key).await.unwrap(); @@ -478,6 +495,7 @@ mod tests { // hit-time allow-list check silently checks nothing. assert_eq!(retrieved.source_url, value.source_url); assert_eq!(retrieved.watermark_source_url, value.watermark_source_url); + assert_eq!(retrieved.etag, value.etag); } #[test] diff --git a/src/handlers.rs b/src/handlers.rs index 914501f..833d854 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -31,7 +31,14 @@ pub async fn preflight_handler(State(state): State>) -> Response { let mut headers = HeaderMap::new(); insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_METHODS, "GET, OPTIONS"); - insert_header(&mut headers, header::ACCESS_CONTROL_ALLOW_HEADERS, "Authorization"); + // The conditional validators are not safelisted either, and this change is + // what added 304 support — a preflight that only granted Authorization + // blocked exactly the revalidation it shipped. + insert_header( + &mut headers, + header::ACCESS_CONTROL_ALLOW_HEADERS, + "Authorization, If-None-Match, If-Modified-Since", + ); insert_header(&mut headers, header::ACCESS_CONTROL_MAX_AGE, "86400"); (StatusCode::NO_CONTENT, headers).into_response() } diff --git a/src/response.rs b/src/response.rs index 142122a..bdd4806 100644 --- a/src/response.rs +++ b/src/response.rs @@ -20,9 +20,24 @@ pub struct DeliveryHeaders { impl DeliveryHeaders { /// Builds the headers for a response produced from `source`. pub fn build(config: &Config, source: &SourceMetadata, body: &[u8], vary: &[&'static str]) -> Self { + Self::assemble(config, source, config.use_etag.then(|| entity_tag(body)), vary) + } + + /// Builds the headers for a cache hit, whose entity tag was computed when + /// the entry was stored. + /// + /// Hashing belongs on the miss, where the body was just produced anyway. + /// Doing it again per hit put a whole-body SHA-256 on the async worker for + /// exactly the requests that were supposed to be cheap. + pub fn for_cache_hit(config: &Config, source: &SourceMetadata, stored_etag: &str, vary: &[&'static str]) -> Self { + let etag = (config.use_etag && !stored_etag.is_empty()).then(|| stored_etag.to_string()); + Self::assemble(config, source, etag, vary) + } + + fn assemble(config: &Config, source: &SourceMetadata, etag: Option, vary: &[&'static str]) -> Self { Self { cache_control: cache_control(config, source.cache_control.as_deref()), - etag: config.use_etag.then(|| entity_tag(body)), + etag, last_modified: config .last_modified_enabled .then(|| source.last_modified.clone()) @@ -91,7 +106,7 @@ fn cache_control(config: &Config, source_cache_control: Option<&str>) -> Option< /// processing options: the same URL against the same source can still produce /// different bytes once content negotiation is in play, and hashing the result /// is the only derivation that cannot disagree with what was actually sent. -fn entity_tag(body: &[u8]) -> String { +pub fn entity_tag(body: &[u8]) -> String { let digest = Sha256::digest(body); let mut tag = String::with_capacity(2 + 32); tag.push('"'); diff --git a/src/service/mod.rs b/src/service/mod.rs index ac37bef..a365da7 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -234,7 +234,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // A cache hit has no source response to draw on, so the origin's own // caching headers are not available; the configured policy still is, // and the entity tag comes from the bytes either way. - let headers = DeliveryHeaders::build(config, &SourceMetadata::default(), &cached_image.bytes, &vary); + let headers = DeliveryHeaders::for_cache_hit(config, &SourceMetadata::default(), &cached_image.etag, &vary); // Enabling the debug headers should not make them appear and disappear // with the cache. A hit never made the source request, so nothing about @@ -444,6 +444,10 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> content_type, source_url: fetched_from.clone(), watermark_source_url: watermark_fetched_from.clone(), + // Hashed here, on the miss that already produced the bytes, + // regardless of the current ETag setting — the entry outlives + // the configuration, and a hit must not have to hash. + etag: crate::response::entity_tag(&processed_image_bytes), }, ) { error!("Failed to cache image: {}", err); @@ -911,6 +915,7 @@ async fn serve_source_response( source_url: fetched_from.to_string(), // A passthrough composites nothing. watermark_source_url: String::new(), + etag: crate::response::entity_tag(&image_bytes), }, ) { error!("Failed to cache raw image: {}", err); diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index cb69055..30ccb66 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -861,6 +861,29 @@ async fn cache_control_comes_from_the_ttl_or_the_origin() { ); } +/// A hit's ETag comes from the entry rather than from rehashing the body, so +/// it has to be identical to the one the miss sent — or revalidation breaks +/// the moment the cache starts answering. +#[tokio::test] +async fn cache_hits_serve_the_stored_entity_tag() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let cache = ImgforgeCache::new(Some(CacheConfig::Memory { capacity: 1024 * 1024 })) + .await + .unwrap(); + let mut config = delivery_config(); + config.use_etag = true; + + let miss = delivery_request_with_cache(config.clone(), cache.clone(), &uri, &[]).await; + let miss_etag = delivery_header(&miss, "etag").expect("the miss carries a tag"); + + let hit = delivery_request_with_cache(config, cache, &uri, &[]).await; + assert_eq!(delivery_header(&hit, "cache-status").as_deref(), Some("HIT")); + assert_eq!(delivery_header(&hit, "etag").as_deref(), Some(miss_etag.as_str())); +} + /// A bearer-protected deployment must not mark responses shared-cacheable: /// `public` invites a CDN to replay the authorised answer to a request that /// carries no token. Neither the TTL default nor the origin's own policy gets From e45793b75dee87aa4ecc6b27d248eea043041aa1 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 21:11:39 -0400 Subject: [PATCH 11/13] Fail closed on signature sizes, key by watermark URL, spare the workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four hardenings from review. An out-of-range signature_size set by a library caller was clamped, so 0 silently became a one-byte HMAC — brute-forceable in about 256 requests; validation now fails closed on it. The resolved watermark_url joins the cache identity, because a relative reference composites a different overlay the moment IMGFORGE_BASE_URL changes while the path stays identical. CORS preflights no longer spend a rate-limit token — the browser asking permission is not a fetch, and at a limit of 1 the preflight starved the request it was asking for. And the miss-path entity tag is hashed once, inside the blocking task that produced the bytes, instead of twice on the async worker. Co-Authored-By: Claude Fable 5 --- src/middleware.rs | 8 ++++++++ src/response.rs | 18 ++++++++++++++---- src/service/cache_key.rs | 14 ++++++++++++++ src/service/mod.rs | 37 ++++++++++++++++++++++++------------- src/service/tests.rs | 23 +++++++++++++++++++++++ src/url.rs | 23 ++++++++++++++++++++++- 6 files changed, 105 insertions(+), 18 deletions(-) diff --git a/src/middleware.rs b/src/middleware.rs index 9c3fb3f..e5e53b2 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -36,6 +36,14 @@ pub async fn status_code_metric_middleware(req: Request, next: Next) -> Re } pub async fn rate_limit_middleware(State(state): State>, request: Request, next: Next) -> Response { + // A CORS preflight is the browser asking permission, not fetching an + // image; it does no fetching or processing worth a token. Charging it + // billed every cross-origin request twice, and at a limit of 1 the + // preflight spent the only token and the real request got the 429. + if request.method() == axum::http::Method::OPTIONS { + return next.run(request).await; + } + if let Some(rate_limiter) = &state.rate_limiter { match rate_limiter.check() { Ok(_) => next.run(request).await, diff --git a/src/response.rs b/src/response.rs index bdd4806..c5d8bda 100644 --- a/src/response.rs +++ b/src/response.rs @@ -25,11 +25,21 @@ impl DeliveryHeaders { /// Builds the headers for a cache hit, whose entity tag was computed when /// the entry was stored. - /// - /// Hashing belongs on the miss, where the body was just produced anyway. - /// Doing it again per hit put a whole-body SHA-256 on the async worker for - /// exactly the requests that were supposed to be cheap. pub fn for_cache_hit(config: &Config, source: &SourceMetadata, stored_etag: &str, vary: &[&'static str]) -> Self { + Self::with_stored_etag(config, source, stored_etag, vary) + } + + /// Builds the headers around an entity tag that was computed elsewhere. + /// + /// Hashing belongs where the body was produced — inside the blocking task + /// on a miss, at store time for a cache entry. Hashing here put a + /// whole-body SHA-256 on the async worker, twice on an ETag-enabled miss. + pub fn with_stored_etag( + config: &Config, + source: &SourceMetadata, + stored_etag: &str, + vary: &[&'static str], + ) -> Self { let etag = (config.use_etag && !stored_etag.is_empty()).then(|| stored_etag.to_string()); Self::assemble(config, source, etag, vary) } diff --git a/src/service/cache_key.rs b/src/service/cache_key.rs index 0df1116..9523f22 100644 --- a/src/service/cache_key.rs +++ b/src/service/cache_key.rs @@ -58,6 +58,15 @@ pub struct CacheKeyParts<'a> { /// every watermarked response while leaving every URL identical, so without /// this the old logo is served until the entries age out. pub watermark_path: Option<&'a str>, + /// The resolved URL of a `watermark_url` watermark, when the request + /// carries one. + /// + /// A relative watermark reference resolves through `IMGFORGE_BASE_URL` + /// exactly as the main source does, so the same URL composites a different + /// overlay the moment that setting changes — while the path and the main + /// source stay identical. Keying by the resolved form retires those + /// entries the same way `source_url` does for the image itself. + pub watermark_url: Option<&'a str>, /// Configured option defaults, when the deployment changes any of them. /// /// These seed the parse, so `IMGFORGE_QUALITY` and its neighbours change the @@ -168,6 +177,11 @@ fn source_limits<'a>(parts: CacheKeyParts<'a>, base: Cow<'a, str>) -> Cow<'a, st None => base, }; + let base = match parts.watermark_url { + Some(url) => Cow::Owned(format!("wmu={}:{}", digest(&[url.as_bytes()]), base)), + None => base, + }; + match parts.option_defaults { Some(defaults) => Cow::Owned(format!("od={}:{}", defaults_digest(&defaults), base)), None => base, diff --git a/src/service/mod.rs b/src/service/mod.rs index a365da7..de87acf 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -175,10 +175,12 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // The watermark's URL is part of the request too, so it is checked where // the main URL is checked — before the cache can answer. Validating it // only on the miss path let a cached composite keep serving pixels from a - // host the allow list no longer names. - if let Some(url) = parsed_options.watermark_url.as_deref() { - permitted_url(config, url)?; - } + // host the allow list no longer names. The resolved form is kept: it is + // part of the entry's identity for the same reason the main source's is. + let watermark_url = match parsed_options.watermark_url.as_deref() { + Some(url) => Some(permitted_url(config, url)?), + None => None, + }; let cache_key = cache_key::processed_cache_key(CacheKeyParts { path, @@ -202,6 +204,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> .watermark_path .as_deref() .filter(|_| parsed_options.watermark.is_some() && parsed_options.watermark_url.is_none()), + watermark_url: watermark_url.as_deref(), // Only when the deployment changes them, so a default configuration // keeps the keys it already had. option_defaults: Some(config.option_defaults()).filter(|defaults| *defaults != OptionDefaults::default()), @@ -338,7 +341,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> let span = tracing::Span::current(); let blocking_queue = ImageOperationTimer::start(ImageOperation::Process, ImageOperationPhase::BlockingQueue); let want_debug = config.enable_debug_headers; - let (processed_image_bytes, output_format, debug) = tokio::task::spawn_blocking(move || { + let (processed_image_bytes, output_format, debug, etag) = tokio::task::spawn_blocking(move || { drop(blocking_queue); drop(waiting); let _active = ImageOperationActivityGuard::active(ImageOperation::Process); @@ -425,7 +428,13 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> } } - Ok::<_, ServiceError>((processed_image_bytes, output_format, debug)) + // Hashed here, inside the blocking task that just produced the bytes, + // and regardless of the current ETag setting — the cache entry + // outlives the configuration, and neither a hit nor the async worker + // should ever have to hash a body. + let etag = crate::response::entity_tag(&processed_image_bytes); + + Ok::<_, ServiceError>((processed_image_bytes, output_format, debug, etag)) }) .await .map_err(|source| ServiceError::BlockingTask { @@ -434,7 +443,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> })??; let content_type = format_to_content_type(&output_format); - let headers = DeliveryHeaders::build(config, &source_metadata, &processed_image_bytes, &vary); + let headers = DeliveryHeaders::with_stored_etag(config, &source_metadata, &etag, &vary); if content_disposition.is_none() && !matches!(state.cache, ImgforgeCache::None) { if let Err(err) = state.cache.insert( @@ -444,10 +453,7 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> content_type, source_url: fetched_from.clone(), watermark_source_url: watermark_fetched_from.clone(), - // Hashed here, on the miss that already produced the bytes, - // regardless of the current ETag setting — the entry outlives - // the configuration, and a hit must not have to hash. - etag: crate::response::entity_tag(&processed_image_bytes), + etag, }, ) { error!("Failed to cache image: {}", err); @@ -906,6 +912,11 @@ async fn serve_source_response( .map(format_to_content_type) .unwrap_or("image/jpeg"); + // Hashed once and shared by the header and the cache entry — a + // passthrough has no blocking task to hide the cost in, but it does not + // have to pay it twice. + let etag = crate::response::entity_tag(&image_bytes); + if content_disposition.is_none() && !matches!(state.cache, ImgforgeCache::None) { if let Err(err) = state.cache.insert( cache_key.to_string(), @@ -915,7 +926,7 @@ async fn serve_source_response( source_url: fetched_from.to_string(), // A passthrough composites nothing. watermark_source_url: String::new(), - etag: crate::response::entity_tag(&image_bytes), + etag: etag.clone(), }, ) { error!("Failed to cache raw image: {}", err); @@ -924,7 +935,7 @@ async fn serve_source_response( info!("Imgforge served source path={} bytes={}", path, image_bytes.len()); - let headers = DeliveryHeaders::build(&state.config, source_metadata, &image_bytes, vary); + let headers = DeliveryHeaders::with_stored_etag(&state.config, source_metadata, &etag, vary); // A passthrough returns the source as the result, so both halves of the // diagnostics describe the same bytes. Leaving the whole struct out diff --git a/src/service/tests.rs b/src/service/tests.rs index 1fa73f5..46811e6 100644 --- a/src/service/tests.rs +++ b/src/service/tests.rs @@ -28,6 +28,7 @@ fn key_parts(path: &str) -> CacheKeyParts<'_> { max_src_file_size: None, allowed_mime_types: None, watermark_path: None, + watermark_url: None, option_defaults: None, negotiated_format: None, client_hints: None, @@ -125,6 +126,28 @@ fn client_hint_dimensions_get_their_own_cache_entries() { ); } +/// A watermark fetched by URL is part of what the response shows, so its +/// resolved address is part of the entry's identity: a relative reference +/// resolves elsewhere the moment `IMGFORGE_BASE_URL` changes, while the path +/// and the main source stay exactly as they were. +#[test] +fn watermark_urls_get_their_own_cache_entries() { + let path = "/unsafe/resize:fit:100:100/example"; + + let plain = processed_cache_key(key_parts(path)); + let old_base = processed_cache_key(CacheKeyParts { + watermark_url: Some("https://a.example.test/mark.png"), + ..key_parts(path) + }); + let new_base = processed_cache_key(CacheKeyParts { + watermark_url: Some("https://b.example.test/mark.png"), + ..key_parts(path) + }); + + assert_ne!(plain, old_base); + assert_ne!(old_base, new_base); +} + #[test] fn negotiated_formats_get_their_own_cache_entries() { let path = "/unsafe/resize:fit:100:100/example"; diff --git a/src/url.rs b/src/url.rs index d9dcc5e..09b216e 100644 --- a/src/url.rs +++ b/src/url.rs @@ -78,7 +78,14 @@ pub fn validate_signature_of_size(key: &[u8], salt: &[u8], signature: &str, path return false; }; - let signature_size = signature_size.clamp(1, FULL_SIGNATURE_SIZE); + // An out-of-range size is a misconfiguration, and authorization fails + // closed on it. The environment path validates the range at startup, but a + // library caller sets `Config.signature_size` directly — and clamping a 0 + // to 1 handed that caller a one-byte HMAC, brute-forceable in about 256 + // requests, in place of the misconfiguration error they should have seen. + if signature_size == 0 || signature_size > FULL_SIGNATURE_SIZE { + return false; + } // A signature of the wrong length is rejected outright. Without this a // single correct byte would pass whenever the configuration allowed a @@ -273,6 +280,20 @@ mod tests { assert!(!validate_signature_of_size(key, salt, &short, path, 16)); assert!(!validate_signature_of_size(key, salt, &short, path, 32)); + // An out-of-range size fails closed rather than being clamped: 0 used + // to silently become a one-byte HMAC for library callers who set the + // field directly. + let full_encoded = URL_SAFE_NO_PAD.encode(&full[..]); + assert!(!validate_signature_of_size(key, salt, &full_encoded, path, 0)); + assert!(!validate_signature_of_size(key, salt, &full_encoded, path, 33)); + assert!(!validate_signature_of_size( + key, + salt, + &URL_SAFE_NO_PAD.encode(&full[..1]), + path, + 0 + )); + // And a wrong prefix still fails at the shortened length. let mut wrong = full[..8].to_vec(); wrong[0] ^= 0xff; From 56fcef3dae3e8943488e6e34de729f948f7de091 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 21:20:56 -0400 Subject: [PATCH 12/13] Keep the origin's delivery metadata across cache hits, bound width hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hit rebuilt its headers from empty source metadata, so a passthrough no-store — the origin explicitly forbidding storage — vanished the moment the cache started answering, and Last-Modified went with it. The entry now stores the origin's Cache-Control and Last-Modified and a hit keeps saying what the origin said. The Width client hint is attacker-adjacent input: a signature covers the path, not the headers, so a reusable signed URL that leaves its width to hints must not let Width: 1000000 size the pipeline. The hint is now bounded by the configured result ceiling when one is in force and by a 16384px hard cap when none is. Co-Authored-By: Claude Fable 5 --- src/caching/cache.rs | 44 +++++++++++++++++++- src/negotiation.rs | 42 +++++++++++++++++++ src/service/mod.rs | 16 ++++++- tests/handlers_integration_tests_extended.rs | 23 ++++++++++ 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/caching/cache.rs b/src/caching/cache.rs index d20b8c2..ba2b65d 100644 --- a/src/caching/cache.rs +++ b/src/caching/cache.rs @@ -49,6 +49,14 @@ pub struct CachedImage { /// The entity tag of `bytes`, computed when the entry was stored so a hit /// does not hash the whole body again on the async worker. pub etag: String, + /// The origin's own `Cache-Control`, empty when it sent none. + /// + /// Kept so a hit under passthrough keeps saying what the origin said — + /// losing a `no-store` the moment the cache answered invited shared caches + /// to store exactly what the origin forbade. + pub origin_cache_control: String, + /// The origin's `Last-Modified`, for the same reason. Empty when absent. + pub origin_last_modified: String, } impl Code for CachedImage { @@ -72,6 +80,14 @@ impl Code for CachedImage { let etag_bytes = self.etag.as_bytes(); etag_bytes.len().encode(writer)?; writer.write_all(etag_bytes).map_err(FoyerError::io_error)?; + + let cache_control_bytes = self.origin_cache_control.as_bytes(); + cache_control_bytes.len().encode(writer)?; + writer.write_all(cache_control_bytes).map_err(FoyerError::io_error)?; + + let last_modified_bytes = self.origin_last_modified.as_bytes(); + last_modified_bytes.len().encode(writer)?; + writer.write_all(last_modified_bytes).map_err(FoyerError::io_error)?; Ok(()) } @@ -105,12 +121,30 @@ impl Code for CachedImage { let etag = String::from_utf8(etag_buf).map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in etag"))?; + let cache_control_len = usize::decode(reader)?; + let mut cache_control_buf = vec![0u8; cache_control_len]; + reader + .read_exact(&mut cache_control_buf) + .map_err(FoyerError::io_error)?; + let origin_cache_control = String::from_utf8(cache_control_buf) + .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in origin cache control"))?; + + let last_modified_len = usize::decode(reader)?; + let mut last_modified_buf = vec![0u8; last_modified_len]; + reader + .read_exact(&mut last_modified_buf) + .map_err(FoyerError::io_error)?; + let origin_last_modified = String::from_utf8(last_modified_buf) + .map_err(|_| FoyerError::new(ErrorKind::Parse, "invalid utf8 in origin last modified"))?; + Ok(CachedImage { bytes: Bytes::from(data), content_type, source_url, watermark_source_url, etag, + origin_cache_control, + origin_last_modified, }) } @@ -120,7 +154,9 @@ impl Code for CachedImage { + self.source_url.len() + self.watermark_source_url.len() + self.etag.len() - + std::mem::size_of::() * 5 + + self.origin_cache_control.len() + + self.origin_last_modified.len() + + std::mem::size_of::() * 7 } } @@ -465,6 +501,8 @@ mod tests { source_url: "https://example.test/cached.png".to_string(), watermark_source_url: "https://cdn.example.test/mark.png".to_string(), etag: "\"abc123\"".to_string(), + origin_cache_control: "no-store".to_string(), + origin_last_modified: "Wed, 21 Oct 2015 07:28:00 GMT".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); @@ -486,6 +524,8 @@ mod tests { source_url: "https://example.test/cached.png".to_string(), watermark_source_url: "https://cdn.example.test/mark.png".to_string(), etag: "\"abc123\"".to_string(), + origin_cache_control: "no-store".to_string(), + origin_last_modified: "Wed, 21 Oct 2015 07:28:00 GMT".to_string(), }; cache.insert(key.clone(), value.clone()).unwrap(); let retrieved = cache.get(&key).await.unwrap(); @@ -496,6 +536,8 @@ mod tests { assert_eq!(retrieved.source_url, value.source_url); assert_eq!(retrieved.watermark_source_url, value.watermark_source_url); assert_eq!(retrieved.etag, value.etag); + assert_eq!(retrieved.origin_cache_control, value.origin_cache_control); + assert_eq!(retrieved.origin_last_modified, value.origin_last_modified); } #[test] diff --git a/src/negotiation.rs b/src/negotiation.rs index a911ce6..305efe0 100644 --- a/src/negotiation.rs +++ b/src/negotiation.rs @@ -186,6 +186,18 @@ pub fn apply_client_hints(options: &mut ParsedOptions, hints: &RequestHints) { return; }; + // The hint is attacker-adjacent input: a signature covers the path, not + // the headers, so a reusable signed URL that leaves its width to hints + // must not let `Width: 1000000` size the pipeline. Bounded by the result + // ceiling when one is in force, and by a hard cap no real screen exceeds + // when none is. + const MAX_HINTED_WIDTH: u32 = 16_384; + let cap = options + .max_result_dimension + .map(|limit| limit.get().min(MAX_HINTED_WIDTH)) + .unwrap_or(MAX_HINTED_WIDTH); + let width = width.min(cap); + // `Width` is already in physical pixels — that is what the client hint // means — while `dpr` is multiplied back onto the resize target later in // the pipeline. Taking the hint at face value therefore applied the ratio @@ -441,6 +453,36 @@ mod tests { ); assert_eq!(options.dpr, Some(1.0), "an explicit dpr:1 refuses the hint"); + // The hint is attacker-adjacent input on a signed URL — the signature + // covers the path, not the headers — so it is bounded before it can + // size the pipeline. + let mut options = ParsedOptions::default(); + apply_client_hints( + &mut options, + &RequestHints { + width: Some(1_000_000), + ..RequestHints::default() + }, + ); + assert_eq!(options.resize.unwrap().width, 16_384, "the hard cap bounds the hint"); + + let mut options = ParsedOptions { + max_result_dimension: Some("2000".parse().unwrap()), + ..ParsedOptions::default() + }; + apply_client_hints( + &mut options, + &RequestHints { + width: Some(1_000_000), + ..RequestHints::default() + }, + ); + assert_eq!( + options.resize.unwrap().width, + 2000, + "a configured result ceiling bounds it tighter still" + ); + // The same division applies when the hint fills a zero-width resize the // URL already named. let mut options = ParsedOptions { diff --git a/src/service/mod.rs b/src/service/mod.rs index de87acf..beeacc9 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -237,7 +237,17 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> // A cache hit has no source response to draw on, so the origin's own // caching headers are not available; the configured policy still is, // and the entity tag comes from the bytes either way. - let headers = DeliveryHeaders::for_cache_hit(config, &SourceMetadata::default(), &cached_image.etag, &vary); + // The origin's delivery metadata was stored with the entry, so a hit + // keeps saying what the origin said — a passthrough `no-store` must + // not vanish the moment the cache starts answering. + let cached_source = SourceMetadata { + cache_control: (!cached_image.origin_cache_control.is_empty()) + .then(|| cached_image.origin_cache_control.clone()), + last_modified: (!cached_image.origin_last_modified.is_empty()) + .then(|| cached_image.origin_last_modified.clone()), + url: None, + }; + let headers = DeliveryHeaders::for_cache_hit(config, &cached_source, &cached_image.etag, &vary); // Enabling the debug headers should not make them appear and disappear // with the cache. A hit never made the source request, so nothing about @@ -454,6 +464,8 @@ pub async fn process_path(state: Arc, request: ProcessRequest<'_>) -> source_url: fetched_from.clone(), watermark_source_url: watermark_fetched_from.clone(), etag, + origin_cache_control: source_metadata.cache_control.clone().unwrap_or_default(), + origin_last_modified: source_metadata.last_modified.clone().unwrap_or_default(), }, ) { error!("Failed to cache image: {}", err); @@ -927,6 +939,8 @@ async fn serve_source_response( // A passthrough composites nothing. watermark_source_url: String::new(), etag: etag.clone(), + origin_cache_control: source_metadata.cache_control.clone().unwrap_or_default(), + origin_last_modified: source_metadata.last_modified.clone().unwrap_or_default(), }, ) { error!("Failed to cache raw image: {}", err); diff --git a/tests/handlers_integration_tests_extended.rs b/tests/handlers_integration_tests_extended.rs index 30ccb66..7772dec 100644 --- a/tests/handlers_integration_tests_extended.rs +++ b/tests/handlers_integration_tests_extended.rs @@ -884,6 +884,29 @@ async fn cache_hits_serve_the_stored_entity_tag() { assert_eq!(delivery_header(&hit, "etag").as_deref(), Some(miss_etag.as_str())); } +/// A passthrough `no-store` must survive the cache: the origin's policy is +/// stored with the entry, because losing it on the hit invited shared caches +/// to store exactly what the origin forbade. +#[tokio::test] +async fn passthrough_cache_control_survives_a_cache_hit() { + let server = MockServer::start().await; + let encoded = delivery_source(&server, &[("Cache-Control", "no-store")]).await; + let uri = format!("/unsafe/rs:fit:20:20/{encoded}"); + + let cache = ImgforgeCache::new(Some(CacheConfig::Memory { capacity: 1024 * 1024 })) + .await + .unwrap(); + let mut config = delivery_config(); + config.cache_control_passthrough = true; + + let miss = delivery_request_with_cache(config.clone(), cache.clone(), &uri, &[]).await; + assert_eq!(delivery_header(&miss, "cache-control").as_deref(), Some("no-store")); + + let hit = delivery_request_with_cache(config, cache, &uri, &[]).await; + assert_eq!(delivery_header(&hit, "cache-status").as_deref(), Some("HIT")); + assert_eq!(delivery_header(&hit, "cache-control").as_deref(), Some("no-store")); +} + /// A bearer-protected deployment must not mark responses shared-cacheable: /// `public` invites a CDN to replay the authorised answer to a request that /// carries no token. Neither the TTL default nor the origin's own policy gets From fba466237f6ce30eb0ff2bea02e6862f2ef7d3be Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 21:33:15 -0400 Subject: [PATCH 13/13] Skip AVIF negotiation expectations where libvips cannot encode it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Negotiation offers only what the build can encode, so the two tests that expect AVIF to win assumed an encoder the CI runner's libvips does not have — they have been failing there since they were written. The same guard the save tests use skips them where the offer cannot exist. Co-Authored-By: Claude Fable 5 --- src/negotiation.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/negotiation.rs b/src/negotiation.rs index 305efe0..4a7d136 100644 --- a/src/negotiation.rs +++ b/src/negotiation.rs @@ -284,6 +284,12 @@ mod tests { #[test] fn avif_is_preferred_when_the_client_takes_both() { init_vips(); + // Negotiation only offers what this libvips build can encode, so the + // AVIF expectations hold only where an AVIF encoder exists — the same + // guard the save tests use. + if !is_format_supported("avif") { + return; + } let config = config_with((true, true), (false, false)); let hints = accepting("image/avif,image/webp,*/*"); @@ -330,6 +336,11 @@ mod tests { #[test] fn the_clients_quality_weights_decide_between_two_offers() { init_vips(); + // The ranking under test needs both offers on the table, and AVIF is + // only offered where this libvips build can encode it. + if !is_format_supported("avif") { + return; + } let config = config_with((true, true), (false, false)); assert_eq!(