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
3a5a9ae to
f2382c1
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested slice: the typed policy renderer, the per-runtime edge-header mapping, and the exact-directive Cache-Control parsing all improve on what they replace, and the privacy hardening closes a real gap (a late private directive coexisting with an authoritative edge header). Docs are unusually thorough for a config surface this large.
One blocking issue: requires_hash_in_filename is the only safety gate validate_policy_shape accepts for immutable = true, and the Vite/Base64URL branch of that gate accepts ordinary mixed-case filenames. With the documented /assets/**/*.png example rule enabled, a hand-named publisher image gets a one-year immutable policy that no republish can invalidate. Details inline.
Blocking
🔧 wrench
- Filename fingerprint gate accepts ordinary mixed-case filenames:
is_vite_base64urlmatches any 8-character alphanumeric suffix with at least one uppercase and one lowercase/digit character, sologo-DarkMode.svg,hero-Portrait.jpg,icon-Facebook.svg,banner-Summer24.pngandphoto-Iceland1.jpgall pass. Becauseimmutableimplies no revalidation, a mutable asset that trips this gate is stuck for a year. (crates/trusted-server-core/src/settings.rs:2265)
Non-blocking
🤔 thinking
- Neutralized Prebid shim now costs a request per page view:
public, max-age=31536000→no-store, privateon a 43-byte stub served from the publisher's (often blocking)prebid.jsURL. (crates/trusted-server-core/src/integrations/prebid.rs:682) - Rehost path overrides a third-party origin's
no-storewhile the publisher path vetoes on it — the asymmetry is deliberate and documented, but it leans entirely on the fingerprint gate flagged above. (crates/trusted-server-core/src/proxy.rs:1195) - Glob
*crosses/:glob::Pattern::matchesdefaults torequire_literal_separator: false, sopath_globs = ["/assets/*.js"]also matches/assets/a/b/c.js. Every doc example uses**, so the difference is invisible to operators. (docs/guide/configuration.md:1046,crates/trusted-server-core/src/settings.rs:2123)
♻️ refactor
cache_rule_method: bool: opaque boolean parameter, andis_getis computed one line earlier from the same method. (crates/trusted-server-core/src/publisher.rs:1083)
📝 note
- Hash-cache doc comment overstates the win: Fastly Compute builds a fresh Wasm instance per request, so the
Mutex<HashMap>never survives a page view there. The real improvement is hashing without allocating the concatenated body. (crates/trusted-server-js/src/bundle.rs:39)
🌱 seedling
EdgeCacheHeader::CdnCacheControlis unused in production: no adapter selects the standards-track variant; it stays dead until another runtime needs it.- Six planned PRs land as one: the plan doc sequences PR 1–6 and this change implements all of them (~3.1k insertions across 4 adapters, core, and the JS build). Nothing to do now, but bisecting a future cache regression inside this commit range will be painful.
👍 praise
?v=hash-match gate for immutability: immutable is granted only when the request'svequals the hash of the current module set, so a mid-rollout request for a new hash landing on an old instance falls back to the 300s policy instead of pinning wrong content under that key for a year. (crates/trusted-server-core/src/publisher.rs:339)- Exact directive matching: replacing
contains("private")kills thenot-private/no-storeyfalse-positive class, andget_allcovers splitCache-Controlfields —publisher_asset_cache_policy_respects_split_no_store_origin_headerlocks that in. (crates/trusted-server-core/src/cache_policy.rs:291) enforce_uncacheable_cache_privacy: closes the windowenforce_set_cookie_cache_privacyalone missed, where a lateprivate/no-storedirective coexists with an independently authoritative edge header. (crates/trusted-server-core/src/response_privacy.rs:29)
CI Status
All 19 check runs pass on head f2382c1:
- cargo fmt: PASS
- cargo test (fastly), cargo test (axum native), cargo test (spin native + wasm32-wasip1), cargo check (cloudflare native + wasm32-unknown-unknown), cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- vitest, format-typescript, format-docs: PASS
- integration tests, integration tests (Fastly EC lifecycle), browser integration tests: PASS
- CodeQL (rust, javascript-typescript, actions): PASS
Separately verified locally that [cache] enabled = true is a recognized Wrangler 4.83 config key — a control run with an invented section produces Unexpected fields found in top-level field, while the checked-in manifests parse clean.
f2382c1 to
412362b
Compare
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Note: this PR will need to be reconciled with the changes introduced by #1008 before merge. #1008 also changes publisher template delivery and cache behavior, including the new [creative_opportunities].enabled switch and the inactive-template max-age=60 policy, while this PR centralizes cache-policy application in publisher.rs and settings.rs. When combining the changes, please ensure the new switch and inactive-template behavior flow through the centralized policy renderer without overriding the privacy or CDN-specific headers established here.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The typed cache-policy work is generally well structured, but two enabled configuration paths either expose the whole gateway to shared caching or silently discard an operator-supplied TTL.
Blocking
🔧 wrench
- Cloudflare cache covers the entire gateway: Global cache.enabled lets ordinary dynamic publisher responses enter Workers Cache (crates/trusted-server-adapter-cloudflare/wrangler.toml:10).
- Private edge-only asset rules drop their only TTL: An enabled visibility = "private" rule can accept edge_ttl_seconds although the renderer never emits it (crates/trusted-server-core/src/settings.rs:2058).
CI Status
- GitHub fmt, Rust checks/tests, JS tests, CodeQL, and integration/browser checks: PASS
- git diff --check: PASS
412362b to
fd6d832
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The cache-header work itself is in good shape: the policy rendering, the operator-facing rule schema, and the privacy hardening all read cleanly, and both blocking findings from the previous round are genuinely closed — private rules now reject edge_ttl_seconds, and the global [cache] enabled = true block is gone from both Wrangler manifests with a cookie-boundary runtime regression behind it.
The blocker is unrelated to caching. origin/main is an ancestor of this branch, yet the diff removes 2,354 lines from publisher.rs, including production code from several already-merged PRs. Reviewed at fd6d8324 against origin/main at f6a2fb85.
Blocking
🔧 wrench
- Branch reverts merged
mainwork inpublisher.rs: 563 added / 2,354 deleted against a base that is literally the currentmaintip.hb_auction_iddiagnostics targeting (#974), thenon_empty/GAM_TARGETING_VALUE_MAX_LENhb_adid hardening (#996), thehas_renderercreative-rejection guard and PBS-cache fallback gating (#956), and realdelivered_winner_slotstelemetry (#997) are all gone; 22 tests were deleted with them, which is why CI is green. Details and the suggested merge strategy are inline atcrates/trusted-server-core/src/publisher.rs:2517. - Duplicate registry API:
IntegrationRegistry::is_enabledis a byte-identical copy of the pre-existingintegration_enabled, which is left with zero call sites (crates/trusted-server-core/src/integrations/registry.rs:1140).
Non-blocking
🤔 thinking
- Publisher asset policy never inspects
Set-Cookie: correct today only because all four adapters run the privacy pass afterwards, but nothing tests that ordering (crates/trusted-server-core/src/publisher.rs:1100).
📝 note
- Fastly static responses drop
s-maxage: edge TTL moves toSurrogate-Controlonly; Axum/Spin retain it viaSMaxageFallback. Same effective TTL, but an undocumented header-shape change on every/static/tsjs=response (crates/trusted-server-core/src/http_util.rs:283).
⛏ nitpick
parse_deferred_module_filenamename/doc: it also resolves the non-deferred diagnostics module (crates/trusted-server-core/src/publisher.rs:366).
👍 praise
- Quoted-string-aware
Cache-Controldirective matching (crates/trusted-server-core/src/cache_policy.rs:314). - Private-visibility TTL validation and the Wrangler cache fix, both with regression coverage (
crates/trusted-server-core/src/settings.rs:2063).
CI Status
All 19 GitHub checks pass on fd6d8324.
- fmt: PASS
- clippy: PASS (covered by the per-adapter check/build jobs)
- rust tests: PASS (fastly, axum native, cloudflare, spin, ts CLI, cross-adapter parity)
- js tests: PASS (vitest, format-typescript)
- docs format: PASS
- integration/browser suites: PASS
Note that the passing suite does not cover the reverted publisher.rs behavior — those tests no longer exist on this branch.
| @@ -2476,12 +2514,20 @@ async fn collect_non_html_auction( | |||
| AuctionTerminalOutcome::Completed { | |||
| request: auction_request, | |||
| result: &result, | |||
| delivered_winner_slots: Some(&delivered_winner_slots), | |||
| delivered_winner_slots: None, | |||
There was a problem hiding this comment.
🔧 wrench — This branch reverts merged main work in publisher.rs
origin/main (f6a2fb85) is an ancestor of this branch — git merge-base --is-ancestor origin/main HEAD succeeds and git rev-list --count $(git merge-base origin/main HEAD)..origin/main is 0. So the branch is main + 8 commits, and every deletion below is a deletion from current main, not a stale base.
git diff origin/main...HEAD -- crates/trusted-server-core/src/publisher.rs --numstat reports 563 added, 2354 deleted (11,590 → 9,799 lines). Symbol counts:
| Lost | main | branch |
|---|---|---|
diagnostics_auction_id (#974) |
9 | 0 |
hb_auction_id targeting (#974) |
4 | 0 |
non_empty / GAM_TARGETING_VALUE_MAX_LEN (#996) |
7 / 3 | 0 |
has_renderer creative-rejection guard (#956) |
2 | 0 |
delivered_winner_slots (#997) |
8 | 3, all literal None |
| test functions | 163 | 141 |
Concrete regressions:
- Render attribution (Attribute GPT renders to Trusted Server on observed evidence #997). This line and
:2564/:3938passdelivered_winner_slots: None.emit_auction_completedthen falls back toresult.winning_bids.len()(auction/telemetry.rs:523) and stops filtering undelivered slots (:708,:733) — re-inflating delivered-winner counts to the pre-Attribute GPT renders to Trusted Server on observed evidence #997 behavior.write_bids_to_stateis also moved to after telemetry emission, so its return value can no longer inform it. - Diagnostics auction ID (Add GPT runtime diagnostics overlay #974).
diagnostics_auction_idandbuild_bid_map_with_auction_idare gone, sohb_auction_idis never emitted into targeting — 0 occurrences repo-wide on this branch. - hb_adid precedence (Always emit hb_adid so server-side ad template creatives render #996).
non_empty()andGAM_TARGETING_VALUE_MAX_LENare deleted, restoring the blank-cacheId/ over-length-value failure modes that PR closed. - Creative rejection (Make creative sanitization opt-in and restore creative iframe origin isolation #956).
build_bid_mapreverts toif !adm.is_empty(), dropping both the “rejected creative with no typed renderer → skip the bid” branch and the entirehb_cache_host/hb_cache_pathcache-fallback emission gated on a non-emptycache_id.
22 tests go with it, including datadome_filter_marker_survives_into_publisher_html_pipeline, suppressed_{navigation,iframe,subresource}_*_conditional_and_range_headers, eligible_navigation_rejects_unexpected_origin_304, noneligible_origin_304_preserves_conditional_response_metadata, initial_navigation_auctions_only_renderable_slots, and the page_bids_* family. CI is green precisely because those tests were deleted alongside the code they guarded — a passing pipeline is not evidence here.
fd6d8324 hand-re-adds some of main's work (is_html_document_request, strip_conditional_and_range_headers, apply_datadome_client_tag_cache_privacy, match_renderable_slots), which is the signature of manual conflict resolution rather than a real merge — and it re-added them incompletely.
Fix: merge origin/main and take main's publisher.rs wholesale, then re-apply only this PR's cache edits on top — the EdgeCacheHeader parameter threading, apply_publisher_asset_cache_policy, serve_tsjs_static, and the handle_tsjs_dynamic signature change. Afterwards, git diff origin/main...HEAD -- crates/trusted-server-core/src/publisher.rs should show close to zero deletions outside those call sites.
| /// Return whether an integration is enabled, including integrations whose | ||
| /// JavaScript is delivered outside the standard module bundles. | ||
| #[must_use] | ||
| pub fn is_enabled(&self, integration_id: &str) -> bool { |
There was a problem hiding this comment.
🔧 wrench — Duplicate of the existing integration_enabled, from the same merge
integration_enabled already exists at registry.rs:1109 with a byte-identical body:
pub fn integration_enabled(&self, integration_id: &str) -> bool {
self.inner.enabled_integration_ids.contains(&integration_id)
}main's handle_tsjs_dynamic calls it for exactly this diagnostics-module check. After this PR, grep -rn '\.integration_enabled(' crates/ returns zero call sites while the new is_enabled takes its place — two public methods, same behavior, one dead.
Fix: drop is_enabled and call integration_enabled(module_id) at publisher.rs:317. This resolves itself if the merge above is redone properly.
| // Compute ETag for conditional caching | ||
| let hash = Sha256::digest(body.as_bytes()); | ||
| let etag = format!("\"sha256-{}\"", hex::encode(hash)); | ||
| let short_policy = CachePolicy::public_short_with_stale( |
There was a problem hiding this comment.
📝 note — Fastly static responses quietly lose s-maxage
Before this change the static ETag response always sent:
Cache-Control: public, max-age=300, s-maxage=300, stale-while-revalidate=60, stale-if-error=86400
Surrogate-Control: max-age=300
cache_control_value now emits s-maxage only under EdgeCacheHeader::SMaxageFallback, so on Fastly (SurrogateControl) the browser header drops to public, max-age=300, stale-while-revalidate=60, stale-if-error=86400. Axum and Spin keep s-maxage via the fallback mode, and Cloudflare moves it to Cloudflare-CDN-Cache-Control.
Net effect is small — any shared cache in front of Fastly that does not read Surrogate-Control now falls back to max-age=300, the same TTL — and Surrogate-Control actually gains the stale directives it did not carry before. Flagging it because it is a header-shape change on every /static/tsjs= response and it is not mentioned in the PR description.
| cache_control_headers_are_private_or_no_store(response.headers()) | ||
| } | ||
|
|
||
| fn apply_publisher_asset_cache_policy( |
There was a problem hiding this comment.
🤔 thinking — The Set-Cookie safety here is entirely inherited, and nothing pins it
The gate covers method, private/no-store, and status, but never looks at Set-Cookie. A publisher-origin asset that returns 200 with a Set-Cookie and no cache directive gets rewritten to public, max-age=… plus an edge header.
That is safe today only because every adapter runs the privacy pass afterwards — enforce_set_cookie_cache_privacy via apply_response_headers_with_cache_privacy in the Axum (middleware.rs:101), Cloudflare (:114) and Spin (:141) finalizers, and via send_edgezero_response on Fastly (main.rs:336), which also runs after EC finalization writes the identity cookie.
So there is no bug to fix, but the correctness of a public asset policy now depends on an ordering invariant enforced four crates away, with no test asserting it. A single publisher-path regression test — rule-matched asset, origin returns Set-Cookie with no cache directive, assert the finalized response is not shared-cacheable — would keep a future finalizer reshuffle from silently reopening it.
| /// | ||
| /// Returns `Some(&'static str)` if the filename matches a known JS module ID, | ||
| /// `None` otherwise. The caller must additionally verify that the module is | ||
| /// both deferred and enabled via the [`IntegrationRegistry`]. | ||
| #[must_use] | ||
| fn parse_single_module_filename(filename: &str) -> Option<&'static str> { | ||
| fn parse_deferred_module_filename(filename: &str) -> Option<&'static str> { |
There was a problem hiding this comment.
⛏ nitpick — The name and doc now undersell what this resolves
It is documented as extracting “a module ID from a deferred-module filename”, but since fd6d8324 the caller also routes the non-deferred GPT diagnostics module through it (publisher.rs:315-319). main kept the broader name parse_single_module_filename for that reason, and its doc comment covers both cases.
Either restore the original name or extend the doc line to say the caller separately admits the conditionally injected diagnostics module.
| quoted = false; | ||
| } | ||
| } else if character == '"' { | ||
| quoted = true; |
There was a problem hiding this comment.
👍 praise — Quoted-string-aware directive splitting
Tracking quoted / escaped while scanning for the , separator closes the last realistic false-positive in this predicate: a quoted extension value such as ext="a,no-store,b" no longer reads as a no-store directive, and \\" inside the quoted string is handled rather than terminating it early.
Combined with the directive-name-exact match against = / ; and the get_all multi-header sweep, cache_control_headers_are_private_or_no_store is now the single trustworthy predicate that the publisher gate, the asset-proxy Normalized guard, and enforce_uncacheable_cache_privacy all depend on. The paired test makes it stick.
| if self.edge_ttl_seconds.is_some() { | ||
| return Err(Report::new(TrustedServerError::Configuration { | ||
| message: format!( | ||
| "cache.asset_rules `{}` sets edge_ttl_seconds with private visibility; private rules must use browser_ttl_seconds", |
There was a problem hiding this comment.
👍 praise — Both prior blocking findings closed properly
visibility = "private" now rejects edge_ttl_seconds outright and requires browser_ttl_seconds, so the configuration that silently discarded its only TTL fails at startup with a message naming the rule and the fix. validate_policy_shape also holds the immutable invariants together: positive browser_ttl_seconds, and fingerprint_style or the nextjs-static preset.
The companion fix landed too — the global [cache] enabled = true block is gone from both wrangler.toml and wrangler.ci.toml, and test_cloudflare_dynamic_publisher_response_does_not_cross_cookie_boundaries backs it with two cookie-distinguished requests against a CookieVaryingOrigin, matching the #[ignore] convention the other nine runtime tests in that file use.
aram356
left a comment
There was a problem hiding this comment.
Summary
The cache-header work itself is well designed: cache_policy.rs is thoroughly unit-tested, the operator-controlled asset-rule engine validates eagerly at startup with a safe disabled-by-default posture, the neutralized Prebid shim moving to no-store, private closes a real caching hole, and the exact-directive Cache-Control parsing in response_privacy is more correct than the substring matching it replaces. However, the rebase that produced the base commit (d7737d3) carried a stale pre-rebase copy of publisher.rs, silently reverting several features that landed on main after this branch was cut. CI is green only because the rebase deleted the regression tests together with the features, so the suite cannot see the reverts.
Blocking
🔧 wrench
All five inline 🔧 comments share one root cause: publisher.rs was clobbered back to its pre-rebase state. The reverted main features are:
-
#945/#997 hydration-deferred adInit:
build_bids_scriptcallsadInit()synchronously again (publisher.rs:3505), re-introducing the React #418 hydration mismatch; the JSscheduleInitialAdInitscheduler is now dead code. -
#996/#998 hb_adid chain + APS renderer carrier: bid map no longer serializes
rendererand loses thenon_emptyprecedence and GAM 40-char guard (publisher.rs:3393). -
Sanitization bypass:
hb_cache_host/hb_cache_pathare emitted unconditionally, letting the Prebid Universal Creative fetch the original unsanitizedadmfrom PBS Cache for creatives TS rejected (publisher.rs:3406). -
#997 GPT render attribution:
hb_auction_idis no longer minted while the GPT bundle still consumes it;delivered_winner_slotstelemetry is hardcoded toNoneon all three auction paths (publisher.rs:2517). -
#918 page-URL sanitization: raw client query strings now leak into
page_url/site.pagetoward SSPs (publisher.rs:3285). -
Deleted regression tests without replacement (~1,900 lines): the whole
ssat_cache_policy_testsmodule (navigation cache bypass, unexpected-origin-304 fail-closed, conditional/Range header handling), the DataDome suppression pipeline tests, the over-limit dynamic GAM slot tests (build_slot_jsonreverted from returningOption), and the auction-id tests. The SSAT runtime behaviors themselves survived, so for those it is test coverage that regressed, but the five items above are genuine behavior reverts.
Suggested fix as one operation instead of five: restore main's publisher.rs (git checkout origin/main -- crates/trusted-server-core/src/publisher.rs), then re-apply only this PR's genuine changes: the edge_header parameter on handle_tsjs_dynamic/handle_publisher_request, serve_tsjs_static + request_version_hash, apply_publisher_asset_cache_policy and its call site, and the earlier apply_datadome_client_tag_cache_privacy call-site move. Then diff publisher.rs against main once more to confirm only cache-related hunks remain.
Non-blocking
🤔 thinking
- Multi-
**glob expansion gap: the**/-stripping recursion never generates mixed combinations for patterns with two recursive segments (settings.rs:2221). - No content-type gate on publisher asset cache policy: a broad operator glob can mark TS-rewritten HTML publicly immutable; docs warn, a
text/htmlguard would prevent it outright (publisher.rs:1100).
♻️ refactor
- Duplicate registry API: new
is_enabledduplicates the existingintegration_enabled(registry.rs:1140).
🌱 seedling
Normalizedasset policy portability: the runtime edge header depends on a Fastly-only finalization re-apply; a comment on the variant would keep future adapters from silently dropping the edge directive (proxy.rs:118).
📝 note
- Unversioned unified TSJS fallback: testlight's default shim src loses cache busting; staleness is bounded by the 5-minute TTL, worth noting in the PR description (tsjs.rs:29).
CI Status
- GitHub checks: all pass (fmt, clippy, cargo test across adapters, cross-adapter parity, vitest, CodeQL);
prepare integration artifactsstill pending at review time. - Local verification in a clean worktree at 7c865ce:
cargo fmt --all -- --checkPASS,cargo clippy-fastlyPASS,cargo test-fastlyPASS. - Note: green CI does not cover the reverts above because their tests were deleted in the same rebase.
| if(typeof s===\"function\")s(b);\ | ||
| else t.bids=b;\ | ||
| }})();</script>", | ||
| "<script>(window.tsjs=window.tsjs||{{}}).bids=JSON.parse(\"{}\");(function(){{var f=window.tsjs.adInit;if(typeof f===\"function\")f();}})();</script>", |
There was a problem hiding this comment.
🔧 wrench - This reverts the hydration-deferred adInit from #945/#997. Main hands the bids payload to tsjs.scheduleInitialAdInit (window load + double requestAnimationFrame, pinned to navigation generation 0); this version calls adInit() synchronously at body-parse time, which re-introduces the React #418 hydration mismatch on Next.js publishers, and the unconditional .bids= assignment lets a stale SSR payload clobber a faster SPA navigation's live bids. The JS bundle still ships scheduleInitialAdInit (crates/trusted-server-js/lib/src/integrations/gpt/index.ts), so it is now dead code that never runs.
Fix: restore main's build_bids_script body and its bids_script_defers_ad_init_until_after_hydration test:
format!(
"<script>(function(){{\
var t=window.tsjs=window.tsjs||{{}};\
var b=JSON.parse(\"{}\");\
var s=t.scheduleInitialAdInit;\
if(typeof s===\"function\")s(b);\
else t.bids=b;\
}})();</script>",
escaped
)| // hb_adid: use PBS Cache UUID when present — the Prebid Universal Creative uses | ||
| // this as the cache lookup key, NOT the OpenRTB bid ID (bid.ad_id). Fall back to | ||
| // bid.ad_id for APS and other non-PBS providers. | ||
| let hb_adid = bid.cache_id.as_deref().or(bid.ad_id.as_deref()); |
There was a problem hiding this comment.
🔧 wrench - This reverts the hb_adid selection chain from #996/#998. Main uses non_empty(cache_id) -> APS renderer bid_id -> adid -> bid_id (blank strings treated as absent), serializes the typed renderer object into the bid map so the APS renderer handshake can complete, and warns when the value exceeds GAM's 40-character targeting-value limit. This version collapses to cache_id.or(ad_id), drops the renderer field entirely (breaking the APS render path validated in #998), lets a blank cacheId win the precedence, and loses the GAM length warning. The bid_map_exposes_aps_renderer_and_selected_bid_id test was deleted with it.
Fix: restore main's build_bid_map hb_adid/renderer logic and the deleted tests.
| // https://<hb_cache_host><hb_cache_path>?uuid=<hb_adid> | ||
| if let Some(ref host) = bid.cache_host { | ||
| obj.insert( | ||
| "hb_cache_host".to_string(), |
There was a problem hiding this comment.
🔧 wrench - Security regression: emitting hb_cache_host/hb_cache_path unconditionally re-enables the PBS Cache fallback for creatives that TS processing rejected. The Prebid Universal Creative fetches the cached bid's ORIGINAL adm via https://<hb_cache_host><hb_cache_path>?uuid=<hb_adid>, so when sanitization strips a hostile or oversized creative (empty processed output, adm omitted below), the client is handed an unprocessed copy of the exact markup TS refused. Main only emits the cache coordinates when the bid supplied no creative at all and cache_id is non-blank.
Fix: restore main's gating (coordinates only for absent creatives with a non-blank cache_id; rejected creatives suppress the fallback) and the deleted tests build_bid_map_suppresses_cache_fallback_for_rejected_creatives / build_bid_map_keeps_cache_fallback_for_absent_creatives.
| @@ -2476,12 +2514,20 @@ async fn collect_non_html_auction( | |||
| AuctionTerminalOutcome::Completed { | |||
| request: auction_request, | |||
| result: &result, | |||
| delivered_winner_slots: Some(&delivered_winner_slots), | |||
| delivered_winner_slots: None, | |||
There was a problem hiding this comment.
🔧 wrench - This reverts the GPT render attribution from #997. diagnostics_auction_id and the per-winner hb_auction_id are removed, but the GPT bundle (crates/trusted-server-js/lib/src/integrations/gpt/index.ts) still consumes hb_auction_id, so render attribution goes silently dead. delivered_winner_slots is also hardcoded to None here and in collect_stream_auction / handle_page_bids, so auction summary telemetry loses the delivered-winner dimension on all three paths.
Fix: restore diagnostics_auction_id, build_bid_map_with_auction_id, the delivered_winner_slots plumb-through, and the deleted auction-id tests from main.
| @@ -3231,11 +3282,10 @@ pub(crate) fn build_auction_request( | |||
| // so SSPs, injected creatives, and brand-safety pixels see the publisher's | |||
| // own origin. On the SSAT proxy path `request_info.host` is the trusted | |||
| // server edge host, which must not leak into the bid request. | |||
| let page_candidate = format!( | |||
| let page_url = format!( | |||
There was a problem hiding this comment.
🔧 wrench - This reverts the page-URL sanitization from #918: the sanitize_publisher_page_url call is removed, so the raw request query string now flows into page_url and site.page in outbound bid requests. Client query data (campaign parameters, tracking tokens) leaks to SSPs. The auction_request_preserves_configured_publisher_domain_with_query test was deleted and the remaining test flipped to assert ?edition=fictional is preserved, which locks in the regression.
Fix:
let page_candidate = format!(
"{}://{}{}",
request_info.scheme, publisher_domain, slots_ctx.request_path
);
let page_url = sanitize_publisher_page_url(Some(&page_candidate), publisher_domain);| /// Return whether an integration is enabled, including integrations whose | ||
| /// JavaScript is delivered outside the standard module bundles. | ||
| #[must_use] | ||
| pub fn is_enabled(&self, integration_id: &str) -> bool { |
There was a problem hiding this comment.
♻️ refactor - is_enabled duplicates the existing integration_enabled (line 1109) with identical semantics; handle_tsjs_dynamic even switched from the existing method to this new one. Drop this method and keep the integration_enabled call so the registry has a single enablement query.
| require_literal_leading_dot: false, | ||
| }; | ||
|
|
||
| fn compile_cache_asset_glob_patterns( |
There was a problem hiding this comment.
🤔 thinking - The **/-stripping expansion only generates suffix-stripped variants, so a pattern with two recursive segments misses mixed combinations: /a/**/b/**/c.js expands to itself, /a/b/**/c.js, and /a/b/c.js, but never /a/**/b/c.js, so /a/x/b/c.js will not match. Unlikely config shape, but worth either expanding all combinations (strip each **/ independently, not just left-to-right) or documenting the limitation next to the recursion.
| cache_control_headers_are_private_or_no_store(response.headers()) | ||
| } | ||
|
|
||
| fn apply_publisher_asset_cache_policy( |
There was a problem hiding this comment.
🤔 thinking - This is gated on method, status, and the private/no-store veto, but not on content type. A broad operator glob (say /news/**) would mark TS-rewritten HTML publicly immutable whenever the ad stack did not run for that navigation. The configuration docs do warn about this, but a content-type guard (skip text/html) would make the misconfiguration impossible rather than documented.
| /// | ||
| /// [`IntegrationRegistry`]: crate::integrations::IntegrationRegistry | ||
| #[must_use] | ||
| pub fn tsjs_unified_script_src() -> String { |
There was a problem hiding this comment.
📝 note - Dropping ?v= here means consumers of the registry-free helper (testlight's default_shim_src) lose cache busting entirely; after a deploy, staleness is bounded only by the 5-minute short TTL. That trade-off is deliberate per the doc comment and reasonable, but it is a behavior change for testlight deployments worth calling out in the PR description.
| /// Reapply `Cache-Control: no-store, private` after standard finalization. | ||
| NoStorePrivate, | ||
| /// Reapply an operator-selected normalized cache policy after finalization. | ||
| Normalized(CachePolicy), |
There was a problem hiding this comment.
🌱 seedling - Normalized applies with EdgeCacheHeader::None at the handler and relies on the Fastly main.rs finalization re-apply to render the runtime edge header. That is correct today because asset rehost routes are Fastly-only, but nothing here records that invariant; if another adapter grows this route it would silently serve normalized policies with no edge directive. A short comment on this variant noting the required adapter-side apply_after_route_finalization call would prevent that trap.
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!)