Add first-class APS OpenRTB integration#918
Conversation
aram356
left a comment
There was a problem hiding this comment.
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-formconfig.biddersslips through to PBS and is sold twice. Separately,head_insertsserializesconfig.bidders/client_side_biddersverbatim intowindow.__tsjs_prebid, soapsstill ships to the browser — the safeguard is server-side only, andclient_side_biddersis never filtered at all. The durable fix for this and the stored-request finding is to rejectapsin[prebid].bidders/client_side_biddersat config-validation time.
Non-blocking
🤔 thinking
- Full client-controlled Referer path+query now enters the bidstream (formats.rs:93-116):
site.pagechanged from a barehttps://{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_mockmediation is last-write-wins and the new test enshrines it (adserver_mock.rs:107, :899):build_bid_indexkeys 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_idnow exists and would disambiguate; the mock only echoescrid. At least raise the collision log fromdebug!towarn!.providers.apsis now write-only and fails silently (creative_opportunities.rs:366,378): the field is a genuinedeny_unknown_fieldsdeser-compat shim (removing it hard-fails existing TOML), butto_ad_slotno longer reads it, so an operator who setsaps.slot_idexpecting 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. Preferas_aps(&self) -> Option<&ApsRendererV1>. Note the genericbid.bid_idfield this PR adds already carries the same value, so publisher.rs could derivehb_adidrenderer-agnostically.
📝 note
- Undocumented
/auctionwire-contract change: for renderer bids,cridgoes from always-present{bidder}-creativetobid.creative_id(absent when the bidder omitscrid),idfrom{bidder}-{slot}to the upstream bid id, andadmis omitted entirely. tsjs absorbs all three, butPOST /auctionis 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 ofrenderApsCreative, 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_idis the only renderer field without a length cap (aps.rs:682) — bounded by the 2 MiB body cap but inconsistent withaccount_id/creative_url.dropped_bid_countanddrop_reasonsdon't reconcile (aps.rs:776-799) — price-losers increment the count with no reason entry; the test assertsdropped=1, drop_reasons={}.- auction/README.md:120 ASCII box right border is off by one column.
trusted-server.example.tomlregressed a fictionalaps.example.com/e/dtb/bidto a real vendor host; several docs/tests use real*.amazon-adsystem.comendpoints against the project's "example.com only" rule. These are functionally-required vendor endpoints (not customer data or credentials) andamazon-adsystempredates 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
left a comment
There was a problem hiding this comment.
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 commit8bb5450dc(#890) added amake_test_bid_with_creative()helper inpublisher.rsthat builds aBid{}literal without this PR's newbid_id/creative_id/rendererfields. Rebasing/merging onto currentmainfails witherror[E0063]: missing fields— this is exactly what's failing thecargo fmtandcargo testCI checks shown on this PR (GitHub builds the synthetic merge commit, not the raw branch tip). See inline comment onauction/types.rs.
❓ question
- Debug mode can leak
admto the browser (crates/trusted-server-core/src/integrations/aps.rs:628): undermines the PR's core claim thatadmnever reaches the client outside the sandbox. See inline comment. - One bad bid can abort the entire
/auctionresponse (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()returningErr), onlyApsConfig::validate()directly.gpt/index.ts:999—renderingAdIdsgrows unbounded on the success path, inconsistent with sibling bounded caches added elsewhere in this PR.
🤔 thinking
aps.rs:1047— context-freeparse_responsesilently discards a valid response if ever called without context; safe today only because all real call sites useparse_response_with_context.aps.rs:380—device.languagehas 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 bothbid.creativeandbid.rendererareSome,creativesilently 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, wortheq_ignore_ascii_casegiven the stated goal.publisher.rs:2207— full client query string is now forwarded intosite.pagefor every provider; unlikeaps.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 omitsinventory_domain/inventory_page_origin, real validated fields documented elsewhere in this PR.
⛏ nitpick
aps.rs:851— aseatbidentry with nobidarray is skipped without incrementingdrop_reasons, making a no-bid response harder to diagnose.render.test.ts:216— no test for "correct nonce, wrongevent.source" rejection specifically (the guard is implemented correctly atrender.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— legacyproviders.aps.slot_idis 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 (themessageSourceBelongsToAdUnitguard 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_bidnever readsadm; 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 noallow-same-origin.orchestrator.rs:provider_request_context/effective_timeoutthreading fix, pinned bydispatched_collection_reuses_provider_launch_context.render.ts:validateApsRenderer's exact-key allowlist cross-checking the outer descriptor vs. the decoded OpenRTB payload; theevent.source === iframe.contentWindowidentity check (the right check for an opaque-origin sandbox).prebid.rs: thetrustedServerbidder-expansion path correctly re-strips"aps"even when reintroduced viaconfig.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
7f448bc to
4847192
Compare
Summary
/e/pb/bid.Changes
crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/auction/*adm.crates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/{publisher.rs,creative_opportunities.rs}crates/trusted-server-js/lib/src/integrations/aps/render.tscrates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.tstrusted-server.example.tomland integration fixturesaccount_id, the APS OpenRTB endpoint, and default-offallow_script_creatives.docs/**,CHANGELOG.md,TESTING.mdCloses
Closes #764
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo 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 buildChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)Rollout note
Broad production rollout and production
tagtype=scriptenablement remain gated on controlled APS-account validation and APS account-team confirmation. Script creatives remain disabled by default.