Register APS renderer via requestId so it survives Prebid field stripping - #963
Register APS renderer via requestId so it survives Prebid field stripping#963aram356 wants to merge 10 commits into
Conversation
…ping 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. Also stash the descriptor keyed by `requestId` (a first-class field Prebid preserves) when the bids are built, and have the bidResponse listener fall back to that stash when the custom field is absent. Additive: the existing field path is unchanged, so cases where the field survives behave exactly as before; the fallback engages only when Prebid has stripped it. Bounded map. Adds a unit test that registers via requestId with the custom field removed.
auction/orchestrator.rs:
- Combined imports (http::Request + std::collections::{HashMap, HashSet}).
- Kept main's post-launch backend-name collision defense and resolved-name
correlation on both parallel and sequential dispatch, while preserving #918's
per-provider effective_timeout in the backend_to_provider 4-tuple (declarations
and read sites already expect the 4th element).
- Took main's test provider fields (configured_timeout_ms / predicted_timeouts)
and its DivergentBackendProvider; updated a #918 test stub to the merged struct
shape (configured_timeout_ms: 125 to preserve its capped-launch-timeout assertion).
publisher.rs:
- Advertise the configured publisher_domain in the page URL (main's fix; the edge
Host must not leak into the bid request per the in-code comment) using #918's
request_path_and_query field (the field the merged MatchedSlotsContext exposes);
updated a stale test literal accordingly.
Verified: cargo check (axum) clean; orchestrator + build_auction_request tests pass.
…aps-renderer-requestid-registration
d6caf2f to
b00655e
Compare
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The requestId fallback is directionally correct, but this PR is not safe to merge in its current state. The stacked base was rebased: GitHub reports the PR as conflicting, the merge base is stale, and the displayed change set has expanded to 43 files rather than the intended two-file fix. The head omits current-base commits covering publisher streaming, inline SSAT rendering, APS security review fixes, and rebased CI fixes; the final-tree comparison removes approximately 1,426 lines from streaming_processor.rs alone.
Please recreate or rebase this branch from the current origin/issue-764-aps-openrtb, then cherry-pick or reapply only 7406ed7d. Confirm that the resulting PR contains only the intended Prebid implementation and test changes, and rerun the complete CI gates. The currently green integration checks predate the latest base commits and do not validate the conflicting merge result.
| }) | ||
| } | ||
|
|
||
| fn debug_headers(headers: &HeaderMap) -> BTreeMap<String, Vec<String>> { |
There was a problem hiding this comment.
🔧 The stale head reintroduces client-visible leakage of all APS upstream response headers
debug_headers copies every upstream response header into APS debug metadata, which is returned to page JavaScript. This can expose Set-Cookie, identity headers, internal tracing data, or authentication-related metadata whenever APS debugging is enabled. The current base intentionally fixes this with a fail-closed allowlist containing only Content-Type.
Please preserve that allowlist while rebasing. Any additional debug headers should be explicitly reviewed and redacted rather than copied wholesale.
| } | ||
| // Prefer the custom field; fall back to the requestId stash when Prebid has stripped | ||
| // it during bid normalization (the field is often gone before this listener runs). | ||
| const renderer = bid[APS_RENDERER_FIELD] ?? takePendingApsRenderer(bid['requestId']); |
There was a problem hiding this comment.
🔧 Pending renderer descriptors are not consumed when the custom field survives
Because ?? short-circuits, takePendingApsRenderer is never called when trustedServerRenderer survives normalization. The descriptor stashed earlier therefore remains until FIFO eviction. A renderer envelope may contain up to 349,528 base64 characters, so retaining 256 entries can preserve roughly 85 Mi characters on a long-lived publisher or SPA page; it also leaves stale descriptors available if a request ID is reused.
Consume the pending value before selecting the primary/fallback renderer:
| const renderer = bid[APS_RENDERER_FIELD] ?? takePendingApsRenderer(bid['requestId']); | |
| const pendingRenderer = takePendingApsRenderer(bid['requestId']); | |
| const renderer = bid[APS_RENDERER_FIELD] ?? pendingRenderer; |
Please also add a regression test that registers through the surviving custom-field path, then sends a field-stripped event with the same request ID and verifies that no stale renderer is registered again.
2 participants
Fixes #962. Stacked on #918 (targets
issue-764-aps-openrtb).Problem
APS bid-by-reference renderers never register, so every APS bid renders empty and Prebid throws
Missing ad markup or URL(reasonnoAd).interpretResponsesets the renderer descriptor on the bid as the customtrustedServerRendererfield; thebidResponselistener registers it intowindow.tsjs.apsPrebidRendererskeyed by the generatedadId. But Prebid drops unknown top-level fields when it normalizes bids duringaddBidResponse, so the custom field is gone before the listener runs (absent as early asbidAccepted). The listener seesrenderer === undefined, returns, and the registry stays empty.Fix (minimal, additive)
Stash the descriptor keyed by
requestId— a first-class field Prebid preserves — when the bids are built, and have thebidResponselistener fall back to that stash when the custom field is absent.interpretResponse(auctionBidsToPrebidBids):if (renderer) stashPendingApsRenderer(requestId, renderer)bidResponselistener:const renderer = bid[APS_RENDERER_FIELD] ?? takePendingApsRenderer(bid['requestId'])Map(256 entries);takedeletes on read.Additive by design: the existing custom-field path is unchanged, so cases where the field survives (e.g. unit/browser harness) behave exactly as before. The fallback engages only when Prebid has stripped the field. No new dependency; the bridge, descriptor format, server, and render path are untouched.
Verification
requestId.noAdfailures dropped sharply, and APS creatives rendered in-slot instead of erroring.