Implement configurable cache header policies#860
Conversation
320b7b0 to
486bf16
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Configurable, safe-by-default cache-header policies across all four adapters. Cache policy is expressed once as typed data (CachePolicy / EdgeCacheHeader) and rendered per-runtime; hash-gated immutability, privacy stripping, and operator-controlled asset rules are all well-tested. No blocking issues — logic is sound and correctly platform-scoped. Findings below are all non-blocking.
Verified during review:
- Hash-gated immutability is safe —
serve_tsjs_staticmarks a responseimmutable(1yr) only when the request?v=equals the hash of the content actually being served, so a stale URL after a redeploy falls back to the short TTL rather than pinning old content. - Injection ↔ serving hash consistency — HTML injection (
html_processor.rs) and the serving path (publisher.rs) both derive the hash fromjs_module_ids_immediate()+concatenated_hash; deferred modules usesingle_module_hashon both sides. - Privacy hardening —
private/no-store/ cookie-bearing responses strip all four edge-cache headers, with the downgrade re-run after operator headers are applied. - Origin
no-storenot upgraded — a split laterCache-Controlfield carryingno-storecorrectly blocks the normalized upgrade. - Asset-proxy finalization is correctly Fastly-only —
handle_asset_proxy_request/apply_after_route_finalizationare wired only on Fastly, so there is no missing-reapplication gap on Cloudflare / Axum / Spin.
Non-blocking
🤔 thinking
- Hex-only fingerprint heuristic:
filename_contains_hashmisses base62/base36 bundler hashes (false negative → silently uncached) and can match coincidental hex stems (false positive → stale). (settings.rs) - Normalized policy overrides origin
no-cache/Vary: upgrade gate checks onlyprivate/no-store. (publisher.rs)
🌱 seedling
handle_publisher_requestis now at the 7-argument CLAUDE.md limit;EdgeCacheHeaderis threaded through several signatures — consider a request-context struct. (publisher.rs)
⛏ nitpick
SURROGATE_CACHE_HEADERSre-export is now a misnomer (contains CDN headers) with no in-tree consumers. (response_privacy.rs:21)
📝 note
#[validate(nested)]oncacheis a no-op; real validation lives inprepare_runtime. (settings.rs)
👍 praise
- Directive-exact
Cache-Controlmatching closes the old substring-match privacy hole. (cache_policy.rs)
CI Status
- fmt: PASS
- clippy (fastly / axum / cloudflare / cloudflare-wasm / spin-native / spin-wasm): PASS
- rust tests (fastly / axum / cloudflare / spin / CLI / parity): PASS
- js tests (vitest): PASS
- docs / typescript format: PASS
- CodeQL + integration/browser tests: PASS
| (!extension.is_empty()).then(|| extension.to_ascii_lowercase()) | ||
| } | ||
|
|
||
| fn filename_contains_hash(path: &str) -> bool { |
There was a problem hiding this comment.
🤔 thinking — The hash heuristic only accepts segments that are entirely ASCII hex (len >= 8 && all is_ascii_hexdigit). Two consequences worth documenting or tightening:
- False negative: many bundlers (Vite, esbuild, webpack
contenthash) emit base62/base36 fingerprints such asapp.a1B2c3D4.js. Those contain non-hex characters, sorequires_hash_in_filename = truesilently declines to cache them — the operator opts in expecting their fingerprinted assets to be cached long-lived, and nothing tells them why it didn't happen. - False positive: a mutable, non-fingerprinted file whose stem happens to be 8+ hex chars (e.g.
/assets/deadbeef.js) is treated as content-addressed and cachedimmutablefor a year → stale-content risk.
Suggest at minimum documenting the hex-only assumption next to the requires_hash_in_filename field, or broadening the match to alphanumeric segments of length ≥ 8 containing at least one digit (accepts base62 while still rejecting plain words).
| edge_header: EdgeCacheHeader, | ||
| response: &mut Response<EdgeBody>, | ||
| ) -> Result<(), Report<TrustedServerError>> { | ||
| if !cache_rule_method || response_cache_control_is_private_or_no_store(response) { |
There was a problem hiding this comment.
🤔 thinking — The upgrade gate only bails on private/no-store. An origin asset matching a configured rule that returns Cache-Control: no-cache or Vary: Cookie still gets rewritten to public, max-age=…, immutable, dropping the origin's revalidation / variant semantics. It's mitigated by the requires_hash_in_filename gate plus explicit operator opt-in, but a shared cache serving a Vary-dependent variant to the wrong user would be a real correctness bug for a misconfigured rule. Worth a one-line caveat in the cache docs that asset rules override origin no-cache/Vary.
| ec_context: &mut EcContext, | ||
| auction: AuctionDispatch<'_>, | ||
| mut req: Request<EdgeBody>, | ||
| edge_header: EdgeCacheHeader, |
There was a problem hiding this comment.
🌱 seedling — With edge_header, this handler is now at exactly 7 parameters — the CLAUDE.md ceiling. EdgeCacheHeader is also threaded through handle_tsjs_dynamic, serve_static_with_etag, apply_after_route_finalization, and every adapter call site. The next per-request knob forces a refactor anyway; consider grouping the edge-cache selection (and related request plumbing) into a small context struct now while the change is localized.
| /// cannot drift apart. | ||
| pub const SURROGATE_CACHE_HEADERS: &[&str] = &["surrogate-control", "fastly-surrogate-control"]; | ||
| /// Runtime edge-cache headers stripped from private or cookie-bearing responses. | ||
| pub use crate::cache_policy::EDGE_CACHE_HEADER_NAMES as SURROGATE_CACHE_HEADERS; |
There was a problem hiding this comment.
⛏ nitpick — This alias now re-exports EDGE_CACHE_HEADER_NAMES, which also contains cdn-cache-control / cloudflare-cdn-cache-control — so SURROGATE_CACHE_HEADERS is a misnomer, and grep shows it has no remaining in-tree consumers. Either drop the alias and use EDGE_CACHE_HEADER_NAMES directly, or rename it to reflect the broader edge-cache scope.
| pub consent: ConsentConfig, | ||
| #[serde(default)] | ||
| #[validate(nested)] | ||
| pub cache: CacheSettings, |
There was a problem hiding this comment.
📝 note — #[validate(nested)] here is effectively a no-op: CacheSettings declares no field validators and asset_rules elements aren't marked #[validate(nested)] (nor does CacheAssetRule derive Validate). The real validation lives in prepare_runtime. Not a bug — just flagging that the attribute reads as if validator enforces rule shape when it doesn't.
| /// Matching is directive-name exact and case-insensitive. Pseudo-directives such | ||
| /// as `not-private` or `no-storey` do not match `private` / `no-store`. | ||
| #[must_use] | ||
| pub fn cache_control_value_has_directive(value: &str, directive: &str) -> bool { |
There was a problem hiding this comment.
👍 praise — Directive-name-exact, case-insensitive matching (splitting on =/; so no-storey and not-private don't match) closes the substring-match hole the old .contains("private") check had. The paired tests and the get_all multi-value check make the privacy hardening genuinely robust.
Summary
s-maxagefallback headers.Changes
crates/trusted-server-core/src/cache_policy.rscrates/trusted-server-core/src/settings.rscache.asset_rulesconfig, matchers/presets, validation, runtime prep, and path-to-policy resolution.trusted-server.example.tomlcrates/trusted-server-core/src/http_util.rscrates/trusted-server-core/src/tsjs.rscrates/trusted-server-js/Cargo.tomlcrates/trusted-server-js/build.rscrates/trusted-server-js/src/bundle.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/response_privacy.rscrates/trusted-server-core/src/integrations/prebid.rsno-store, privateinstead of long-lived public cache.crates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/lib.rscache_policymodule.crates/trusted-server-adapter-axum/src/app.rss-maxagefallback edge header mode to TSJS and publisher handlers.crates/trusted-server-adapter-cloudflare/src/app.rscrates/trusted-server-adapter-fastly/src/app.rsSurrogate-Controlmode through EdgeZero fallback dispatch.crates/trusted-server-adapter-fastly/src/main.rsSurrogate-Controlin legacy and EdgeZero flows.crates/trusted-server-adapter-fastly/src/route_tests.rscrates/trusted-server-adapter-spin/src/app.rss-maxagefallback edge header mode to TSJS and publisher handlers.docs/superpowers/specs/2026-07-06-cache-control-header-design.mddocs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.mdCloses
Closes #293
Follow-up: #859 tracks deferred dynamic response/template caching.
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test-cloudflare && cargo test-spincargo clippy-cloudflare && cargo clippy-spin-native && cargo clippy-spin-wasmcd crates/trusted-server-js/lib && node build-all.mjsgit diff --checknpx prettier --check docs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.md docs/superpowers/specs/2026-07-06-cache-control-header-design.mdNote:
cd docs && npm run formatfailed because docs-local Prettier was not installed; touched docs were checked withnpx prettierinstead.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)