Name the guard that stops an APS creative render

[jevansnyc]

Why

Investigating blank ads on a live publisher page, APS bids were winning the
auction and Ad Manager was filling the slot, yet the creative never drew. The
tester framework reported the slot as "filled" at 1x1, which is exactly what a
successful universal-creative render looks like before it resizes. Nothing
downstream distinguished the two.

Two blind spots made this close to undiagnosable from the outside:

• Every guard on the APS render path returned silently. A frame that rejected
  the descriptor, one that never received it, and one that timed out all looked
  identical: an iframe that loaded and did nothing.
• The APS capability handshake never reported to GPT diagnostics, so ts_console
  showed delivery: unknown on every request cycle and zero creative failures,
  while APS bids were rendering blank.

On the page under investigation that was 24 of 24 cycles unattributed.

What changed

The sandboxed renderer document (aps.rs) reports which guard stopped it,
on the existing failure message: bad_hash, source_mismatch,
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) records a creative attempt around the
handshake and names each silent return: aps_consumed_tombstone,
aps_source_not_in_ad_unit, aps_descriptor_fields, aps_tombstone_capacity,
aps_missing_renderer_url. A successful post records a response, so this path
reports trusted_server_response_sent rather than unknown.

Notes for review

The APS path runs on the publisher's own Prebid ad units, which never pass
through Trusted Server slot mapping. No creative opportunity exists for them, so
the store rejected the attempt as creative_request_without_slot. The bridge now
resolves the GPT slot by element ID and records the opportunity first. That is
the least obvious part of the change and the part most worth a look.

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, so an unrelated sender cannot consume the report or learn from it.
  Traffic not shaped like the handshake stays silent, as before.
• Relayed reasons resolve through a null-prototype allowlist, so a hostile
  __proto__, constructor, or toString resolves to undefined.
• The attempt is taken from our own tombstone, never from the message.
• The relay is recording only and never influences delivery.

This is instrumentation. It does not attempt a fix, because the root cause is
still unknown: the evidence says the handshake completes and the renderer frame
loads, then the chain dies before Amazon's prebid-creative.js is requested.
These reason codes are what will name it on the next occurrence.

Testing

• render.test.ts: relay of a frame reason, frame timeout, and the allowlist
  including inherited-key and non-string rejection
• ad_init.test.ts: creative attempt recorded for a registered APS renderer,
  and the tombstone reason on a replayed ad ID
• aps.rs: every guard reports a reason, reporting is one-shot, no descriptor
  echo, and the frame never answers the sender

Gates run: cargo fmt --check, clippy (fastly target, -D warnings),
cargo test -p trusted-server-core (2143 passed), JS build, JS format, and the
full vitest suite (850 passed).

Two caveats. The vitest suite has 27 pre-existing failures in
sourcepoint/index.test.ts and permutive/segments.test.ts, all one
localStorage error from running Node 26 against a repo pinned to 24.12.0; the
count is unchanged by this branch. And the remaining adapter gates (axum,
cloudflare, spin, parity) were not run locally, because a .cargo/config.toml
alias conflict with a stale sibling checkout meant cargo had to be invoked from
outside the repo without workspace aliases. CI covers those.

[aram356]

@aram356 Will test in staging before merging in #1019

[aram356]

@ChristianPavilonis to understand if belongs in #1019

[ChristianPavilonis]

Summary

Reviewed cb1de4777cdd5efe2a32892c03d00c1b071795f6. Requesting changes because the new reporting path can alter APS delivery and the diagnostics pipeline does not currently retain the APS failure evidence this PR introduces. Four actionable findings are posted inline.

[ChristianPavilonis]

🔧 P1: Wire the new APS reasons through the real diagnostics consumers

The runtime validator in gpt_diagnostics/store.ts:168-174 still accepts only the four original failure values, so every new aps_* reason is discarded by recordTrustedServerCreativeFailure(). The overlay switch in gpt_diagnostics/overlay.ts:185-194 also has no APS cases. As a result, ts_console and exported snapshots will continue showing no APS creative failures. The new tests mock the recorder and therefore do not exercise either consumer.

Update the store allowlist and overlay labels for every new category, make the presentation switch exhaustive, and add a test that passes an APS reason through the actual recorder/store and verifies the snapshot and overlay output.

[ChristianPavilonis]

🔧 P1: Do not let a foreign sender terminate rendering

A shaped message from a non-parent source now emits source_mismatch without a nonce. The Universal Creative wrapper accepts nonce-less failure messages from this iframe at render.ts:623, calls fail(), removes the iframe, and rejects the render. A sibling or ancestor window that obtains the iframe's WindowProxy can therefore race the valid parent message and suppress APS delivery. Previously this traffic was ignored, and the PR describes reporting as unable to influence delivery.

Keep event.source !== parent silent, or route this observation through a path that cannot trigger terminal renderer failure. Please add a test where a foreign shaped message arrives before the valid nonce-bound parent handshake and confirm the render still succeeds.

[ChristianPavilonis]

🔧 P1: Keep the attempt writable for downstream renderer failures

This marks the attempt completed as soon as port.postMessage returns. The store then clears its cycle at gpt_diagnostics/store.ts:529-531, and recordTrustedServerCreativeFailure() drops all completed attempts at line 549. Renderer timeouts, descriptor failures, Amazon script failures, and consumed-ID replays necessarily arrive after this response, so they cannot be retained even after the APS allowlist is fixed. The existing store test at store.test.ts:1707 explicitly verifies that post-completion failures are ignored.

Keep the cycle association writable for the 30-second mutation window after recording the independent response timestamp, or add a separate post-response failure channel. Cover response sent followed by a frame failure and a replay using the real diagnostics store.

[ChristianPavilonis]

🔧 P2: Do not create next-request evidence while associating the current APS attempt

This handshake occurs after the current GPT request, but recordTrustedServerOpportunity() explicitly records evidence for the slot's next request (gpt_diagnostics/store.ts:334 and :925-953). The creative attempt attaches to the latest existing cycle while this pending intent survives. A refresh within five seconds can therefore mislabel the next cycle as trusted_server_direct with a renderable candidate, or as competing when its real Prebid or publisher marker is also present.

Use a separate operation that associates the APS ad-unit code with the current GPT slot without creating next-request intent, or add an APS-specific current-cycle attempt method. Add a two-cycle test proving the following refresh receives only its own request-path evidence.

[prk-Jr]

Summary

Instrumentation for the APS Universal Creative render path: each silent guard now names itself, the creative frame relays its reason to the top window through a null-prototype allowlist, and the GPT bridge opens a diagnostics attempt around the capability handshake. The security reasoning in the description is unusually careful and the allowlist is the right shape for a cross-origin relay.

Three defects block it, all verified with runnable probes rather than read off the diff. The headline one is that none of the fifteen new reason codes can reach the store: isCreativeFailure in store.ts still allowlists only the original four, so every recordTrustedServerCreativeFailure(attemptId, 'aps_*') call added here is a no-op. ts_console will still report zero creative failures on the APS path, which is the exact blind spot the PR exists to close. Separately, adding a reason key to the renderer document's failure message breaks the direct (non-Prebid) render path, which gates on an exact two-key match.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change touches files or lines outside this diff and can't be auto-applied.

Blocking

🔧 wrench

• Every new aps_* reason is dropped before it reaches the store — see inline at crates/trusted-server-js/lib/src/core/types.ts:213
• creativeFailureFact is non-exhaustive; ts_console will render undefined — see inline at crates/trusted-server-js/lib/src/core/types.ts:229
• Direct APS render path stops tearing down on renderer-failed — see inline at crates/trusted-server-core/src/integrations/aps.rs:60
• The new bridge tests mock the recorder, so they cannot catch the store gap — see inline at crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:3379
• The relay branch has no test — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1725

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note

• source_mismatch burns the one-shot report and is attacker-triggerable — see inline at crates/trusted-server-core/src/integrations/aps.rs:103
• The opportunity lands on the slot's next request cycle, mislabeling it — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1619
• beginApsCreativeAttempt runs before the source check — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1760
• window.googletag untyped access — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1616 (carries a suggestion)

👍 praise

• Null-prototype allowlist, and tests that actually prove it — see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:56

Cross-cutting / body-level findings

• 📝 New type errors introduced, though tsc is not a gate here. A tsc --noEmit delta between the merge-base and this head shows 8 new errors: the overlay.ts TS2366 and the gpt/index.ts TS2339 covered inline, plus 6 × TS2532 (Object is possibly 'undefined') in ad_init.test.ts. The base already carries 289 errors so this is not a regression in gate terms, but the first two are pointing at real defects — worth noting that the type system did flag both of the JS-side bugs found in this review, and nothing was listening.

CI Status

• browser integration tests: PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo fmt: PASS
• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• format-docs: PASS
• format-typescript: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• vitest: PASS

All 14 checks green. Every finding below survives green CI — noted in each comment where the existing tests structurally cannot catch the defect.

[prk-Jr]

🔧 Every new aps_* reason is dropped before it reaches the store.

The union grew fifteen members here, but the runtime allowlist it is validated against did not. store.ts:168 is unchanged by this PR:

function isCreativeFailure(reason: unknown): reason is GptDiagnosticsCreativeFailure {
  return (
    reason === 'missing_render_source' ||
    reason === 'cache_fetch_failed' ||
    reason === 'invalid_cache_payload' ||
    reason === 'response_post_failed'
  );
}

recordTrustedServerCreativeFailure returns early at store.ts:540 on !isCreativeFailure(reason), so every safelyRecordCreativeFailure(attemptId, 'aps_*') call added in this PR is a no-op.

Verified against the real GptDiagnosticsStore — both reasons recorded on one live attempt, then snapshotted:

has response_post_failed: true
has aps_frame_timeout:    false

"trustedServerCreativeFailures": ["response_post_failed"]

So ts_console will still show zero creative failures on the APS path. delivery: trusted_server_response_sent does work — I confirmed that separately — but the reason codes, which are the point of the change, never land.

Fix in store.ts:168-175: extend the guard with all fifteen members. Worth deriving both the type and the guard from one const array of literals so they can't drift apart again — that drift is the whole bug, and it will recur the next time a reason is added.

Not offered as a suggestion: the fix is in a file outside this diff.

[prk-Jr]

🔧 creativeFailureFact is non-exhaustive; ts_console will render undefined.

overlay.ts:182-195 switches over this union with no default and a declared : string return. The fifteen new members fall through and return undefined, which overlay.ts:268-270 pushes straight into the facts list:

for (const failure of new Set(cycle.trustedServerCreativeFailures ?? [])) {
  facts.push(creativeFailureFact(failure));
}

This is latent today only because the store gap above blocks every aps_* reason from ever reaching the overlay. Fix that one alone and ts_console starts printing undefined lines instead of failure reasons — so these two need to ship together.

TypeScript does flag it (overlay.ts(184,4): error TS2366: Function lacks ending return statement and return type does not include 'undefined'), confirmed new by diffing tsc --noEmit between the merge-base and this head. It isn't a CI gate here, since the base already carries 289 pre-existing errors.

Fix: add a case per new member in creativeFailureFact. Not offered as a suggestion — the fix is in a file outside this diff.

[prk-Jr]

🔧 This breaks the direct APS render path's failure teardown.

report() always posts three keys — message, nonce, reason — including when nonce is undefined. The direct (non-Prebid) render path still gates on an exact two-key match, at render.ts:573, unchanged by this PR:

if (event.source !== iframe.contentWindow || !hasExactKeys(event.data, ['message', 'nonce'])) {
  return;
}

hasExactKeys compares the full sorted key set, so every failure message from this document is now silently discarded there and fail() never runs. The frame is no longer torn down on an explicit failure — it lingers for the full RENDERER_READY_TIMEOUT_MS (10s) before the timeout restores publisher content.

Verified by driving renderApsCreative with both message shapes:

OLD shape {message,nonce}        -> frame removed: true
NEW shape {message,nonce,reason} -> frame removed: false

renderer-ready still posts exactly two keys, so success is unaffected — only the failure fast path is dead. CI stayed green because the nearest existing test (leaves existing slot content intact when validation or loading fails) exercises the iframe error event, never the message, and the Rust test added here is string-matching on APS_RENDERER_DOCUMENT so it structurally cannot see the mismatch.

Fix at render.ts:573, accepting both shapes:

if (
  event.source !== iframe.contentWindow ||
  (!hasExactKeys(event.data, ['message', 'nonce']) &&
    !hasExactKeys(event.data, ['message', 'nonce', 'reason']))
) {
  return;
}

Worth a regression test on that path too, since nothing currently covers the message-driven teardown. Not offered as a suggestion: the fix is in a file outside this diff.

[prk-Jr]

🤔 Moving the source check below the shape check lets a foreign sender consume the one-shot report.

Previously event.source !== parent returned first, so a non-parent sender could never reach report(). Now any window that can post a well-shaped {nonce, renderer} to this frame reaches this line, sets reported = true, and burns the frame's single report.

That is reachable in practice: indexed access on a cross-origin WindowProxy is allowed by spec, so any iframe on the page can traverse top.frames[...] down to this frame and postMessage to it. The consequence is precisely the failure mode this PR exists to fix — the frame's real later failure, amazon_script_error, is never reported, and a false aps_source_mismatch is recorded in its place, pointing the next investigation at the wrong guard.

The description says "an unrelated sender cannot consume the report or learn from it." The second half holds — answering through parent is right. The first half doesn't.

source_mismatch also has close to zero diagnostic value on its own: our Universal Creative source always posts via f.contentWindow.postMessage, so event.source is parent on every legitimate path. The only way this reason can fire is a foreign sender, which makes it a pure attack surface against the diagnostics rather than a signal.

Proposed — restore the ordering and drop the reason:

function receive(event){
 if(event.source!==parent)return;
 var message=event.data;
 // Stay silent for traffic that is not shaped like the render handshake, so an
 // unrelated sender cannot consume this frame's single report.
 if(!keys(message,['nonce','renderer']))return;
 if(message.nonce!==expected){report('nonce_mismatch');return;}

If you'd rather keep the reason, the alternative is to report it without setting reported, so a foreign sender can't suppress the real one.

Not offered as a one-click suggestion because either shape needs matching updates: renderer_document_reports_a_reason_for_every_silent_guard asserts the document contains source_mismatch, and types.ts carries the aps_source_mismatch member.

[prk-Jr]

♻️ Untyped window access — every other call site in this file casts to GptWindow.

See :958 ((window as GptWindow).googletag), :630, and :826. This one is bare, which produces a new TS2339: Property 'googletag' does not exist on type 'Window & typeof globalThis'.

Suggested change

	const pubads = window.googletag?.pubads?.();
	const pubads = (window as GptWindow).googletag?.pubads?.();

Scratch-verified in an isolated worktree at this head: removes the TS2339 with no new errors in the tsc --noEmit delta, eslint --max-warnings=0 clean, prettier --check clean, and all 124 ad_init.test.ts tests still pass.

[prk-Jr]

🤔 This stamps the slot's next request cycle as Trusted Server-driven, not the one being rendered.

recordTrustedServerOpportunity writes a pending intent via recordRequestIntentSource (store.ts:932), which attaches to the slot's next GPT request. But recordTrustedServerCreativeRequest, called immediately after, resolves against the slot's latest already-recorded cycle. The two land on different cycles.

Reproduced against the real store with the ordering the APS path actually produces (GPT request fires, then the UC handshake arrives):

cycle 1: requestPath "unattributed",
         delivery    "trusted_server_response_sent"          <- the actual APS render
cycle 2: requestPath "trusted_server_direct",
         trustedServerOpportunity "renderable_candidate"     <- phantom

The good news is the headline claim holds: cycle 1 reads trusted_server_response_sent, because delivery() keys off trustedServerCreativeResponseAtMs rather than requestPath. So this is not blocking.

The cost is that every APS handshake also labels the slot's next refresh as Trusted Server-driven with a renderable_candidate opportunity, when TS never participated in it. On a page with APS refreshes that is a steady stream of false trusted_server_direct cycles in the console this PR exists to sharpen.

All recordTrustedServerCreativeRequest actually needs from this call is the slot association (trustedServerSlots.set, store.ts:352). The pending intent is an unwanted side effect of borrowing recordTrustedServerOpportunity to establish it — a narrower recorder entry point that registers the association without writing an intent would avoid it.

[prk-Jr]

📝 Note: the attempt opens before the source is checked.

beginApsCreativeAttempt runs ahead of messageSourceBelongsToAdUnit, so a foreign iframe posting Prebid Request with a known TS ad ID opens a diagnostics attempt (and records the opportunity) before being rejected on the next line.

This looks deliberate given the fail-closed comment above, and the blast radius is bounded — attemptIdsByCycle reuses one live attempt per cycle, and recordTrustedServerCreativeFailure dedupes by reason (store.ts:563-565), so there's no unbounded growth. Recording it as diagnostics pollution rather than a defect, but worth naming since the reader of ts_console can't distinguish these attempts from real ones.

[prk-Jr]

🔧 This branch has no test.

It's the one place a cross-origin reason becomes a recorded attempt — the untrusted-input boundary of the whole feature — and nothing exercises it. render.test.ts covers the emission side (relay of a frame reason, frame timeout) and apsRenderFailureReason in isolation, but nothing covers consumption: the tombstone lookup, the attemptId resolution, or the drop when failedAdId matches no tombstone.

Worth covering at least:

• a relayed reason with a known tombstone ad ID recording against that tombstone's attemptId
• an unknown ad ID resolving to undefined and recording nothing
• a reason outside the allowlist being dropped

[prk-Jr]

🔧 These tests cannot catch the store gap, by construction.

Both new tests replace gptDiagnosticsRecorder wholesale with four vi.fn() spies and then assert the bridge called them. That verifies the call site and nothing downstream — which is exactly why the isCreativeFailure gap flagged on types.ts:213 shipped with green CI. The assertion expect(recordTrustedServerCreativeFailure).toHaveBeenCalledWith(11, 'aps_consumed_tombstone') passes whether or not the store accepts aps_consumed_tombstone, and it does not.

A single test driving the real GptDiagnosticsStore — record an opportunity and a creative request, feed one aps_* failure, assert it appears in snapshot().slots[].requests[].trustedServerCreativeFailures — would have failed immediately and caught it. That end-to-end assertion is the one worth adding here, since the reason codes reaching the console is the deliverable, not the recorder being called.

Same shape of gap on the Rust side: renderer_document_reports_a_reason_for_every_silent_guard string-matches APS_RENDERER_DOCUMENT rather than executing it, so it can't see the message-shape break flagged on aps.rs:60.

[prk-Jr]

👍 Right shape for a cross-origin relay, and the tests actually prove it.

Null-prototype backing object plus Object.freeze, with resolution through a typeof value === 'string' guard, is the correct defence here — and the tests don't just assert the happy path. __proto__, constructor, toString, a non-string, and { toString: () => 'frame_timeout' } are all covered, which is the set that usually gets missed. Keeping the rejected descriptor out of the reason entirely, and answering through parent rather than the sender, are both the right calls.

[aram356]

Summary

This is careful, well-documented instrumentation, and the reasoning in the PR description about the security posture of the relay is sound. But the change does not currently deliver the outcome it is written for: I verified against a real GptDiagnosticsStore that every one of the 15 new aps_* reasons is discarded before it reaches a diagnostics record, so a blank APS render still reports delivery: unknown. Two further defects sit behind that one, and one of them is a behavioural regression on the direct render path rather than a diagnostics-only concern.

The full JS suite passes on this branch (899/899). That is precisely the problem: the new tests mock gptDiagnosticsRecorder with vi.fn()s and never exercise the store, so the gap between the widened type union and the store's runtime guard is invisible to them.

A note on the PR description: the 27 vitest failures you reported are not reproducible here. On the repo-pinned Node 24.12.0 the suite is fully green, which matches your own diagnosis that they were a Node 26 artifact.

None of the inline comments below carry a one-click suggestion. Each fix either lands in a file outside this diff (store.ts), touches lines outside a diff hunk, or needs a design decision, so all of them describe the change in prose instead.

Blocking

wrench

• The store rejects all 15 new aps_* reasons; nothing is ever recorded - see inline at crates/trusted-server-js/lib/src/core/types.ts:197
• Relayed frame failures land on a completed attempt and are dropped - see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1809
• beginApsCreativeAttempt corrupts attribution for the next request cycle - see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1616
• The new 3-key failure message breaks the direct render path - see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:771
• Reordering the source check lets a co-resident script cancel a successful render - see inline at crates/trusted-server-core/src/integrations/aps.rs:104

Non-blocking

thinking / nitpick

• renderable_candidate is semantically wrong on this path - see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1618
• The relay handler applies no source validation - see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1723
• m.nonce===undefined widens what can cancel a pending render - see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:775
• The guard-coverage test is close to a tautology - see inline at crates/trusted-server-core/src/integrations/aps.rs:2629
• The server-side APS path gets no instrumentation - see the cross-cutting section below

Cross-cutting / body-level findings

• thinking - The server-side APS render path is left uninstrumented. installTsRenderBridge has two APS paths. This PR instruments the Prebid-capability path thoroughly, but the server-side path at gpt/index.ts:1848-1875 (reached when matchedBid.renderer !== undefined) still has the same silent returns the PR is written to eliminate: if (!renderer) return; and if (!rendererUrl) return false;, with no safelyRecordCreativeFailure on either. Given the PR's framing, a reader will assume APS render failures are now attributable in general, and on this path they are not. If the omission is deliberate because the live investigation only involved the publisher-Prebid path, that is a reasonable scope decision, but it is worth a sentence in the PR description so the next person debugging a blank server-APS slot is not misled by the reason codes' apparent coverage.

• note - How these findings were verified. Findings 1, 2, and 3 were each confirmed by running throwaway tests against the real GptDiagnosticsStore rather than by reading alone, and finding 4 by driving renderApsCreative end to end. The scratch tests were discarded; the observed outputs are quoted in the relevant inline comments. Gates run locally on this head: cargo fmt --check PASS, cargo clippy-fastly PASS, cargo test -p trusted-server-core 2255 passed / 0 failed, npx vitest run 899 passed / 0 failed, npm run format PASS.

CI Status

• No GitHub checks reported on aps-renderer-failure-diagnostics (gh pr checks returns "no checks reported on the branch"); not run remotely. Locally on this head: cargo fmt --check PASS, cargo clippy-fastly PASS, cargo test -p trusted-server-core PASS (2255), npx vitest run PASS (899), npm run format PASS. The remaining adapter gates (axum, cloudflare, spin, parity) were not run.

[aram356]

wrench - The store rejects all 15 new aps_* reasons, so nothing this PR adds is ever recorded.

This union is the compile-time contract, but the runtime contract is isCreativeFailure in crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:168-175, which still lists only the original four:

function isCreativeFailure(reason: unknown): reason is GptDiagnosticsCreativeFailure {
  return (
    reason === 'missing_render_source' ||
    reason === 'cache_fetch_failed' ||
    reason === 'invalid_cache_payload' ||
    reason === 'response_post_failed'
  );
}

recordTrustedServerCreativeFailure gates on it at store.ts:540 and returns immediately for anything unlisted. Widening the union did not widen the guard.

I confirmed this against a real GptDiagnosticsStore (not a mock): with a live attempt, before any response is recorded,

LIVE attempt, aps_ reason    -> undefined            (dropped)
LIVE attempt, legacy reason  -> ["response_post_failed"]

So every reason this PR introduces is silently discarded: the relayed frame reasons and the five handshake reasons (aps_consumed_tombstone, aps_source_not_in_ad_unit, aps_descriptor_fields, aps_tombstone_capacity, aps_missing_renderer_url) alike. The feature is inert end to end, and delivery stays unknown on exactly the cycles the PR set out to attribute.

The reason the suite does not catch this is that ad_init.test.ts stubs gptDiagnosticsRecorder with vi.fn()s, so the assertions confirm the bridge called the recorder, never that the store accepted the value.

Proposed fix (apply manually - store.ts is not part of this diff, so it cannot be a suggestion):

const CREATIVE_FAILURES: ReadonlySet<GptDiagnosticsCreativeFailure> = new Set([
  'missing_render_source',
  'cache_fetch_failed',
  'invalid_cache_payload',
  'response_post_failed',
  'aps_bad_hash',
  'aps_nonce_mismatch',
  'aps_source_mismatch',
  'aps_descriptor_keys',
  'aps_descriptor_fields',
  'aps_descriptor_envelope',
  'aps_runner_script_error',
  'aps_frame_timeout',
  'aps_frame_load_error',
  'aps_frame_reported_failure',
  'aps_unknown',
  'aps_consumed_tombstone',
  'aps_source_not_in_ad_unit',
  'aps_missing_renderer_url',
  'aps_tombstone_capacity',
]);

function isCreativeFailure(reason: unknown): reason is GptDiagnosticsCreativeFailure {
  return (
    typeof reason === 'string' &&
    CREATIVE_FAILURES.has(reason as GptDiagnosticsCreativeFailure)
  );
}

Worth pairing that with a type-level exhaustiveness assertion, so a future widening of this union cannot again leave the guard behind silently. Something like a const _exhaustive: Record<GptDiagnosticsCreativeFailure, true> keyed off the same list would make the divergence a compile error rather than a runtime no-op. There is currently no test tying the two together, which is what let them drift.

[aram356]

wrench - Recording the response here closes the attempt, so every relayed frame failure is dropped even after the allowlist is fixed.

recordTrustedServerCreativeResponse sets the attempt terminal (store.ts:530-531):

attempt.cycle.trustedServerCreativeResponseAtMs ??= timestampMs;
attempt.status = 'completed';
attempt.cycle = undefined;

and recordTrustedServerCreativeFailure then bails on the first check (store.ts:549):

if (attempt.status === 'completed') return;

Verified against a real store:

COMPLETED attempt, legacy reason -> undefined

The timing makes this unconditional for the relay path. frame_timeout fires 10s after the frame is created (RENDERER_READY_TIMEOUT_MS), and amazon_script_error / descriptor_* only after the frame has loaded and processed the descriptor. Both are strictly later than this line, which runs the moment port.postMessage returns. So every reason arriving through APS_RENDER_FAILED_MESSAGE meets a completed attempt and is discarded. CREATIVE_ATTEMPT_WINDOW_MS (30s) would drop a sufficiently late relay too, independently.

This is the deeper half of the problem, and it is worth stating plainly because it is the PR's own premise: a successful port.postMessage means the markup was handed over, not that the creative drew. Treating the post as completion is exactly the conflation the PR exists to break, so fixing the allowlist alone will not surface a single blank render.

This one needs a design decision rather than a patch, so I will not propose specific bytes. Two shapes that would work:

• Defer safelyRecordCreativeResponse(attemptId) until the renderer frame acknowledges (the renderer-ready path), leaving the attempt live across the window where a blank render is detectable. This matches the semantics best but changes when a healthy render is marked complete.
• Keep this call where it is, and give the store a distinct post-response failure channel so a completed attempt can still carry a render-phase failure. More code, but it does not disturb the existing meaning of completed for the non-APS paths that share it.

Whichever way it goes, a test that drives a real GptDiagnosticsStore through the relay would pin the behaviour down; the current mock-based tests cannot observe it.

[aram356]

wrench - Recording the opportunity at render time mis-attributes the next request cycle on this div.

recordTrustedServerOpportunity does not annotate the current cycle. It plants a pending intent (store.ts:359, via recordRequestIntentSource) which is consumed by the next slotRequested callback (store.ts:594-596):

const intent = this.consumeRequestIntent(slot, timestampMs);
const trustedServerEvidence = intent?.sources.get('trusted_server_direct');
const requestPath = this.requestPath(intent);

Every other caller runs at slot-request time, so the intent describes the request it is attached to. This one runs during a render-time APS handshake, so the intent lands on whatever GPT requests that div next, inside the 5s REQUEST_PATH_ATTRIBUTION_WINDOW_MS. On a page with publisher refreshes, that is a purely publisher-side request.

I confirmed the leak against a real store, driving a publisher request, then a render-time opportunity, then a publisher refresh:

cycle1 requestPath: unattributed              <- real publisher request
cycle2 requestPath: trusted_server_direct     <- publisher refresh, now mis-attributed
        trustedServerOpportunity: renderable_candidate

Two consequences worth weighing. A publisher refresh is now reported as Trusted Server traffic, and if a genuine publisher-side source is also recorded in the same window, intent.sources.size > 1 and requestPath becomes 'competing' (store.ts:1023). The inherited renderable_candidate also feeds the delivery derivation on that later cycle, so it can move a cycle off unknown on evidence that belongs to a different render.

That makes this the most consequential of the five: the other findings mean the diagnostics stay silent, but this one makes them say something untrue, on the same field the investigation is reading.

Proposed direction (apply manually - the fix depends on which store semantics you want, and touches store.ts): the APS path needs a way to associate a slot for attribution lookup without planting a request intent. Splitting the association from the intent inside recordTrustedServerOpportunity, or adding a narrower recorder entry point that performs only the trustedServerSlots registration, would let beginApsCreativeAttempt get the slot association it actually needs for safelyRecordCreativeRequest to resolve, without claiming anything about the next request.

[aram356]

wrench - The renderer document's new 3-key failure message is silently ignored by the direct render path, which is a behavioural regression, not a diagnostics gap.

report() in aps.rs:61-65 now posts three keys:

parent.postMessage({message:'trusted-server/aps/renderer-failed',nonce:nonce,reason:reason},'*');

There are two consumers of that message and only one was updated. The Universal Creative source on the line below matches on m.message with no key-count check, so it is fine. The direct/native consumer in renderApsCreative is not (render.ts:725):

if (event.source !== iframe.contentWindow || !hasExactKeys(event.data, ['message', 'nonce'])) {
  return;
}

hasExactKeys (render.ts:194-205) requires the key set to match exactly, so a third key fails it and the handler returns before ever reaching the RENDERER_FAILED_MESSAGE branch. This path is live: core/request.ts:59 wires renderApsCreative as the trustedServer branch of dispatchApsRendering.

I confirmed it by driving renderApsCreative end to end and posting both shapes from the frame:

AFTER 3-KEY (new) frame still connected: true     <- failure ignored
AFTER 2-KEY (old) frame still connected: false    <- pre-PR behaviour

So on this path an amazon_script_error no longer tears the frame down. Before this PR, script.onerror posted a 2-key message and the frame was cancelled immediately; now the slot sits through the full 10s RENDERER_READY_TIMEOUT_MS before cleanup. The same applies to nonce_mismatch, bad_hash, and the descriptor_* reasons, which are newly emitted here but all dropped.

The new Rust test cannot catch this: it asserts on substrings of APS_RENDERER_DOCUMENT and never touches the TypeScript consumer, and the message shape is precisely the seam between them.

Proposed fix (apply manually - render.ts:725 is outside this diff's hunks, so it cannot be expressed as a suggestion):

function receive(event: MessageEvent): void {
  if (
    event.source !== iframe.contentWindow ||
    !(
      hasExactKeys(event.data, ['message', 'nonce']) ||
      hasExactKeys(event.data, ['message', 'nonce', 'reason'])
    )
  ) {
    return;
  }
  if (event.data.nonce !== nonce) return;
  if (event.data.message === RENDERER_READY_MESSAGE) commit();
  else if (event.data.message === RENDERER_FAILED_MESSAGE) fail();
}

A regression test that posts the 3-key shape and asserts the frame is torn down would keep the two consumers honest with each other.

[aram356]

wrench - Moving the source check below the shape check lets a co-resident script cancel a render that would have succeeded.

Two things changed together here: event.source!==parent moved from the first line of receive to third place, and it now calls report(...), which latches the one-shot reported flag (aps.rs:60-65).

The comment just above says the shape check means "an unrelated sender cannot consume this frame's single report." That holds only against unshaped noise. An adversary who has read these two lines sends {nonce:'x', renderer:{}}, which passes keys(message,['nonce','renderer']), fails the source check, and burns the report permanently. reported is a single var with no reset, so the later, genuine amazon_script_error at aps.rs:122 returns without posting anything.

The more serious half is on the consumer side. The Universal Creative handler treats a renderer-failed message as terminal: fail() sets done=true and calls f.remove() (render.ts:772). So a forged pre-handshake message does not merely burn a diagnostic, it removes the renderer frame of a render that was going to succeed. The direct path is accidentally shielded from this today only by the hasExactKeys mismatch in the finding above, which means fixing that finding without fixing this one would extend the exposure to both paths.

On reachability: the frame is never named and cross-origin siblings cannot enumerate it, so this is not reachable from an arbitrary third party. It is reachable from anything running in the same document as the frame's parent, which on the Universal Creative path is Amazon/GAM-served creative code. That is a real population for an ad-serving surface, even if it is not the open internet.

Proposed fix (apply manually - restoring the guard's position and dropping the report is a two-part edit across the reordered block):

function receive(event){
 if(event.source!==parent)return;
 var message=event.data;
 if(!keys(message,['nonce','renderer']))return;
 if(message.nonce!==expected){report('nonce_mismatch');return;}
 var problem=rendererProblem(message.renderer);
 if(problem){report(problem,message.nonce);return;}

A message from a non-parent source is by definition not attributable to the real render, so reporting it buys no diagnostic value and costs the one-shot budget. That also lets source_mismatch drop out of the reason list, the types.ts union, and the allowlist. If you would rather keep the reason for its own sake, the alternative is to scope reported per phase so a pre-acceptance rejection cannot consume the post-acceptance budget, but not reporting at all is both simpler and strictly safer here.

[aram356]

thinking - renderable_candidate asserts a Trusted Server bid that does not exist on this path.

The only other producer of this value is trustedServerOpportunity(bid) at gpt/index.ts:59-68, which derives it from actual Trusted Server auction targeting: an hb_adid plus either adm or a cache host/path. It means "TS had a bid for this slot and the bid looked renderable."

The APS capability path has no Trusted Server bid at all. The docstring six lines above says so directly: these ad units "never pass through Trusted Server slot mapping." So the literal here records a claim the code has just finished explaining is untrue, and it is the value a reader of ts_console will use to decide whether TS was in the running for that slot.

This is entangled with the blocking finding above, so it may resolve itself: if the association is split from the request intent, the opportunity value may not need to be asserted here at all. If a value is still required, it is worth considering whether the union needs a member that means "TS served a capability handshake, not a bid," rather than borrowing one that means something else. Non-blocking on its own, but worth settling while the surrounding code is being reworked.

[aram356]

thinking - The relay handler applies no source validation, where the sibling path in the same listener does.

The PR description reasons carefully about this block's security posture and the reasoning is sound as far as it goes: the handler is record-only, never answers the sender, resolves the reason through a null-prototype allowlist, and takes the attempt from our own tombstone rather than from the message. I agree none of those is a hole.

The gap is one the description does not mention. The 'Prebid Request' path a few lines below validates the sender against the ad unit's DOM subtree (messageSourceBelongsToAdUnit(e.source, ...), gpt/index.ts:1758), and this block validates neither e.source nor e.origin. There is no origin check anywhere in this listener, so the new block is not bypassing an existing one, but it is the looser of the two siblings.

The practical consequence is diagnostic pollution rather than a delivery risk: any frame on the page that learns or guesses a live adId can attach an arbitrary allowlisted failure reason to that attempt. Since adId values also travel to the creative frame with targetOrigin: "*" (render.ts:771), the set of frames that can learn one is not tiny.

For a subsystem whose output is used to decide why a slot went blank, injectable reasons are worth guarding even though they cannot affect rendering. A source check equivalent to the one the Prebid path already performs would close it, and would cost nothing on the legitimate path since the relay comes from a frame inside the ad unit.

One small aside: this block sits above the data['message'] !== 'Prebid Request' bail, so a Date.now() and a map prune now run for any page message carrying a matching message field. Negligible, but it is a listener on every message the page receives.

[aram356]

thinking - Accepting m.nonce===undefined widens what can cancel a pending render.

main required m.nonce===n on both branches. This line keeps the strict check for renderer-ready but relaxes it for renderer-failed, accepting a message with no nonce at all.

The reason is clear enough from the other side: report('bad_hash') and report('nonce_mismatch') deliberately post without a nonce, correctly, since echoing an attacker-supplied nonce back would be worse. So the relaxation is what makes those two reasons observable, and the intent is sound.

The cost is that a nonce is no longer required to trigger fail(), and fail() is terminal: it removes the frame and rejects the render. The e.source!==f.contentWindow check above still constrains this to the renderer frame itself, so the exposure is narrow and largely overlaps the source_mismatch finding in aps.rs rather than adding much on top of it.

Mostly worth a comment on this line explaining why the unnonced case is accepted, so the asymmetry between the two branches does not read as an oversight to the next reader. If the source_mismatch report is dropped per the finding above, it would also be worth re-checking whether the unnonced case still needs to be terminal, or whether those two reasons could be relayed for diagnostics without cancelling the render.

[aram356]

nitpick - This test is close to a tautology and would not have caught either regression in this PR.

It searches a const &str defined in the same file for substrings the test author also wrote, which limits what it can detect.

Concretely: descriptor_fields appears at six sites in the document and descriptor_envelope at seven. Reverting any five of the six descriptor_fields returns back to return false leaves the string present and the test green. Neutering the body of report entirely to return; also leaves every listed substring intact and the test green. What it does catch is the outright deletion of one of the five singly-occurring reasons, which is real but narrow.

Two assertions are also coupled to minified spelling rather than behaviour: contains("reason:reason") and contains("if(reported)return") both break if someone adds a space after a colon, with no behavioural change. That makes them a source of false failures rather than a safety net.

The negative assertions (!contains("JSON.stringify(renderer)"), !contains("event.source.postMessage")) have more value as guardrails against a specific future mistake, though var s=event.source;s.postMessage(...) would slip past.

None of this is a blocker, and I recognise the constraint: the renderer is a JS program embedded in a Rust string with no JS-side harness, so substring matching is nearly the only tool available in mod tests. Two options that would give real coverage, if it is worth the effort:

• Assert the reason list against the TypeScript allowlist keys, ideally from a shared source of truth. That is the seam where finding 4 lives, and a test of that shape would have caught it.
• Or move APS_RENDERER_DOCUMENT somewhere it can be evaluated under vitest with a fake parent, which is exactly the technique render.test.ts:830-874 already uses successfully for APS_UNIVERSAL_CREATIVE_RENDERER. That would let the guards be tested by behaviour instead of by spelling.