Uh oh!
There was an error while loading. Please reload this page.
Implement server-side ad slot templates with PBS and APS auction - #680
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
Replace the head-injected __ts_bids design with a server-cached bid delivery model fetched by the client via a new /ts-bids endpoint. The auction never blocks page rendering — </head> flushes immediately, body parses without waiting for bids, and the client fetches bids in parallel with content paint. Key changes: - §2 Goal: bid delivery decoupled from page rendering; FCP unchanged from no-TS baseline - §4.3 Auction Trigger: drop buffered/streaming dichotomy; single mode forces chunked encoding on all origins (WordPress, NextJS, etc.) - §4.4 Head Injection: only __ts_ad_slots and __ts_request_id injected at <head> open; bid results moved to /ts-bids endpoint - §4.6 Client Residual: __tsAdInit defines slots immediately, fetches bids via /ts-bids, applies targeting and fires refresh() after resolve - §4.7 (new) Caching Behavior: explicit cacheability table for HTML, JS, CSS, tsjs bundle, bid results; Fastly edge HTTP cache leveraged for origin HTML - §5 Request-Time Sequence: full mermaid diagram covering content + creative + burl flow with cache-hit and cache-miss branches; separate text sequences for cache-hit (~80ms FCP, ~900ms ad-visible) and cache-miss (~250ms FCP, ~1,050ms ad-visible) - §6 Performance Summary: cache-hit and cache-miss columns; FCP added as a tracked metric - §7 Implementation Scope: add bid_cache.rs, /ts-bids endpoint, force chunked encoding step - §8 Edge Cases: origin-agnostic entries; new entries for /ts-bids 404 and client-never-fetches-/ts-bids Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pivot from the /ts-bids fetch endpoint + in-process bid_cache design to
inline __ts_bids injection before </body>. The earlier design relied on
shared state that doesn't reliably survive Fastly Compute's per-request Wasm
isolate model — body injection achieves the same FCP property in a single
response with no shared-state requirement.
Key changes:
- §4.3: replace /ts-bids long-poll with bounded </body> hold tied to
A_deadline. Body content above </body> paints first; close-tag held
until auction completes or A_deadline fires (graceful __ts_bids = {}
fallback).
- §4.3: add auction-eligibility gating (consent, bot UA, prefetch hints,
HEAD method, slot match) so auctions fire on real first-page-load
impressions only.
- §4.4: replace __ts_request_id + /ts-bids machinery with two inline
<script> blocks — __ts_ad_slots at <head> open, __ts_bids before
</body> via lol_html el.on_end_tag().
- §4.5: move both nurl and burl to client-side firing from
slotRenderEnded after hb_adid match. Server-side firing rejected to
avoid billing inflation on bids that never render.
- §4.6: replace fetch+Promise pattern with synchronous __ts_bids read.
Add lazy slim-Prebid loader (post-window.load) for scroll/refresh
auctions and Phase B identity warm-up. Add ts_initial=1 slot-ownership
sentinel.
- §4.7: switch Cache-Control from private, no-store to private,
max-age=0 to preserve browser BFCache eligibility while still
preventing intermediate-cache leaks.
- §4.8 (new): document the EC/KV identity model as load-bearing auction
input — Phase A retrieval at request time, Phase B post-render
enrichment via slim-Prebid userID modules. Add bare-EC first-impression
caveat and auction_eid_count metric. Note federated-consortium
passphrase property and clickstream-compounding speed win.
- §5: update mermaid + cache-hit/miss timelines for bounded body hold;
ad-visible converges to ~870ms (hit) / ~1,020ms (miss).
- §6: drop /ts-bids RTT row; add DCL row; add clickstream-compounding,
TS-overhead, identity-coverage, and confidence-interval framing.
- §7: drop bid_cache.rs and /ts-bids endpoint from scope; add
auction-eligibility gating and slim-Prebid bundle build target. Add
explicit "Deleted" subsection.
- §8: drop /ts-bids edge cases; add SPA/pushState, bare-EC, bot/prefetch,
HEAD, BFCache restoration cases.
- §9.6: server-side GAM downgraded from "Phase 2 commitment" to
aspirational and contingent on Google agreement. §9.8 (slim-Prebid
bundle composition), §9.9 (Privacy Sandbox), §9.10 (per-bidder consent)
added as follow-ups.
Implementation plan at docs/superpowers/plans/2026-04-30-server-side-ad-templates.md
is now stale relative to this spec; needs regenerating before code lands.…ities.toml Adds the creative_opportunities field to Settings struct to deserialize configuration for the server-side ad auction feature. Includes build.rs stubs for types required during build-time configuration validation. Creates creative-opportunities.toml with example slot configuration and updates trusted-server.toml with the [creative_opportunities] section defining GAM network ID, auction timeout, and price granularity settings. Tests pass with proper TOML parsing of the creative_opportunities section.
…ared auction state
- Add `ad_slots_script: Option<String>` and `ad_bids_state: Arc<RwLock<Option<String>>>` fields to `HtmlProcessorConfig`
- Update `from_settings` to initialize both new fields with safe defaults
- Prepend `ad_slots_script` inside the existing `<head>` handler before integration inserts
- Add `element!("body", ...)` handler that uses `end_tag_handlers()` to inject `__ts_bids` before `</body>`; falls back to empty `{}` when auction state is `None`
- Add `IntegrationRegistry::empty_for_tests()` test helper
- Add three new tests covering all injection paths…gibility gates; max-age=0 - Make handle_publisher_request async; add orchestrator and slots_file params - Dispatch origin request with send_async before running auction in parallel - Gate auction on GET, no prefetch, no bot, matched slots, TCF purpose-1 consent - Run server-side auction and write bucketed bids to ad_bids_state Arc<RwLock> - Compute ad_slots_script after response headers; set Cache-Control: private, max-age=0 - Fix Stream arm to thread actual ad_slots_script and ad_bids_state through - Add build_auction_request, build_bid_map, build_bids_script, build_ad_slots_script helpers - Update route_tests.rs to pass empty slots_file to route_request
- build_bid_map now returns serde_json::Map with full bid objects (hb_pb,
hb_bidder, hb_adid, nurl, burl) instead of a plain CPM string map
- build_bids_script / build_ad_slots_script now emit full <script> tags
using JSON.parse("…") for safe inline embedding; add html_escape_for_script helper
- build_ad_slots_script uses correct property names (gam_unit_path, div_id,
formats, targeting) matching the client-side TSJS bundle expectations
- Replace map_or(false, …) with is_some_and(…) on lines 546, 549, 567
- Add # Panics doc sections to handle_publisher_request and create_html_processor…nities.toml at startup
… from slotRenderEnded; slim-Prebid lazy loader
- Enable APS and adserver_mock in auction config; set providers and mediator - Increase auction_timeout_ms from 500ms to 3000ms — 500ms was too tight for HTTPS round-trips to mocktioneer, leaving the mediator zero budget - Fix mediation request: send numeric price instead of opaque encoded_price; mocktioneer requires a decoded price field and does not support encoded_price - Expand creative-opportunities slot page_patterns to include /news/**
Define SlotRenderEndedEvent, SlotRenderEvent, and TestWindow types to eliminate all @typescript-eslint/no-explicit-any violations in gpt/index.ts and gpt/index.test.ts. Extend GptWindow with __tsjs_slim_prebid_url so installSlimPrebidLoader avoids the any cast.
Set gam_network_id to 88059007 (autoblog production network). Update atf_sidebar_ad slot to /88059007/autoblog/news with div_id ad-atf_sidebar-0-_r_2_ (desktop ATF sidebar, 300x250); restrict page_patterns to article paths only (/20**, /news/**) since that div does not exist on the homepage. Add homepage_header_ad slot targeting /88059007/autoblog/homepage with ad-header-0-_R_jpalubtak5lb_ for 970x90/728x90/970x250 leaderboard formats. Reduce auction_timeout_ms from 3000 to 500 to cap TTFB at the spec-recommended ceiling.
Blocking: - Move tokio to [dev-dependencies] in trusted-server-core; it was only used by #[tokio::test] and was linking the runtime into the wasm prod build. Confirmed the release wasm adapter build no longer pulls tokio. - Roll back the SPA currentPath on a failed /__ts/page-bids fetch so a transient error no longer permanently strands that route (gpt/index.ts). Build/runtime parity and diagnostics: - Bound creative-opportunity format width/height to u32 range at build time so values the runtime u32 cannot hold are rejected early. - Add #[serde(deny_unknown_fields)] to the build.rs config stub to match the runtime type and reject mistyped table keys at build time. - Warn when the <body> end-tag handler is absent so a silently non-rendering server-side ad feature is diagnosable. - Log dropped slot bidders that are neither configured nor the aps provider. - Log build_bid_index collisions (multiple bids per seat/imp). JS correctness: - Narrow uid.atype to a number before the range check in sanitizeAuctionUid. - Resolve findInjectedSlotForRefresh by exact/container match before the prefix fallback, with a regression test for prefix-overlapping div_ids. - Guard the gpt_bootstrap prefix scan against an empty div_id. - Route injectAdmIntoSlot through findSlotElementByDivId for consistency. Cleanup and docs: - Remove the dead has_post_processors routing dependency from classify_response_route and (now unused) handle_publisher_request. - Extract the duplicated EID resolution/consent-gating/device tail shared by the initial-page and page-bids dispatch paths into one helper. - Anchor the surrogate cache-header list in a shared const so the legacy and EdgeZero Set-Cookie privacy paths stay aligned. - Refresh stale docs (PublisherResponse::Stream, the publisher module platform-coupling note, and UserInfo.eids consent-gate location).
Moving tokio to trusted-server-core dev-dependencies removed it from the crate's normal dependency list, so the integration-tests lockfile (which resolves core's non-dev deps) no longer pins tokio under core. Keeps `cargo --locked` green for the integration job.
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review: I reviewed PR #680 at ae9d50a against main, including the current diff, CI, and existing review threads. CI is green and I did not find a P0/blocking security or data-loss issue, but I found one high-severity compatibility/correctness issue for the non-Fastly adapters plus a few medium-risk edge cases in the new streaming and browser paths.
Review Summary
This is a very large server-side ad-template change spanning Rust adapters/core and browser GPT/Prebid code. The current Fastly legacy path appears to have the expected route wiring, consent gates, cache protection, and auction lifecycle; the highest remaining risk is adapter parity: Axum/Cloudflare/Spin accept the new config and inject the browser hook, but do not actually wire the server-side slot templates or /__ts/page-bids endpoint.
Findings
P0 / Blockers
None found.
P1 / High
- Non-Fastly adapters silently disable configured slot templates and lack the SPA page-bids endpoint —
crates/trusted-server-adapter-axum/src/app.rs:174- Issue: Axum, Cloudflare, and Spin all build an
AuctionDispatchwithslots: &[](cloudflare/src/app.rs:303,spin/src/app.rs:605have the same pattern), and none of those adapters registersGET/OPTIONS /__ts/page-bids. At the same time the shared GPT integration can still installinstallSpaAuctionHook(), which fetches/__ts/page-bidson SPA navigation. Operators can configure[[creative_opportunities.slot]]successfully, but these adapters ignore it and route the SPA fetch to the publisher origin/fallback instead of the core handler. - Why it matters: This is a cross-adapter compatibility trap. A config that works on Fastly legacy silently loses initial server-side slots/auctions on Axum/Cloudflare/Spin, and SPA navigations can make internal endpoint requests to the origin, typically producing non-JSON failures and no refreshed server-side bids.
- Suggested fix: Either wire these adapters to pass
state.settings.creative_opportunity_slots()and register the same/__ts/page-bidsGET plus fail-closed OPTIONS behavior, or fail startup / suppress the GPT SPA hook when configured slots are present on adapters where server-side templates are intentionally unsupported.
- Issue: Axum, Cloudflare, and Spin all build an
P2 / Medium
Slim-loaded Prebid misses user-ID module initialization after
window.load—crates/trusted-server-js/lib/src/integrations/prebid/index.ts:928- Issue: The GPT slim loader appends the Prebid script from a
window.loadhandler. When that deferred script executes,window.loadhas already fired, but Prebid self-init only schedulesinstallUserIdModules()via anotherwindow.addEventListener('load', ...). - Why it matters: In the slim-loaded path, the user-sync/EID warm-up setup can be skipped, so later server-side auctions lose the identity enrichment this PR is trying to preserve.
- Suggested fix: During Prebid self-init, call
installUserIdModules()immediately whendocument.readyState === 'complete'; otherwise register the load listener with{ once: true }.
- Issue: The GPT slim loader appends the Prebid script from a
Raw
</bodybyte scanning can hold the stream on non-tag text —crates/trusted-server-core/src/publisher.rs:900- Issue:
BodyCloseHoldBufferstarts the auction hold at the first case-insensitive byte sequence</body, regardless of HTML parser state. That also matches strings/comments/raw-text content such as an inline script containing"</body>"before the real closing body tag. - Why it matters: A false positive near the top of the page would hold the rest of the document behind auction collection, defeating the intended “only delay the body tail / DCL” behavior and making performance dependent on page text.
- Suggested fix: Detect the actual body end through the HTML processor/parser state, or make the scanner HTML-aware enough to ignore comments, quoted attributes, and raw-text elements. Add a regression test with
</body>inside a script before visible body content.
- Issue:
Prefix-based slot lookup can misattribute overlapping dynamic div IDs —
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:48- Issue: After exact lookup fails,
findSlotElementByDivId()returns the first DOM id that starts with the configured prefix. With overlapping configured prefixes such asad-andad-header-, DOM order can let the shorter prefix claim the longer slot’s element/iframe. - Why it matters: The render bridge then resolves the message source to the wrong slot, rejects the real bid/slot match, and can skip creative rendering or win/billing beacons for valid server-side bids.
- Suggested fix: Prefer exact/container matches across all configured slots first, then resolve prefix matches by longest prefix (or by a precomputed
divToSlotIdmap). Add an overlapping-prefix regression test.
- Issue: After exact lookup fails,
P3 / Low
None.
CI / Existing Reviews
gh pr checks 680 reports all current checks passing, including CodeQL, cargo fmt/tests, adapter checks, browser integration tests, vitest, and docs/TS formatting. I reviewed existing review comments and avoided re-reporting items that were already raised and fixed in prior passes.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
These EdgeZero-style adapters finalize buffered, and the sync
`buffer_publisher_response` drives `stream_publisher_body`, which ignores
`params.dispatched_auction` — so they injected an empty `tsjs.bids = {}`
while Fastly (legacy streaming finalize) served real bids.
- Add `buffer_publisher_response_async` in core: for the Stream variant it
drives `stream_publisher_body_async`, which awaits
`collect_dispatched_auction`, writes `ad_bids_state`, and injects the bids
before `</body>`.
- Pass the configured `creative_opportunities.slot` (not empty) to
`handle_publisher_request` on all three adapters; it matches them against
the request path internally.
- Call the async finalize from each adapter (Cloudflare/Spin via their
now-async `resolve_publisher_response`).
EID targeting stays off for now (these adapters pass `kv: None`).…ters The auction consent gate (`consent_allows_server_side_auction`) reads jurisdiction and TCF consent from the EC context. The adapters passed `EcContext::default()` to `handle_publisher_request`, leaving jurisdiction Unknown with no consent — so the gate failed closed and no auction ran (empty `tsjs.bids`), even though the slots matched. Build the context via `read_from_request_with_geo` (consent from the request, geo from the platform), mirroring the Fastly entry point, and fall back to default on a parse error. Cloudflare resolves geo from the Workers `cf` object when deployed; Axum and Spin have no-op geo providers, so on those a known non-GDPR jurisdiction requires the request to carry geo or the gate needs a TCF consent signal.
When the auction does not run, this pinpoints which gate suppressed it (slots, bot, navigation, consent, or orchestrator kill switch) instead of only seeing `dispatch_auction: None`. Pair with the EC-context jurisdiction log when consent_allows_auction is false.
The EdgeZero buffered path passed empty slots and finalized via the sync `buffer_publisher_response`, so configured creative-opportunity slots were routed to the legacy path by `edgezero_can_handle_settings`. Now that `buffer_publisher_response_async` collects the dispatched auction, EdgeZero can run the full ad stack: - Pass the configured `creative_opportunities.slot` and finalize via `buffer_publisher_response_async` (the path's `ec.ec_context` already carries consent + platform geo). EID targeting stays off (`registry: None`). - Drop the `edgezero_can_handle_settings` gate, its routing branch, the three tests, and the now-unused test settings helpers — EdgeZero handles configured slots, so the legacy fallback for them is obsolete.
The EdgeZero publisher path dispatched the auction with registry: None, so the bid request carried no KV identity-graph EIDs (only client cookie EIDs). It already passes ec.kv_graph as the identity KV, so wire the matching PartnerRegistry::from_config(settings.ec.partners) into the AuctionDispatch to resolve server-side partner EIDs — matching the legacy auction path. Fastly-only: the sync EC identity graph (KvIdentityGraph/EcKvStore) works on Fastly's sync KV; the async-KV portability adapters are unaffected (they still pass registry: None until the EC graph supports async stores).
aram356
requested changes
Jul 1, 2026
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary (fifth review pass)
Re-review at head 59683ac5. Every fourth-pass finding is resolved (tokio → dev-deps, SPA currentPath rollback, slot-resolution prefix collision, atype narrow, injectAdmIntoSlot resolver, u32::MAX format bound, build-stub deny_unknown_fields, silent-bidder-drop → log, build_bid_index collision → log, empty-div_id guard, </body> no-op → warn, EID/device block extracted, stale docs). The core (auction math, consent gating, build/runtime validation parity, XSS escaping) is solid.
This round wires the server-side auction into all adapters (Axum, Cloudflare, Spin, Fastly EdgeZero). The three blocking findings are the same shape: /auction, /__ts/page-bids, and Set-Cookie cache-privacy were not brought to parity across platforms. Inline comments carry the pinpointable non-blocking items; cross-adapter and context-line findings are below.
CI note: checks are currently pending on this push (not yet green) — not a code finding, just flagging that the automated gates haven't completed.
Blocking
🔧 wrench
POST /auctionis fail-closed-dead on Axum, Cloudflare, and Spin — all three callhandle_auction(..., &EcContext::default(), ...)(axum/app.rs:371, cloudflare/app.rs:416, spin/app.rs:509). Default → jurisdictionUnknown→consent_allows_server_side_auctionreturns false → the endpoint always returns no-bid, even for consented users. The fallback publisher path on each adapter (and both Fastly paths) correctly builds EC context viaread_from_request_with_geo. (TheEcContext::default()at these lines predates this PR, but this PR fixed the fallback path on the same adapters, leaving/auctionas the lone un-fixed entry point for the identical auction.) Fix: build EC context withread_from_request_with_geoin the/auctionhandler on all three, mirroring the fallback path a few lines away.GET /__ts/page-bids(+ itsOPTIONS→403 CSRF guard) is wired only on the Fastly legacy path — present atfastly/main.rs:1128; absent from Fastly EdgeZeroapp.rsand all three portability adapters. The shipped GPT JS fetchesGET /__ts/page-bidson every SPApushState/popstate(gpt/index.ts:690); on Axum/Cloudflare/Spin — and on Fastly when the EdgeZero rollout flag is on — it falls through to the publisher fallback, is proxied to origin (404), and SPA server-side re-auction silently doesn't happen. Fix: wireGET+OPTIONS→403 for/__ts/page-bidson the EdgeZero path and the three adapters (reusehandle_page_bids+ the same EC-context construction), or document SPA re-auction as unsupported there and gate the JS. When it is wired anywhere, it must ship theOPTIONS→403 guard too.- Set-Cookie cache-privacy hardening added only to the Fastly
apply_finalize_headers— Fastlymiddleware.rs:225hasenforce_set_cookie_cache_privacy+ the uncacheable-operator-header guard; Axum/Cloudflare/Spinmiddleware.rsstill apply operatorresponse_headers(incl.Cache-Control/Surrogate-Control) with no cookie check. On Cloudflare (a real shared cache), an operator/originCache-Control: publicon a cookie-bearing response would be served as-is. Blast radius is limited today (portability adapters don't yet emit the ECSet-Cookieon the fallback path), but any origin/filterSet-Cookieis still mis-cacheable. Fix: hoistenforce_set_cookie_cache_privacy+ the guard into a shared helper (byte-identical logic overedgezero_core::http::Response) and call it from every adapter'sapply_finalize_headers. (This also collapses the note-level duplication that already exists between Fastly'smain.rs:1382andmiddleware.rs:271.)
Non-blocking (context-line or cross-cutting; pinpointable items are inline)
❓ question
- Regular-provider parse diverges across paths — the collect path calls
provider.parse_response_with_context(orchestrator.rs:353) whilerun_providers_parallelcalls plainparse_response(:544). The PR aligned the mediator parse across paths, but non-mediator providers now differ: a future provider overridingparse_response_with_contextgets context on the collect (publisher) path but not on the parallel (/auction, page-bids) path. Switch:544toparse_response_with_contexttoo, or document that only mediators may override it.
🤔 thinking
- Collect select-loop lacks the parallel path's in-loop deadline check —
orchestrator.rscollect_dispatched_auctionunconditionally drains every remaining handle (relying on per-backend transport timeouts), whilerun_providers_parallelbreaks onceelapsed >= deadline. If a backend timeout fails to fire promptly, collect can block past A_deadline and extend TTFB. Add the same post-iteration deadline guard (keeping already-collected responses) for symmetry.
🌱 seedling
handle_auctionbuffers the full body before the post-read size check —endpoints.rs:135into_bytes()materializes the whole body before the> MAX_AUCTION_BODY_SIZEcheck; the Content-Length pre-check covers honest clients, but a lying/omitted-length chunked body isn't peak-bounded. Use a capped streaming read if the platform allows.spa_hook.test.tsdoesn't cover thecurrentPathrollback — the 4th-pass fix (retry a path after a failed load) has no regression test; a droppedcurrentPath = previousPathline would still pass. Add a two-navigation test (fail then succeed on the same path).[debug]table header is now uncommented intrusted-server.toml— inert (all flags default false and the two new ones stay commented), but the section header is now live in the checked-in default.- No test for the
u32::MAXformat-dimension rejection — the 4th-pass fix (width = 5_000_000_000→ build error) is untested; add a case to lock it in.
📝 note
- Dead
pubsyncbuffer_publisher_response—publisher.rs:465; all four adapters usebuffer_publisher_response_async. Beingpub, no dead-code warning catches it, and it carries the "ignoresdispatched_auction, always-empty bids" footgun.#[deprecated]or remove it.
⛏ nitpick
- Placeholder
Requestunwrap_or_elsecontradicts the downstreamdebug_assert_eq!—publisher.rs:687/:1130fall back to a default-URIRequestthat would tripmake_collect_context'sdebug_assert_eq!(uri == MEDIATOR_PLACEHOLDER_URL). Since the URL is a compile-time const the builder never fails; prefer.expect("MEDIATOR_PLACEHOLDER_URL should be a valid URI")(and factor the two copies into one helper).
📌 out of scope
- Server-side KV EID enrichment is Fastly-only — Axum/Cloudflare/Spin pass
Noneforkv/registry(no KV identity store in theirAppState). By design, and client cookie EIDs are still resolved + consent-gated on every adapter, so no adapter forwards unconsented EIDs. Worth a one-line doc note.
CI Status
- Checks are PENDING at review time (recent push). Not verified green.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Resolve the fifth code-review pass. The blocking findings were all cross-adapter parity gaps in the server-side auction: - Build the geo-aware EC context in the /auction handlers on Axum, Cloudflare, and Spin. They passed EcContext::default(), leaving jurisdiction Unknown and failing the consent gate closed even for consented users. A shared per-adapter build_ec_context helper now serves /auction, page-bids, and the publisher fallback, and logs (rather than swallows) a malformed-consent read error. - Wire GET /__ts/page-bids and its OPTIONS->403 CSRF guard on the Fastly EdgeZero path and all three portability adapters, reusing core handle_page_bids and a shared page_bids_preflight_denied() helper. Previously it was Fastly-legacy-only, so SPA re-auction silently fell through to the origin on every other path. - Add trusted_server_core::response_privacy with the Set-Cookie cache-privacy downgrade and the uncacheable-operator-header guard, and call it from every adapter's apply_finalize_headers so a shared cache (Cloudflare) can no longer serve an operator/origin public Cache-Control on a cookie-bearing response. Also address the inline and non-blocking findings: warn on a dropped dispatched auction for bodiless responses, extract build_slot_json shared by the initial-page and page-bids paths, use creative_opportunity_slots() everywhere, drop the PBS id->ad_id fallback, log APS slot-id collisions, align the parallel provider parse with the collect path, remove the dead sync buffer_publisher_response, factor the mediator placeholder request, drop the unused toml dependency, guard MediaType against a future serde(default), and document the Fastly-only KV EID enrichment. JS: dedup win/billing beacons across concurrent renders, add the SSR guard to installSlimPrebidLoader, and short-circuit waitForSlotElements on an already-aborted signal. Add regression tests for the currentPath rollback and the u32::MAX format-dimension rejection.
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #680 at 9c0c4249133029c1ee03fbcb562ba98dbac7cdc3 against main, focusing on the current server-side ad-template implementation, auction/privacy gates, cross-adapter route parity, and GPT/Prebid browser flows after the latest fix commits. I did not find any new blocking correctness, security, data-loss, authorization, or severe compatibility issues beyond the existing review threads.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
None newly reported in this pass.
P3 / Low
None newly reported in this pass.
CI / Existing Reviews
gh pr checks 680 reports all current checks passing, including fmt, clippy/test jobs, integration/browser tests, vitest, CodeQL, and formatting checks. I also reviewed the existing review/comment history and avoided re-posting duplicate inline feedback; recent author replies indicate the previously raised adapter parity, cache-privacy, page-bids, EID, and several JS race/cleanup findings have been addressed at the current head.
Adopt main's ts-CLI config model over the branch's build-time embedding: drop the build.rs config generation and [build-dependencies], accept the trusted-server.toml deletion, remove the now-orphaned creative_slot_build_check module, and migrate the [creative_opportunities] example into trusted-server.example.toml. Slot validation is preserved via Settings::prepare_runtime, which the CLI runs at config push time. Adopt main's refactored EdgeZero finalize path in the Fastly adapter and centralize the per-user Set-Cookie cache-privacy guard in send_edgezero_response so it covers every EdgeZero send path. Fix three core test helpers for main's Body::into_bytes -> Option change.
ChristianPavilonis
approved these changes
Jul 2, 2026
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Summary
💯 Approving to keep this moving, and leaving the findings inline using the CREG emoji guide. The inline 🔧 notes are the follow-ups I would address before relying on these paths heavily in production.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Stop forwarding client-supplied X-Forwarded-For to Prebid Server; synthesize it from the platform-attested client IP instead - Make the APS slot ID remapping request-scoped by threading the AuctionRequest through parse_response_with_context, removing the shared provider-instance mutex that concurrent auctions could race - Guard dispatch_auction against multi-provider fan-out on platforms whose HTTP client executes requests sequentially, mirroring run_providers_parallel - Reject negative and non-finite creative-opportunity floor prices at config validation - Re-run the Set-Cookie cache-privacy downgrade after operator response headers are applied so configured Set-Cookie plus public cache headers cannot produce a shared-cacheable response - Install Prebid user ID modules immediately when the bundle loads after window.load (slim-Prebid lazy path), with a once-only listener - Drop allow-same-origin from debug ADM fallback iframes and validate extracted iframe src schemes to http(s) only
aram356
approved these changes
Jul 6, 2026
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary (sixth review pass — approving)
Re-review at head a92e7b24. This round is a clean response to the fifth-pass review: all three blocking cross-adapter findings and every non-blocking item are resolved, CI is green across all adapters, and no new blocking issues were found. Approving with a set of optional follow-ups.
Fifth-pass blocking findings — all resolved ✅
POST /auctionfail-closed-dead on Axum/Cloudflare/Spin → all four adapters now build a geo-aware EC context viaread_from_request_with_geo(build_ec_context/build_ec_request_state) beforehandle_auction; covered by cross-adapterparity.rs.GET /__ts/page-bids+OPTIONS→403 wired only on Fastly legacy → now wired on the Fastly EdgeZero path and Axum/Cloudflare/Spin, reusing the sharedhandle_page_bidsand the core same-origin/X-TSJS-Page-Bidsgate +page_bids_preflight_denied().- Set-Cookie cache-privacy hardening Fastly-only → hoisted into the new shared
response_privacy.rs(two-pass enforce, surrogate stripping, operator-header guard) and called from every adapter's finalize path. The module is well-tested and portable (wasm32-unknown-unknown safe).
Fifth-pass non-blocking findings — all addressed ✅
Parse-path divergence aligned (parse_response_with_context on both paths); collect-loop deadline documented; body-size an explicit tested 413/BadRequest contract; MediaType default warned against #[serde(default)]; build_slot_json extracted; bodiless-auction log::warn! added; dead sync buffer_publisher_response removed; placeholder-request factored into mediator_placeholder_request() with .expect(...); JS beacon double-fire closed via an in-flight renderingAdIds gate; currentPath rollback test added; installSlimPrebidLoader window guard + waitForSlotElements abort short-circuit added. The +362 APS rework correctly migrated off per-instance Mutex state to context-based build_slot_id_map, with a new out-of-order concurrency regression test and a duplicate-slotID collision log — no new bugs.
Optional follow-ups (non-blocking)
🤔 thinking
- Cloudflare doesn't register non-primary methods on named routes to the publisher fallback —
cloudflare/app.rs:340-548. Axum/Spin/Fastly loop everypublisher_fallback_methodonto each named route; Cloudflare omits it, soPOST /__ts/page-bids,HEAD /auction, etc. return a router-level 405 on Cloudflare vs proxied on the other three. The three security routes (auction POST, page-bids GET, page-bids OPTIONS) are all explicitly registered, so this is not security-affecting — but it's a genuine cross-adapter routing inconsistency from the named-route refactor. Mirror the per-route fallback loop or document the difference. - (inline) SPA
currentPathrollback to an aborted intermediate path —gpt/index.ts:740.
🌱 seedling — coverage & robustness
- No
/__ts/page-bidsroute tests on Axum/Cloudflare/Spin — neither the GET route nor theOPTIONS→403 reject is asserted per-adapter (only Fastly does). Wiring is verified-correct by reading and the gate is tested in core, so this is coverage, not a defect. Add aparity.rsassertion thatOPTIONS /__ts/page-bidsis 403 on every adapter. - The concurrent render-bridge double-fire fix (
renderingAdIds) is untested —ad_init.test.ts:~917. The existing test resolves the first fetch before dispatching the second same-adId message, so it exercises the persistentfiredBeaconsdedup, not the new in-flight gate. Add a test with a deferredfetchstub that dispatches two same-adId messages before resolving, asserting one fetch and two beacons. - No
u32::MAXformat-dimension rejection test for the build-time bound. - Streaming-body rejection in
handle_auctionis incidental —endpoints.rs:135relies on empty-parse-failure of aBody::Stream; the pinned edgezero rev now exposesBody::into_bytes_bounded(max)for an explicit bounded read that would surviveAdRequestever gaining defaultable fields. - (inline) Dispatch guard keys off configured provider count —
orchestrator.rs:782. - APS advertises
Videoinsupports_media_typebutto_aps_requestsends only Banner sizes —aps.rs:702vs:365; a video-only slot is sent with emptysizes. Pre-existing (identical at base), out of scope for this PR — worth a separate follow-up.
♻️ / 📝 / ⛏ minor
- (inline) Duplicated 403 construction in the page-bids GET gate —
publisher.rs:1988. firedBeaconsdoc key is stale (saysslotId|bidIdentity, actual key is 4-partslotId|bidIdentity|kind|url) —core/types.ts:73.- Comment "Reject oversized bodies before any allocation" overstates the core guard — the Fastly adapter already fully buffers the body in
compat.rsbefore the handler; soften to "before core buffers/parses" —endpoints.rs:113. - Stale comment referencing the removed sync
buffer_publisher_response—fastly/app.rs:721. - (inline)
tokiostill a dev-dep for two#[tokio::test]s that could useblock_on—endpoints.rs:559.
CI Status
- fmt / clippy / cargo test (all adapters incl. cross-adapter parity) / vitest / CodeQL / integration / EdgeZero integration: all PASS.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Roll SPA page-bids navigation `currentPath` back to the last applied path instead of the immediately-previous one, so an aborted-then-failed navigation can no longer strand a route behind the no-op guard; add a regression test. - Add a concurrent render-bridge test: two same-adId messages before the cache fetch resolves must collapse to one fetch (in-flight gate), two beacons. - Assert `OPTIONS /__ts/page-bids` is denied with 403 on every adapter (Axum/Cloudflare/Spin) in cross-adapter parity. - Add a `u32::MAX` banner-format test covering the imp-drop branch when all formats exceed `i32::MAX`. - Dedup the page-bids GET 403 into `page_bids_preflight_denied()`. - Fix stale comments/docs: `buffer_publisher_response_async`, soften the oversized-body comment, and correct the `firedBeacons` key doc.
Resolve#744 (legacy entry-point removal + streaming publisher) against the server-side auction: keep the buffered auction path on all adapters, supersede the streaming publisher for publisher navigation. Verified: fastly/core/spin/ axum/cloudflare tests + clippy + fmt clean.
Uh oh!
There was an error while loading. Please reload this page.
ChristianPavilonis pushed a commit
that referenced
this pull request
Jul 7, 2026
- Fix examnple.com typo in sourcepoint.rs test fixture - Guard SPA navigation race: check inflight controller identity after await res.json() before writing __ts_ad_slots/__ts_bids - Extract build_empty_bids_script() helper; html_processor.rs now calls it instead of duplicating the inline literal - Add invariant comment to unreachable None branch of prepend_auction_debug_comment - Cap parse_ts_eids_cookie to 32 eids / 32 uids per eid; log and return None when exceeded - Add #[serde(deny_unknown_fields)] to openrtb::Eid and Uid - Add #[serde(deny_unknown_fields)] to CreativeOpportunitiesFile and CreativeOpportunitySlot - Log debug message when adserver_mock crid does not match <bidder>-creative convention - Skip zero-dimension bids in adserver_mock with debug log - Fail closed in APS parse_aps_slot on malformed size string instead of producing 0x0 bid
prk-Jr added a commit
that referenced
this pull request
Jul 8, 2026
Main squash-merged the same server-side ad-templates feature (#680), then added Tinybird auction telemetry (#818), edgezero v0.0.4 pins (#862), and external first-party Prebid bundle loading (#743). Resolutions: - Files where this branch matched the impl branch tip take main's version (telemetry/prebid-bundle additions on top of identical feature code): orchestrator, endpoints, openrtb, prebid integration, fastly app, .gitignore, prebid docs - creative_opportunities.rs keeps this branch's version (adds the shared ad-stack gate and pattern machinery used by the ts CLI; main's copy was identical minus those additions) - publisher.rs takes main's version with this branch's should_run_server_side_ad_stack delegation to evaluate_ad_stack_gate re-applied - run.rs keeps both sides' CLI tests (ad-templates/audit + prebid bundle)
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[creative_opportunities]intrusted-server.toml. Matching slots are selected from the incoming document URL at the edge, andwindow.tsjs.adSlotsis injected at<head>open so initial GPT setup does not need a separate slot-discovery request.window.tsjs.bidsbefore</body>.window.tsjs.adInitGPT runtime path. It readswindow.tsjs.adSlotsandwindow.tsjs.bidssynchronously, defines or reuses GPT slots, applies slot-level andhb_*targeting, setsts_initial=1, refreshes the initial slots, and firesnurl/burlonly afterslotRenderEndedconfirms the TS bid won viahb_adid.[integrations.prebid].suppress_nurl_bidders, while retaining the deployment-wide[integrations.prebid].suppress_nurlcompatibility switch.window.tsjs.adSlots/ GPT metadata instead of placeholder refresh sizes.ts-eidscookie written by TSJS, gates EIDs by consent, and forwards them as OpenRTBuser.ext.eidsto PBS.ClientInfointo the server-side PBS request so bidders see the browser client context rather than the Fastly edge context.GET /__ts/page-bidshook, which updateswindow.tsjs.adSlots/window.tsjs.bidsand re-runswindow.tsjs.adInit()afterpushState,replaceState, orpopstatenavigations.What the server-side auction sends to PBS
user.idts-eccookie or generated EC identityuser.consent/user.ext.consentuser.ext.eidsts-eidscookie plus EC identity resolutionuser.ext.ec_freshdevice.uaUser-Agentheaderdevice.ipClientInfo.client_ipdevice.geodevice.dnttrueif setDNT: 1headerdevice.languageAccept-Languageheadersite.domain/site.pagesite.refRefererheaderregs.gdpr/regs.us_privacy/regs.gppimp.*[creative_opportunities]slot templates and Prebid configtmax[creative_opportunities].auction_timeout_ms, falling back to[auction].timeout_msChanges
trusted-server.toml[creative_opportunities]slot templates and documents Prebid nurl suppression knobs..env.example/ docsSUPPRESS_NURL_BIDDERS.crates/trusted-server-core/src/creative_opportunities.rscrates/trusted-server-core/src/settings.rs/build.rstrusted-server.toml.crates/trusted-server-core/src/publisher.rswindow.tsjs.adSlotsandwindow.tsjs.bids, applies cache-control safeguards, handles page-bids responses, and forwards client context.crates/trusted-server-core/src/html_processor.rstsjs.bidsinjection.crates/trusted-server-core/src/auction/*crates/trusted-server-core/src/integrations/prebid.rsnurl/burlpropagation, and per-bidder suppression.crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/gpt.rs/gpt_bootstrap.jswindow.tsjs.adInitbootstrap path.crates/js/lib/src/integrations/gpt/index.tswindow.tsjs.adSlots,window.tsjs.bids, render-confirmed beacon firing, SPA updates, slim-Prebid loading, and Prebid creative rendering bridge.crates/js/lib/src/integrations/prebid/index.tscrates/trusted-server-adapter-fastly/src/main.rsCloses
Closes#677
Closes#697
Closes#698
Closes#699
Closes#700
Closes#702
Test plan
Automated
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkgit diff --checkcd crates/js/lib && node build-all.mjscd crates/js/lib && npm run lintcd crates/js/lib && npm run formatcd docs && npm run formatcd crates/js/lib && npx vitest runcurrently fails before test discovery in this workspace withERR_REQUIRE_ESMfromhtml-encoding-snifferrequiring@exodus/bytes/encoding-lite.js; no local JS test assertions execute under that failure mode.Manual end-to-end (browser DevTools console)
The steps below build on each other. Use a URL whose path matches one of the configured
[[creative_opportunities.slot]]page_patterns.Step 1 - Verify slot config is injected at
<head>openExpected: an array of slot objects. Each entry has
id,gam_unit_path,div_id,formats, andtargeting. Note thediv_idvalue from one matching slot for step 3.Step 2 - Verify server-side auction results are injected before
</body>Expected: an object keyed by slot ID. Winning slots include
hb_bidder,hb_pb, and, for Prebid cache-backed bids,hb_adid/ cache fields.Step 3 - Verify
window.tsjs.adInitwired GPT targetingReplace
SLOT_DIV_IDwith thediv_idfrom step 1.Expected: the slot has the configured GAM unit path and targeting includes
hb_pb,hb_bidder, any slot-level keys such aspos/zone, andts_initial: ["1"].Step 4 - Verify slot matching is page-pattern-aware
Navigate to a different configured path, for example
/when homepage slots are configured, and repeat step 2.Expected:
window.tsjs.bidsis keyed by the slots matching that page, not by slots from the previous page type.Step 5 - Confirm no duplicate bids injection
View page source and search for
.bids=JSON.parse.Expected: exactly one
window.tsjs.bidsassignment before</body>on pages where an auction ran.Pending (GAM line items required)
Creative delivery requires standard GAM line items targeting
hb_pb,hb_bidder, and related Prebid keys. That setup is outside this PR.Checklist
unwrap()in production codelogmacros instead ofprintln!