From 496bc5c8482e846742e271949f70789f4cea466c Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 18 Oct 2026 07:09:19 +0800 Subject: [PATCH] fix(fetch): revalidate disk cache for request control headers --- moli-fetch/src/blocking/cache/vary.rs | 14 ++- .../src/tests/cache_request_controls.rs | 91 +++++++++++++++++++ moli-fetch/src/tests/mod.rs | 1 + moli-http-cache/src/lib.rs | 5 +- moli-http-cache/src/policy.rs | 36 ++++++++ 5 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 moli-fetch/src/tests/cache_request_controls.rs diff --git a/moli-fetch/src/blocking/cache/vary.rs b/moli-fetch/src/blocking/cache/vary.rs index da643af9a9..0940a6364f 100644 --- a/moli-fetch/src/blocking/cache/vary.rs +++ b/moli-fetch/src/blocking/cache/vary.rs @@ -1,6 +1,9 @@ use std::str; -use moli_http_cache::{HttpCacheVaryHeader, response_vary_header_names}; +use moli_http_cache::{ + HttpCacheVaryHeader, request_cache_control_forbids_storage, request_header_requires_validation, + response_vary_header_names, +}; use url::Url; use crate::{FetchConfig, Request}; @@ -62,8 +65,13 @@ pub(super) fn vary_headers_match( } pub(super) fn request_headers_allow_http_cache(headers: &[(String, String)]) -> bool { - headers.iter().enumerate().all(|(index, (name, _))| { - supported_cache_request_header(name) + headers.iter().enumerate().all(|(index, (name, value))| { + // Revalidation controls select the existing conditional-cache path; + // they must not turn a fresh entry into an unconditional cache miss. + // Other unsupported controls keep the conservative bypass policy. + (supported_cache_request_header(name) || request_header_requires_validation(name, value)) + && !(name.eq_ignore_ascii_case("cache-control") + && request_cache_control_forbids_storage(value)) && !headers[..index] .iter() .any(|(previous, _)| previous.eq_ignore_ascii_case(name)) diff --git a/moli-fetch/src/tests/cache_request_controls.rs b/moli-fetch/src/tests/cache_request_controls.rs new file mode 100644 index 0000000000..0be78b7cb4 --- /dev/null +++ b/moli-fetch/src/tests/cache_request_controls.rs @@ -0,0 +1,91 @@ +use super::*; + +#[test] +fn request_cache_control_headers_revalidate_fresh_cached_responses() { + for (name, value) in [ + ("Cache-Control", "max-age=0"), + ("cache-control", "no-cache"), + ("Pragma", "no-cache"), + ] { + let cache_dir = unique_test_cache_dir(); + let server = ScriptedHttpServer::spawn(vec![ + ScriptedResponse::ok("cached body") + .with_header("Cache-Control", "max-age=60") + .with_header("ETag", "\"version-1\""), + ScriptedResponse::status(304, "Not Modified") + .with_header("Cache-Control", "max-age=600") + .with_header("ETag", "\"version-1\""), + ]); + let mut config = FetchConfig::default(); + config.set_http_cache_dir(Some(cache_dir.display().to_string())); + let first = + fetch_with_config_for_test(&config, Request::get(&server.url()).unwrap()).unwrap(); + assert_eq!(first.body_text(), "cached body"); + let cached = + fetch_with_config_for_test(&config, Request::get(&server.url()).unwrap()).unwrap(); + assert!(cached.from_cache); + assert_eq!(server.hits(), 1); + + let mut request = Request::get(&server.url()).unwrap(); + request.request_headers = vec![(name.to_owned(), value.to_owned())].into(); + let validated = fetch_with_config_for_test(&config, request).unwrap(); + assert_eq!( + validated.status, 200, + "{name}: {value} must merge the cached 200 response with its 304 validation" + ); + assert_eq!(validated.body_text(), "cached body"); + assert!(validated.from_cache); + let requests = server.requests(); + assert_eq!(requests.len(), 2); + assert!( + requests[1] + .to_ascii_lowercase() + .contains("if-none-match: \"version-1\""), + "cached validators missing from {}", + requests[1] + ); + assert!(requests[1].to_ascii_lowercase().contains(&format!( + "{}: {}", + name.to_ascii_lowercase(), + value + ))); + + let cached = + fetch_with_config_for_test(&config, Request::get(&server.url()).unwrap()).unwrap(); + assert_eq!(cached.body_text(), "cached body"); + assert!(cached.from_cache); + assert_eq!(server.hits(), 2); + server.shutdown(); + fs::remove_dir_all(cache_dir).unwrap(); + } +} + +#[test] +fn request_no_store_does_not_reuse_or_replace_an_existing_cached_response() { + let cache_dir = unique_test_cache_dir(); + let server = ScriptedHttpServer::spawn(vec![ + ScriptedResponse::ok("original").with_header("Cache-Control", "max-age=600"), + ScriptedResponse::ok("private one").with_header("Cache-Control", "max-age=600"), + ScriptedResponse::ok("private two").with_header("Cache-Control", "max-age=600"), + ]); + let mut config = FetchConfig::default(); + config.set_http_cache_dir(Some(cache_dir.display().to_string())); + let original = + fetch_with_config_for_test(&config, Request::get(&server.url()).unwrap()).unwrap(); + assert_eq!(original.body_text(), "original"); + for body in ["private one", "private two"] { + let mut request = Request::get(&server.url()).unwrap(); + request.request_headers = + vec![("Cache-Control".to_owned(), "max-age=0, No-Store".to_owned())].into(); + let response = fetch_with_config_for_test(&config, request).unwrap(); + assert_eq!(response.body_text(), body); + assert!(!response.from_cache); + let cached = + fetch_with_config_for_test(&config, Request::get(&server.url()).unwrap()).unwrap(); + assert_eq!(cached.body_text(), "original"); + assert!(cached.from_cache); + } + assert_eq!(server.hits(), 3); + server.shutdown(); + fs::remove_dir_all(cache_dir).unwrap(); +} diff --git a/moli-fetch/src/tests/mod.rs b/moli-fetch/src/tests/mod.rs index a8cb678d1f..bdff2b222f 100644 --- a/moli-fetch/src/tests/mod.rs +++ b/moli-fetch/src/tests/mod.rs @@ -1,3 +1,4 @@ +mod cache_request_controls; mod cookie_context; mod cookie_store; mod header_bytes; diff --git a/moli-http-cache/src/lib.rs b/moli-http-cache/src/lib.rs index 99640fad16..678603b2c7 100644 --- a/moli-http-cache/src/lib.rs +++ b/moli-http-cache/src/lib.rs @@ -26,8 +26,9 @@ pub use self::{ policy::{ HttpCacheResponsePolicy, cache_expires_at_unix_ms, cacheable_response_parts_policy, cached_response_is_fresh_immutable, cached_response_is_stale, - request_cache_control_requires_validation, request_header_requires_validation, - request_pragma_requires_validation, response_cache_policy, unix_now_ms, + request_cache_control_forbids_storage, request_cache_control_requires_validation, + request_header_requires_validation, request_pragma_requires_validation, + response_cache_policy, unix_now_ms, }, revalidation::{merge_not_modified_headers, validation_headers_from_headers}, store::{HttpCacheEntryInfo, HttpCacheStats, HttpCacheStore, HttpCachedEntryReader}, diff --git a/moli-http-cache/src/policy.rs b/moli-http-cache/src/policy.rs index dd1afb240d..e759ef7aef 100644 --- a/moli-http-cache/src/policy.rs +++ b/moli-http-cache/src/policy.rs @@ -170,6 +170,21 @@ pub fn request_header_requires_validation(name: &str, value: &str) -> bool { } } +/// Request `no-store` cannot be overridden by a revalidation directive in the +/// same field. Quoted extension arguments are not independent directives. +pub fn request_cache_control_forbids_storage(value: &str) -> bool { + split_outside_quoted_strings(value, ',') + .into_iter() + .map(str::trim) + .any(|directive| { + directive + .split_once('=') + .map(|(name, _)| name.trim()) + .unwrap_or(directive) + .eq_ignore_ascii_case("no-store") + }) +} + pub fn request_cache_control_requires_validation(value: &str) -> bool { split_outside_quoted_strings(value, ',') .into_iter() @@ -222,6 +237,27 @@ pub fn unix_now_ms() -> u64 { mod tests { use super::*; + #[test] + fn request_no_store_is_not_hidden_by_validation_or_forged_by_quoted_extensions() { + for value in [ + "no-store", + "NO-STORE", + "max-age=0, no-store", + "no-store, no-cache", + ] { + assert!(request_cache_control_forbids_storage(value), "{value}"); + } + for value in [ + "", + "max-age=0", + "no-cache", + "other=\"no-store\"", + "other=\"x, no-store, y\"", + ] { + assert!(!request_cache_control_forbids_storage(value), "{value}"); + } + } + #[test] fn freshness_requires_expiration_to_be_strictly_after_now() { let immutable_headers: Vec<(String, Vec)> = vec![(