[aram356]

Why

A blank APS creative render is indistinguishable from a delivered one, and nothing on the render path says why. An APS bid that wins Prebid targeting is served by Ad Manager as a 1x1 universal creative that resizes itself only after the creative draws, so Ad Manager reports a non-empty 1x1 render either way and the tester framework reports "filled". Every guard on the render path returned silently: a frame that rejected the descriptor, one that never received it, and one that timed out all looked identical.

This replaces #1052, which is being closed rather than revived. That branch had not moved since 2026-09-01, carried five unaddressed CHANGES_REQUESTED reviews, conflicts with current main, and was anchored to the closed #1019. Its security shape was sound and is carried forward here; the six defects that stopped it are fixed or deferred explicitly.

Companion issue: #1299.

What changed

The sandboxed renderer document (aps.rs) reports which guard stopped it, on the existing failure message: bad_hash, nonce_mismatch, descriptor_keys, descriptor_fields, descriptor_envelope, amazon_script_error.

The Universal Creative source (render.ts) labels its own frame_timeout and frame_load_error and relays whichever reason it holds to the top window.

The GPT bridge (gpt/index.ts) consumes that relay and records the reason against the ad unit the capability was served to.

The diagnostics store and overlay learn the new categories, so a reason is recorded and displayed rather than discarded. This is the part #1052 was missing, and the reason nothing it emitted ever reached ts_console.

Three deliberate differences from #1052

One source of truth for the failure vocabulary. #1052 widened the type union in types.ts while the store's runtime allowlist and the overlay's switch stayed as they were, so all fifteen new reasons were dropped before being recorded. Here the union is derived from a single const array and the store validates against that same array, so a category that exists at compile time is recordable at runtime by construction. The overlay keeps its exhaustive switch with no default, which is what makes a new category a compile error there instead of an undefined fact.

The source check stays first. #1052 moved it below the shape check so a non-parent sender could be told source_mismatch. That reason is dropped here and the ordering is left alone: a sender that is not the parent is rejected before any reporting, so it cannot consume the frame's single report. The source_mismatch category does not exist.

The relay is source-checked. #1052 accepted a relayed failure with no check on e.source, so any frame with a top handle and a live ad ID could forge one, including the security-signal categories. The tombstone now carries the ad unit it was served to, and the relay requires the sender to be a frame inside it. The ad ID is not secret from the ad-serving chain, since it is published as hb_adid targeting.

The direct-path regression, fixed

Adding reason makes the failure message three keys. renderApsCreative's listener exact-matched two, so it dropped every failure and the hidden frame survived over publisher content for the full 10s timeout instead of failing fast. That listener now accepts a reason-bearing failure, including the nonce-less bad_hash report a frame sends before it has learned a nonce, while still rejecting a foreign nonce and still requiring an exact key set on the ready message. #1052 shipped this regression untested; it has four tests here.

Notes for review

The renderer bundle version moves 4 to 5 because the Universal Creative source changed, and the one test that hardcoded 4 now reads the constant.

The Prebid shim imports two unrelated helpers from aps/render.ts, and a module-scope Object.freeze on the reason table defeated tree-shaking and pushed the shim from 40787 to 41225 bytes, past the 41000-byte guard in prebid-artifact-integration.test.mjs. The table is built on first use instead, so the shim stays at its previous size and the guard is unchanged. The guard's comment asks for growth to be re-justified rather than raised, so it was not raised.

Security posture, since a reason crosses an origin boundary:

• Reasons are fixed categories. A rejected descriptor is never echoed back.
• Reporting from the frame is one-shot and answers through parent, never the sender.
• Relayed reasons resolve through a null-prototype allowlist, so a hostile __proto__, constructor, or toString resolves to undefined.
• The relay is recording only and never influences delivery.

Deferred, tracked in #1299

This does not attempt the attempt-lifecycle redesign, so a relayed failure is not yet attributed to the render it belongs to in every case:

• recordTrustedServerCreativeResponse is recorded when the descriptor is posted, which closes the attempt before the frame can report. A narrower store API, or a post-completion failure state, is needed.
• The slot association the APS path needs plants a request intent that belongs to the slot's next cycle, which mis-attributes it.

Both are described in #1299 with the measurements behind them. They need design review rather than a port, which is why they are not here.

Testing

Gates run locally, all green:

• cargo fmt --all -- --check
• cargo clippy-fastly
• cargo test -p trusted-server-core (2912 passed)
• cd crates/trusted-server-js/lib && npx vitest run (1225 passed, 50 files, no type errors)
• cd crates/trusted-server-js/lib && npm run format

tsc --noEmit over src/ was compared against the merge base: the error set is byte-identical, so this branch introduces no new type errors. (The repo has 352 pre-existing ones and no CI gate for them.)

New coverage:

• render.test.ts: direct-path teardown on a reason-bearing failure, on nonce-less bad_hash, rejection of a foreign nonce, rejection of a ready message with an extra key; the allowlist including inherited-key and non-string rejection; every mapped reason resolving to a category the store accepts; UC relay of a frame reason, frame timeout, and frame load error
• store.test.ts: every declared category is recordable, iterating the array so a future addition cannot silently become unrecordable
• ad_init.test.ts: a relayed failure recorded against a real GptDiagnosticsStore rather than a mock recorder, plus drops for a foreign sender and for unlisted reasons

The two tests guarding the #1052 defects were checked against the broken versions and fail there: the store test fails with expected undefined to deeply equal [ 'aps_bad_hash' ] against the old hand-written allowlist, and the bridge test fails with expected undefined to deeply equal [ 'aps_frame_timeout' ] with the relay recording disabled.

[prk-Jr]

Summary

Labelling the guard that stopped an APS render is the right fix, and the security shape of the relay (source check first, null-prototype allowlist, fixed categories, one-shot reporting) is sound. However, the relay records nothing in a realistic configuration: the Prebid path keys the store by the Prebid ad unit code while associations are keyed by the Trusted Server slot id, and the server-side APS path is never consulted. Both were reproduced against a real GptDiagnosticsStore using this suite's own homepage_header / div-header fixture.

3 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied. Each suggestion was applied alone and as a batch in a scratch worktree: prettier, the full vitest suite (1225 passed, including the Prebid shim size guard), and the core integrations::aps tests all pass.

Blocking

🔧 wrench

• Prebid-path relay records under the Prebid ad unit code, not the Trusted Server slot id — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:2007
• Server-side APS render failures are never relayed — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:2005

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note / ⛏ nitpick

• 🤔 nonce_mismatch can never be recorded (suggestion) — see inline at crates/trusted-server-core/src/integrations/aps.rs:126
• 🤔 Nonce-less failure acceptance makes reasons forgeable from inside the renderer frame (suggestions) — see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:767 and :812
• ♻️ No test executes the renderer document's reasons — see inline at crates/trusted-server-core/src/integrations/aps.rs:3327
• 📝 aps_frame_load_error is effectively unreachable — see inline at crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:201
• ⛏ "Adding a member here is sufficient" overstates — see inline at crates/trusted-server-js/lib/src/core/types.ts:189

👍 praise

• Single source of truth for the failure vocabulary — see inline at crates/trusted-server-js/lib/src/core/types.ts:196

Cross-cutting / body-level findings

• ♻️ Operator docs still list four failure categories — the "Creative-bridge failures" table in docs/guide/integrations/gpt-diagnostics.md (around line 225) documents only missing_render_source, cache_fetch_failed, invalid_cache_payload, and response_post_failed. This PR adds ten aps_* categories that now appear in the overlay; each needs a row describing where it is observed (renderer document vs. Universal Creative source) and, ideally, a sentence that APS reasons are relayed diagnostics, not bridge failures.
• 🌱 The direct render path discards the reason — renderApsCreative (reached from core/request.ts:59, the client /auction path) now accepts reason-bearing failures but ignores reason, and fail() logs APS renderer: frame load failed for every cause, including the 10s timeout. Logging the allowlisted reason (via apsRenderFailureReason) would make that path debuggable at no cost; recording it can follow the attempt-lifecycle work in #1299.
• 📝 Repeated relays are bounded — checked while reviewing: a frame inside the ad unit can post the relay repeatedly, but recordTrustedServerCreativeRequest returns the existing live attempt for the cycle and failures are deduplicated, so this cannot flood the store. No action needed.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• CodeQL: PASS
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (axum native): PASS
• CLAUDE.md symlink guard: PASS
• Analyze (actions): PASS
• Analyze (python): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• Analyze (javascript-typescript): PASS
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo test: PASS (required)
• cargo fmt: PASS (required)
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• prepare integration artifacts: PASS
• vitest: PASS
• Analyze (rust): PASS

[dhruv8sh]

Summary

Giving each APS render guard a name, and having one array define the failure categories, are both the right moves. The renderer-document and direct-path changes look correct. However, the relay that is meant to carry a reason to ts_console cannot record anything in a real browser yet. Three independent gaps each drop the reason, and this review adds one that the unit tests cannot see: the message is posted from PUC's hidden renderer frame, so it fails the bridge's source check.

None of the inline comments below carry a one-click suggestion. The fix touches the Universal Creative source and its tests together, so it is described in prose.

Blocking

🔧 wrench

• Relay is posted from PUC's hidden renderer frame, so the source check always fails — see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:808

Cross-cutting / body-level findings

• 🔧 I agree with the two open wrenches from the earlier review, and they add to this one. The Prebid-path relay records under the Prebid ad unit code instead of the Trusted Server slot id (gpt/index.ts:2007). The server-side APS path is never consulted (gpt/index.ts:2005). I confirmed both against GptDiagnosticsStore.recordTrustedServerCreativeRequest, which looks up trustedServerSlots by slot id. To get a single reason recorded end to end, all three have to be fixed. The new ad_init.test.ts case records a relayed APS render failure against a real diagnostics store passes only because it uses a synthetic source and associates the slot under 'div-header'. Neither happens in production.
• 🌱 The tombstone can expire before the frame times out. The relay looks up consumedPrebidApsIds, and each tombstone expires at the renderer entry's expiresAt. A capability consumed with less than 10s of TTL left will lose the tombstone before frame_timeout fires, and the relay will be dropped. That is rare with the default 300s TTL. Once the relay works, keeping the tombstone until at least now + RENDERER_READY_TIMEOUT_MS would close the gap. It also keeps the ad ID blocked longer, which is the safer direction.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• CodeQL: PASS
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (axum native): PASS
• CLAUDE.md symlink guard: PASS
• Analyze (actions): PASS
• Analyze (python): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• Analyze (javascript-typescript): PASS
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo test: PASS (required)
• cargo fmt: PASS (required)
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• prepare integration artifacts: PASS
• vitest: PASS
• Analyze (rust): PASS

[ChristianPavilonis]

Approved with non-blocking comments on the APS failure-reporting path:

• Send the relay from the creative window so source authentication accepts it, rather than rejecting PUC’s dynamic-renderer iframe.
• Recognize server-side APS winners as well as Prebid winners, preserving slot-scoped ownership.
• Resolve the Trusted Server slot ID before recording failures when it differs from the Prebid div code.
• Report nonce mismatches using the frame’s expected nonce so receivers retain nonce_mismatch rather than falling back to frame_timeout.

[aram356]

Addressed at a90f2f8. All four blocking findings were reproduced before fixing, and every fix is pinned by a test that fails without it.

Realm (@dhruv8sh, @ChristianPavilonis). Confirmed, and the diagnosis was exactly right. Verified in headless Chromium against a page mirroring Universal Creative's structure, with the real crossDomain.js from the installed Prebid.js (v10.26.0, lines 82-99) as the reference for that structure. The previous call reports the hidden renderer frame as event.source and matches no slot iframe; the helper reports the creative iframe and matches. Two controls worth recording: w.top vs w.parent resolve to different windows yet both report the hidden frame, and Reflect.apply(w.top.postMessage, w.top, ...) also reports the hidden frame. So it is the realm of the running function, not the target or the receiver.

One correction to the suggested fix: sendMessage is (type, payload, responseListener), not a bare poster, and it force-stamps message and adId via Object.assign({message: type, adId}, payload). Since payload is spread last a caller can override both, but the relay does not need to: the helper's own adId is the one the bridge wants. It also JSON.stringifys the body, which the bridge already parses, and targets pubDomain rather than '*', which is strictly better. d.adId is no longer read.

Slot-id keying (@prk-Jr, @ChristianPavilonis). Confirmed against the real store with production's association: keying on the div id gives attemptId = undefined, no failure, and ['creative_request_without_slot', 'creative_attempt_unknown']. Resolved through divToSlotId. My test did mask this by associating under 'div-header'; it now associates under 'homepage_header' and maps the div to it, and fails if the resolution is removed.

Server-side path (@prk-Jr, @ChristianPavilonis). Confirmed: both branches serve the same renderer (gpt/index.ts:2075 and :2154) but only the Prebid tombstone was consulted. Now resolved via consumedServerApsBySlot, requiring the sender to sit in the slot that consumed the ad ID since hb_adid is not unique per slot. New test, mutation-proven.

nonce_mismatch unreachable (@prk-Jr, @ChristianPavilonis). Confirmed by inspection and fixed as suggested: the report echoes expected. Only parent receives it, and parent set the fragment, so nothing new is exposed.

Nonce-less forgery (@prk-Jr). Narrowed to reason === 'bad_hash' in both parents, as suggested.

Tombstone expiry (@dhruv8sh 🌱). Fixed rather than deferred: tombstones now live at least APS_NATIVE_RENDERER_TIMEOUT_MS, which also keeps the ad ID blocked longer.

Renderer-document coverage (@prk-Jr ♻️). Added to aps-renderer.spec.ts, asserting reason and nonce for the mismatch-slot, missing-fragment-slot, and malformed-slot cases. This is the only coverage that executes the document rather than matching substrings, and it is where the nonce_mismatch bug would have surfaced.

Also done: operator docs now have an APS table covering all ten categories and where each is observed, with a note that an HTTP error on the renderer route surfaces as aps_frame_timeout rather than aps_frame_load_error (@prk-Jr 📝) and a softer overlay label to match; the direct path logs the allowlisted reason instead of reporting every cause as a load failure (@prk-Jr 🌱); the types.ts comment no longer overstates what adding a member achieves (@prk-Jr ⛏).

Local gates: cargo fmt, clippy-fastly, integrations::aps (38 passed), vitest (1227 passed, 50 files, no type errors), npm run format for both lib and docs. The 11 settings::tests failures in my local cargo test are present at the unmodified branch HEAD too, so they are environmental and not from this change.

Attempt-lifecycle work remains deferred to #1299 and is unchanged by this round.

[dhruv8sh]

Summary

The follow-up commit closes every blocking finding from the earlier reviews. The relay now goes out through Universal Creative's sendMessage helper, so it arrives from the creative iframe. Failures are recorded under the Trusted Server slot id, and server-side APS winners are relayed too. nonce_mismatch is now reachable, nonce-less acceptance is narrowed to bad_hash, the browser spec executes the renderer document, and the docs list all ten categories. What remains is non-blocking.

2 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comment is a note with no action required.

Non-blocking

🤔 thinking / ⛏ nitpick / 📝 note

• 🤔 The tombstone minimum has no margin, so a late frame_timeout relay is still dropped (suggestion). See inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1951.
• ⛏ The Rust test's doc comment overstates the JS-side coverage (suggestion). See inline at crates/trusted-server-core/src/integrations/aps.rs:3328.
• 📝 aps_unknown is effectively unreachable, and an unrecognised wire reason is dropped silently. See inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:834.

Cross-cutting / body-level findings

• ♻️ No real-browser test covers the main fix (which window the relay comes from). Routing the relay through sendMessage only matters in a real browser, because the sending window is determined by where the posting code runs. The vitest relay tests use a pucHelper stand-in that posts from the top window. The new browser-spec assertion covers the renderer document's own renderer-failed messages, not the relay. The commit message describes a manual headless-Chromium check, but nothing automated would catch a return to w.top.postMessage. The real-PUC test in crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts (around line 228) already loads PUC_BANNER and is the natural place to lock it in. Apply manually: the test is outside this PR's diff hunks.

  // In "renders through real PUC and expands only its authenticated 1x1 shell",
  // make the renderer document report a failure (e.g. serve it a descriptor with
  // a bad envelope), then assert the top window received the relay from the
  // creative iframe itself:
  const relay = await page.evaluate(() => (window as unknown as {
    apsRelays: Array<{ fromCreative: boolean; data: Record<string, unknown> }>;
  }).apsRelays.find(({ data }) => data.message === 'trusted-server/aps/render-failed'));
  expect(relay).toEqual({ fromCreative: true, data: expect.objectContaining({ reason: 'descriptor_envelope' }) });

• 👍 Thorough follow-up. The fix commit addresses every earlier finding and explains each one. The new browser-spec assertion executes the real renderer document and checks the echoed nonce. That is the coverage that would have caught the nonce_mismatch bug. The relay also keeps per-slot sender checks on both the Prebid and server-side paths.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• CodeQL: PASS
• cargo test (ts CLI, native) (macos-latest): PASS
• CLAUDE.md symlink guard: PASS
• format-docs: PASS (required)
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• Analyze (javascript-typescript): PASS
• cargo fmt: PASS (required)
• cargo test (axum native): PASS
• Analyze (actions): PASS
• format-typescript: PASS (required)
• Analyze (rust): PASS
• cargo test: PASS (required)
• vitest: PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• cargo test (cross-adapter parity): PASS
• prepare integration artifacts: PASS
• Analyze (python): PASS

[prk-Jr]

The follow-up fixes the earlier blocking findings: the failure relay uses Universal Creative's creative-realm helper, resolves exact div IDs to Trusted Server slot IDs, covers server-side APS winners, and reports nonce mismatches using the expected nonce. The descriptor validation, sandbox boundaries, fixed reason vocabulary, and source-first authentication remain intact.

One remaining correctness issue affects supported prefix-based ad units: authentication resolves the actual creative frame, but the new slot lookup uses the unresolved prefix as a map key. The inline comment includes a verified one-line correction.

The already-reported tombstone timeout margin and real-browser relay regression coverage remain relevant; this review does not duplicate those comments. Attempt association and post-completion evidence remain explicitly deferred to #1299, so this PR does not establish reliable attribution across refresh cycles.

Validation: all 22 GitHub checks pass. The three targeted JS suites pass locally under Node 24: 304 tests covering APS rendering, the GPT bridge, and the diagnostics store. Rust and browser execution are covered by the passing remote checks; they were not rerun locally.

[aram356]

Addressed at 64a0545.

Prefix-based ad units. Confirmed and fixed as suggested. I reproduced it exactly as described: changing only the existing relay test's adUnitCode from div-header to div- serves the capability but turns the expected ['aps_frame_timeout'] into undefined. The relay now resolves the slot via slotFrameForMessageSource(e.source)?.slotId, which handles exact ids, configured prefixes, and ambiguity. The capability's own sourceFrameForAdUnit check still runs first, so a sender outside the served ad unit is rejected before any slot is resolved. There is now a prefix-based regression test beside the exact-id one, and it fails if the resolution is reverted to the map lookup.

Real-browser relay coverage. This was the right thing to push on, so I did it rather than defer it. The last round added renderer-document reason coverage, but nothing asserted the relay reaching diagnostics in a browser, which is precisely the class of gap the realm bug and this prefix bug both fell into. aps-renderer.spec.ts now drives real Universal Creative against a renderer route that loads and never acknowledges, through a slot configured with the div- prefix, and asserts the recorder receives aps_frame_timeout. It exercises the full chain: creative-realm helper, source authentication, prefix resolution, allowlist, recorder.

Tombstone timeout margin. Already fixed in the previous round (a90f2f8) rather than deferred: recordConsumedPrebidApsId holds a tombstone for at least APS_NATIVE_RENDERER_TIMEOUT_MS, so a capability consumed late in its TTL keeps its tombstone until after the frame timeout fires. Flagging it here since the review listed it as still relevant.

Attempt association and post-completion evidence stay deferred to #1299, as you note. Agreed that this PR does not establish reliable attribution across refresh cycles, and the PR description says so.

Local gates: cargo fmt, clippy-fastly, integrations::aps (38 passed), vitest (1228 passed, 50 files, no type errors), npm run format. The new browser test is the one thing I could not run locally, since the suite needs a built WASM artifact plus Viceroy and the Docker origins; it typechecks, and the browser integration tests check covers it.