From 7d12f53314bfcebd389cd20a5ec12c22d742325e Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 4 Sep 2026 21:28:59 +0000 Subject: [PATCH] Make the origin's Cache-Control headers CDN-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An audit of the live site ahead of the Cloudflare move found three ways a cache-everything rule would have gone wrong. First, several routes named no policy at all — `/login`, `/issues.json`, `/files/epub/*`, `/files/xtc/*`, `/robots.txt`, the error pages — so the edge would have applied its own default TTL (Cloudflare: two hours on a 200). For `/files/*` that means an authenticated download becoming a publicly cached one. Those routes now say what they mean, and `security_headers` fails closed: a response that set no `Cache-Control` gets `no-store`, so a route added later cannot silently inherit the CDN's default. Everything cookie- or Basic-auth gated (`/files/*`, `/opds*`) is `private, no-store` on every response, 401s and 404s included. Second, the public pages said `max-age=300` alone. They now say `public, max-age=300, s-maxage=86400`: five minutes for the browser, a day for the edge, which is safe because publishing purges the edge. A request carrying a `daily_session=` cookie still gets `private, no-store`. Third, `/static/favicon.svg` and `/static/speculation.json` were referenced without `?v=` while being served `immutable` for a year — editing either one could never have reached a browser again. Both are now in the `ASSET_VERSION` hash and referenced with the version, and `static_asset` only promises a year when the URL actually carries `?v=`; a bare `/static/…` URL gets an hour and revalidates against the same ETag. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Va5eMEmWEnjMXBsBob5FDW --- src/server.rs | 43 ++++++- src/web/issue.rs | 18 ++- src/web/mod.rs | 236 ++++++++++++++++++++++++++++++---- src/web/public.rs | 27 +++- src/web/templates/layout.html | 2 +- 5 files changed, 289 insertions(+), 37 deletions(-) diff --git a/src/server.rs b/src/server.rs index e0f9d1e..715a8a4 100644 --- a/src/server.rs +++ b/src/server.rs @@ -13,7 +13,10 @@ //! | `GET /healthz` | liveness | //! | `GET /issues.json` | the last 30 run reports, newest first | //! -//! `/opds/*` and `/files/*` sit behind optional Basic auth (`server.basic_auth_*`). +//! `/opds/*` and `/files/*` sit behind optional Basic auth (`server.basic_auth_*`) +//! and are therefore `private, no-store` on every response, 401s and 404s +//! included: the site sits behind a CDN whose cache-everything rule would +//! otherwise turn an authenticated download into a public one (§3.12). //! //! The EPUB article footer (§3.10) mints its three verdict links with the very same //! [`rating_url`] this module verifies with — both re-export [`crate::auth`], @@ -44,6 +47,7 @@ use tower_http::trace::TraceLayer; use crate::config::Config; use crate::db::Db; use crate::types::{ArticleId, RatingEvent, Vote}; +use crate::web::public::{PRIVATE_CACHE, PUBLIC_CACHE}; /// Characters of the hex HMAC kept in rating links (§3.9). pub const TOKEN_LEN: usize = crate::auth::TOKEN_LEN; @@ -300,9 +304,13 @@ async fn handle_issues_json(State(state): State) -> Response { }) .collect(); match serde_json::to_string_pretty(&issues) { + // Only publishing changes this, and publishing purges the edge (§3.12). Ok(body) => ( StatusCode::OK, - [(header::CONTENT_TYPE, "application/json")], + [ + (header::CONTENT_TYPE, "application/json"), + (header::CACHE_CONTROL, PUBLIC_CACHE), + ], body, ) .into_response(), @@ -406,7 +414,7 @@ async fn handle_opds(State(state): State, headers: HeaderMap) -> Respo StatusCode::OK, [ (header::CONTENT_TYPE, OPDS_CONTENT_TYPE), - (header::CACHE_CONTROL, "no-cache"), + (header::CACHE_CONTROL, PRIVATE_CACHE), ], feed, ) @@ -415,6 +423,7 @@ async fn handle_opds(State(state): State, headers: HeaderMap) -> Respo tracing::error!(error = %e, "could not build the OPDS feed"); ( StatusCode::INTERNAL_SERVER_ERROR, + [(header::CACHE_CONTROL, PRIVATE_CACHE)], "could not build the feed", ) .into_response() @@ -465,7 +474,12 @@ async fn serve_file( } let Some(path) = safe_join(dir, name) else { tracing::warn!(name, "rejected an unsafe file name"); - return (StatusCode::BAD_REQUEST, "bad file name").into_response(); + return ( + StatusCode::BAD_REQUEST, + [(header::CACHE_CONTROL, PRIVATE_CACHE)], + "bad file name", + ) + .into_response(); }; // An XTCH issue is a pre-rendered page bitmap per page — ~100 MB for a full // day. Stream it rather than buffering the whole file per request (§3.11). @@ -476,7 +490,12 @@ async fn serve_file( } Err(e) => { tracing::warn!(error = %e, path = %path.display(), "file not found"); - return (StatusCode::NOT_FOUND, "not found").into_response(); + return ( + StatusCode::NOT_FOUND, + [(header::CACHE_CONTROL, PRIVATE_CACHE)], + "not found", + ) + .into_response(); } }; // CrossPoint dispatches on the saved file's extension, not on this header, @@ -488,6 +507,12 @@ async fn serve_file( }; let mut headers = HeaderMap::new(); headers.insert(header::CONTENT_TYPE, HeaderValue::from_static(content_type)); + // Downloads are cookie- or Basic-auth gated: a shared cache must never hold + // one, whatever cache rule the CDN in front of us is configured with (§3.12). + headers.insert( + header::CACHE_CONTROL, + HeaderValue::from_static(PRIVATE_CACHE), + ); if let Ok(value) = HeaderValue::from_str(&format!( "attachment; filename=\"{}\"", name.replace('"', "") @@ -563,7 +588,13 @@ fn check_basic_auth(config: &Config, headers: &HeaderMap) -> Option { Some( ( StatusCode::UNAUTHORIZED, - [(header::WWW_AUTHENTICATE, challenge)], + [ + (header::WWW_AUTHENTICATE, challenge), + ( + header::CACHE_CONTROL, + HeaderValue::from_static(PRIVATE_CACHE), + ), + ], "authentication required", ) .into_response(), diff --git a/src/web/issue.rs b/src/web/issue.rs index fc5c219..e37464e 100644 --- a/src/web/issue.rs +++ b/src/web/issue.rs @@ -1671,7 +1671,7 @@ mod tests { assert_eq!(issue.status(), StatusCode::OK); assert_eq!( issue.headers().get(header::CACHE_CONTROL).unwrap(), - "public, max-age=300" + "public, max-age=300, s-maxage=86400" ); let html = String::from_utf8( to_bytes(issue.into_body(), 1024 * 1024) @@ -1703,6 +1703,10 @@ mod tests { .await .unwrap(); assert_eq!(archive.status(), StatusCode::OK); + assert_eq!( + archive.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=300, s-maxage=86400" + ); let feed = app .clone() @@ -1718,6 +1722,10 @@ mod tests { feed.headers().get(header::CONTENT_TYPE).unwrap(), "application/atom+xml; charset=utf-8" ); + assert_eq!( + feed.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=300, s-maxage=86400" + ); let feed = String::from_utf8( to_bytes(feed.into_body(), 1024 * 1024) .await @@ -1750,6 +1758,10 @@ mod tests { ) .await .unwrap(); + assert_eq!( + robots.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=86400" + ); let robots = String::from_utf8(to_bytes(robots.into_body(), 4096).await.unwrap().to_vec()).unwrap(); assert!(robots.contains("Disallow: /dashboard")); @@ -1763,6 +1775,10 @@ mod tests { ) .await .unwrap(); + assert_eq!( + reports.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=300, s-maxage=86400" + ); let reports = String::from_utf8(to_bytes(reports.into_body(), 4096).await.unwrap().to_vec()).unwrap(); assert!(reports.contains("\"status\": \"ok\"")); diff --git a/src/web/mod.rs b/src/web/mod.rs index bb7337d..8eb9fa7 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -214,13 +214,13 @@ pub struct Page { pub flash: Option, pub active_nav: String, pub version: &'static str, - /// Cache-busting token for `/static/*.css|js` URLs: a content hash, so - /// any stylesheet or script change reaches browsers that cached the - /// previous build (they are served with an immutable one-year `max-age`). + /// Cache-busting token for every `/static/*` URL the site references: a + /// content hash, so any asset change reaches browsers and edge caches that + /// hold the previous build (a `?v=`-carrying `/static/*` URL is served with + /// an immutable one-year `max-age`). pub asset_version: &'static str, } -/// First 12 hex digits of the SHA-256 over the embedded CSS and JS assets. const NEWSREADER: &[u8] = include_bytes!("static/fonts/Newsreader.woff2"); const NEWSREADER_ITALIC: &[u8] = include_bytes!("static/fonts/Newsreader-italic.woff2"); @@ -235,12 +235,12 @@ const NEWSREADER_ITALIC: &[u8] = include_bytes!("static/fonts/Newsreader-italic. /// ~305 KB and pushed first paint out by more than a second on mobile. /// /// So the URLs stay URLs, carrying `?v=` so the immutable -/// one-year `max-age` on `/static/*` is safe across deploys. The fonts are part -/// of the `ASSET_VERSION` hash, so a new face mints a new URL. Neither face is -/// preloaded — that only takes bandwidth from this sheet, which is what first -/// paint actually waits on. Instead both use `font-display: swap` behind -/// metric-matched local fallbacks, so first paint is immediate and the swap -/// shifts nothing. +/// one-year `max-age` on a versioned `/static/*` URL is safe across deploys. +/// The fonts are part of the `ASSET_VERSION` hash, so a new face mints a new +/// URL. Neither face is preloaded — that only takes bandwidth from this sheet, +/// which is what first paint actually waits on. Instead both use +/// `font-display: swap` behind metric-matched local fallbacks, so first paint +/// is immediate and the swap shifts nothing. pub static APP_CSS: LazyLock = LazyLock::new(|| { let version = ASSET_VERSION.as_str(); include_str!("static/app.css") @@ -254,6 +254,14 @@ pub static APP_CSS: LazyLock = LazyLock::new(|| { ) }); +/// First 12 hex digits of the SHA-256 over *every* embedded static asset. +/// +/// Every file listed here must also be referenced with `?v={ASSET_VERSION}`, +/// and every file referenced with `?v=` must be hashed here — the two halves +/// are what make the immutable one-year `max-age` safe. The favicon and the +/// speculation rules are in the hash for exactly that reason: they used to be +/// referenced by bare URL while still being served `immutable`, so editing +/// either one could never have reached a browser (or the CDN) again. pub static ASSET_VERSION: LazyLock = LazyLock::new(|| { let mut hasher = Sha256::new(); hasher.update(include_str!("static/app.css")); @@ -261,9 +269,23 @@ pub static ASSET_VERSION: LazyLock = LazyLock::new(|| { hasher.update(NEWSREADER_ITALIC); hasher.update(include_str!("static/app.js")); hasher.update(include_str!("static/theme.js")); + hasher.update(include_str!("static/favicon.svg")); + hasher.update(include_str!("static/speculation.json")); hex::encode(hasher.finalize())[..12].to_string() }); +/// The `Speculation-Rules` header value: a versioned URL, so a change to +/// `speculation.json` is picked up rather than pinned behind the immutable +/// one-year `max-age` on `/static/*`. Built once, like [`APP_CSS`], because it +/// interpolates [`ASSET_VERSION`] and so cannot be a `from_static`. +static SPECULATION_RULES: LazyLock = LazyLock::new(|| { + HeaderValue::from_str(&format!( + "\"/static/speculation.json?v={}\"", + ASSET_VERSION.as_str() + )) + .expect("the asset version is hex, so the header value is valid") +}); + impl Page { pub fn new(title: impl Into, viewer: Option, active_nav: &str) -> Self { Self { @@ -478,6 +500,15 @@ pub async fn security_headers(request: Request, next: Next) -> Response { if path.starts_with("/dashboard") { headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store")); } + // Fail closed. A response that names no policy of its own — `/login`, + // `/account`, a redirect, an error page, or whatever route is added next — + // is uncacheable, because a CDN with a cache-everything rule would + // otherwise apply its own default TTL (Cloudflare: two hours on a 200) to + // a page that may well be personalised. Handlers that mean to be cached + // say so explicitly, and this never overrides them (§3.12). + if !headers.contains_key(header::CACHE_CONTROL) { + headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store")); + } if headers .get(header::CONTENT_TYPE) .and_then(|value| value.to_str().ok()) @@ -486,7 +517,7 @@ pub async fn security_headers(request: Request, next: Next) -> Response { headers.append(header::VARY, HeaderValue::from_static("Cookie")); headers.insert( header::HeaderName::from_static("speculation-rules"), - HeaderValue::from_static("\"/static/speculation.json\""), + SPECULATION_RULES.clone(), ); } response @@ -585,8 +616,24 @@ async fn map_forbidden(request: Request, next: Next) -> Response { } } +/// The `?v=` cache-buster on a `/static/*` URL, when the reference carries one. +#[derive(Debug, Deserialize)] +struct AssetQuery { + #[serde(default)] + v: Option, +} + +/// A versioned URL names one immutable build, so it may be held for a year. +const VERSIONED_CACHE: &str = "public, max-age=31536000, immutable"; + +/// A bare `/static/…` URL is not a promise about its content, so it gets an +/// hour and revalidates against the ETag. Someone else's link or an old +/// bookmark must not pin a stale asset for a year. +const UNVERSIONED_CACHE: &str = "public, max-age=3600"; + async fn static_asset( axum::extract::Path(file): axum::extract::Path, + axum::extract::Query(query): axum::extract::Query, headers: axum::http::HeaderMap, ) -> Response { let asset: (&str, &'static [u8]) = match file.as_str() { @@ -611,6 +658,11 @@ async fn static_asset( "Newsreader-italic.woff2" => ("font/woff2", NEWSREADER_ITALIC), _ => return WebError::NotFound.into_response(), }; + let cache_control = if query.v.is_some() { + VERSIONED_CACHE + } else { + UNVERSIONED_CACHE + }; let etag = format!("\"{}\"", hex::encode(Sha256::digest(asset.1))); if headers .get(header::IF_NONE_MATCH) @@ -621,10 +673,7 @@ async fn static_asset( StatusCode::NOT_MODIFIED, [ (header::ETAG, etag), - ( - header::CACHE_CONTROL, - "public, max-age=31536000, immutable".into(), - ), + (header::CACHE_CONTROL, cache_control.to_string()), ], ) .into_response(); @@ -633,10 +682,7 @@ async fn static_asset( StatusCode::OK, [ (header::CONTENT_TYPE, asset.0.to_string()), - ( - header::CACHE_CONTROL, - "public, max-age=31536000, immutable".into(), - ), + (header::CACHE_CONTROL, cache_control.to_string()), (header::ETAG, etag), ], asset.1, @@ -725,11 +771,11 @@ mod tests { assert!(anonymous.headers().get(header::SET_COOKIE).is_none()); assert_eq!( anonymous.headers().get(header::CACHE_CONTROL).unwrap(), - "public, max-age=300" + "public, max-age=300, s-maxage=86400" ); assert_eq!( anonymous.headers().get("speculation-rules").unwrap(), - "\"/static/speculation.json\"" + &format!("\"/static/speculation.json?v={}\"", ASSET_VERSION.as_str()) ); let response = app @@ -1207,11 +1253,12 @@ mod tests { async fn static_assets_use_content_hash_etags() { let (_dir, state) = test_state(Config::default()).await; let app = router(state); + let versioned = format!("/static/app.css?v={}", ASSET_VERSION.as_str()); let first = app .clone() .oneshot( Request::builder() - .uri("/static/app.css") + .uri(versioned.as_str()) .body(Body::empty()) .unwrap(), ) @@ -1227,8 +1274,8 @@ mod tests { .clone() .oneshot( Request::builder() - .uri("/static/app.css") - .header(header::IF_NONE_MATCH, etag) + .uri(versioned.as_str()) + .header(header::IF_NONE_MATCH, etag.clone()) .body(Body::empty()) .unwrap(), ) @@ -1240,10 +1287,48 @@ mod tests { "public, max-age=31536000, immutable" ); + // Without `?v=` the URL is not a promise about its content: an hour, + // revalidated against the same ETag, never a pinned year. + let bare = app + .clone() + .oneshot( + Request::builder() + .uri("/static/app.css") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(bare.status(), StatusCode::OK); + assert_eq!( + bare.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=3600" + ); + assert_eq!(bare.headers().get(header::ETAG).unwrap(), &etag); + let bare_cached = app + .clone() + .oneshot( + Request::builder() + .uri("/static/app.css") + .header(header::IF_NONE_MATCH, etag) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(bare_cached.status(), StatusCode::NOT_MODIFIED); + assert_eq!( + bare_cached.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=3600" + ); + let rules = app .oneshot( Request::builder() - .uri("/static/speculation.json") + .uri(format!( + "/static/speculation.json?v={}", + ASSET_VERSION.as_str() + )) .body(Body::empty()) .unwrap(), ) @@ -1254,6 +1339,107 @@ mod tests { rules.headers().get(header::CONTENT_TYPE).unwrap(), "application/speculationrules+json" ); + assert_eq!( + rules.headers().get(header::CACHE_CONTROL).unwrap(), + "public, max-age=31536000, immutable" + ); + } + + /// Everything the layout and the headers point at must carry `?v=`: an + /// unversioned reference to an immutably cached asset can never be updated. + #[tokio::test] + async fn every_referenced_static_url_is_versioned() { + let (_dir, state) = test_state(Config::default()).await; + let app = router(state); + let response = app + .oneshot(Request::builder().uri("/").body(Body::empty()).unwrap()) + .await + .unwrap(); + let version = ASSET_VERSION.as_str(); + assert_eq!( + response.headers().get("speculation-rules").unwrap(), + &format!("\"/static/speculation.json?v={version}\"") + ); + let html = response_text(response).await; + for asset in ["favicon.svg", "app.css", "theme.js"] { + assert!( + html.contains(&format!("/static/{asset}?v={version}")), + "{asset} is referenced without ?v= in {html}" + ); + assert!( + !html.contains(&format!("\"/static/{asset}\"")), + "{asset} still has a bare reference in {html}" + ); + } + // The fonts are reached through the stylesheet, not the layout. + assert!(APP_CSS.contains(&format!("url(/static/Newsreader.woff2?v={version})"))); + } + + /// A response that names no policy of its own must not be cacheable: a CDN + /// cache-everything rule would otherwise give it the CDN's default TTL. + #[tokio::test] + async fn responses_without_a_policy_default_to_no_store() { + let (_dir, state) = test_state(Config::default()).await; + let app = router(state); + for uri in ["/login", "/healthz", "/no-such-page"] { + let response = app + .clone() + .oneshot(Request::builder().uri(uri).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!( + response.headers().get(header::CACHE_CONTROL).unwrap(), + "no-store", + "{uri}" + ); + } + } + + /// Cookie- and Basic-auth-gated routes must be uncacheable everywhere, on + /// every status code, whatever the CDN is told to do. + #[tokio::test] + async fn download_and_opds_routes_are_private_no_store() { + let mut config = Config::default(); + config.publish.epub_dir = std::path::PathBuf::from("/nonexistent/epub"); + config.publish.xtc_dir = std::path::PathBuf::from("/nonexistent/xtc"); + let (_dir, state) = test_state(config).await; + let app = router(state); + for uri in [ + "/opds", + "/opds/", + crate::publish::OPDS_PATH, + "/files/epub/missing.epub", + "/files/xtc/missing.xtch", + "/files/epub/..%2Fescape", + ] { + let response = app + .clone() + .oneshot(Request::builder().uri(uri).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!( + response.headers().get(header::CACHE_CONTROL).unwrap(), + "private, no-store", + "{uri} answered {}", + response.status() + ); + } + + // The Basic auth challenge is a response too. + let mut config = Config::default(); + config.server.basic_auth_user = Some("daily".into()); + config.server.basic_auth_pass = Some("hunter2".into()); + let (_dir, state) = test_state(config).await; + let app = router(state); + let challenge = app + .oneshot(Request::builder().uri("/opds").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(challenge.status(), StatusCode::UNAUTHORIZED); + assert_eq!( + challenge.headers().get(header::CACHE_CONTROL).unwrap(), + "private, no-store" + ); } #[tokio::test] diff --git a/src/web/public.rs b/src/web/public.rs index d4a1a2e..bb7cdf9 100644 --- a/src/web/public.rs +++ b/src/web/public.rs @@ -339,7 +339,7 @@ pub async fn feed(State(state): State) -> Result { StatusCode::OK, [ (header::CONTENT_TYPE, "application/atom+xml; charset=utf-8"), - (header::CACHE_CONTROL, "public, max-age=300"), + (header::CACHE_CONTROL, PUBLIC_CACHE), ], body, ) @@ -349,21 +349,40 @@ pub async fn feed(State(state): State) -> Result { pub async fn robots() -> Response { ( StatusCode::OK, - [(header::CONTENT_TYPE, "text/plain; charset=utf-8")], + [ + (header::CONTENT_TYPE, "text/plain; charset=utf-8"), + (header::CACHE_CONTROL, "public, max-age=86400"), + ], "User-agent: *\nAllow: /\nAllow: /issues\nDisallow: /dashboard\nDisallow: /login\nDisallow: /files\nDisallow: /r\nDisallow: /opds\n", ) .into_response() } +/// `Cache-Control` for an anonymous public page (§3.12). +/// +/// The two ages are aimed at two different caches. `max-age=300` is the +/// browser's: a reader who leaves a tab open revalidates within five minutes of +/// a new issue landing. `s-maxage=86400` is the CDN's: an issue changes once a +/// day, so the edge should be allowed to answer for a day rather than asking +/// the origin every five minutes. That long edge life is only safe because +/// [`crate::cdn::purge_all`] runs right after a publish; without the purge the +/// edge would keep yesterday's paper for its whole day. +pub const PUBLIC_CACHE: &str = "public, max-age=300, s-maxage=86400"; + +/// `Cache-Control` for anything a signed-in reader sees, and for every +/// authenticated download. `private` keeps it out of shared caches even if a +/// CDN cache rule is misconfigured; `no-store` keeps it off disk. +pub const PRIVATE_CACHE: &str = "private, no-store"; + fn public_cache(mut response: Response, request_headers: &HeaderMap) -> Response { let value = if request_headers .get(header::COOKIE) .and_then(|value| value.to_str().ok()) .is_some_and(|cookies| cookies.contains("daily_session=")) { - "private, no-store" + PRIVATE_CACHE } else { - "public, max-age=300" + PUBLIC_CACHE }; response.headers_mut().insert( header::CACHE_CONTROL, diff --git a/src/web/templates/layout.html b/src/web/templates/layout.html index 3ccfdd2..ce9ac1d 100644 --- a/src/web/templates/layout.html +++ b/src/web/templates/layout.html @@ -18,7 +18,7 @@ flush (Bugzilla 1459305). Chrome blocks rendering on the sheet regardless. #} - + Skip to content