Skip to content

Add a server-side ad template switch and cache policy - #1008

Open
ChristianPavilonis wants to merge 5 commits into
mainfrom
issue-1007-cache-control
Open

Add a server-side ad template switch and cache policy#1008
ChristianPavilonis wants to merge 5 commits into
mainfrom
issue-1007-cache-control

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add a clear [creative_opportunities].enabled switch for publisher server-side ad-template delivery.
  • Set inactive successful GET publisher documents to the issue Improve cache header for html content when SSAT is off #1007 browser cache policy (max-age=60), intentionally replacing the origin browser policy while leaving non-200/non-document responses and CDN-specific cache headers unchanged.
  • Keep direct POST /auction available when publisher templates are disabled.

Issue #1007 exposed that publisher HTML caching was tied to whether the server-side ad stack ran, while the global auction setting also controlled unrelated auction behavior. This change separates publisher template delivery from the direct auction API and makes the cache behavior explicit.

Changes

File Change
crates/trusted-server-core/src/creative_opportunities.rs Adds the default-true enabled configuration field and serialization coverage.
crates/trusted-server-core/src/settings.rs Hides creative-opportunity slots from runtime handlers when template delivery is disabled and tests environment overrides.
crates/trusted-server-core/src/config.rs Verifies compatibility for omitted defaults and explicit disabled values.
crates/trusted-server-core/src/publisher.rs Gates publisher HTML and page-bids template delivery, records the disabled-template reason, and tests cache behavior.
crates/trusted-server-core/src/auction/endpoints.rs Proves direct POST /auction still dispatches when templates are disabled.
trusted-server.example.toml Documents the new setting in the example configuration.
docs/guide/configuration.md Documents the switch, cache policy, and environment override.
CHANGELOG.md Records the new switch and cache behavior.
crates/trusted-server-js/lib/src/core/index.ts Clarifies browser defaults when template delivery is gated.
crates/trusted-server-js/lib/src/integrations/gpt/index.ts Clarifies page-bids behavior when template delivery is disabled.
crates/trusted-server-js/lib/test/integrations/gpt/spa_hook.test.ts Updates the related regression-test explanation.

Scope

The change is limited to configuration, core publisher/page-bids execution, direct-auction regression coverage, browser comments, and documentation. Existing adapter routes already use the centralized settings accessor, so no divergent adapter-specific switch was needed. Active server-side templates retain private, no-store; inactive 200 OK GET document HTML uses exactly max-age=60, intentionally replacing the origin browser policy per #1007. Non-200, non-GET, and non-document responses retain the origin policy, request-scoped privacy finalization still takes precedence, and validators plus CDN-specific headers remain unchanged. An empty slot list could disable delivery rollback-safely, but the dedicated switch preserves configured slot definitions for reversible operations; because explicit enabled = false is serialized, the guide documents the required config re-push before rolling back to a pre-field binary.

Closes

Closes #1007

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, focused publisher tests, and all configured native/WASM clippy targets

Checklist

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

@ChristianPavilonis ChristianPavilonis self-assigned this Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a short browser cache policy for non-SSAT HTML Use a browser cache policy for non-SSAT HTML Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a browser cache policy for non-SSAT HTML Add a server-side ad template switch and cache policy Aug 7, 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

Separating publisher template delivery from the global [auction].enabled kill switch is the right boundary, and the direct-/auction regression test proves the separation holds. The configuration half of this PR is well covered.

The cache-policy half needs work. The new else branch does not just preserve origin policy when templates are inactive — it overwrites it, and its only protection is a two-token check for private/no-store. I verified against this branch (throwaway probes in ssat_cache_policy_tests, cargo test -p trusted-server-core) that origin no-cache, max-age=0, must-revalidate and s-maxage=0 are all replaced with max-age=60, and that 500/404/503 HTML responses become cacheable for 60 seconds. Details inline.

Blocking

🔧 wrench

  • Origin revalidation directives silently overwritten — the private/no-store guard misses no-cache, max-age=0, must-revalidate, s-maxage=0. A personalized page marked no-cache by origin, on a repeat visit that emits no Set-Cookie (so the adapter cookie-privacy net does not fire), gets max-age=60 with no private and no Vary — shared-cacheable and replayable to other users for 60s. (crates/trusted-server-core/src/publisher.rs:2986)
  • Non-2xx HTML becomes cacheable for 60s — no status gate on the new branch, so a transient origin 5xx is pinned in every browser and intermediary for a minute past recovery. (crates/trusted-server-core/src/publisher.rs:2961)
  • Rollback with enabled = false is a site-wide 500, and is undocumenteddeny_unknown_fields makes an older binary reject the blob, and load_settings_from_config_store() failing returns 500 for every request (crates/trusted-server-adapter-fastly/src/main.rs:112-118). The plan file states the "fail loud" intent, but neither docs/guide/configuration.md:1315 nor the CHANGELOG.md:12 entry warns operators what "loud" means here. (crates/trusted-server-core/src/config.rs:333)

❓ question

  • Absent [creative_opportunities] section also gets its cache policy rewrittenis_some_and makes "never configured" behave like "explicitly disabled", so deployments that never enabled server-side ad templates have their origin Cache-Control replaced with max-age=60. On main those responses passed through untouched. Intended blast radius? (crates/trusted-server-core/src/publisher.rs:2643)

Non-blocking

🤔 thinking

  • max-age=60 is an unexplained magic constant — no derivation in the CHANGELOG, configuration guide, or plan file, and not operator-tunable. (crates/trusted-server-core/src/publisher.rs:2992)
  • The empty-slot disable already existed and was rollback-safe — worth stating in the PR body why the new field's rollback cost was accepted. (crates/trusted-server-core/src/creative_opportunities.rs:206)

♻️ refactor

  • ad_templates_enabled / ad_templates_disabled are not complements — both are false when the section is absent; an explicit three-state enum would make that unmissable. (crates/trusted-server-core/src/publisher.rs:2644)

⛏ nitpick

  • should_run_server_side_ad_stack still takes 7 arguments — the new struct absorbed only 2 of the 8 flags, leaving 6 positional bools at every call site. (crates/trusted-server-core/src/publisher.rs:1765)
  • Test name contradicts its assertiondisabled_creative_opportunities_flag_is_visible_to_legacy_schema asserts expect_err, i.e. the legacy schema rejects the field. ..._is_rejected_by_legacy_schema would read correctly. (crates/trusted-server-core/src/config.rs:333)

📝 note

  • Cache-policy test matrix has the same gap as the codenavigation_without_matched_slots_preserves_private_origin_cache_policy covers "private, max-age=0" and "No-Store" only. Once the two wrench findings are settled, no-cache and a non-200 status belong in the same loop, or the regressions will not be caught. (crates/trusted-server-core/src/publisher.rs:5071)

👍 praise

  • Direct /auction regression testTemplateSwitchProbeProvider counts real provider invocations rather than asserting a status code, so it would actually fail if the template flag were later threaded into handle_auction. (crates/trusted-server-core/src/auction/endpoints.rs:707)
  • Rollback-compatible serialization of the defaultskip_serializing_if keeping default true out of pushed blobs matches the existing section_root precedent and keeps the no-opt-in case safe. (crates/trusted-server-core/src/creative_opportunities.rs:204)

CI Status

All 19 GitHub checks pass on 58054463.

  • fmt: PASS
  • clippy (fastly / axum / cloudflare native+wasm / spin native+wasm): PASS
  • rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • format-typescript / format-docs: PASS
  • integration + browser integration tests: PASS

The findings above are behavioral gaps that the current test matrix does not exercise, not CI failures.

Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.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

The switch mechanics are solid: default-true serde field with rollback-aware serialization, consistent accessor/handler gating, POST /auction independence proven by a provider-probe test, and validation still runs when disabled. The blocking concern is concentrated in the new cache-clamp branch, which overrides origin freshness directives beyond the private/no-store preserve-guard.

Blocking

🔧 wrench

  • Cache clamp overrides origin freshness directives beyond private/no-store: origins sending no-cache, must-revalidate, or max-age=0 get replaced with max-age=60 (crates/trusted-server-core/src/publisher.rs:2988 — see inline comment)

Non-blocking

🤔 thinking

  • Clamp blast radius: applies to all HTML, all methods, and publishers with no [creative_opportunities] section at all (crates/trusted-server-core/src/publisher.rs:2980 — see inline comment)
  • enabled = false config blobs break not-yet-upgraded binaries: the explicit-false rollback hazard is codified in a test but undocumented for operators (crates/trusted-server-core/src/config.rs:333 — see inline comment)

🌱 seedling

  • Hardcoded 60-second TTL: likely needs to become configurable when SSAT is re-architected for cacheability (crates/trusted-server-core/src/publisher.rs:2992 — see inline comment)

CI Status

  • fmt: PASS
  • clippy (all targets): PASS
  • rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
  • js tests (vitest): PASS
  • browser integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
@aram356 aram356 added this to the 202608 milestone Aug 13, 2026

@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 update resolves four of the five round-one concerns: the inactive policy is now correctly narrowed to 200 OK GET document HTML (with regression coverage for 206/404/500/503, POST, and non-document fetches), the enabled = false rollback hazard is documented with the correct re-push sequencing in the guide/example/CHANGELOG, GPT-diagnostics privacy now takes precedence via the finalize_response reordering, and the 60s TTL source is documented. The direct-auction probe test survived the rebase intact.

One blocking concern remains — and it was introduced by the round-one fix itself: the preserve guard was removed entirely rather than extended, so origin private/no-store HTML is now rewritten to a shared-cacheable policy.

Blocking

🔧 wrench

  • Removing the preserve guard makes origin private/no-store HTML shared-cacheable: inactive 200 OK GET documents replace origin privacy directives with bare max-age=60, and the test at crates/trusted-server-core/src/publisher.rs:5240 pins that in (crates/trusted-server-core/src/publisher.rs:2995 — see inline comment)

CI Status

  • fmt: PASS
  • clippy (all targets): PASS
  • rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
  • js tests (vitest): PASS
  • browser integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
aram356 added a commit that referenced this pull request Aug 16, 2026
#1010)

Resolved publisher.rs to keep #1008's inactive-SSAT cache policy;
datadome protection.rs resolved to main's final #992 squash.

@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

Round-two pass, scoped to what the update introduces rather than re-litigating round one. The status/method/document narrowing and the finalize_response reordering both look right, and I verified that nothing per-user is injected on the inactive path — so the residual risk in the new branch is entirely about origin-declared policy and cache-variant mixing, not injected identity.

Two new blocking findings, neither of which overlaps the outstanding preserve-guard concern. The first is additive to it: even with origin private/no-store preserved, a no-cache origin still exposes an ad-free variant. The second is an operator-facing documentation/test defect in the new environment override.

Blocking

🔧 wrench

  • Request-scoped suppression makes the ad-free variant the only shared-cacheable one: the inactive branch has no !is_bot / !is_prefetch / consent gate, so bot, prefetch, and consent-denied navigations of an ad-eligible URL get bare max-age=60 while consenting humans get private, no-store — with no Vary on the publisher path (crates/trusted-server-core/src/publisher.rs:2995, see inline comment for probe output).
  • The documented environment override for the new switch silently no-ops on the real deploy path: Settings::from_toml_and_env is #[cfg(test)]-only, and the EdgeZero overlay cannot create missing TOML leaves (docs/guide/configuration.md:1381, see inline comment).

Non-blocking

🤔 thinking

  • max-age=60 is documented only where its largest affected audience won't read it: the policy also applies with enabled = true, with no [creative_opportunities] section at all, and on bot/prefetch/consent-denied requests (docs/guide/configuration.md:1350, trusted-server.example.toml:184-188).
  • Undocumented, untested exception — first-visit navigations never get max-age=60: enforce_set_cookie_cache_privacy rewrites any Set-Cookie-bearing response to private, max-age=0 (crates/trusted-server-core/src/publisher.rs:2999).

📝 note

  • Verified: nothing per-user is injected when the ad stack is inactive. The </body> bids/adInit() script is skipped (crates/trusted-server-core/src/html_processor.rs:373-377), every integration head_inserts implementation ignores the request context and serializes config values only (gpt.rs:489, prebid.rs:1077, sourcepoint.rs:1021, didomi.rs:340, datadome.rs:768), and the injected bundle src is a content hash rather than a per-user token (crates/trusted-server-core/src/tsjs.rs:5-9). Recording this so the blocking discussion stays on the actual mechanism: origin-declared policy replacement plus variant mixing, not leaked identity in the markup.
  • No Vary is written anywhere on the publisher HTML path. crates/trusted-server-core/src/publisher.rs contains no Vary writes; the existing writers all target other response types (static/tsjs bundles, /identify, proxied GPT and Sourcepoint assets, the fingerprint debug endpoint). Only an origin-supplied Vary survives into the max-age=60 response.

👍 praise

  • The finalize_response reordering is pinned by a test that asserts ordering, which is what actually regressed (crates/trusted-server-core/src/publisher.rs:5337).
  • The narrowing tests lock the blast radius round one asked about: 206/404/500/503, POST, and non-document coverage all assert origin-policy preservation rather than just a status code.

CI Status

  • fmt: PASS
  • clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • browser + Fastly EC lifecycle integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
}

#[tokio::test]
async fn disabled_ad_templates_use_short_browser_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.

nitpick — The guide and CHANGELOG name "a disabled auction" as a structural-inactive case, but the structural cache tests cover disabled templates, unmatched slots, and an absent section — there is no direct case for [auction].enabled = falsemax-age=60. A settings variant with [auction]\nenabled = false in one of these tests would lock that documented behavior in. Not worth another round on its own.

@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

Adds a dedicated [creative_opportunities].enabled switch so publisher HTML and page-bids template delivery can be turned off independently of [auction].enabled, and applies the issue #1007 Cache-Control: max-age=60 policy to successful GET publisher documents when the server-side ad stack is structurally inactive. The structural-vs-request-scoped split is the right design and the test matrix around it is thorough. One blocking concern on the emitted directive and one question on scope.

Blocking

🔧 wrench

  • max-age=60 is shared-cacheable, not a browser-only policy: a bare max-age=60 with no private authorizes shared caches, contradicting the "browser-facing" framing in the code comment and the configuration guide. An origin no-cache on a personalized document becomes 60s of shared cacheability for any intermediary that does not read the CDN-specific headers (crates/trusted-server-core/src/publisher.rs:3054).

❓ question

  • Absent [creative_opportunities] adopts the new policy too: deployments that never configured the feature now have origin Cache-Control replaced on every successful GET HTML navigation, with no opt-out. Intended? (crates/trusted-server-core/src/publisher.rs:3033)

Non-blocking

♻️ refactor

  • Duplicated request-eligibility gate: the cache branch re-spells the request-scoped half of should_run_server_side_ad_stack; a future gate added to one will not reach the other (crates/trusted-server-core/src/publisher.rs:3037).
  • Third copy of the "already uncacheable" check: the same lowercase + contains pattern now exists in publisher.rs and twice in response_privacy.rs (crates/trusted-server-core/src/publisher.rs:3050).

🤔 thinking

  • ServerSideAdStackConfig bundles 2 of 8 gates: six positional bools remain, which is where mis-ordering actually bites (crates/trusted-server-core/src/publisher.rs:1804).
  • Rollback failure mode is documentation-only: enabled = false plus deny_unknown_fields means an older binary fails to load settings and every request fails; consider a ts config push warning alongside the guide's warning block (crates/trusted-server-core/src/creative_opportunities.rs:206).

🌱 seedling

  • Publisher-HTML body coverage: disabled_ad_templates_use_short_browser_cache_policy asserts headers only; the page-bids path has an equivalent body assertion, the publisher path does not (crates/trusted-server-core/src/publisher.rs:5278).

⛏ nitpick

  • Redundant Option walks: the new creative_opportunities local is bound and then re-derived on the next line (crates/trusted-server-core/src/publisher.rs:2708).
  • Unrelated test fixture weakened: enforce_set_cookie_cache_privacy is untouched by this PR, but its fixture lost the origin-public scenario it was written for (crates/trusted-server-adapter-fastly/src/middleware.rs:432).

👍 praise

  • Structural vs request-scoped split, with bot/prefetch/consent-denied retaining the origin policy, and a test matrix covering non-200, non-GET, non-document, and mixed-case No-Store (crates/trusted-server-core/src/publisher.rs:5433).
  • Moving gpt_diagnostics::finalize_response below the cache block so diagnostics privacy wins, pinned by a test (crates/trusted-server-core/src/publisher.rs:3060).
  • Direct /auction regression test with a probe provider, proving the PR's central premise end to end (crates/trusted-server-core/src/auction/endpoints.rs:738).

CI Status

All 19 checks pass on 38c9636.

  • fmt: PASS
  • clippy / cargo check (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
  • integration tests (browser, Fastly EC lifecycle): PASS
  • js tests (vitest) and format (typescript, docs): PASS

{
response.headers_mut().insert(
header::CACHE_CONTROL,
HeaderValue::from_static("max-age=60"),

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.

🔧 wrenchmax-age=60 is shared-cacheable, not a browser-only policy.

The comment above says this "caps browser caching" and docs/guide/configuration.md calls it the "browser-facing" policy, but a bare max-age=60 with no private also authorizes shared caches: per RFC 9111 §5.2.2.1 a 200 OK GET response absent private is storable by any cache, shared ones included.

The practical consequence: an origin that deliberately sent Cache-Control: no-cache or must-revalidate on a logged-in/personalized publisher document now has that replaced with a 60-second shared-cacheable policy. The preserved Surrogate-Control / Fastly-Surrogate-Control / CDN-Cache-Control / Cloudflare-CDN-Cache-Control headers only protect CDNs that read those headers, and only when the origin actually sets them. Any intermediary that reads only Cache-Control — a customer CDN sitting in front of the Trusted Server service, a corporate or ISP proxy — can now store and replay one visitor's HTML to another for 60s.

The existing Set-Cookie net (enforce_set_cookie_cache_privacy) covers the first-visit EC case, but a returning visitor's personalized page carries no Set-Cookie and so is not downgraded.

Fix:

response.headers_mut().insert(
    header::CACHE_CONTROL,
    HeaderValue::from_static("private, max-age=60"),
);

This matches the stated "browser-facing" intent and still delivers the #1007 TTFB win. Issue #1007 does spell max-age=60 literally, so if shared cacheability is deliberate, please state that explicitly in this comment and in the configuration guide, and note the assumption that deployments always set the CDN-specific headers.

if is_html_content_type(origin_content_type) {
if should_run_ad_stack {
enforce_synthesized_html_cache_privacy(&mut response);
} else if is_get

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.

question — should an absent [creative_opportunities] section really adopt this policy?

ad_templates_enabled is false when the section is absent, so this branch is reached for deployments that never configured creative opportunities at all — a pure proxy or EC-only service. Every successful GET HTML navigation on such a deployment now has its Cache-Control replaced with max-age=60, discarding the origin's directives wholesale: no-cache, must-revalidate, s-maxage, stale-while-revalidate, no-transform, and immutable all disappear because the header is overwritten rather than merged. absent_creative_opportunities_use_short_browser_cache_policy locks the behavior in.

There is also no opt-out. [creative_opportunities].enabled = false selects the same policy rather than restoring pass-through, and settings.response_headers is a global static override, not a per-response restore.

Is this intended for deployments that do not use the server-side ad stack at all? If so, it is worth calling out in CHANGELOG.md as an operator-visible change for non-SSAT services. If not, consider treating an absent section as "feature unavailable, pass the origin policy through" and scoping the new policy to configured-but-inactive stacks.

&& is_navigation
&& !is_prefetch
&& !is_bot
&& consent_allows_auction

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 — this re-spells the request-scoped half of should_run_server_side_ad_stack.

is_get && is_navigation && !is_prefetch && !is_bot && consent_allows_auction is exactly the request-scoped subset of the gate function, with the structural gates (ad_templates_enabled, has_matched_slots, auction_enabled) deliberately left out. The split is the right design, but the condition is now spelled twice in two different places, so a future request-scoped gate added to should_run_server_side_ad_stack will silently fail to reach this branch.

Fix: extract the shared predicate and call it from both sites.

/// Request-scoped eligibility: signals that vary per request for the same URL.
fn is_ad_eligible_navigation(
    is_get: bool,
    is_navigation: bool,
    is_prefetch: bool,
    is_bot: bool,
    consent_allows_auction: bool,
) -> bool {
    is_get && is_navigation && !is_prefetch && !is_bot && consent_allows_auction
}

.map(str::to_ascii_lowercase);
if !origin_cache_control
.as_deref()
.is_some_and(|value| value.contains("private") || value.contains("no-store"))

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 — third copy of the "already uncacheable" test.

The lowercase-then-contains("private") || contains("no-store") pattern already exists twice in crates/trusted-server-core/src/response_privacy.rs (enforce_set_cookie_cache_privacy and apply_response_headers_with_cache_privacy). Three independent copies of the same RFC 9111 §5.2 case-insensitivity reasoning can drift.

Fix: promote it to the module that already owns cache-privacy semantics.

// response_privacy.rs
/// Returns true when `Cache-Control` already forbids shared caching.
pub(crate) fn is_uncacheable_policy(headers: &HeaderMap) -> bool {
    headers
        .get(header::CACHE_CONTROL)
        .and_then(|value| value.to_str().ok())
        .map(str::to_ascii_lowercase)
        .is_some_and(|value| value.contains("private") || value.contains("no-store"))
}

/// Returns true only when the publisher request should run the full
/// server-side ad stack: auction dispatch plus initial ad-slot injection.
#[derive(Debug, Clone, Copy)]
struct ServerSideAdStackConfig {

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 new struct bundles 2 of the 8 gates, leaving 6 positional bools.

Arity is unchanged at 7, right at the CLAUDE.md cap, and the parameters that actually invite a mis-ordering bug are the six adjacent bools, not the two config flags. Call sites still read like should_run_server_side_ad_stack(true, true, false, false, true, false, enabled_config).

If the struct is being introduced anyway, folding every gate into it would make the call sites self-describing and drop the arity to one:

struct ServerSideAdStackGates {
    is_get: bool,
    is_navigation: bool,
    is_prefetch: bool,
    is_bot: bool,
    has_matched_slots: bool,
    consent_allows_auction: bool,
    ad_templates_enabled: bool,
    auction_enabled: bool,
}

Not blocking — the current shape compiles and is tested.


#[test]
fn enforce_set_cookie_cache_privacy_downgrades_late_cookie() {
fn enforce_set_cookie_cache_privacy_downgrades_inactive_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.

nitpick — this weakens an existing test that the PR does not otherwise touch.

enforce_set_cookie_cache_privacy is unchanged by this PR, but the fixture moved from public, max-age=600 to max-age=60, which drops the explicit "origin sent a public policy" scenario the test was written to cover. Both values exercise the same branch, so nothing is caught today — but the named scenario is gone.

Fix: keep the original case and add the new one, e.g. loop over ["public, max-age=600", "max-age=60"] so both the origin-public and inactive-template policies are proven to downgrade.

}

#[tokio::test]
async fn disabled_ad_templates_use_short_browser_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.

🌱 seedling — this test asserts headers only.

The page-bids path has disabled_server_side_ad_templates_return_no_slots_or_bids proving the response body carries no slots, but on the publisher-HTML path there is no equivalent body assertion that tsjs.adSlots is absent when enabled = false. The gate unit test covers the boolean, so this is defence in depth rather than a hole — worth adding when this area is next touched.

}

#[tokio::test]
async fn request_scoped_ad_stack_suppression_preserves_origin_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.

👍 praise — this is the finding that makes the whole design safe.

Separating structural inactivity (config, no matched slot, disabled auction) from request-scoped suppression (bot, prefetch, consent denial) is exactly the right cut: a request-scoped skip can produce a different representation for the same URL, so keeping the origin policy there is what stops the short policy from becoming a cross-request-class cache hazard. The matrix around it — non-200 statuses, non-GET and non-document requests, and mixed-case No-Store preservation — covers the boundaries properly.

}
}

crate::integrations::gpt_diagnostics::finalize_response(&gpt_diagnostics, &mut response);

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 — good catch moving this below the cache block.

Running gpt_diagnostics::finalize_response after the new policy is applied is what lets requires_private_no_store() win over max-age=60, and inactive_ad_stack_preserves_gpt_diagnostics_cache_privacy pins the ordering so a future refactor cannot silently reintroduce a cacheable diagnostics response. The DataDome suppression path (apply_datadome_client_tag_cache_privacy) lands after this too, so that override is preserved as well.

}

#[tokio::test]
async fn direct_auction_remains_available_when_templates_are_disabled() {

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 — this test proves the actual premise of the PR.

The whole justification for a dedicated switch instead of reusing [auction].enabled is that POST /auction must keep working. Standing up a probe provider and asserting calls == 1 verifies that end to end rather than asserting the absence of a code path, which is what makes the separation credible.

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.

Improve cache header for html content when SSAT is off

3 participants