From 638fa6dc7e7866637ecfc4f477ba92eff8a7c2c8 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Mon, 26 Oct 2026 22:32:36 +0800 Subject: [PATCH] fix(fetch): gate HTTP cache on effective literal cookies Share literal Cookie selection between header emission and cache eligibility. Unbound data, out-of-scope hosts and credentials-suppressed literals no longer disable the disk cache. Requests that actually send cookies still bypass lookup and storage, preserving any earlier public entry. Cover raw and HTML streams with fresh network runtimes and both request-level and configured Cookie data, including credentials omit and cross-origin same-origin requests. --- moli-fetch/src/blocking/cache/key.rs | 4 +- moli-fetch/src/blocking/mod.rs | 28 ++- .../src/tests/cache_request_controls.rs | 177 ++++++++++++++++++ 3 files changed, 198 insertions(+), 11 deletions(-) diff --git a/moli-fetch/src/blocking/cache/key.rs b/moli-fetch/src/blocking/cache/key.rs index 68b7428b2b..7f4a61d8f5 100644 --- a/moli-fetch/src/blocking/cache/key.rs +++ b/moli-fetch/src/blocking/cache/key.rs @@ -6,6 +6,7 @@ use url::Url; use crate::{FetchConfig, Request}; +use super::super::literal_cookie_header_for_request; use super::vary::request_headers_allow_http_cache; pub(super) fn cache_store_and_key_for_request( @@ -23,8 +24,7 @@ pub(super) fn cache_store_and_key_for_request( || !config.default_request_headers().is_empty() || !subresource_validation_allows_http_cache(request) || cookie_header.is_some() - || request.additional_cookies().is_some() - || config.additional_cookies().is_some() + || literal_cookie_header_for_request(config, request, request_url).is_some() || !request_headers_allow_http_cache(&request.request_headers.to_byte_strings()) || !matches!(request_url.scheme(), "http" | "https") || !request_url.username().is_empty() diff --git a/moli-fetch/src/blocking/mod.rs b/moli-fetch/src/blocking/mod.rs index a1e49f467d..65fea24f50 100644 --- a/moli-fetch/src/blocking/mod.rs +++ b/moli-fetch/src/blocking/mod.rs @@ -171,15 +171,14 @@ pub(crate) fn outgoing_request_headers_for_url( .to_byte_strings() } -fn outgoing_request_header_bytes_for_url( - config: &FetchConfig, - request: &Request, +// Header emission and cache eligibility must select the same literal data, +// including host scope, credentials, empty values and explicit-header overrides. +fn literal_cookie_header_for_request<'a>( + config: &'a FetchConfig, + request: &'a Request, request_url: &Url, - cookie_header: Option<&str>, -) -> crate::RequestHeaders { - let mut outgoing = crate::RequestHeaders::default(); - - let additional = request +) -> Option<&'a str> { + request .additional_cookies() .or_else(|| config.additional_cookies_for_request(request, request_url)) .filter(|value| !value.is_empty()) @@ -193,7 +192,18 @@ fn outgoing_request_header_bytes_for_url( .default_request_headers() .iter() .any(|(name, _)| name.eq_ignore_ascii_case("cookie")) - }); + }) +} + +fn outgoing_request_header_bytes_for_url( + config: &FetchConfig, + request: &Request, + request_url: &Url, + cookie_header: Option<&str>, +) -> crate::RequestHeaders { + let mut outgoing = crate::RequestHeaders::default(); + + let additional = literal_cookie_header_for_request(config, request, request_url); let cookie_header = match (cookie_header, additional) { (Some(stored), Some(literal)) => { Some(std::borrow::Cow::Owned(format!("{stored}; {literal}"))) diff --git a/moli-fetch/src/tests/cache_request_controls.rs b/moli-fetch/src/tests/cache_request_controls.rs index 0be78b7cb4..de555053a8 100644 --- a/moli-fetch/src/tests/cache_request_controls.rs +++ b/moli-fetch/src/tests/cache_request_controls.rs @@ -1,5 +1,182 @@ use super::*; +#[derive(Clone, Copy, Debug)] +enum CacheTransport { + Html, + Raw, +} + +impl CacheTransport { + async fn fetch(self, config: &FetchConfig, request: Request) -> Result { + // Reopen the client on every request to exercise the shared disk cache. + let client = FetchClient::new(config, new_shared_browser_cookie_store()); + let response = match self { + Self::Html => { + client + .fetch_html_stream(request) + .await? + .into_materialized_text_response() + .await + } + Self::Raw => client.fetch(request).await, + }; + assert!(client.shutdown().is_clean()); + response + } +} + +fn cacheable_response(body: &str) -> ScriptedResponse { + ScriptedResponse::ok(body).with_header("Cache-Control", "max-age=600") +} + +#[tokio::test] +async fn literal_cookie_cache_ignores_unbound_and_out_of_scope_configuration() -> Result<()> { + for transport in [CacheTransport::Html, CacheTransport::Raw] { + for bound in [false, true] { + let cache_dir = unique_test_cache_dir(); + let server = ScriptedHttpServer::spawn(vec![cacheable_response("public"); 2]); + let mut config = FetchConfig::default(); + config.set_http_cache_dir(Some(cache_dir.display().to_string())); + config.set_additional_cookies(Some("session=literal".to_owned())); + if bound { + config.bind_additional_cookie_scope(&Request::get("https://app.test/start")?); + } + let request = Request::new("GET", &server.url(), None, Vec::new())? + .with_request_mode(RequestMode::NoCors) + .with_credentials_mode(RequestCredentialsMode::Include); + + for from_cache in [false, true] { + let response = transport.fetch(&config, request.clone()).await?; + assert_eq!( + response.body_text(), + "public", + "{transport:?}, bound={bound}" + ); + assert_eq!( + response.from_cache, from_cache, + "{transport:?}, bound={bound}" + ); + } + assert_eq!(server.hits(), 1, "{transport:?}, bound={bound}"); + assert!( + !server.requests()[0] + .to_ascii_lowercase() + .contains("\r\ncookie:") + ); + server.shutdown(); + fs::remove_dir_all(cache_dir)?; + } + } + Ok(()) +} + +#[tokio::test] +async fn literal_cookie_cache_obeys_browser_credentials() -> Result<()> { + for transport in [CacheTransport::Html, CacheTransport::Raw] { + for in_config in [false, true] { + for credentials in [ + RequestCredentialsMode::Omit, + RequestCredentialsMode::SameOrigin, + ] { + let cache_dir = unique_test_cache_dir(); + let server = ScriptedHttpServer::spawn(vec![cacheable_response("public"); 2]); + let mut config = FetchConfig::default(); + config.set_http_cache_dir(Some(cache_dir.display().to_string())); + let mut request = Request::new_browser( + "GET", + Url::parse(&server.url())?, + None, + Vec::new(), + moli_url::WebOrigin::from_url(&Url::parse("https://other.test/page")?), + ) + .with_request_mode(RequestMode::NoCors) + .with_credentials_mode(credentials); + if in_config { + config.set_additional_cookies(Some("session=literal".to_owned())); + config.bind_additional_cookie_scope(&Request::get(&server.url())?); + } else { + request = request.with_additional_cookies(Some("session=literal".to_owned())); + } + + for from_cache in [false, true] { + let response = transport.fetch(&config, request.clone()).await?; + assert_eq!(response.body_text(), "public"); + assert_eq!( + response.from_cache, from_cache, + "{transport:?}, in_config={in_config}, {credentials:?}" + ); + } + assert_eq!(server.hits(), 1); + assert!( + !server.requests()[0] + .to_ascii_lowercase() + .contains("\r\ncookie:") + ); + server.shutdown(); + fs::remove_dir_all(cache_dir)?; + } + } + } + Ok(()) +} + +#[tokio::test] +async fn literal_cookie_cache_keeps_credentialed_responses_private() -> Result<()> { + for transport in [CacheTransport::Html, CacheTransport::Raw] { + for in_config in [false, true] { + let cache_dir = unique_test_cache_dir(); + let server = ScriptedHttpServer::spawn(vec![ + cacheable_response("public"), + cacheable_response("private one"), + cacheable_response("private two"), + ]); + let mut config = FetchConfig::default(); + config.set_http_cache_dir(Some(cache_dir.display().to_string())); + let public = Request::new("GET", &server.url(), None, Vec::new())? + .with_request_mode(RequestMode::NoCors) + .with_credentials_mode(RequestCredentialsMode::Include); + let response = transport.fetch(&config, public.clone()).await?; + assert_eq!(response.body_text(), "public"); + assert!(!response.from_cache); + + let mut private = public.clone(); + if in_config { + config.set_additional_cookies(Some("session=literal".to_owned())); + config.bind_additional_cookie_scope(&Request::get(&server.url())?); + } else { + private = private.with_additional_cookies(Some("session=literal".to_owned())); + } + for body in ["private one", "private two"] { + let response = transport.fetch(&config, private.clone()).await?; + assert_eq!( + response.body_text(), + body, + "{transport:?}, in_config={in_config}" + ); + assert!(!response.from_cache); + } + + config.set_additional_cookies(None); + let response = transport.fetch(&config, public).await?; + assert_eq!(response.body_text(), "public"); + assert!(response.from_cache); + assert_eq!(server.hits(), 3); + for (index, request) in server.requests().iter().enumerate() { + assert_eq!( + request + .to_ascii_lowercase() + .contains("\r\ncookie: session=literal\r\n"), + index != 0, + "{transport:?}, in_config={in_config}: {request}" + ); + } + server.shutdown(); + fs::remove_dir_all(cache_dir)?; + } + } + Ok(()) +} + #[test] fn request_cache_control_headers_revalidate_fresh_cached_responses() { for (name, value) in [