Render SSAT winning creative inline without PBS Cache round trip#899
Render SSAT winning creative inline without PBS Cache round trip#899prk-Jr wants to merge 11 commits into
Conversation
- Drop render_adm param: always include adm when creative present
- Gate GAM-bypass on per-bid debug_bid instead of a global window.tsjs flag
(removes SPA-staleness edge case, TsjsApi change, and whole flag-emit task)
- Correct fallback scope: cache fallback only when adm absent; render failure
after adm is supplied is not detectable
- Qualify sandbox claim: TS guarantees script-context escaping; bridge frame
isolation depends on the Prebid Universal Creative
- Reconcile with existing ad_init.test.ts coverage (rename, no duplicates)
- Fix test assertion messages to expect("should ..."); list exact clippy gates
- Task 3: assert observable DOM (GAM iframe src) instead of spying on the module-private injectAdmIntoSlot - Use existing make_bid helper + set .creative (make_test_bid_with_creative does not exist on this branch) - Hostile-adm regression test covers both U+2028 and U+2029 - Add 'cd docs && npm run format' to verification
build_bid_map now always inserts the winning creative as adm so the pbRender bridge can render it locally (no PBS Cache round trip); the verbose debug_bid blob and the GAM-bypass gate stay behind inject_adm_for_testing. Rename the param include_adm -> include_debug_bid and thread it through write_bids_to_state. Reconcile the by-default test to the new behavior, drop the now-redundant debug-only-adm test, and pin script-context escaping for a hostile adm (</script> + U+2028/U+2029).
The direct GAM-replace path (injectAdmIntoSlot) now fires only when the bid carries debug_bid, which is present only under inject_adm_for_testing. In production the always-present adm is rendered by the pbRender bridge and GAM stays in the loop. Add observable-DOM tests (GAM iframe src unchanged without debug_bid, rewritten with it), strengthen the bridge test to prove inline adm is preferred even when cache coords are present, and rename debug-adm terminology to inline adm.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Review summary
The inline adm path is a useful latency improvement, and the per-bid debug gate is a good way to avoid stale SPA-global state. I am requesting changes for two production contract gaps before this ships:
- The new SSAT path bypasses the existing creative sanitization, URL-rewrite, and size-limit boundary.
- The advertised PBS Cache fallback forwards a cached bid JSON document as creative markup instead of extracting its
adm.
Additional non-blocking finding
♻️ Update the public contracts to match the new semantics. Several comments outside this diff still say adm is present only under inject_adm_for_testing: Settings::inject_adm_for_testing, AuctionBidData.adm, and the GPT integration comments at lines 279–285 and 825–835. Please update them to distinguish production inline adm from the testing-only direct GAM replacement gated by debug_bid.
Verification
- Reviewed all five changed files against base
0e5dbb211da5e61598681df3abfda239484afa5f. - Current GitHub CI is green (19 reported checks).
- Focused GPT Vitest suite: 26/26 passing.
- Focused Rust regressions: 2/2 passing.
git diff --check: clean.- Local
cargo test-fastlycompiled, but Viceroy could not access the macOS certificate keychain (No keychain is available); the corresponding GitHub Fastly job passes, so this is a local environment limitation rather than a PR failure.
…code cached bids The inline-adm path fed raw bid.creative into window.tsjs.bids, bypassing the creative-processing boundary the /auction path applies (sanitize_creative_html then rewrite_creative_html, which also enforces the 1 MiB creative cap). Run the same boundary in build_bid_map before inserting adm; omit adm when the creative is rejected so the bridge falls back to the PBS Cache coordinates. Thread &Settings through build_bid_map and write_bids_to_state for rewrite_creative_html. The pbRender bridge's PBS Cache fallback forwarded the raw GET body as the ad, but PBS Cache returns a JSON bid object and the Prebid Universal Creative renders bidObject.adm. Add extractCachedAdm to parse the cached bid and extract its adm, keep raw-markup compatibility, and decline to render when no adm is present. Add hostile and oversized creative coverage in Rust, and realistic returnCreative=false, malformed, and raw-markup cache-response coverage in JS. Update the inject_adm_for_testing, AuctionBidData, GPT bridge, and design-spec docs to distinguish production inline adm from the testing-only debug_bid path.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed against base 0e5dbb211, focusing on 1c983027c and the two blocking findings from the last round.
The PBS Cache decode finding is resolved: extractCachedAdm mirrors PUC's bidObject.adm extraction, keeps raw-markup compatibility, declines on malformed payloads, and the new tests use realistic returnCreative=false shapes. The doc updates also resolve the earlier contract-drift finding.
The sanitize/rewrite finding is only half resolved. Sanitization and the 1 MiB cap are correctly applied, but rewrite_creative_html was designed for creatives rendered from the first-party origin, and reusing it here rewrites the inline adm into root-relative URLs that will not resolve in the context the pbRender bridge hands them to. Details inline on publisher.rs:2003.
Blocking
🔧 wrench
- Rewritten inline
admemits root-relative URLs that will not resolve in PUC's render context (crates/trusted-server-core/src/publisher.rs:2003)
Non-blocking
🤔 thinking
- The rewriter injects the full tsjs bundle into every creative (
crates/trusted-server-core/src/publisher.rs:2002) - Declining after
stopImmediatePropagation()leaves the slot blank (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:967) - The 1 MiB cap is per-creative, not aggregate.
write_bids_to_stateinlines the creative for every winning slot into the page's<script>on every page load, so N slots can inline up to N MiB with no aggregate bound. Per this PR's own test plan, GAM usually awards its own demand and the bridge never fires — in that common case the inlined bytes are pure document-weight/TTFB cost. Worth considering an aggregate cap, or a narrower condition for inlining.
♻️ refactor
- No test covers the rewrite half of the boundary (
crates/trusted-server-core/src/publisher.rs:3967)
📌 out of scope
- Please do file the PUC tracking issue you offered. Deferring the PUC version/template pin was reasonable on its own terms, but the blocking finding above makes it load-bearing: the render context that pin would establish is exactly what determines whether the inline
adm's rewritten URLs resolve. Confirming the GAM-served PUC bundle and adding a browser contract test against it would settle both that finding and the earlierrendererVersion/renderer-field question.
CI Status
- fmt: PASS (GitHub)
- clippy: PASS (GitHub)
- rust tests: PASS (GitHub;
cargo test, axum native, cloudflare, spin, parity, CLI all green). Localpublisher::tests84/84 pass. - js tests: PASS (GitHub vitest; local GPT suite 77/77 pass)
- All 19 reported checks are green.
The inline `adm` in window.tsjs.bids is rendered by the Prebid Universal Creative inside GAM's iframe (f.srcdoc = d.ad), a foreign origin. rewrite_creative_html emitted root-relative /first-party/... URLs that resolve against GAM's origin there and 404, and injected the tsjs bundle into every creative iframe. Add rewrite_inline_creative_html: emits absolute https://<publisher.domain>/first-party/... URLs (resolve regardless of the document base URL or whether PUC honors the custom renderer) and skips the tsjs bundle injection. Route build_bid_map's inline adm through it; the first-party /auction path keeps the root-relative rewriter. One shared rewriter via base_origin/inject_tsjs. Add tests for the absolute-URL shape and absent tsjs injection at the creative and build_bid_map boundaries.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the current head after removing the earlier first-party iframe-proxy blocker. The previous raw-creative, cache-JSON, root-relative URL, and unwanted TSJS-injection findings are resolved. CI is green, but the new default inline-render path still has three high-confidence compatibility/correctness gaps, plus one cache-fallback contract gap and stale design records. Details are inline.
| /// core-plus-integrations bundle into every creative iframe is pure weight. | ||
| #[must_use] | ||
| pub fn rewrite_inline_creative_html(settings: &Settings, markup: &str) -> String { | ||
| let base_origin = format!("https://{}", settings.publisher.domain); |
There was a problem hiding this comment.
🔧 High: build inline URLs from the actual request origin.
This hard-codes https://{settings.publisher.domain}, discarding the incoming scheme, subdomain, and port. publisher.domain cannot contain a port, deployments may receive traffic on a subdomain, and Axum/Viceroy development runs over HTTP with a port. Affected creatives therefore request proxy/click endpoints from the wrong origin, with no render-time fallback.
Please pass the trusted request origin through build_bid_map and write_bids_to_state. Initial navigation already retains request_scheme/request_host, and SPA page-bids already has RequestInfo. Add coverage for HTTP localhost with a port, a request subdomain differing from publisher.domain, and a non-default HTTPS port.
| if let Some(ref raw_creative) = bid.creative { | ||
| let sanitized = crate::creative::sanitize_creative_html(raw_creative); | ||
| let adm = crate::creative::rewrite_inline_creative_html(settings, &sanitized); | ||
| if !adm.is_empty() { |
There was a problem hiding this comment.
🔧 High: include the winning creative dimensions with production adm.
The bridge currently sizes every response from slot.formats[0], while the actual winning Bid.width/height are emitted only inside testing-only debug_bid. For a multi-size slot, a 300×250 winner can therefore be rendered as another configured size, causing clipping, whitespace, or broken layout.
Please emit winning width and height in the normal bid map and AuctionBidData, then prefer those values in the inline response. A regression where the winner matches the second configured format would pin the contract.
| // in which case the entry is omitted and the bridge falls back to | ||
| // the PBS Cache coordinates. | ||
| if let Some(ref raw_creative) = bid.creative { | ||
| let sanitized = crate::creative::sanitize_creative_html(raw_creative); |
There was a problem hiding this comment.
🔧 High: resolve ${AUCTION_PRICE} before sanitizing, rewriting, and signing.
The new inline path forwards adm without PUC-equivalent auction-price preparation. URL rewriting serializes query pairs, turning ${AUCTION_PRICE} into %24%7BAUCTION_PRICE%7D; the JS bridge then forwards the markup unchanged. Tracking or billing URLs can consequently receive a literal/encoded macro instead of the clearing price, and substitution after signing would invalidate the signed value.
Please expand the macro from the exact winning CPM before sanitize_creative_html and rewrite_inline_creative_html. Add tests for both ordinary markup and an absolute URL query containing the macro, asserting neither literal nor encoded macro survives.
| * JSON payload carries no usable string `adm` (missing or malformed), so the | ||
| * caller can decline to render instead of injecting a serialized object. | ||
| */ | ||
| export function extractCachedAdm(body: string): string | undefined { |
There was a problem hiding this comment.
🤔 Medium: preserve the cached bid fields needed for rendering.
Reducing the PBS Cache object to adm discards its price, width/height or w/h, and wurl. The fallback then uses the first slot format and performs no cached-price macro preparation. PUC's cache path uses those fields, so fallback creatives can be mis-sized, retain unresolved price macros, or omit the cached win notification.
Please decode a validated cached-bid structure rather than only a string, retaining price, dimensions, and notification metadata. Prepare the creative before responding and deduplicate notifications; keep raw-markup compatibility as a separate response variant. A realistic cache-object test should cover these fields.
|
|
||
| - **Always** insert `adm` for a winner when present — there is no runtime reason | ||
| to withhold it, so it is not parameterized. The creative is first run through | ||
| the **same sanitize/rewrite boundary as the `/auction` path** (`auction::formats`): |
There was a problem hiding this comment.
📝 Low: reconcile the design and implementation plan with the final implementation.
The spec still names rewrite_creative_html, although the code now relies on rewrite_inline_creative_html with absolute URLs and no TSJS injection. The accompanying plan also omits changed components, shows obsolete signatures/raw-adm insertion, and leaves every task unchecked.
Please update the design to describe the final inline rewriter and render metadata requirements, and reconcile the plan as completed or mark it superseded.
3 participants
Summary
admis now always included in
window.tsjs.bids, so the existing render bridgeserves it locally when GAM's Prebid line item fires — eliminating the
render-time PBS Cache round trip and matching how client-side
/auctionrenders from the copy the browser already holds.
hb_pb, andhb_cache_host/hb_cache_pathremain as the fallback for an absentadm.No GAM bypass in production.
debug_bidblob stays behindinject_adm_for_testing. TheGAM-bypass path (
injectAdmIntoSlot) is now gated on the per-biddebug_bidfield instead of
bid.adm— no new global flag, noTsjsApichange, correctacross SPA auction responses.
Changes
crates/trusted-server-core/src/publisher.rsbuild_bid_mapalways insertsadmwhenbid.creativeisSome; paraminclude_admrenamed toinclude_debug_bid(now gates only thedebug_bidblob);write_bids_to_stateparam renamed to match. Added a hostile-admregression test proving</script>/U+2028/U+2029cannot break out of the emitted<script>.crates/trusted-server-js/lib/src/integrations/gpt/index.tsbid.adm && bid.debug_bid; render-bridge log message updated to "from inline adm".crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.tsdebug_bidis present); renamed "debug adm" terminology to "inline/local adm"; bridge fixtures now carry bothhb_cache_*coords and inlineadm, proving the bridge prefers localadm.docs/superpowers/specs/2026-07-13-ssat-render-inline-creative-design.mddocs/superpowers/plans/2026-07-13-ssat-render-inline-creative.mdCloses
Closes #898
Test plan
cargo test-fastly && cargo test-axum(alsocargo test-cloudflare && cargo test-spin— all green)cargo clippy-fastly && cargo clippy-axum(onlyclippy-fastlyrun locally; full matrix delegated to CI)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(407 passed)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1(delegated to CI)fastly compute serveadmnow present inwindow.tsjs.bidsfor all slots. The full inline-render path (GAM awards the Prebid line item → creative renders with no PBS Cache fetch) is not yet exercised locally — mocktioneer's $0.20 bid loses to GAM's own demand, so GAM serves its own creative and the bridge never fires. Requires a winning Prebid line item in GAM to verify end-to-end.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — no new Rust logging introduced