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
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid, well-shaped abstraction — expressing cache policy as typed data and rendering it per-runtime is the right call, and the multi-value Cache-Control handling (get_all + directive-name-exact matching, so no-storey / not-private don't false-match) is careful work.
Four blocking issues, though. The most important is that the rehosted-asset path will override an origin's explicit no-store, using a guard that this very commit wrote for the publisher path but didn't wire into the asset proxy. The other three are a missing immutable safety check, a fingerprint heuristic that can't match the two most common bundlers, and a docs/behavior mismatch on disabled rules that can hard-fail startup.
Findings below were verified by running the code or by an adversarial pass. Four other hypotheses I chased (a missing GET/HEAD gate on asset routes, an EC-cookie shared-cache leak on Fastly, "zero caching" from the hash gate, and a broad TSJS regression) all turned out to be false and are deliberately not reported.
Blocking
🔧 wrench
- Asset-proxy rehost overrides an origin
no-store/private—proxy.rs:1173. Only the status is checked;publisher.rs:470guards this correctly for the same feature. The PR's own test (proxy.rs:3755) feeds an originno-storeand asserts it becomespublic, max-age=31536000, immutable. No downstream rescue: theSet-Cookiebackstop can't fire because the asset proxy stripsset-cookie. Normalizedre-publicizes after privacy hardening —proxy.rs:127. The same root cause at a second layer.apply_after_route_finalizationused to only ever make responses more private, so running it last was safe; the newNormalizedarm makes them more public and still runs last. This inverts the invariant in the plan doc (L56-57): hardening "runs after any new policy application". Both sites need the fix.immutable = trueaccepted with no fingerprint requirement —settings.rs:1983.requires_hash_in_filenamedefaults tofalse, sopath_prefix = "/assets/"+immutable = trueputs a non-revalidatable year-long policy on an unfingerprinted/assets/app.js. Contradicts the plan doc (L47-49): "immutable only for TS-fingerprinted rehosted URLs".- Hex-only fingerprint gate never matches Vite or esbuild —
settings.rs:2204, with the reachable trap atconfiguration.md:1041. Verified against real builds: Vite 8 emits/assets/index-DA15JTLU.js(base64url), esbuild/assets/app-VRTVD5R5.js(base32)./assets/is Vite's default output dir — exactly what theenabled = truedocs example globs. And it isn't a clean no-op: ~0.02% of Vite hashes are all-hex by chance, so the rule fires on ~1 in 5,000 assets, varying per build. - "Disabled rules are ignored" is false —
configuration.md:998.prepare_runtimevalidates every rule regardless ofenabled. Confirmed by execution: a disabled rule with a bad regex, or a disabled placeholder with no matcher, both fail startup — which per line 54 of the same page means the service returns its startup-error response. This path has no test, which CLAUDE.md's reviewer checklist explicitly asks for.
❓ question
- What consumes
edge_ttl_secondson Fastly today? —configuration.md:1014. Fastly's read-through cache stores the backend's response and decides TTL from the backend's headers atsend(); this PR rewrites headers on egress, after that decision. Caching a Wasm-synthesized response needs an explicit Core/Simple Cache call, and the repo has zero uses offastly::cache/CacheOverride/SimpleCache. Is there a service-layer piece outside the repo? To be fair: theSurrogate-Controlemission predates this PR, so it's not a regression here — but this PR is what turns it into a documented operator knob. - Is spec acceptance criterion #138 handled at the service layer? The design doc requires "Runtime cache-key configuration preserves the
vquery parameter for/static/tsjs=". This matters more now: the same path serves either a 1-year immutable response (matching?v=) or a 300s one (bare/mismatched), discriminated only by query string — and this PR makes the bare URL a real, emitted URL for the first time. If any shared cache normalizes the query away, those two cross-contaminate. There's no cache-key config infastly.toml/edgezero.toml, and the operator docs never mention the requirement. All 13 acceptance criteria in the shipped spec are still unchecked.
Non-blocking
🌱 seedling
- Cloudflare is ~2 lines from actually working. Cloudflare's Workers Cache (GA 2026-07-06) documents
cloudflare-cdn-cache-controlas its highest-precedence cache directive — exactly what this PR emits. But it's opt-in, and neitherwrangler.tomlnorwrangler.ci.tomlhas a[cache]block, so the header is inert today. Adding[cache]\nenabled = true(Wrangler ≥ 4.69.0) would turnEdgeCacheHeader::CloudflareCdnCacheControlfrom a no-op into a fully effective directive — plausibly the highest-ROI change available here. (compatibility_date = "2024-09-23"is also stale.) tsjs_unified_script_src()dropped?v=—tsjs.rs:30. Bounded ~6-minute post-deploy staleness on the ad-creative path only. Details inline; suggest a follow-up issue rather than expanding this PR.
📌 out of scope
-
The runtime half of this belongs in
edgezero, nottrusted-server-core. Worth a follow-up issue, not a change to this PR.EdgeCacheHeaderencodes a purely platform fact — which shared-cache header does this runtime speak. The tell is that all four adapters hand-thread a per-adapter constant (SurrogateControl/CloudflareCdnCacheControl/SMaxageFallback) intohandle_tsjs_dynamicandhandle_publisher_request. The adapter already knows its own runtime; it shouldn't have to tell core what platform it is. That plumbing is also what pushedhandle_publisher_requestto exactly 7 parameters, CLAUDE.md's stated ceiling.More importantly, the part that would make
edge_ttl_secondsactually work can only be built in edgezero.edgezero-corecurrently has no cache concept at all, andedgezero-adapter-fastly/src/proxy.rs:31sends upstream withsend_async_streaming(&backend_name)and noCacheOverride— so the store/TTL decision for proxied responses is made inside edgezero, before this PR's egress-time header rewrite ever runs. Trusted Server cannot fix that from where it sits.A split that seems right:
- edgezero —
EdgeCacheHeaderand the edge-header-name registry; theCachePolicy → platform headersrender step (ideally behind an adapter method likeapply_cache_policy(&policy, &mut resp), so the parameter disappears from core signatures entirely);CacheOverrideon backend sends; the Core/Simple Cache API for synthetic responses; the wrangler[cache]block. - trusted-server —
CachePolicyas typed domain data, thecache.asset_rulesconfig surface and matching, and theresponse_privacyinvariants (which would consume edgezero's header registry rather than own it).
None of this blocks the PR — the browser-facing half is real and useful today. But it does mean the edge half is currently a promise the runtime layer can't keep, which is worth being explicit about before operators configure
edge_ttl_secondsexpecting shared-cache behavior. - edgezero —
♻️ refactor
SURROGATE_CACHE_HEADERShas zero consumers —response_privacy.rs:21. Dead re-export, and now a misnomer since it includes the CDN headers. Delete it.
🤔 thinking
- No validation that a rule sets any TTL.
visibility = "public"with neitherbrowser_ttl_secondsnoredge_ttl_secondsrenders a bareCache-Control: public, which hands the response to heuristic freshness. Probably worth rejecting at config load. - The cache-rule path is completely silent. There isn't a single
log::statement insettings.rs:1890-2215or incache_policy.rs, and the application site is a bareif letwith noelse. A rule that matches nothing — the hash-gate case above, for instance — is undebuggable in production. Alog::debug!on gate rejection would pay for itself.
📝 note
#[validate(nested)]onSettings.cacheis a no-op.CacheSettingsdeclares no field validators andCacheAssetRuledoesn't deriveValidate, so the attribute does nothing today. Harmless, but it reads as protection that isn't there.- The PR description says asset cache policies are reapplied "in legacy and EdgeZero flows", but there's only one call site (
main.rs:206).
CI Status
All 19 checks green at a5eb7a3, verified via gh pr checks:
- fmt: PASS
- clippy (fastly / axum / cloudflare / cloudflare-wasm / spin-native / spin-wasm): PASS
- rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
- js tests (vitest) + format-typescript + format-docs: PASS
- integration + browser integration + CodeQL: PASS
|
@ChristianPavilonis Please resolve conflicts |
6048c26 to
e3c03af
Compare
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-Controlat the shared finalization point used by both 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-ups:
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!)