Skip to content

Implement configurable cache header policies - #860

Open
ChristianPavilonis wants to merge 9 commits into
mainfrom
refactor/cache-headers
Open

Implement configurable cache header policies#860
ChristianPavilonis wants to merge 9 commits into
mainfrom
refactor/cache-headers

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Standardizes cache policy rendering across Fastly, Cloudflare, CDN, and s-maxage fallback headers.
  • Adds safe, configurable caching for hash-validated TSJS, publisher-origin static assets, and rehosted asset proxy responses.
  • Hardens privacy handling so private, no-store, and cookie-bearing responses strip shared edge-cache headers.

Changes

File Change
crates/trusted-server-core/src/cache_policy.rs Adds typed cache-policy rendering for browser and edge headers, including no-store/private cleanup.
crates/trusted-server-core/src/settings.rs Adds cache.asset_rules config, matchers/presets, validation, runtime prep, and path-to-policy resolution.
trusted-server.example.toml Documents disabled operator-controlled static/fingerprinted asset cache-rule examples.
crates/trusted-server-core/src/http_util.rs Routes static ETag responses through the cache-policy renderer.
crates/trusted-server-core/src/tsjs.rs Uses exact module-set hashes when available and avoids unverifiable fallback hashes.
crates/trusted-server-js/Cargo.toml Makes hashing dependencies available to the build script.
crates/trusted-server-js/build.rs Generates per-module SHA-256 metadata for bundled JS modules.
crates/trusted-server-js/src/bundle.rs Exposes per-module hashes and caches concatenated bundle hashes.
crates/trusted-server-core/src/publisher.rs Applies hash-validated immutable TSJS caching and configured publisher asset cache rules.
crates/trusted-server-core/src/proxy.rs Applies normalized cache policies to rehosted asset proxy responses and reapplies them after finalization.
crates/trusted-server-core/src/response_privacy.rs Removes shared-cache headers from private/no-store/cookie-bearing responses.
crates/trusted-server-core/src/integrations/prebid.rs Makes the neutralized Prebid shim no-store, private instead of long-lived public cache.
crates/trusted-server-core/src/integrations/testlight.rs Updates the default TSJS fallback comment/source behavior for registry-free configuration.
crates/trusted-server-core/src/lib.rs Exports the new cache_policy module.
crates/trusted-server-adapter-axum/src/app.rs Passes the portable s-maxage fallback edge header mode to TSJS and publisher handlers.
crates/trusted-server-adapter-cloudflare/src/app.rs Passes the Cloudflare-specific CDN cache header mode to TSJS and publisher handlers.
crates/trusted-server-adapter-fastly/src/app.rs Passes Fastly Surrogate-Control mode through EdgeZero fallback dispatch.
crates/trusted-server-adapter-fastly/src/main.rs Reapplies asset cache policies with Fastly Surrogate-Control at the shared finalization point used by both legacy and EdgeZero flows.
crates/trusted-server-adapter-fastly/src/route_tests.rs Updates route-test finalization for selected edge cache headers.
crates/trusted-server-adapter-spin/src/app.rs Passes the portable s-maxage fallback edge header mode to TSJS and publisher handlers.
docs/superpowers/specs/2026-07-06-cache-control-header-design.md Adds the cache-control design scope and deferred dynamic caching notes.
docs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.md Adds the implementation and verification plan for cache-header work.

Closes

Closes #293

Follow-ups:

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test-cloudflare && cargo test-spin
  • Other: cargo clippy-cloudflare && cargo clippy-spin-native && cargo clippy-spin-wasm
  • Other: cd crates/trusted-server-js/lib && node build-all.mjs
  • Other: git diff --check
  • Other: npx prettier --check docs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.md docs/superpowers/specs/2026-07-06-cache-control-header-design.md

Note: cd docs && npm run format failed because docs-local Prettier was not installed; touched docs were checked with npx prettier instead.

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis
ChristianPavilonis changed the base branch from main to server-side-ad-templates-impl July 7, 2026 17:59
Base automatically changed from server-side-ad-templates-impl to main July 7, 2026 20:08
ChristianPavilonis

This comment was marked as low quality.

@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review July 8, 2026 19:00

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 safeserve_tsjs_static marks a response immutable (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 from js_module_ids_immediate() + concatenated_hash; deferred modules use single_module_hash on both sides.
  • Privacy hardeningprivate / no-store / cookie-bearing responses strip all four edge-cache headers, with the downgrade re-run after operator headers are applied.
  • Origin no-store not upgraded — a split later Cache-Control field carrying no-store correctly blocks the normalized upgrade.
  • Asset-proxy finalization is correctly Fastly-onlyhandle_asset_proxy_request / apply_after_route_finalization are wired only on Fastly, so there is no missing-reapplication gap on Cloudflare / Axum / Spin.

Non-blocking

🤔 thinking

  • Hex-only fingerprint heuristic: filename_contains_hash misses 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 only private/no-store. (publisher.rs)

🌱 seedling

  • handle_publisher_request is now at the 7-argument CLAUDE.md limit; EdgeCacheHeader is threaded through several signatures — consider a request-context struct. (publisher.rs)

⛏ nitpick

  • SURROGATE_CACHE_HEADERS re-export is now a misnomer (contains CDN headers) with no in-tree consumers. (response_privacy.rs:21)

📝 note

  • #[validate(nested)] on cache is a no-op; real validation lives in prepare_runtime. (settings.rs)

👍 praise

  • Directive-exact Cache-Control matching 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

Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs
Comment thread crates/trusted-server-core/src/cache_policy.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / privateproxy.rs:1173. Only the status is checked; publisher.rs:470 guards this correctly for the same feature. The PR's own test (proxy.rs:3755) feeds an origin no-store and asserts it becomes public, max-age=31536000, immutable. No downstream rescue: the Set-Cookie backstop can't fire because the asset proxy strips set-cookie.
  • Normalized re-publicizes after privacy hardeningproxy.rs:127. The same root cause at a second layer. apply_after_route_finalization used to only ever make responses more private, so running it last was safe; the new Normalized arm 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 = true accepted with no fingerprint requirementsettings.rs:1983. requires_hash_in_filename defaults to false, so path_prefix = "/assets/" + immutable = true puts 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 esbuildsettings.rs:2204, with the reachable trap at configuration.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 the enabled = true docs 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 falseconfiguration.md:998. prepare_runtime validates every rule regardless of enabled. 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_seconds on Fastly today?configuration.md:1014. Fastly's read-through cache stores the backend's response and decides TTL from the backend's headers at send(); 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 of fastly::cache / CacheOverride / SimpleCache. Is there a service-layer piece outside the repo? To be fair: the Surrogate-Control emission 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 v query 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 in fastly.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-control as its highest-precedence cache directive — exactly what this PR emits. But it's opt-in, and neither wrangler.toml nor wrangler.ci.toml has a [cache] block, so the header is inert today. Adding [cache]\nenabled = true (Wrangler ≥ 4.69.0) would turn EdgeCacheHeader::CloudflareCdnCacheControl from 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, not trusted-server-core. Worth a follow-up issue, not a change to this PR.

    EdgeCacheHeader encodes 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) into handle_tsjs_dynamic and handle_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 pushed handle_publisher_request to exactly 7 parameters, CLAUDE.md's stated ceiling.

    More importantly, the part that would make edge_ttl_seconds actually work can only be built in edgezero. edgezero-core currently has no cache concept at all, and edgezero-adapter-fastly/src/proxy.rs:31 sends upstream with send_async_streaming(&backend_name) and no CacheOverride — 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:

    • edgezeroEdgeCacheHeader and the edge-header-name registry; the CachePolicy → platform headers render step (ideally behind an adapter method like apply_cache_policy(&policy, &mut resp), so the parameter disappears from core signatures entirely); CacheOverride on backend sends; the Core/Simple Cache API for synthetic responses; the wrangler [cache] block.
    • trusted-serverCachePolicy as typed domain data, the cache.asset_rules config surface and matching, and the response_privacy invariants (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_seconds expecting shared-cache behavior.

♻️ refactor

  • SURROGATE_CACHE_HEADERS has zero consumersresponse_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 neither browser_ttl_seconds nor edge_ttl_seconds renders a bare Cache-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 in settings.rs:1890-2215 or in cache_policy.rs, and the application site is a bare if let with no else. A rule that matches nothing — the hash-gate case above, for instance — is undebuggable in production. A log::debug! on gate rejection would pay for itself.

📝 note

  • #[validate(nested)] on Settings.cache is a no-op. CacheSettings declares no field validators and CacheAssetRule doesn't derive Validate, 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

Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/response_privacy.rs Outdated
Comment thread crates/trusted-server-core/src/tsjs.rs
@aram356

aram356 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis Please resolve conflicts

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_base64url matches any 8-character alphanumeric suffix with at least one uppercase and one lowercase/digit character, so logo-DarkMode.svg, hero-Portrait.jpg, icon-Facebook.svg, banner-Summer24.png and photo-Iceland1.jpg all pass. Because immutable implies 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=31536000no-store, private on a 43-byte stub served from the publisher's (often blocking) prebid.js URL. (crates/trusted-server-core/src/integrations/prebid.rs:682)
  • Rehost path overrides a third-party origin's no-store while 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::matches defaults to require_literal_separator: false, so path_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, and is_get is 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::CdnCacheControl is 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's v equals 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 the not-private / no-storey false-positive class, and get_all covers split Cache-Control fields — publisher_asset_cache_policy_respects_split_no_store_origin_header locks that in. (crates/trusted-server-core/src/cache_policy.rs:291)
  • enforce_uncacheable_cache_privacy: closes the window enforce_set_cookie_cache_privacy alone missed, where a late private/no-store directive 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.

Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/prebid.rs
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-js/src/bundle.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/cache_policy.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread crates/trusted-server-adapter-cloudflare/wrangler.toml Outdated
Comment thread crates/trusted-server-core/src/settings.rs
@aram356 aram356 added this to the 202608 milestone Aug 17, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 main work in publisher.rs: 563 added / 2,354 deleted against a base that is literally the current main tip. hb_auction_id diagnostics targeting (#974), the non_empty / GAM_TARGETING_VALUE_MAX_LEN hb_adid hardening (#996), the has_renderer creative-rejection guard and PBS-cache fallback gating (#956), and real delivered_winner_slots telemetry (#997) are all gone; 22 tests were deleted with them, which is why CI is green. Details and the suggested merge strategy are inline at crates/trusted-server-core/src/publisher.rs:2517.
  • Duplicate registry API: IntegrationRegistry::is_enabled is a byte-identical copy of the pre-existing integration_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 to Surrogate-Control only; Axum/Spin retain it via SMaxageFallback. 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_filename name/doc: it also resolves the non-deferred diagnostics module (crates/trusted-server-core/src/publisher.rs:366).

👍 praise

  • Quoted-string-aware Cache-Control directive 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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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:

  1. Render attribution (Attribute GPT renders to Trusted Server on observed evidence #997). This line and :2564 / :3938 pass delivered_winner_slots: None. emit_auction_completed then falls back to result.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_state is also moved to after telemetry emission, so its return value can no longer inform it.
  2. Diagnostics auction ID (Add GPT runtime diagnostics overlay #974). diagnostics_auction_id and build_bid_map_with_auction_id are gone, so hb_auction_id is never emitted into targeting — 0 occurrences repo-wide on this branch.
  3. hb_adid precedence (Always emit hb_adid so server-side ad template creatives render #996). non_empty() and GAM_TARGETING_VALUE_MAX_LEN are deleted, restoring the blank-cacheId / over-length-value failure modes that PR closed.
  4. Creative rejection (Make creative sanitization opt-in and restore creative iframe origin isolation #956). build_bid_map reverts to if !adm.is_empty(), dropping both the “rejected creative with no typed renderer → skip the bid” branch and the entire hb_cache_host / hb_cache_path cache-fallback emission gated on a non-empty cache_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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_script calls adInit() synchronously again (publisher.rs:3505), re-introducing the React #418 hydration mismatch; the JS scheduleInitialAdInit scheduler is now dead code.

  • #996/#998 hb_adid chain + APS renderer carrier: bid map no longer serializes renderer and loses the non_empty precedence and GAM 40-char guard (publisher.rs:3393).

  • Sanitization bypass: hb_cache_host/hb_cache_path are emitted unconditionally, letting the Prebid Universal Creative fetch the original unsanitized adm from PBS Cache for creatives TS rejected (publisher.rs:3406).

  • #997 GPT render attribution: hb_auction_id is no longer minted while the GPT bundle still consumes it; delivered_winner_slots telemetry is hardcoded to None on all three auction paths (publisher.rs:2517).

  • #918 page-URL sanitization: raw client query strings now leak into page_url/site.page toward SSPs (publisher.rs:3285).

  • Deleted regression tests without replacement (~1,900 lines): the whole ssat_cache_policy_tests module (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_json reverted from returning Option), 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/html guard would prevent it outright (publisher.rs:1100).

♻️ refactor

  • Duplicate registry API: new is_enabled duplicates the existing integration_enabled (registry.rs:1140).

🌱 seedling

  • Normalized asset 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 artifacts still pending at review time.
  • Local verification in a clean worktree at 7c865ce: cargo fmt --all -- --check PASS, cargo clippy-fastly PASS, cargo test-fastly PASS.
  • 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>",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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!(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor and standardize how Trusted Server sets cache response headers

3 participants