Add first-class APS OpenRTB integration

[ChristianPavilonis]

Summary

• Replace the legacy APS contextual/TAM request path with a first-class APS OpenRTB provider using /e/pb/bid.
• Carry minimized, typed APS renderer state through direct auctions, navigation/page-bids, mediation, and GPT/Universal Creative rendering.
• Add default-off script-creative gating, opaque-origin rendering, nonce-bound messaging, PBS coexistence safeguards, migration docs, and regression coverage.

Changes

File	Change
crates/trusted-server-core/src/integrations/aps.rs	Builds independent APS OpenRTB requests, validates and reduces responses, creates minimized renderer envelopes, and serves the sandboxed static renderer.
crates/trusted-server-core/src/auction/*	Adds typed APS renderer metadata, preserves bid identity through floors and orchestration, and exposes APS winners without adm.
crates/trusted-server-core/src/integrations/prebid.rs	Prevents APS from also being sent to Prebid Server for Trusted Server APS cohorts.
crates/trusted-server-core/src/{publisher.rs,creative_opportunities.rs}	Carries APS renderer data through navigation/page-bids while retaining legacy slot configuration only for compatibility.
crates/trusted-server-js/lib/src/integrations/aps/render.ts	Validates exact renderer descriptors and envelopes, creates opaque sandboxed frames, and enforces fragment-bound nonces and readiness acknowledgements.
crates/trusted-server-js/lib/src/integrations/gpt/index.ts	Routes APS winners through the Universal Creative dynamic-renderer bridge without cache fetches, beacons, or native APS display calls.
crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts	Verifies CSP sandboxing, nonce and replay protections, same-origin rejection, restrictive-CSP compatibility, and parent-DOM isolation.
trusted-server.example.toml and integration fixtures	Adds canonical account_id, the APS OpenRTB endpoint, and default-off allow_script_creatives.
docs/**, CHANGELOG.md, TESTING.md	Documents migration, security boundaries, orchestration behavior, rollout gates, and validation coverage.

Closes

Closes #764

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: cargo test-cloudflare, cargo test-spin, all Cloudflare/Spin clippy aliases, 13 cross-adapter parity tests, browser TypeScript compilation, full Next.js/WordPress Playwright suite, TSJS bundle build, and VitePress build

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses project log macros (not println!)
• New code has tests
• No secrets or credentials committed

Rollout note

Broad production rollout and production tagtype=script enablement remain gated on controlled APS-account validation and APS account-team confirmation. Script creatives remain disabled by default.

[aram356]

Can we add APS metadata to result of the auction?

[aram356]

Summary

Replaces the legacy APS TAM/contextual path with a first-class OpenRTB provider, adds a sandboxed opaque-origin creative renderer, a Prebid-adapter rendering route, PBS coexistence safeguards, and an inventory-identity override. The security core is well-hardened and I verified rather than assumed it — allow-same-origin is never combined with allow-scripts (pinned by a regression test), the nonce/envelope contract matches byte-for-byte across Rust and JS, and the two CodeQL alerts were genuinely resolved. CI is green.

That said, I found 7 blocking defects — an auction-wide failure from one malformed bid, two ways APS demand can still reach Prebid Server, two blank-slot / consumed-impression bugs in the new rendering routes, a privacy leak in the identity override, and a silent config break on upgrade. Requesting changes.

A process note: the PR body's change table documents only the first commit. Two later commits ("Add APS inventory identity overrides", "Render APS bids from the trustedServer Prebid adapter") added whole subsystems it omits — worth refreshing before merge.

Most findings are filed as inline comments. The cross-cutting ones are below.

Blocking

🔧 wrench

• APS exclusion is case-sensitive and leaks via the head-insert path (prebid.rs:1427/1434, prebid.rs:909): Both PBS guards compare name == "aps" exactly, so "APS"/"Aps" in the free-form config.bidders slips through to PBS and is sold twice. Separately, head_inserts serializes config.bidders/client_side_bidders verbatim into window.__tsjs_prebid, so aps still ships to the browser — the safeguard is server-side only, and client_side_bidders is never filtered at all. The durable fix for this and the stored-request finding is to reject aps in [prebid].bidders/client_side_bidders at config-validation time.

Non-blocking

🤔 thinking

• Full client-controlled Referer path+query now enters the bidstream (formats.rs:93-116): site.page changed from a bare https://{domain} to the full same-origin Referer including query string, so publisher URLs carrying PII (?email=, session tokens, gclid) are forwarded to every SSP — up to 8 KiB of attacker-chosen data (the host check constrains only the authority). This mirrors client-side Prebid so it may be intentional, but for a privacy-preserving edge server it deserves an explicit decision: strip query/fragment by default, or gate behind config.
• adserver_mock mediation is last-write-wins and the new test enshrines it (adserver_mock.rs:107, :899): build_bid_index keys on (provider, slot, bidder); if APS ever stops reducing to one bid per slot, the mediator restores the losing candidate's renderer against the winning bid's price — wrong creative at wrong price, silently. bid_id now exists and would disambiguate; the mock only echoes crid. At least raise the collision log from debug! to warn!.
• providers.aps is now write-only and fails silently (creative_opportunities.rs:366,378): the field is a genuine deny_unknown_fields deser-compat shim (removing it hard-fails existing TOML), but to_ad_slot no longer reads it, so an operator who sets aps.slot_id expecting routing gets a no-op with zero diagnostics. Add a load-time warning.

♻️ refactor

• BidRenderer::aps() is an infallible accessor on a single-variant enum (types.rs:217-225, used at publisher.rs:1957): the moment a second variant lands the natural "fix" is a panic in the fallback arm. Prefer as_aps(&self) -> Option<&ApsRendererV1>. Note the generic bid.bid_id field this PR adds already carries the same value, so publisher.rs could derive hb_adid renderer-agnostically.

📝 note

• Undocumented /auction wire-contract change: for renderer bids, crid goes from always-present {bidder}-creative to bid.creative_id (absent when the bidder omits crid), id from {bidder}-{slot} to the upstream bid id, and adm is omitted entirely. tsjs absorbs all three, but POST /auction is a documented OpenRTB endpoint; any non-tsjs consumer reading those fields breaks. The CHANGELOG's Breaking section doesn't mention the response shape.
• Test quality across the new suites: the browser same-origin rejection test (aps-renderer.spec.ts:731-805) is effectively vacuous — a fixed waitForTimeout(100) with no positive control, driven through a hand-rolled harness instead of renderApsCreative, so it would pass if the guard broke. Several sandbox assertions (:464,:555) assert the harness's own input rather than product behavior. And there is no JS-side boundary coverage for the size caps (envelope 256 KiB / base64 349528, account-id 1024, creative-url 4096) nor a regression test for the two blocking rendering-route bugs. (I did confirm the same-origin guard at aps.rs:74 is live, not dead code, via a browser test — so that check is real; the test just doesn't exercise it.)

⛏ nitpick

• creative_id is the only renderer field without a length cap (aps.rs:682) — bounded by the 2 MiB body cap but inconsistent with account_id/creative_url.
• dropped_bid_count and drop_reasons don't reconcile (aps.rs:776-799) — price-losers increment the count with no reason entry; the test asserts dropped=1, drop_reasons={}.
• auction/README.md:120 ASCII box right border is off by one column.
• trusted-server.example.toml regressed a fictional aps.example.com/e/dtb/bid to a real vendor host; several docs/tests use real *.amazon-adsystem.com endpoints against the project's "example.com only" rule. These are functionally-required vendor endpoints (not customer data or credentials) and amazon-adsystem predates this PR, so noting rather than blocking.

CI Status

• fmt: PASS
• clippy (all six adapter targets): PASS
• rust tests (fastly/axum/cloudflare/spin + parity): PASS
• js tests (vitest): PASS
• browser integration + CodeQL: PASS

[prk-Jr]

Summary

First-class APS OpenRTB integration replacing the legacy contextual/TAM path, with a sandboxed (opaque-origin, nonce-gated, CSP-enforced) renderer that deliberately withholds adm from the client outside that sandbox. The core security design (bid parsing never reads adm, envelope byte-bounds, iframe sandbox without allow-same-origin, identity-checked postMessage, server-side CSP header) is solid and well-tested, including a genuinely adversarial browser test suite (nonce replay, CSP-only isolation with the sandbox attribute omitted, live same-origin attack attempts). The main blocker is that this branch does not currently merge cleanly with main — see Blocking #1, which is also the root cause of the two failing CI checks.

Blocking

🔧 wrench

• PR does not merge cleanly with main: main's commit 8bb5450dc (#890) added a make_test_bid_with_creative() helper in publisher.rs that builds a Bid{} literal without this PR's new bid_id/creative_id/renderer fields. Rebasing/merging onto current main fails with error[E0063]: missing fields — this is exactly what's failing the cargo fmt and cargo test CI checks shown on this PR (GitHub builds the synthetic merge commit, not the raw branch tip). See inline comment on auction/types.rs.

❓ question

• Debug mode can leak adm to the browser (crates/trusted-server-core/src/integrations/aps.rs:628): undermines the PR's core claim that adm never reaches the client outside the sandbox. See inline comment.
• One bad bid can abort the entire /auction response (crates/trusted-server-core/src/auction/formats.rs:313): no multi-slot blast-radius test exists. See inline comment.

Non-blocking

♻️ refactor

• Bid (types.rs:253) has no #[derive(Default)] despite every field type supporting it — deriving it directly prevents recurrence of Blocking #1 the next time a field is added.
• aps.rs:1169 — no test exercises the fail-closed startup path (enabled+invalid config → register_providers() returning Err), only ApsConfig::validate() directly.
• gpt/index.ts:999 — renderingAdIds grows unbounded on the success path, inconsistent with sibling bounded caches added elsewhere in this PR.

🤔 thinking

• aps.rs:1047 — context-free parse_response silently discards a valid response if ever called without context; safe today only because all real call sites use parse_response_with_context.
• aps.rs:380 — device.language has no explicit byte cap, unlike other forwarded fields in this file.
• orchestrator.rs:28 — backend_to_provider's bare 4-tuple is duplicated across 3 sites; same fragility class as Blocking #1.
• formats.rs:302 — when both bid.creative and bid.renderer are Some, creative silently wins with no log/assert.
• prebid.rs:1427 — the "aps" bidder exclusion (preventing APS double-serving through Prebid Server) is case-sensitive; pre-existing pattern, worth eq_ignore_ascii_case given the stated goal.
• publisher.rs:2207 — full client query string is now forwarded into site.page for every provider; unlike aps.rs (8192-byte cap), prebid.rs's equivalent has no bound.
• render.ts:325 — rapid re-render of the same slot orphans the prior frame's message listener/timeout instead of routing through its own cleanup (self-heals within 10s).
• TESTING.md:49 — still describes APS as "mocked" / lists "Implement real APS" as a future step, contradicting this PR and the log excerpt this PR itself just updated a few lines above.
• docs/guide/auction-orchestration.md:623 — config reference table omits inventory_domain/inventory_page_origin, real validated fields documented elsewhere in this PR.

⛏ nitpick

• aps.rs:851 — a seatbid entry with no bid array is skipped without incrementing drop_reasons, making a no-bid response harder to diagnose.
• render.test.ts:216 — no test for "correct nonce, wrong event.source" rejection specifically (the guard is implemented correctly at render.ts:351, just not locked in by that exact case).
• render.ts:388 — the embedded renderer-document string hand-duplicates constants (including the 10000ms timeout) instead of interpolating them.

🌱 seedling

• Byte-limit constants (MAX_ACCOUNT_ID_BYTES, etc.) exist as 3 independently hardcoded copies across Rust, TS, and the embedded HTML string — worth a single source of truth eventually.
• creative_opportunities.rs:366 — legacy providers.aps.slot_id is now a silent no-op with no operator-facing warning if left configured.
• aps-renderer.spec.ts:169 — no browser-level test for cross-ad-unit capability theft (the messageSourceBelongsToAdUnit guard exists and works, just isn't e2e-tested against a mismatched ad unit).

📝 note

• auction/README.md:364 — this example is stale relative to the new renderer flow (pre-existing drift, not touched by this PR).

👍 praise

• aps.rs: parse_bid never reads adm; envelope is byte-bounded and fixture-verified byte-for-byte between Rust and TS; CSP header (APS_RENDERER_CSP) is correctly sent server-side with no allow-same-origin.
• orchestrator.rs: provider_request_context/effective_timeout threading fix, pinned by dispatched_collection_reuses_provider_launch_context.
• render.ts: validateApsRenderer's exact-key allowlist cross-checking the outer descriptor vs. the decoded OpenRTB payload; the event.source === iframe.contentWindow identity check (the right check for an opaque-origin sandbox).
• prebid.rs: the trustedServer bidder-expansion path correctly re-strips "aps" even when reintroduced via config.bidders, with a dedicated regression test.
• aps-renderer.spec.ts: nonce-replay, CSP-only isolation (with the sandbox attribute omitted), and same-origin attack tests are genuinely adversarial — real attempts against a live server, not tautological checks of config strings.

CI Status

• fmt: FAIL (caused by the merge-conflict issue above, not this branch's own code)
• rust tests: FAIL (same root cause)
• clippy (all adapters): PASS
• cross-adapter parity: PASS
• browser/integration tests: PASS
• vitest: PASS
• CodeQL: PASS

[aram356]

Summary

Re-reviewed at 10cd6d3ce. Every finding from the previous review is fixed — I verified all seven blocking items and the non-blocking ones in source, and several were implemented exactly as suggested. CI is green across all 19 checks.

Two new blocking issues, both introduced by work landed since the last review: the fix for the case-sensitive APS exclusion over-corrected into a config hard-fail that can take a service down on upgrade, and the new debug-metadata commit captures upstream response headers without the fail-closed filtering this codebase applies elsewhere. Both have small fixes.

Blocking

🔧 wrench

• The aps config rejection is an undocumented hard-fail that bricks every request (prebid.rs:346-368): config.validate() runs at settings.rs:257 before the is_enabled() check at line 265, and IntegrationRegistry::new (registry.rs:791) propagates the error with ?. An operator whose published config contains bidders = ["aps", "kargo"] — previously harmless, silently stripped — deploys this WASM and every request fails, including when the prebid integration is disabled entirely. The CHANGELOG Breaking entry says operators must "disable native APS demand" but never states the config will refuse to boot; docs/guide/configuration.md doesn't either. This PR's own tests construct ["kargo", "aps"], so it is a config the codebase expects to exist in the wild. The hard-fail is also redundant with the runtime filter added in the same commit (lines 1571/1578 already strip aps case-insensitively). Either downgrade to warn-and-strip, or keep the error and add a dedicated **Breaking** CHANGELOG line plus a docs migration note — and confirm ts config push rejects it before the runtime does.

Non-blocking

🤔 thinking

• Skipped winners are still counted as wins in telemetry (formats.rs:341,351): emit_auction_events_best_effort_lazy(... Completed { result }) runs at auction/endpoints.rs:320, before response conversion, and telemetry.rs:697-711 emits is_win = 1 per winning bid. A bid dropped by the new continue is reported as a win in analytics but never delivered, and endpoints.rs:331 still logs the pre-drop count. There's also no machine-readable drop counter — only a log::warn! — which is inconsistent with the drop_reasons metadata this same PR adds on the APS side. Consider counting skips into ResponseExt.orchestrator, or emitting the terminal event after conversion.
• An all-APS page now reports a Prebid provider error rather than a no-bid (prebid.rs:1598-1604): when every slot is APS-only the imp vector is empty and the existing guard returns Err(TrustedServerError::Prebid), surfacing as BidStatus::Error in provider_details. That's a normal configuration, not a failure, and it will pollute provider error rates and alerting. Suggest an explicit no-bid response for "all imps intentionally dropped", reserving Err for "no valid banner formats".
• Debug body capture inherits the 2 MiB upstream cap (aps.rs:945-956, 970-978): captured bytes pass through String::from_utf8_lossy (3 bytes per invalid byte) and then JSON escaping, so an adversarial 2 MiB upstream body can inflate the /auction response to several MB, buffered in WASM heap. Per-auction rather than per-bid, so it doesn't accumulate, but a dedicated smaller preview cap mirroring MAX_BID_CREATIVE_DUMP_BYTES would be safer.
• The Referer privacy fix is only half-applied: /auction sanitizes, but the SSR path builds publisher.page_url at publisher.rs:2018 as scheme://host{request_path_and_query} — full query string and edge host, unfiltered — and passes it onward. Worth routing both producers through one sanitizer. Related: HeaderValue::from_str failing on a non-ASCII URL silently drops Referer with no log.
• Debug requestbody is reconstructed, not captured (aps.rs:596-605): debug_request() re-runs build_openrtb_request at parse time and presents the result as what was sent. That holds only while the function stays pure. If it ever gains a uuid, timestamp, or any mutation between build and send, debug will silently report a payload APS never received and an operator will chase a fabricated request. Worth a test asserting the debug body byte-matches the captured outbound body, plus a doc comment recording the invariant.
• One browser assertion passes vacuously (aps-renderer.spec.ts:364-366): expect(result.foreignUniversalCreativeResponse).not.toEqual(expect.objectContaining({ apsRenderer })) succeeds when the promise times out to undefined, and would also succeed if the bridge served a slightly different renderer. The intended contract is "a foreign frame gets nothing" — assert toBeUndefined().

♻️ refactor

• validateApsRenderer now runs 3–4× per APS bid on the critical path: admission (prebid/index.ts:213), registration (render.ts:210), and the GPT bridge (gpt/index.ts:998). Each pass allocates a TextEncoder, base64-decodes up to 256 KB, and JSON.parses it. The admission call is new. Consider caching the validated descriptor on first success.
• Four-element anonymous tuple in backend_to_provider (orchestrator.rs:28, 460, 877-880): (String, Instant, Arc<dyn AuctionProvider>, u32) destructured as (provider_name, start_time, _, _) at five call sites, with an un-self-describing u32. A named struct would match the convention the project applies to argument lists.

📝 note

• Two trade-offs here came from my own suggestions last round — flagging so they get an explicit decision rather than sliding in as side effects. (1) Suffix host matching (formats.rs:110-119) is implemented correctly — the prefix.ends_with('.') check properly rejects evilpublisher.example — but it does mean any subdomain can now determine the site.page every SSP sees; on a deployment with user-content subdomains that is worth a conscious accept. (2) Unconditional set_query(None) is good for privacy but collapses every page of a query-driven site (/index.php?id=123) to a single URL, degrading contextual targeting and per-page reporting. Consider a key allowlist or a config switch, and document the trade-off either way.
• debug flag discoverability (aps.rs:132-136): the one-line doc comment carries no risk warning, whereas the analogous flags in settings.rs:1897-1923 each have multi-line "never enable in production" rustdoc. An operator auditing the [debug] section also won't find this switch, since it lives under [integrations.aps]. Worth mirroring the stronger wording.

⛏ nitpick

• attach_debug_metadata is skipped only on the missing_request_context branch (aps.rs:996-1002), contradicting the docs' claim that the exchange is emitted for success, 204, malformed and non-success statuses. Unreachable today, so latent rather than live.
• The debug requestheaders map is a hardcoded content-type: application/json (aps.rs:604-608) rather than the headers actually sent at line 1042. They agree today and will drift; it also slightly misrepresents the PBS httpcalls shape it mirrors.
• On failed renderer registration the descriptor is now retained on the bid (prebid/index.ts:546-552). I checked reachability and it is effectively defensive-only — admission already validated the descriptor, adId is Prebid-generated, and validPrebidIdentity accepts GAM-style codes — so this is hygiene, not a live bug.
• publisher.rs:2139 still uses expect("should serialize typed renderer") while formats.rs:331-342 was hardened to handle the identical serialization failing; worth resolving the asymmetry in one direction.

CI Status

• fmt: PASS
• clippy (all six adapter targets): PASS
• rust tests (fastly/axum/cloudflare/spin + parity): PASS
• js tests (vitest): PASS
• browser integration + CodeQL: PASS

[aram356]

Pushed bbaf5735a to this branch to fix the CI failures (all three workflows — Run Tests, Run Format, Integration Tests — were failing at compile on 0cb06efdb).

Root cause: when the APS stack was rebased onto main, the conflict resolution in ece42a022 resolved publisher.rs against its pre-streaming shape (−3,905 lines). That silently reverted the publisher-side integration of #867 (async end-to-end streaming) and the inline SSAT adm path — both merged in main — while streaming_processor.rs kept the full #867 machinery. The orphaned BodyStreamDecoder/BodyStreamEncoder family then tripped -D warnings dead-code errors in every workflow, so no tests ran on the branch. Everything else from #867 (settings, platform trait, fastly adapter hooks, proxy.rs) had survived the rebase intact; the damage was confined to publisher.rs.

The fix is a three-way re-merge of publisher.rs (base: pre-streaming main 5efa34c95, ours: branch tip, theirs: main). Only 3 conflict regions, all in build_bid_map:

• main's signature (settings, request_origin, include_debug_bid), zero-dimension w/h omission, and always-inline sanitized adm are restored, with the APS renderer field and renderer-aware hb_adid fallback (cache_id → renderer bid_id → ad_id) reapplied on top
• the APS bid-map test moves to the new signature; the include_adm flag it exercised is superseded by main's debug_bid design

Also fixed a masked test regression: debug_metadata_preserves_malformed_and_error_responses still asserted retry-after appears in APS debug headers, which the fail-closed allowlist from the security review deliberately redacts. The compile failure had hidden this since the allowlist landed. The test now asserts content-type is preserved and retry-after is redacted.

Verification (local): cargo fmt --check, fastly release wasm build with -D warnings, test-fastly (1745 core tests), test-axum, test-cloudflare, test-spin, cloudflare wasm check with -D warnings, and the parity suite all pass. Worth a live Fastly smoke of the streaming path since publisher.rs is the merge of two large features — the unit/parity suites cover it, but the end-to-end TTFB behavior was the point of #867.

[prk-Jr]

Summary

The APS OpenRTB integration itself is in good shape: the typed BidRenderer contract, the dual Rust-build/JS-revalidate envelope check, the delivery report that stops dropped winners from being counted as wins, and the browser suite with real positive controls all read well, and every finding from the previous rounds that was marked "fixed" verifies as fixed.

The blocker is not in the APS code. crates/trusted-server-core/src/integrations/prebid.rs and crates/trusted-server-core/src/auction/orchestrator.rs are based on a pre-merge revision and lost work that is currently on main. The merge base is bf61fa189 (current main tip), so this is not a stale-branch artifact that a merge will fix — the deletions are in the diff. CI is green because the regression tests that covered the deleted code were deleted with it.

What is gone, versus origin/main:

Deleted at HEAD	Source on main	Effect
is_unusable_bidder_params + the deterministic expanded/direct merge + 9 tests	2dc55d755 (#899)	Fabricated {} bidder params reach PBS; real inline params can be clobbered by HashMap order
Duplicate-provider and mediator-as-provider startup validation + 2 tests	orchestrator.rs:200–230	Double-dispatch and double-called mediator are accepted again
Pre-launch and post-launch backend-name collision guards + 4 tests	orchestrator.rs:540, :1006	Colliding backend names silently overwrite backend_to_provider
All 11 canonicalize_transport_timeout_ms call sites + 4 tests	#847 work	Budget-derived timeouts are no longer quantized, so backend names churn per request

Recovering those two files onto the current main versions and then re-applying the APS-specific edits on top is the smallest safe path. Everything else here can land as-is.

Blocking

🔧 wrench

• Fabricated empty bidder params ship to PBS again — is_unusable_bidder_params and its drop pass were deleted; expand_trusted_server_bidders still fabricates {} for configured bidders with no inline params (prebid.rs:1610). Ported main's own test onto this HEAD and it fails.
• Real inline bidder params can be clobbered nondeterministically — the expanded/direct split that made the merge order-independent is gone; bidder.extend(...) now races the direct insert on slot.bidders iteration order (prebid.rs:1567–1596).
• Backend-name collisions and duplicate providers are no longer rejected — orchestrator.rs:550 / :976 insert unconditionally; the startup validation at orchestrator.rs:228 and both collision guards are gone.
• AuctionBidData no longer declares w/h, and its adm doc is now wrong — gpt/index.ts:1149-1150 still reads both fields (two new tsc errors), and adm is emitted whenever a creative survives sanitize, not only under inject_adm_for_testing.

Non-blocking

🤔 thinking

• Missing price still aborts the whole /auction response while a missing render source now drops one bid (formats.rs:344). Worth unifying under the new AuctionDeliveryReport.
• Debug requestheaders is now near-empty — the response allowlist is also applied to the outbound request, so only content-type survives (aps.rs:638). Combined with the still-reconstructed requestbody, the debug block no longer shows what was actually sent.

🌱 seedling

• The external Prebid bundle is not deployed by the deployer — CI now builds build:prebid-external for the browser suite, but trusted-prebid.js still ships out of band, so the APS registry changes in prebid/index.ts can half-ship in production.
• The renderer document is served with no cache headers (aps.rs:1216), so every render refetches it.

👍 praise

• Envelope cross-validation on both sides — the Rust-built envelope and the JS re-validation check every duplicated descriptor field against the decoded bid, and the base64 round-trip closes the non-canonical-encoding gap from the last round.
• The browser suite proves its negative with a positive control in the same test (aps-renderer.spec.ts:361–372) — the foreign Universal Creative request is asserted undefined next to a full toEqual on the legitimate one, so the rejection assertion cannot pass vacuously.
• Delivery report plumbed into telemetry — delivered_winner_slots closes the "skipped winner still counted as a win" gap end to end, across /auction, /_ts/page-bids, and both buffered paths.

CI Status

All 19 checks pass on 488f7b108.

• fmt: PASS
• clippy (fastly / axum / cloudflare / cloudflare-wasm / spin native / spin wasm): PASS
• rust tests (fastly, axum, cloudflare, spin, parity, ts CLI): PASS
• js tests (vitest): PASS
• integration + browser integration tests: PASS
• format-typescript / format-docs / CodeQL: PASS

Note that green CI does not cover the four regressions above: the tests that covered the deleted code were deleted in the same commit (ebc0a8070), and there is no tsc --noEmit gate, so the two new type errors are invisible to CI.

[aram356]

Summary

Re-reviewed at 0cb06efdb. Both round-2 blockers are fixed (the config hard-fail became warn-and-strip; the debug header capture became a fail-closed allowlist), and the round-1 render-bridge fixes survived the rewrite. But the rebase onto newer main and the 368-line gpt/index.ts rewrite introduced blocking regressions — two of them revert already-merged PRs (#865 and #899) and silently removed those PRs' regression tests in the same diff, which is why CI stays green.

Note on CI: all 19 checks pass, but that is not sufficient signal here — two of the blocking findings are reverted merged changes whose guarding tests were deleted alongside the code.

Two blocking findings and one non-blocking are cross-cutting and appear below; the rest are inline.

Blocking

🔧 wrench

• The rebase reverted merged PR #865 (auction transport-timeout quantization) across the shared orchestrator, tests included (orchestrator.rs:320, 487, 904, 1187): all four transport-timeout computations now use remaining_ms.min(provider.timeout_ms()) instead of context.services.backend().canonicalize_transport_timeout_ms(remaining_ms, provider.timeout_ms()). I verified the causal chain rather than inferring it: the merge-base b5e9771107 and current origin/main both contain the quantization (11 references each), #865 (5efa34c95) is an ancestor of the base, and the base...HEAD diff deletes it (27 removed lines) along with #865's orchestrator regression test (the test backend overriding canonicalize_transport_timeout_ms plus its assertions, previously at orchestrator.rs:2260-2379). On Fastly the first-byte/between-bytes timeout is embedded in the dynamic backend name; without quantization each request's near-unique remaining_ms mints a new dynamic backend name for every provider (prebid, APS, mediator), defeating connection pooling and accumulating registrations toward Fastly's per-service dynamic-backend limit — exactly what #865 was merged to prevent. This regresses the whole auction path, not just APS. Fix: re-apply the four canonicalize_transport_timeout_ms calls (mediator_context.timeout_ms at 320, the two provider-loop effective_timeout bindings at 487/904, and mediator_timeout at 1187) and restore #865's orchestrator test so the regression cannot recur silently.

• This PR reverts merged PR #899's inline-render sizing and deletes its tests, leaving a broken client read (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1056, crates/trusted-server-js/lib/src/core/types.ts, crates/trusted-server-core/src/publisher.rs): #899 established inline SSAT sizing from the winning bid's own dimensions — AuctionBidData declared w?/h?, the server bid-map builder emitted top-level "w"/"h", and tests asserted the round-trip. This PR's base...HEAD diff removes all three: the w?/h? type fields (types.ts:56/58 deleted), the obj.insert("w"/"h", …) server emission (publisher.rs, deleted), and #899's round-trip tests (obj.get("w").is_none() etc., deleted) — while leaving const width = matchedBid.w ?? fallbackWidth (gpt/index.ts:1056) untouched. Because matchedBid.w/.h are now always undefined (the values survive only inside the debug-only debug_bid blob), every inline SSAT render falls back to slot.formats[0], so a multi-size slot whose winner is not the first configured format renders in a wrong-sized iframe. The PR also adds a new test (ad_init.test.ts:1802) that sets w:300,h:250 on a fabricated bid and asserts width===300, which passes vacuously and masks the removal; no tsc runs in CI to catch the now-undeclared-property read. Fix: restore #899's server-side w/h emission and the AuctionBidData fields (and its round-trip tests), or if the relocation into debug_bid is intentional, source the dimensions the bridge actually uses and drop the dead matchedBid.w read plus its misleading comment/test.

Non-blocking

🤔 thinking

• An all-APS page surfaces as a Prebid provider error rather than a no-bid (prebid.rs:2315): when every slot is APS-only the imp vector is empty and the existing guard returns Err(TrustedServerError::Prebid), showing up as BidStatus::Error in provider_details. That's a normal configuration mid-migration, not a failure; it will inflate provider error rates and alerting. (Raised in round 2; still open.) Suggest an explicit no-bid AuctionResponse for the "all imps intentionally dropped" case, reserving Err for "no valid banner formats".

CI Status

• fmt / clippy (six adapter targets): PASS
• rust tests (fastly/axum/cloudflare/spin + parity): PASS — but see the two reverted-merge findings: #865's and #899's regression tests were removed in this diff, so green here does not cover those behaviors
• js tests (vitest): PASS
• browser integration + CodeQL: PASS

[prk-Jr]

Summary

The APS OpenRTB implementation has no additional blocking findings in this review, and the branch-level Rust verification passed. However, the PR currently conflicts with main across core auction, publisher rendering, TypeScript integration, tests, configuration, and documentation, so it cannot be merged safely in its present state.

Blocking

🔧 wrench

• Resolve conflicts with current main: GitHub reports this PR as CONFLICTING / DIRTY, and a merge analysis confirms conflicts in important paths including auction/formats.rs, auction/orchestrator.rs, publisher.rs, gpt/index.ts, and their tests. Please rebase or merge current main, preserve both the APS behavior and newer main functionality during resolution, then rerun the full CI suite.

CI Status

• fmt: PASS locally
• Fastly/Core Rust tests: PASS locally (111 adapter, 1,764 core, 21 OpenRTB)
• Axum tests: PASS locally (32 tests)
• CodeQL JavaScript/TypeScript analysis: PASS on GitHub
• JavaScript tests: local run blocked before test execution by the existing dependency tree; rerun through clean CI after conflict resolution
• Remaining GitHub CI checks: not reported for the current head

[prk-Jr]

Summary

A large, well-structured replacement of the legacy APS contextual path with a first-class APS OpenRTB provider. The renderer security model — opaque-origin sandbox, fragment-bound nonce, exact-envelope cross-checking, default-off script creatives — is genuinely well built, and the browser suite backing it is thorough. Rust-side test coverage is strong throughout.

Requesting changes on three blocking items: a build-level defect that ships a second copy of Prebid core inside the server-served tsjs shim, the new browser test failing deterministically in CI because it never loads the shim it exercises, and the ESLint gate failing. One open question about a removed PBS stored-request fallback.

Blocking

🔧 wrench

• Prebid core duplicated into dist/tsjs-prebid.js: the deep import of prebid.js/src/adRendering.js plus the new build-all.mjs alias inline the whole Prebid core (auctionManager, adapterManager, events, config, …) into the server-served shim. Verified by building both trees: dist/tsjs-prebid.js grows 16,671 → 125,718 bytes and _pbjsGlobals appears in the shim bundle (0 occurrences on main). This contradicts build-all.mjs's own header comment, and markWinner then emits BID_WON / calls auctionManager.addWinningBid() on the shim's private instance, so the real pbjs never records the win. (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:15, crates/trusted-server-js/lib/build-all.mjs:57)

• aps-renderer.spec.ts fails deterministically in CI: 4/4 attempts across both the Next.js and WordPress runs, with Error: APS bid was not accepted by Prebid. The test loads tsjs-gpt.js and the pure external Prebid bundle but never dist/tsjs-prebid.js, so the trustedServer adapter is never registered and /auction is never called. (crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts:216)

• ESLint gate fails — format-typescript reports 15 errors, all in test files added or changed by this PR. import/order at test/core/request.test.ts:1, test/integrations/aps/render.test.ts:1, test/integrations/gpt/ad_init.test.ts:4-5, test/integrations/prebid/index.test.ts:1-2; @typescript-eslint/no-explicit-any at test/core/request.test.ts:83,108,136,162 and test/integrations/prebid/index.test.ts:393,394,436,481,482. Four are --fix-able.

❓ question

• PBS stored-request fallback removed for APS-only slots: an imp whose only bidder key is aps is now dropped rather than shipping storedrequest.id = slot.id, and the test that covered that path was deleted. Config-driven creative-opportunity slots no longer emit aps, but client-supplied ad units can. Was it confirmed no operator relies on stored-request demand for those slots? (crates/trusted-server-core/src/integrations/prebid.rs:1659)

Non-blocking

🤔 thinking

• remove_aps_bidders logs-and-strips rather than failing startup, against the convention that invalid enabled config surfaces as a configuration error (crates/trusted-server-core/src/integrations/prebid.rs:333)
• Mediator comment asserts the opposite of the note it deletes and references text no longer in the file; the deleted TRANSPORT-DEADLINE note and its #849 pointer explained why per-backend timeouts are not an absolute bound (crates/trusted-server-core/src/auction/orchestrator.rs:1302, also :662)
• PBS Referer / site.ref semantics changed for all traffic, not just APS cohorts, and not listed in the CHANGELOG breaking changes (crates/trusted-server-core/src/integrations/prebid.rs:1401)
• /auction site.page now derived from the client Referer; host is validated and query/fragment stripped, but the client-chosen path now reaches every SSP (crates/trusted-server-core/src/auction/formats.rs:138)

♻️ refactor

• Envelope contract validated in three hand-written copies with duplicated magic limits, including the bare 349528 in the embedded renderer document, which is untied to MAX_RENDER_ENVELOPE_BYTES (crates/trusted-server-core/src/integrations/aps.rs:73)

🌱 seedling

• APS tombstone capacity refusal dead-ends a slot — stopImmediatePropagation() has already fired, so neither Trusted Server nor Prebid answers (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1239)

⛏ nitpick

• Universal Creative renderer accepts http: while every other renderer-URL check is HTTPS-only (crates/trusted-server-js/lib/src/integrations/aps/render.ts:416)
• Unrelated semicolon removal on slot.validate_runtime()? (crates/trusted-server-core/src/creative_opportunities.rs:335)

👍 praise

• Renderer security construction: opaque-origin CSP sandbox, fragment-bound nonce, single-use listener, every descriptor field re-derived from the decoded envelope (crates/trusted-server-core/src/integrations/aps.rs:49)
• hb_adid now prefers the unique upstream bid ID for renderer bids, addressing the known non-unique-hb_adid blank-slot class without touching PBS behaviour (crates/trusted-server-core/src/publisher.rs:3285)

CI Status

• fmt: PASS
• clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
• rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
• js tests (vitest): PASS
• js lint (format-typescript / ESLint): FAIL — 15 errors
• docs format: PASS
• integration tests / Fastly EC lifecycle: PASS
• browser integration tests: FAIL — aps-renderer.spec.ts:170, 4/4 attempts across both frameworks

[prk-Jr]

🔧 wrench — This deep import pulls the whole Prebid core into the server-served tsjs shim, creating a second Prebid instance on the page.

prebid.js/src/adRendering.js imports auctionManager, adapterManager, events, config, targeting, Renderer, and creativeRenderers. Combined with the new alias in build-all.mjs, all of that is bundled into dist/tsjs-prebid.js. Building both trees:

main:  dist/tsjs-prebid.js   16,671 bytes   `_pbjsGlobals` occurrences: 0
PR:    dist/tsjs-prebid.js  125,718 bytes   `_pbjsGlobals` occurrences: 1

This contradicts build-all.mjs's own header comment ("The prebid integration builds here as the tsjs shim only — Prebid.js itself is never bundled into tsjs"). The real Prebid ships as a separate artifact from build-prebid-external.mjs, whose generated entry imports only prebid.js and its modules — never this shim. So a live page loads two independent copies of Prebid core.

Consequence: markWinner is

export const markWinningBid = hook('sync', function (bid) {
  ...
  events.emit(BID_WON, bid);
  auctionManager.addWinningBid(bid);
});

Both events and auctionManager resolve to the shim's private duplicate. On the real window.pbjs: bidWon never fires, getAllWinningBids() never contains the APS bid, and no registered analytics adapter sees the win. Only markBidAsRendered happens to work, because it just sets bidResponse.status on the shared bid object.

Fix — drop the deep import (and the build-all.mjs alias) and go through the public global, which does markWinningBid + markBidAsRendered against the correct core:

{
  markWinner: () =>
    pbjs.markWinningBidAsUsed({ adId: bid['adId'], analytics: true, events: true }),
  markRendered: () => {},
}

If both callbacks must stay separate, mark the specifier external in both build scripts so it resolves to the already-loaded bundle rather than being inlined.

[prk-Jr]

🔧 wrench — This alias is what inlines Prebid core into dist/tsjs-prebid.js (16,671 → 125,718 bytes, and _pbjsGlobals now appears in the shim bundle). It also invalidates the invariant stated in this file's own header comment eleven lines above the build function. See the detailed finding on src/integrations/prebid/index.ts:15; removing the deep import removes the need for this alias.

[prk-Jr]

🔧 wrench — This test never loads the shim it exercises, and fails deterministically in CI (4/4 attempts across both the Next.js and WordPress runs):

Error: page.evaluate: Error: APS bid was not accepted by Prebid
    at .../shared/aps-renderer.spec.ts:218:35

bundles.prebid is dist/prebid/<manifest.filename> — the artifact from build-prebid-external.mjs, whose generated entry imports only prebid.js plus consent/userId modules and bid adapters. It does not contain src/integrations/prebid/index.ts. The routed page body is <!doctype html><div id="div-aps"></div><div id="div-other"></div> with no server-injected script, so nothing else supplies the shim either.

Without dist/tsjs-prebid.js there is no trustedServer adapter, no wrapped requestBids, and no installApsBidResponseRegistry. The ad unit gets zero bids, /auction is never called (so the expect(auctionRequests).toBe(1) assertion is never even reached), and getBidResponsesForAdUnitCode('div-aps') is empty.

Fix — add the shim (and bundles.core, which clientAuctionBundlePaths() already computes but this test never uses):

await page.addScriptTag({ path: bundles.core });
await page.addScriptTag({ path: bundles.gpt });
await page.addScriptTag({ path: bundles.prebid });
await page.addScriptTag({ path: resolve(TSJS_CRATE, 'dist/tsjs-prebid.js') });

Note the assertions on bidWon, renderSucceeded, winningAdIds, and acceptedStatus === 'rendered' will still fail after this until the duplicate-Prebid-core issue on src/integrations/prebid/index.ts:15 is resolved — those are exactly the signals that land on the wrong instance.

[prk-Jr]

❓ question — This removes the PBS stored-request fallback for any imp whose only bidder key is aps; previously the slot shipped storedrequest.id = slot.id and PBS resolved bidder config server-side. The deleted to_openrtb_uses_stored_request_when_slot_has_no_pbs_bidder_params test covered exactly that path.

Creative-opportunity slots no longer emit an aps key (to_ad_slot change in this PR), so config-driven slots are unaffected — but client-supplied ad units reaching POST /auction still can carry one.

Was it confirmed that no operator relies on stored-request PBS demand for slots that also name aps? If any do, this is a silent revenue drop rather than a visible no-bid: the imp never reaches PBS at all, and the only signal is a warn line.

[prk-Jr]

🤔 thinking — aps in integrations.prebid.bidders is now genuinely invalid configuration (it will never reach PBS), but this strips it with a warn and continues.

The project convention is that invalid enabled configuration surfaces as a startup/config error rather than being logged-and-disabled — the same rule that made bid_param_zone_overrides non-object values a hard startup failure in this release. An operator who leaves aps in prebid.bidders after upgrading gets a warn buried in edge logs and silently different demand.

Consider returning a TrustedServerError::Configuration from build() instead, with the same "configure APS under [integrations.aps]" guidance.

[prk-Jr]

🌱 seedling — A capacity refusal here dead-ends the slot: e.stopImmediatePropagation() already fired 8 lines above, so returning without consuming means neither Trusted Server nor Prebid answers the request and the slot stays blank permanently.

With 256 tombstones plus TTL pruning this needs 256 concurrent unexpired APS renders on one page, so the risk is low — but the failure mode is silent-blank rather than degraded. Consider evicting the oldest tombstone instead of refusing, matching what registerApsPrebidRenderer already does for the registry itself.

[prk-Jr]

⛏ nitpick — This accepts http: while every other renderer-URL check in the PR is HTTPS-only (apsRendererUrl, validCreativeUrl, validate_aps_endpoint, and the APS_RENDERER_DOCUMENT creative-URL check). rendererUrl is produced by apsRendererUrl(), which already guarantees same-origin, so the http: branch only matters on a plaintext page — where it is the weaker choice anyway. Dropping it would make the policy uniform.

[prk-Jr]

⛏ nitpick — Unrelated churn: the trailing ; was dropped from slot.validate_runtime()?. No behavioural difference, but it adds a line to a diff that is already large.

[prk-Jr]

👍 praise — Preferring the unique upstream bid.id for renderer bids (with cache_id still winning and ad_id retained as the fallback for everyone else) directly addresses the known blank-slot class where hb_adid fell back to a creative ID a bidder reuses across slots. Scoping it to renderer bids keeps the existing PBS behaviour untouched.

[prk-Jr]

👍 praise — Strong security construction here: CSP sandbox with no allow-same-origin forces an opaque origin, default-src 'none' with an explicit allowlist, plus nosniff and no-referrer. Combined with the fragment-bound nonce, the event.source !== parent check, the single-use listener teardown, and re-deriving every descriptor field from the decoded envelope, the trust boundary is genuinely defended rather than asserted. The 935-line browser suite covering CSP sandboxing, replay, same-origin rejection, restrictive-CSP compatibility, and parent-DOM isolation is the right level of rigour for this surface.