Fix APS PUC rendering and collapsed GAM shells - #998
Conversation
APS bids are bid-by-reference: interpretResponse sets the renderer descriptor on the Prebid bid as the custom trustedServerRenderer field, and a bidResponse listener registers it in window.tsjs.apsPrebidRenderers keyed by Prebid's generated adId so the Universal Creative can later request it. Prebid normalizes each bid into its own object during addBidResponse and drops unknown top-level fields, so the custom field can be gone before the bidResponse listener runs (observed in production: absent as early as bidAccepted). The listener then saw renderer === undefined and returned without registering, leaving the registry empty; the Universal Creative's request found nothing and Prebid's default renderer threw "Missing ad markup or URL" (reason noAd) for every APS bid. Carry the descriptor in bid.meta as well. meta is a first-class field Prebid preserves through normalization (bidderFactory assigns bid.meta onto the normalized bid), and it is per bid, so multiple APS bids on the same imp each keep their own descriptor. The bidResponse listener falls back to the meta copy when the top-level field is absent and scrubs both copies unconditionally after the registration attempt, keeping the executable capability only in the bounded one-time registry. Adds unit tests for registration via meta with the top-level field stripped, distinct renderer registration for multiple APS bids on one imp, and that a stripped bid carrying no descriptor registers nothing.
- Register and scrub the APS renderer descriptor on bidAccepted, the first event after Prebid assigns adId, so bidResponse and analytics consumers of later events never observe the meta copy; the bidResponse pass remains as a fallback that no-ops once the bidAccepted pass has scrubbed. - Guard the meta read with a narrow object check so a module overwriting meta with a non-object value cannot break registration or scrubbing. - Consolidate the dual-carrier rationale onto APS_RENDERER_FIELD, stating the belt-and-braces intent of keeping the custom top-level copy; both call sites now reference it in one line.
The APS adapter spec loads the external Prebid bundle and assumed it carries the tsjs shim (and the trustedServer adapter) inside it. Once the shim is decoupled (PR #967) the external bundle is pure Prebid.js, no adapter ever registers, and the in-page auction times out. Detect the decoupled world through the window.__tsjs_prebid_bundle manifest stamp and load dist/tsjs-prebid.js after the external bundle, matching the script order the server actually serves. On a coupled bundle the stamp is absent and the spec behaves exactly as before.
f65164d to
78d72e6
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Moves APS PUC delivery onto a nonce-bound private MessageChannel while keeping the legacy direct-postMessage path in the renderer document, and adds a narrowly-guarded resize for collapsed 1x1 GAM shells. The transport rework is backward-compatible by construction and the new PUC 1.17.2 browser test is a genuine regression net. One blocking item: the JS format gate fails on a stray blank line. One question on renderer versioning.
Blocking
🔧 wrench
- JS format gate fails: stray double blank line left by the moved APS branch —
prettier --checkrejectssrc/integrations/gpt/index.ts, failing CI gate 6 (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1206, inline).
❓ question
- Renderer version not bumped despite a wire-protocol change (
crates/trusted-server-js/lib/src/integrations/aps/render.ts:7):APS_UNIVERSAL_CREATIVE_RENDERER_VERSIONstays4whileAPS_UNIVERSAL_CREATIVE_RENDERERswitches from a directpostMessage({nonce, renderer})to a transferredMessageChannel. PUC keys its cached dynamic renderer onrendererVersion. Is holding at 4 deliberate — i.e. the renderer document retains the legacy direct branch precisely so a PUC-cached v4 source keeps working against the new document? If so, a comment on that constant saying the version is intentionally pinned would make the coupling explicit; if not, it needs a bump. (Not inline: line 7 is outside the diff.)
Non-blocking
🤔 thinking
- Inline sizing is permanent across GPT refresh (
.../gpt/index.ts:185, inline) - Port branch drops the parent-source check the direct branch keeps (
crates/trusted-server-core/src/integrations/aps.rs:120, inline) - Capability consumed before the response is known to be delivered (
.../gpt/index.ts:1158, inline) var bdeclared twice in the renderer source (.../aps/render.ts:421, inline)
⛏ nitpick
- Inconsistent
tryscope between the two response paths (.../gpt/index.ts:1238, inline) usesFixedPositioningalso matchessticky(.../gpt/index.ts:145, inline)
📝 note
- Resize applies to more than the APS path: the description says "resize only the authenticated, ordinary collapsed 1x1 GAM shell after APS rendering", but
resizeCollapsedCreativeFrameis also called on the inline-admpath and the cached-adm path. The behaviour change is wider than the summary states — worth reflecting in the PR description so the blast radius is clear to anyone bisecting a sizing regression later. - New browser-test dependency:
prebid-universal-creative@1.17.2is pinned exactly and lands as"dev": true(93 packages in that lockfile, all dev). Reasonable price for real PUC coverage; noting the added CI install surface (gulp-cli,babel-runtime,core-js@2.6.12) for awareness.
👍 praise
- Conservative resize guard set (
.../gpt/index.ts:151, inline) - Real-PUC topology coverage (
.../browser/tests/shared/aps-renderer.spec.ts:208, inline)
CI Status
GitHub checks on this head: 4/4 pass (browser integration tests, integration tests, integration tests (Fastly EC lifecycle), prepare integration artifacts). Because the base branch is fix/aps-renderer-spec-decoupled-shim rather than main, the fmt / clippy / unit-test workflows did not run on this PR — the results below are from running them locally at 5f29a74.
cargo fmt --all -- --check: PASScargo test-fastly aps: PASS (60 passed)npx vitest run: PASS (528 passed, 31 files)npm run lint(eslint): PASSnpm run format(prettier --check): FAIL —src/integrations/gpt/index.ts- clippy: not run locally
(npx tsc --noEmit reports errors in test/integrations/prebid/index.test.ts and test/shared/beacon_guard.test.ts; those are pre-existing and unrelated to this PR.)
| ); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔧 wrench — CI format gate fails here.
Moving the matchedBid.renderer branch below the width/height computation left a second consecutive blank line, so prettier --check rejects this file and CI gate 6 (cd crates/trusted-server-js/lib && npm run format) fails:
$ npx prettier --check src/integrations/gpt/index.ts
[warn] src/integrations/gpt/index.ts
[warn] Code style issues found in the above file.
$ npx prettier src/integrations/gpt/index.ts | diff - src/integrations/gpt/index.ts
1205a1206
>
Fix: delete the blank line. Note this PR's base is not main, so the fmt/clippy/unit-test workflows never ran on this head — only the four integration-test checks did.
| var n=w.btoa(s).replace(/\+/g,"-").replace(/\//g,"_").replace(/=+$/g,""); | ||
| var f=w.document.createElement("iframe"),done=false,t; | ||
| function clean(){w.removeEventListener("message",receive);if(t)w.clearTimeout(t);} | ||
| var x=w.document,b=x.body,f=x.createElement("iframe"),q=new MessageChannel(),done=false,sent=false,t; |
There was a problem hiding this comment.
🤔 thinking — b is declared twice in this renderer source.
Two lines above, var b=new Uint8Array(16) holds the nonce bytes; here var b=x.body rebinds the same name. It is correct today only because the byte buffer is dead by the time this line runs. Any future edit that moves the nonce derivation below the body capture would silently base64-encode the body element instead of the random bytes, and the resulting nonce would still match /^[A-Za-z0-9_-]{22,128}$/ in the renderer document — so it would fail as a silent loss of entropy, not a thrown error.
Fix: rename one of them, e.g.
var by=new Uint8Array(16);c.getRandomValues(by);var s="";for(var i=0;i<by.length;i++)s+=String.fromCharCode(by[i]);| return; | ||
| } | ||
|
|
||
| frame.iframe.style.width = `${width}px`; |
There was a problem hiding this comment.
🤔 thinking — the inline sizing written here is permanent and never cleared.
frame.iframe.style.width/height (and the wrapper's, below) are inline styles, which beat GAM's presentational width/height attributes. Two consequences on a GPT slot refresh that reuses the same iframe element:
- GAM re-sizes the shell via attributes for the next creative — the stale inline
300px × 250pxwins and the slot renders at the previous creative's box. - GAM collapses the slot back to 1x1 via attributes — the computed size is still 300px, so
hasOnePixelDimensionnow returnsfalseand this function can never re-arm for that slot.
Suggestion: record what was set and clear it when the frame is observed re-collapsing (attributes back to 1), or scope the resize to the APS renderer path where the collapsed-shell topology is the actual problem being fixed.
| start(message.renderer,function(result){parent.postMessage({message:result,nonce:nonce},'*');}); | ||
| return; | ||
| } | ||
| if(!keys(message,['nonce'])||event.ports.length!==1||message.nonce!==expected)return; |
There was a problem hiding this comment.
🤔 thinking — the MessagePort branch drops the parent-source check the direct branch keeps.
Three lines above, the direct branch guards with event.source!==parent||event.ports.length!==0. This port branch checks only key shape, port count and nonce. The 128-bit fragment nonce is unreadable cross-origin, so this is not exploitable — but the asymmetry reads as a deliberate relaxation, and the accompanying test now asserts only event.ports.length!==1 for this branch.
Fix: keep the two branches identical in intent for one extra comparison.
if(!keys(message,['nonce'])||event.source!==parent||event.ports.length!==1||message.nonce!==expected)return;| // an unrelated iframe when this slot-bound capability rejects it. | ||
| e.stopImmediatePropagation(); | ||
| if (!messageSourceBelongsToAdUnit(e.source, prebidRendererEntry.adUnitCode)) return; | ||
| const sourceFrame = sourceFrameForAdUnit(e.source, prebidRendererEntry.adUnitCode); |
There was a problem hiding this comment.
🤔 thinking — the capability is consumed before the response is known to be delivered.
In this branch the order is stopImmediatePropagation() → consumeApsPrebidRenderer() → markWinner() → port.postMessage(...). The new catch below logs and returns, which leaves the slot in a terminal bad state: the one-shot renderer entry is already consumed, the bid is marked winner, Prebid's own handler was already suppressed, and markRendered() never runs. No retry path exists — the slot stays blank.
The same shape applies to the sibling branch further down, which also consumes before posting.
Suggestion: post first and consume on success, or restore the entry in the catch before returning. A MessagePort.postMessage of a string realistically only throws on a detached/closed port, but that is exactly the case where the fallback matters.
| height: renderer.height, | ||
| }) | ||
| ); | ||
| resizeCollapsedCreativeFrame(e.source, sourceSlotFrame, renderer.width, renderer.height); |
There was a problem hiding this comment.
⛏ nitpick — inconsistent try scope between the two response paths.
Here resizeCollapsedCreativeFrame and log.debug sit inside the try, so a throw from the resize logs pbRender bridge: APS response failed even though the response was delivered successfully — misleading during triage. The matchedBid.adm branch below keeps its resize outside the try.
Fix: match the adm branch — keep only port.postMessage inside the try, and run the resize and the debug log after it.
| return match !== null && Number(match[1]) <= 1; | ||
| } | ||
|
|
||
| function usesFixedPositioning(element: HTMLElement): boolean { |
There was a problem hiding this comment.
⛏ nitpick — usesFixedPositioning also matches sticky.
The name says fixed but the predicate covers both, which is the right behaviour for anchor/rail ads. Renaming to usesPinnedPositioning (or adding a one-line doc comment) keeps the call sites at lines 172 and 182 self-explanatory.
| } | ||
|
|
||
| /** Resize only an authenticated, still-collapsed ordinary display-creative shell. */ | ||
| function resizeCollapsedCreativeFrame( |
There was a problem hiding this comment.
👍 praise — the guard set here fails closed at every step.
Requiring both the width/height attributes and the computed dimensions to be ≤1px, re-checking isConnected and contentWindow === source after the async hop, rejecting fixed/sticky frames, and carving out ins[data-anchor-status] means a hostile or merely unusual frame gets ignored rather than resized. The wrapper pass repeats the same 1x1 test before touching the parent. Good defensive design for code that mutates publisher DOM.
| })();`; | ||
|
|
||
| test.describe("APS opaque renderer", () => { | ||
| test("renders APS through the real PUC dynamic-renderer topology and resizes only its 1x1 shell", async ({ |
There was a problem hiding this comment.
👍 praise — this is the right shape of test for this bug class.
Driving the real PUC 1.17.2 banner.js through its dynamic-renderer branch (by deliberately leaving pbjs absent) reproduces the actual sibling-frame topology instead of a hand-rolled approximation, and the assertions cover geometry at all three frame levels plus the outer shell. The #div-other sibling slot that must stay 1x1 is what turns this from a happy-path test into a real regression net for the resize guard.
417f5e0 to
2b4b17f
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The new work here is sound: the nonce-bound MessageChannel handshake correctly fixes PUC sibling-frame rendering (where the sender is the hidden dynamic-renderer frame, so the old event.source !== parent check could never pass), the collapsed 1x1 GAM shell resize is tightly guarded, and the real PUC 1.17.2 end-to-end spec is strong coverage. Two blocking items: a JS format failure and the branch/base divergence below.
Blocking
🔧 wrench
- PR does not merge into its base:
git merge-treeagainstfix/aps-renderer-spec-decoupled-shimreports content conflicts in 15 files (aps.rs, orchestrator.rs, formats.rs, prebid.rs, publisher.rs, creative_opportunities.rs, render.ts, gpt/index.ts, types.ts, several tests/docs, plus add/add conflicts on the new files). This branch was rebuilt on a stale merge-base (bf61fa189) while the base chain advanced, so the two sides now hold divergent copies of the same logical commits: this branch carries themarkWinner/markRenderedsplit from the #963 feedback round while the base tip still hasmarkUsed, and the base carries ~16k lines of unrelated newer work this branch lacks. As a result the PR is unmergeable as-is, and the 48-file diff attributes ~9.5k lines of already-reviewed stacked content (#918/#963/#988) to this PR. Fix: update the base branch to the rebased chain, or rebase this branch onto the current base tip, so the PR diff reduces to the actual new work (9 files, +1,628/−98). - JS format gate fails on
crates/trusted-server-js/lib/src/integrations/gpt/index.ts(see inline comment): CI gate 6 (npm run format) rejects the double blank line left where the old renderer branch was removed.
CI Status
- GitHub checks: integration tests (4 jobs) PASS
- Local vitest: 528/528 PASS
- Local
cargo test(wasm32-wasip1, aps-filtered): 60/60 PASS - JS format: FAIL (
gpt/index.ts) - fmt/clippy: not run locally
| // pulling slot B's creative and firing slot B's win/billing beacons. | ||
| if (!matchedBid || matchedBid.hb_adid !== adId) return; | ||
|
|
||
|
|
There was a problem hiding this comment.
🔧 wrench: removing the old renderer branch left a double blank line here. npx prettier --check fails on exactly this (verified locally), so the npm run format CI gate will reject the PR.
Fix: delete one of the two blank lines.
| start(message.renderer,function(result){parent.postMessage({message:result,nonce:nonce},'*');}); | ||
| return; | ||
| } | ||
| if(!keys(message,['nonce'])||event.ports.length!==1||message.nonce!==expected)return; |
There was a problem hiding this comment.
🤔 thinking: the port path deliberately omits the event.source===parent check that the legacy path above keeps. That omission is load-bearing: in PUC's sibling-frame topology the handshake sender is the hidden dynamic-renderer frame, not this frame's parent, so a source check would re-break exactly the bug this PR fixes. Nothing in the code records that, and a future hardening pass could plausibly "restore" the check.
Suggestion: add a short comment above this branch, e.g. // Port path is nonce-only by design: PUC's dynamic renderer posts from a hidden sibling frame, not this frame's parent.
| if(accepted||portEvent.ports.length!==0||!keys(descriptor,['renderer'])){port.close();return;} | ||
| var nonce=expected; | ||
| if(!start(descriptor.renderer,function(result){try{port.postMessage({message:result,nonce:nonce});}finally{port.close();}}))port.close(); | ||
| }; |
There was a problem hiding this comment.
♻️ refactor: when the descriptor fails validRenderer, the frame closes the port without replying. Port closure is not observable cross-browser, so the parent waits the full 10s RENDERER_READY_TIMEOUT_MS before rejecting, and PUC reports adRenderSucceeded/adRenderFailed 10 seconds late.
Suggestion: post trusted-server/aps/renderer-failed on the port before closing it (and likewise return a failure reply in the legacy path), so invalid descriptors fail fast instead of timing out.
| if(typeof MessageChannel!=="function")throw new Error("APS renderer channel unavailable"); | ||
| var b=new Uint8Array(16);c.getRandomValues(b);var s="";for(var i=0;i<b.length;i++)s+=String.fromCharCode(b[i]); | ||
| var n=w.btoa(s).replace(/\+/g,"-").replace(/\//g,"_").replace(/=+$/g,""); | ||
| var x=w.document,b=x.body,f=x.createElement("iframe"),q=new MessageChannel(),done=false,sent=false,t; |
There was a problem hiding this comment.
⛏ nitpick: var b is declared twice in this renderer source: first as the nonce byte array on line 419, then reassigned to x.body here. It only works because the reuse happens after the nonce is derived; a future reorder would silently corrupt the nonce.
Suggestion: use a different variable name for the body reference.
| "devDependencies": { | ||
| "@playwright/test": "^1.49.0" | ||
| "@playwright/test": "^1.49.0", | ||
| "prebid-universal-creative": "1.17.2" |
There was a problem hiding this comment.
📝 note: prebid-universal-creative@1.17.2 pulls 93 packages into the browser-test harness (including gulp-cli and core-js, which has an install script) for the single dist/banner.js file the spec reads. Pinned, dev-only, and integrity-hashed, so this is acceptable; if the footprint becomes a concern, consider npm ci --ignore-scripts in CI or vendoring just the dist file.
| const rendererUrl = apsRendererUrl(); | ||
| if (!renderer || !rendererUrl) return; | ||
|
|
||
| // Ownership and the complete validated envelope are valid before this |
There was a problem hiding this comment.
⛏ nitpick: this comment does not parse ("Ownership and the complete validated envelope are valid before this handler claims the message..."); please reword, e.g. "Validate ownership and the complete envelope before this handler claims the message or suppresses another legitimate handler."
Relatedly, the PBS Cache path below is now the only response path whose port.postMessage has no dedicated try/catch: a throw there is swallowed by the fetch .catch and logged as "PBS Cache fetch failed", which is misleading.
3 participants
Summary
Validation