Add publisher-native rendering for selected APS bids - #1042
Add publisher-native rendering for selected APS bids#1042ChristianPavilonis wants to merge 7 commits into
Conversation
Provide an opt-in publisher hook for testing whether a Trusted Server-selected APS bid can use publisher-owned rendering without silently starting another auction or falling back to the custom renderer. Keep the existing opaque renderer as the default while real publisher and APS compatibility is validated.\n\nSee also: #999
The test publisher cannot install a custom rendering hook. Reuse the existing APS Prebid creative runner contract in a publisher-origin friendly frame so the experiment remains self-contained while preserving the selected server bid and avoiding a second auction.\n\nDocument the larger security surface and retain the opaque renderer as the default.\n\nSee also: #999
ChristianPavilonis
changed the title
ChristianPavilonis
changed the title
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
An opt-in publisher_native APS rendering mode that hands the server-selected bid to APS's fixed Prebid creative runner inside a publisher-origin friendly frame, keeping the opaque static renderer as the default. The ownership discipline is careful and well tested; the blocking concerns are slot-container resolution on real GAM pages and an unpinned security-relevant default.
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 multiple files, spans several hunks, or would not survivecargo fmtas a single contiguous replacement.
Blocking
wrench
findApsContainerbypasses the codebase's slot-root resolution — see inline atcrates/trusted-server-js/lib/src/integrations/aps/render.ts:54- No test pins the default mode to an empty head insert — see inline at
crates/trusted-server-core/src/integrations/aps.rs:2432
Non-blocking
thinking / refactor / note
- Native mode leaves the Universal Creative port unanswered and deletes GAM's own creative iframe — see inline at
crates/trusted-server-js/lib/src/integrations/aps/render.ts:455 prepareApsRunnerDocumentreimplements the Rust document's one-line CSS — see inline atcrates/trusted-server-js/lib/src/integrations/aps/render.ts:385publisher_native+allow_script_creativeshas no guard or operator signal — see inline atcrates/trusted-server-core/src/integrations/aps.rs:1255- GAM doc paragraph's trailing sentence is still unconditional — see inline at
docs/guide/integrations/aps.md:198 - Prebid path now consumes the capability before
apsRendererUrl()is checked — see inline atcrates/trusted-server-js/lib/src/integrations/gpt/index.ts:1701
Cross-cutting / body-level findings
-
Friendly-frame
document.open()/write()/close()is proven on Chromium only.crates/trusted-server-integration-tests/browser/playwright.config.*declares a single project,chromium. The premise of this PR is observed vendor behavior in a real browser, and synchronously writing into a freshly appended src-less iframe is exactly where Safari and Firefox have historically diverged on document-replacement timing — the initialabout:blanknavigation can land after the write. Worth running the new native spec againstwebkitandfirefox, even if only locally, before the cohort test. This does not block the merge; it blocks trusting the result. -
The one-shot ownership discipline is genuinely careful. The dispatch
Symbolplus thereleaseNativeDispatchstale-completion guard, the "an invalid replacement still cancels the older pending frame" invariant, deferringmarkUsed()until the runner actually loads, and the decline-never-falls-back-to-the-opaque-renderer rule each have a matching contract test.expect(portMessages).toEqual([])is the right way to pin the no-Universal-Creative-response contract — it asserts the absence that the whole mode depends on, rather than only asserting the happy path.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- format-typescript: PASS (required)
- Analyze (actions): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test: PASS (required)
- cargo test (axum native): PASS
- Analyze (rust): PASS
- format-docs: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- vitest: PASS
- prepare integration artifacts: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Opt-in publisher_native rendering mode for selected APS bids: config enum + head marker on the Rust side, a shared rendering-owner dispatcher across the direct, server/GPT, and Prebid paths, and a friendly-frame runner with fail-closed, no-fallback semantics. The envelope validation, one-shot ownership (capability consume + tombstone before dispatch), and preserved publisher content on failure are consistent across all three paths, and coverage is thorough (unit contract tests plus a real Playwright friendly-frame proof). Requesting changes for the mode-switch trust model and the missing config guard below; the remaining findings are non-blocking.
Blocking
🔧 wrench
- Rendering-mode switch is a content-injectable DOM marker:
isPublisherNativeApsRendering()trusts any matching<meta>indocument.head, so injected markup can flip a default-mode deployment into the unsandboxed friendly frame (crates/trusted-server-js/lib/src/integrations/aps/render.ts:316). - No config guard for
publisher_native+allow_script_creatives = true: nothing in code enforces the rollout ordering the docs mandate for the highest-risk combination (crates/trusted-server-core/src/integrations/aps.rs:158).
Non-blocking
♻️ refactor
- Spurious "ignored stale completion" warning on every supersede: the normal replacement path always logs it (crates/trusted-server-js/lib/src/integrations/aps/render.ts:372).
- Rust test gap: no assertion that
TrustedServermode emits no head marker (crates/trusted-server-core/src/integrations/aps.rs:2374).
📝 note
- Mode flip strands live sessions and cached HTML: switching
trusted_server→publisher_nativeremoves/integrations/aps/rendererwhile already-served pages (no marker) still target it — those slots fail closed after the 10s ready timeout until reload. Fail-closed is the right behavior, but a sentence in the Rollout or Troubleshooting section ofdocs/guide/integrations/aps.mdabout in-flight sessions and HTML caches would save an operator a confusing debugging session.
⛏ nitpick
- Unused exports:
renderApsPublisherNative/RenderApsPublisherNativeOptions(crates/trusted-server-js/lib/src/integrations/aps/render.ts:409). - Renderer route gated in two places:
routes()andregister()must stay in sync (crates/trusted-server-core/src/integrations/aps.rs:1213).
CI Status
- fmt: PASS
- clippy (all six targets): PASS
- rust tests (fastly/axum/cloudflare/spin/CLI/parity): PASS
- js tests (vitest): PASS
- browser integration tests: PASS
- CodeQL / docs + TS format: PASS
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Staff-level review pass. Reviewed in a clean detached worktree at head 13a6ecf3, with every claim below scratch-verified before posting.
What this PR does, mechanically
Adds a second, opt-in rendering owner for server-selected APS bids. Rust side: a strict ApsRenderingMode enum (trusted_server default / publisher_native) on ApsConfig; in native mode register() drops the /integrations/aps/renderer proxy route entirely and instead registers ApsRendererIntegration as an IntegrationHeadInjector whose only job is to stamp data-ts-aps-rendering-mode="publisher_native" onto the server-generated TSJS bundle <script> tag (head_inserts deliberately returns empty — the earlier forgeable <meta> marker was removed in 69b30c0d). JS side: render.ts gains a dispatchApsRendering() fan-out that reads that attribute once at module-eval time via document.currentScript, then routes each validated descriptor to exactly one owner — the existing opaque-sandbox renderApsCreative, or the new renderApsPublisherNative() which builds an unsandboxed same-origin friendly iframe, seeds the frame's account-keyed _aps queue with a prebid/creative/render CustomEvent, and loads Amazon's prebid-creative.js. All three call sites are rewired through the dispatcher; in native mode the GPT paths send no Universal Creative response and never fall back.
Verification run locally
| Check | Result |
|---|---|
npx vitest run |
PASS — 44 files, 863 tests, no type errors |
npm run format (JS) / (docs) |
PASS / PASS |
cargo fmt --all -- --check |
PASS |
clippy-fastly / -axum / -cloudflare / -cloudflare-wasm / -spin-native / -spin-wasm |
PASS ×6 |
cargo test -p trusted-server-core --target aarch64-apple-darwin |
PASS — 1982 passed, + 2 doc-tests |
node build-all.mjs |
PASS — 13 modules |
GitHub CI (gh pr checks) |
19/19 pass, incl. cross-adapter parity, browser integration, CodeQL |
cargo test-fastly (Viceroy) was not run — all Rust changes are in trusted-server-core, which was exercised natively instead, and GitHub's cargo test job is green.
Verdict
The experiment itself is well-built: descriptor validation, capability-consumption ordering, supersede semantics and fail-closed paths are unusually careful, the security tradeoff is documented honestly rather than buried, and native-mode coverage is strong (unit + jsdom + a real Playwright fixture with a publisher CSP). Rust-side changes are minimal and parity-safe.
Requesting changes on one issue: an experiment behind a non-default config flag must not change the default path, and this one does. dispatchApsRendering moves the pending-frame cancel ahead of validation for all operators, including everyone still on trusted_server — I proved by scratch test that a rejected descriptor now destroys an in-flight valid render that main would have completed. Blank-slot regression on the shipping path, no test covers it, one-line fix that keeps all 863 tests green. The two other suggestion-block findings (inherited referrer policy, stale docs bullet) are mechanical and both verified — worth riding along.
The three design findings (per-bundle rather than per-page slot ownership; commit-on-script-load destroying the GAM iframe; publisher_native + allow_script_creatives guarded only by a log line) need not block the merge, but all three should be answered before any traffic is pointed at publisher_native — particularly commit-on-script-load, which is the decision most likely to cost impressions in a live cohort.
Checked and found clean
- Cross-adapter parity. No
stream_response, nofastly::*, no platform-gated API.tsjs_script_tag_attributesis an existing adapter-agnostic core mechanism (registry.rs:1063, consumed athtml_processor.rs:359). All six clippy targets and the parity job green. - The third-render-context URL-rewrite hazard does not apply. Native mode touches neither
rewrite_creative_htmlnorrewrite_inline_creative_html— noadmHTML is injected. The runner receives the base64aaxResponseenvelope and loadsrenderer.creativeUrl, whichvalidCreativeUrlalready requires to be absolute HTTPS, credential-free, and non-publisher-origin. No relative-vs-absolute trap. hb_adidnon-uniqueness. The GPT server path still resolves viaslotIdForMessageSource(e.source)and rejects onmatchedBid.hb_adid !== adId; requesting-slot-first ordering and its comment untouched. The Prebid path still gates onmessageSourceBelongsToAdUnit. The newsourceplumb-through intofindApsContainerreuses the existingcontentWindow === sourceidiom.sourceMatchedCandidatesfallback-to-all-candidates. Tried to turn this into a wrong-slot-render finding and could not substantiate it — both native call sites validate the source upstream, anduniqueSlotCandidaterequires exactly one match, so ambiguity falls through to the next strategy rather than guessing. Dropped.- No publisher-side changes required. The attribute lands on TS's own generated
<script id="trustedserver-js">. - Mode-attribute forgeability. Captured once at IIFE eval from
document.currentScript, and the immediate bundle tag is neitherasyncnordefer(tsjs.rs:43-46), so later-injected markup cannot flip the mode — covered by test. - No real operator values in docs, config or tests (
example-account,publisher.example,creative.example, fictional throughout). One nit noted inline. - Serde contract.
snake_case,#[default] TrustedServer,#[serde(default)], unknown values reject — covered by test. - Project conventions. No
unwrap()in prod code, noanyhow, no thiserror, noprintln!, no wildcard imports, comments above code, tests colocated. JS tests usevi.spyOn/vi.resetModules(novi.mock()factory, sovi.hoisted()doesn't apply). - TDZ / cleanup / leak audit of
renderApsPublisherNative.cleanup()only ever runs after thetimeoutIdassignment,settledguards re-entry, the timeout is the guaranteed terminal path sonativeDispatchesentries always release, andactiveFrames.setcorrectly follows the previous frame's cancel. The un-disconnect()edMutationObserverdies with the frame's browsing context oniframe.remove()— not a leak. Dropped. - Cache/rollback semantics of a mode change. The route-disappears-under-cached-HTML hazard is documented in both the Migration and Rollout sections — good catch by the author.
Pre-existing, not this PR
The APS renderer branch never calls safelyRecordCreativeRequest in either mode, so APS renders are invisible to gptDiagnosticsRecorder — main returns early before that point the same way. Native-mode renders inherit the blind spot rather than creating it. Worth a follow-up issue if the cohort needs render-rate telemetry, since blank-slot triage here has historically leaned on tsjs.renders vs tsjs.bids.
| // Every attempt supersedes a pending frame for this slot, including an invalid | ||
| // replacement that fails before a new frame can be created. | ||
| cancelPendingApsRendering(slotId, source); |
There was a problem hiding this comment.
HIGH — this changes the default trusted_server path, not just the experiment.
cancelPendingApsRendering() runs unconditionally here, before validateApsRenderer and regardless of rendering mode. On main, the equivalent pendingFrameCancels.get(container)?.() sits after both the validation early-return and the getElementById early-return inside renderApsCreative — so on main a rejected descriptor never disturbs an in-flight frame.
Failure scenario (default mode, no config change): slot S has an APS frame in flight — created, <iframe> appended, waiting on the renderer-ready postMessage, a window of up to RENDERER_READY_TIMEOUT_MS = 10_000. A second dispatch arrives for S with a descriptor that fails validation (truncated or re-encoded aaxResponse, bid.w / renderer.width mismatch, non-standard base64, envelope cross-check failure). Before this PR: the invalid descriptor returned false and the in-flight valid frame went on to render. After: the in-flight frame is destroyed, dispatch returns false, and nothing replaces it — permanently blank slot. That is the failure class #418 / #958 have been chasing.
Verified with a scratch vitest pair comparing main semantics against this branch:
✓ BASELINE main behaviour: invalid second dispatch does NOT cancel the pending first frame
✗ PR behaviour: invalid second dispatch cancels the pending first frame in DEFAULT mode
first frame still connected after invalid dispatch: false
No test covers this ordering in default mode — the test that does cover it (lets an invalid replacement cancel an older pending runner) runs with publisherNativeRendering === true, so the native intent is preserved by the gate below.
Second, sharper variant of the same root cause: this cancel resolves its container through the fuzzy findApsContainer(), while default-mode rendering resolves through the exact document.getElementById() — so the two can target different elements. Scratch-reproduced cross-slot destruction (dispatch for logical slot homepage_header with divToSlotId {'div-header': 'homepage_header'} returned false with slot not found, yet left #div-header iframes now: 0 — it killed a frame in a slot it never rendered into). Latent rather than routinely reachable, but the same-slotId case above is directly reachable.
Fix applied and verified in a scratch worktree: full suite 863 passed, prettier clean, both scratch cases green (first frame still connected after invalid dispatch: true), native-mode supersede test still passing.
| // Every attempt supersedes a pending frame for this slot, including an invalid | |
| // replacement that fails before a new frame can be created. | |
| cancelPendingApsRendering(slotId, source); | |
| // Every native attempt supersedes a pending frame for this slot, including an | |
| // invalid replacement that fails before a new frame can be created. Default mode | |
| // keeps renderApsCreative's semantics, where a rejected descriptor never disturbs | |
| // an in-flight frame. | |
| if (publisherNativeRendering) cancelPendingApsRendering(slotId, source); |
| frameDocument.write( | ||
| '<!doctype html><html><head><meta charset="utf-8"></head><body></body></html>' | ||
| ); |
There was a problem hiding this comment.
MEDIUM — the friendly frame silently drops no-referrer, and inherits the publisher's CSP.
trusted_server mode serves the renderer document with three protections (see aps.rs:1236-1239): referrer-policy: no-referrer, x-content-type-options: nosniff, and APS_RENDERER_CSP = "default-src 'none'; …" plus a CSP-level sandbox. This document.writen about:blank frame inherits the publisher document's referrer policy and CSP instead. The docs disclose the sandbox loss ("this friendly frame deliberately has no opaque-origin sandbox") but say nothing about referrer or CSP.
Failure scenario: publisher page runs the common default (strict-origin-when-cross-origin) or something looser (unsafe-url, no-referrer-when-downgrade). Every creative subresource, tracking pixel and click beacon the APS runner loads now carries the publisher origin — or under unsafe-url the full page URL — as Referer, and Origin: https://publisher.example instead of the sandbox's Origin: null. On a publisher with no CSP at all, the creative runs with no connect-src / img-src / frame-src restriction whatsoever, where trusted_server mode pinned it to default-src 'none' plus https: allowlists. This PR's own browser fixture demonstrates the inheritance — it had to add frame-src https://creative.example to the page CSP for the creative to load (aps-renderer.spec.ts:~1029).
The referrer half restores in one line (verified: 863 passed, prettier clean):
| frameDocument.write( | |
| '<!doctype html><html><head><meta charset="utf-8"></head><body></body></html>' | |
| ); | |
| frameDocument.write( | |
| '<!doctype html><html><head><meta charset="utf-8">' + | |
| '<meta name="referrer" content="no-referrer"></head><body></body></html>' | |
| ); |
The CSP half can't be a one-liner — it needs a decision on how much of APS_RENDERER_CSP the runner tolerates without sandbox. Either add a <meta http-equiv="Content-Security-Policy"> mirroring APS_RENDERER_CSP minus the sandbox directive, or state the CSP-inheritance delta explicitly in the security paragraph alongside the sandbox one.
| - Confirm `GET /integrations/aps/renderer` returns HTML with its CSP and `Referrer-Policy: no-referrer`. | ||
| - Confirm publisher CSP permits `frame-src 'self'`. | ||
| - In `trusted_server` mode, confirm `GET /integrations/aps/renderer` returns HTML with its CSP and `Referrer-Policy: no-referrer`, and that publisher CSP permits `frame-src 'self'`. | ||
| - In `publisher_native` mode, confirm the mode `<meta>` marker is present, the slot receives a hidden friendly iframe, and `https://client.aps.amazon-adsystem.com/prebid-creative.js` is not blocked by CSP. The static renderer route is intentionally absent in this mode. |
There was a problem hiding this comment.
MEDIUM — this troubleshooting step points at a marker that was deliberately removed.
head_inserts returns Vec::new(), and the test at aps.rs:2483 asserts "should not inject a forgeable native-mode marker". There is no <meta> marker to find. 4b8e04e4 did emit <meta name="trusted-server-aps-rendering-mode" content="publisher_native">; 69b30c0d Harden APS publisher-native rendering replaced it with the script-tag attribute and updated the main paragraph (~line 54, now correct) but left this bullet behind.
Consequence: an operator debugging a non-rendering native cohort follows this step, finds no <meta>, and concludes the mode never activated — when the real signal is data-ts-aps-rendering-mode="publisher_native" on the #trustedserver-js script tag.
Verified docs prettier clean with this wording:
| - In `publisher_native` mode, confirm the mode `<meta>` marker is present, the slot receives a hidden friendly iframe, and `https://client.aps.amazon-adsystem.com/prebid-creative.js` is not blocked by CSP. The static renderer route is intentionally absent in this mode. | |
| - In `publisher_native` mode, confirm the TSJS bundle tag carries `data-ts-aps-rendering-mode="publisher_native"`, the slot receives a hidden friendly iframe, and `https://client.aps.amazon-adsystem.com/prebid-creative.js` is not blocked by CSP. The static renderer route is intentionally absent in this mode. |
| const nativeDispatches = new Map<string, symbol>(); | ||
| const publisherNativeRendering = | ||
| typeof document !== 'undefined' && | ||
| document.currentScript?.getAttribute(APS_RENDERING_MODE_ATTRIBUTE_NAME) === 'publisher_native'; |
There was a problem hiding this comment.
MEDIUM (design question) — "one owner per slot" is enforced per-bundle, not per-page.
nativeDispatches and publisherNativeRendering here, and activeFrames / pendingFrameCancels at lines 31-32, are all module-scope. There is no src/integrations/aps/index.ts, so render.ts is inlined into every importing entry point as a separate esbuild IIFE. The docs' claim — "This one-owner behavior avoids a second render path" — holds inside a bundle, not across the page.
Confirmed by building and grepping the emitted bundles (node build-all.mjs):
tsjs-core.js (19342 B): mode-attr 1 native-renderer-str 5 runner-url 1
tsjs-gpt.js (37989 B): mode-attr 1 native-renderer-str 5 runner-url 1
tsjs-prebid.js (22545 B): mode-attr 1 native-renderer-str 0 runner-url 0
Each dist file is its own (function(){"use strict"; … })();, so the scopes do not merge when the Rust server concatenates them at request time.
Mechanism: on a native-mode page where both core's requestAds() and the gpt bridge target slot S, cancelPendingApsRendering in the gpt copy cannot see the core copy's pending cancel. Two friendly frames get appended to the same container and both load prebid-creative.js, so both can fire impression/billing beacons before the winner's finish(true) strips siblings. The DOM self-heals (the loser sees !iframe.isConnected and resolves false), but the duplicated vendor runner load is not undone.
What I could not prove: that core's requestAds and the gpt bridge both activate for the same bid on the same page load — they read different bid sources (the live /auction POST vs. the frozen page-load window.tsjs.bids). So the mechanism is real but the trigger is unproven. Flagging as a design question, not a confirmed live bug.
Fix shape (not one-click — needs a new file plus import rewrites in three modules): promote the frame registries to a single page-scoped owner, e.g. lazily hung off window.tsjs (tsjs.__apsFrames ??= new WeakMap()) the way apsPrebidRenderers already is, or add src/integrations/aps/index.ts and register APS as a real JS module so exactly one copy ships.
Related, LOW: the document.currentScript gate on lines 47-49 survives tree-shaking into tsjs-prebid.js (grep count 1) even though the dispatcher and renderApsPublisherNative do not (count 0). prebid is a deferred module and tsjs_deferred_script_tags (html_processor.rs:373) does not carry the attribute; the external R2 trusted-prebid.js is loaded by a tag TS doesn't control at all. So that copy's publisherNativeRendering is permanently false. Harmless today, but if anyone later calls the dispatcher from the prebid module, native mode is silently off there — and since trusted-prebid.js ships to R2 outside the deployer, the breakage would be invisible. Worth a comment on the constant, or resolving the mode from a shared window.tsjs field set by whichever bundle reads the tag first.
|
|
||
| runner = frameDocument.createElement('script'); | ||
| runner.src = APS_PREBID_CREATIVE_RUNNER_URL; | ||
| runner.addEventListener('load', commit, { once: true }); |
There was a problem hiding this comment.
MEDIUM (design) — this commits on runner script load, not on creative render, and the commit destroys the GAM iframe.
runner.addEventListener('load', commit) resolves the dispatch true the moment prebid-creative.js finishes downloading. finish(true) then removes every other child of the container and reveals the frame.
Failure scenario: the runner loads (200 OK, load fires) but cannot render — malformed aaxResponse that got past TSJS's checks, a creative subresource blocked by the publisher CSP, an APS-side error. Result: a visible empty 300×250 frame, the publisher's original content and the GAM / Universal Creative iframe removed from that div, no UC response ever posted (the capability was already consumed), and no fallback by design. Net: blank paid slot plus a destroyed GAM impression.
Both halves are explicit in the PR's own docs — "Trusted Server treats runner load as successful handoff" and "This removes the GAM or Universal Creative iframe when it is inside that div" — so this is a deliberate choice, not an oversight. But the combination (script-load as the commit signal + sibling destruction + no fallback) is the highest-blast-radius decision in the PR, and worth settling before any cohort rollout.
Fix shape (not one-click — restructures the promise plumbing): prepareApsRunnerDocument already installs a MutationObserver on the runner frame's body (line ~453). Reuse it as the commit signal — commit on runner load and the first node appended to the frame body, with the existing 10s timeout as the backstop — so a runner that loads but produces nothing declines the bid and leaves the GAM iframe intact.
| if config.rendering_mode == ApsRenderingMode::PublisherNative && config.allow_script_creatives { | ||
| log::warn!( | ||
| "APS publisher-native rendering with script creatives is ON; selected bidder scripts execute with publisher-origin privileges" | ||
| ); | ||
| } |
There was a problem hiding this comment.
MEDIUM (question) — should publisher_native + allow_script_creatives be a hard validation failure rather than a warning?
In native mode the frame is same-origin with the publisher, so a tagType: "script" creative injected by the vendor runner executes with full publisher-origin privileges: publisher DOM, document.cookie (including the Trusted Server EC ID cookie), and same-origin fetch to publisher endpoints. The only guard on that combination is this log line, and the PR explicitly locks the combination in as supported (publisher_native_script_creatives_remain_available_for_controlled_validation, lines 2508-2530; line 791 is the only place allow_script_creatives gates anything).
ApsConfig already has a #[validate(schema(...))] hook (used by validate_inventory_identity_override). Would it be better to make this combination a hard config-validation failure behind a third explicit opt-in, so it cannot be reached by config drift or a stale cached config blob? A warning in edge logs is easy to miss for a blast radius this large.
No suggestion block — this contradicts one of the PR's own tests, so it needs an answer rather than a patch.
| const PUBLISHER_CORE_URL = | ||
| "https://client.aps.amazon-adsystem.com/trusted-server-core.js"; |
There was a problem hiding this comment.
LOW — this serves Trusted Server's own bundle from Amazon's real CDN hostname.
https://client.aps.amazon-adsystem.com/trusted-server-core.js is a fabricated path on a real third-party vendor domain, used only to satisfy the fixture's script-src https://client.aps.amazon-adsystem.com. It reads as though Trusted Server ships a bundle from Amazon's CDN.
Cheaper and clearer to use a fictional host:
const PUBLISHER_CORE_URL = "https://tsjs.example/trusted-server-core.js";Not offered as a one-click suggestion because it also needs https://tsjs.example added to the script-src list in this test's CSP header (~line 1029) — a one-hunk suggestion would break the fixture.
| if (typeof dispatched === 'boolean') { | ||
| if (dispatched) markUsed(); | ||
| } else { | ||
| void dispatched.then((accepted) => { | ||
| if (accepted) markUsed(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
LOW — dispatchApsRendering returning boolean | Promise<boolean> forces every caller to branch on typeof.
This call site typeof-switches; the other (core/request.ts:55-61) wraps in Promise.resolve and discards. Having the dispatcher always return Promise<boolean> (return Promise.resolve(trustedServer(renderer)) on the default path) would reduce both call sites to a plain .then, and would remove the only reason this block needs the duplicated markUsed().
Behaviour-neutral; safe to defer.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review at head 07dfc189f. Every finding from the previous review is resolved: the rendering-mode signal moved from the injectable head <meta> marker to the server-owned data-ts-aps-rendering-mode bundle-tag attribute captured once at module init, with a regression test proving post-init injected meta tags and spoofed script tags are ignored, and the Playwright proof now runs under a CSP without 'unsafe-inline'. The script-creatives combination is settled with a warn-level startup log plus a test documenting the controlled-validation intent. The stale-completion warning, default-mode marker assertions, mode-flip/cached-HTML docs, and module-private renderer internals are all in place. The findApsContainer rework (requesting-frame disambiguation, -container inner resolution, unique-candidate dynamic prefixes) is conservative where candidates are ambiguous and is covered by new unit tests on both the render and bridge sides.
The findings below are non-blocking.
Non-blocking
❓ question
- Foreign-author "probe" commit on the branch (
d97bda69f, authored byprk-Jr): a 1.1MB commit duplicating main's content (ESI designs, template cache) under the message "probe", later netted out by the07dfc189fmain merge. Verified it leaks nothing into the PR — the diff vs main is exactly the 10 expected APS files (+1324/−59) — and squash-merge keeps it out of main's history. Was this push intentional? Dropping it with a rebase would keep the branch history honest; otherwise a quick confirmation here is enough.
🌱 seedling
- Deferred-bundle copy of
render.tscannot see the mode attribute: the deferredtsjs-prebid.jsbundles its own copy of this module, and deferred tags fromtsjs_deferred_script_tags()do not carrytsjs_script_tag_attributes()(crates/trusted-server-js/lib/src/integrations/aps/render.ts:47).
⛏ nitpick
- Renderer route gated in two places:
routes()andregister()both gate on the mode and must stay in sync (crates/trusted-server-core/src/integrations/aps.rs:1213).
CI Status
- fmt: PASS
- clippy (all six targets): PASS
- rust tests (fastly/axum/cloudflare/spin/CLI/parity): PASS
- js tests (vitest): PASS
- integration + browser integration tests (including the APS publisher-native Playwright proof): PASS
- CodeQL / docs + TS format: PASS
| }; | ||
| const validatedRendererCache = new WeakMap<object, ValidatedRendererCacheEntry>(); | ||
| const nativeDispatches = new Map<string, symbol>(); | ||
| const publisherNativeRendering = |
There was a problem hiding this comment.
🌱 seedling — The deferred tsjs-prebid.js bundle contains its own copy of this module (it imports registerApsPrebidRenderer), and deferred tags generated by tsjs_deferred_script_tags() do not carry tsjs_script_tag_attributes() — so that copy's publisherNativeRendering is always false. Safe today because dispatch only ever runs from the immediate bundle (core/gpt share the attributed tag), but the invariant is implicit: a future dispatchApsRendering call from a deferred module would silently render in default mode even on a native-mode deployment.
A short comment here naming the invariant — or applying the attributes to deferred tags server-side — would pin it down before someone trips over it.
| @@ -1193,7 +1211,10 @@ impl IntegrationProxy for ApsRendererIntegration { | |||
| } | |||
|
|
|||
| fn routes(&self) -> Vec<IntegrationEndpoint> { | |||
There was a problem hiding this comment.
⛏ nitpick — Carry-over from the previous review: the renderer route is still gated on rendering_mode both here in routes() and in register() (which skips .with_proxy() in native mode). Either alone suffices, and the two must stay in sync. If belt-and-braces is intended, a short comment cross-referencing the other gate would keep a future cleanup from removing the wrong one.
3 participants
Summary
publisher_nativemode for rendering Trusted Server-selected APS bids through APS's fixed Prebid creative runner in a publisher-origin friendly iframe.rendering_mode = "publisher_native"is explicitly configured.The existing opaque renderer can leave APS creatives blank when their nested HTTPS frames require their real origin. A controlled browser test through the publisher setup confirmed that the new friendly-frame path renders a real selected APS creative, while
apstag.renderImp(document, bidId)cannot render the server-selected bid because it is absent from APS's browser-auction state.The working path reuses the existing
prebid/creative/renderrunner contract inside a friendly frame. Trusted Server does not callapstag.fetchBids(), callapstag.setDisplayBids(), mutate the publisher's APS SDK, or start another auction. The runner contract is observed vendor behavior rather than a documented external-response API, so APS account-team validation is still required before production use.Changes
crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.tscrates/trusted-server-js/lib/src/core/request.tscrates/trusted-server-js/lib/src/integrations/aps/render.tscrates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-js/lib/test/core/request.test.tscrates/trusted-server-js/lib/test/integrations/aps/render.test.tscrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.tsdocs/guide/integrations/aps.mdtrusted-server.example.tomlScope
This PR is intentionally limited to configuration, bid ownership, browser rendering, focused tests, and operator documentation. It does not add infrastructure, change bid selection, call publisher
apstagmethods, or claim a publicly supported APS external-response API.The Rust and TypeScript changes are both required: the mode must be selected and exposed server-side, then enforced consistently across direct auctions and Google Ad Manager/Prebid rendering paths. Native mode deliberately uses a friendly iframe without the default opaque-origin sandbox, so it has a larger security surface and must remain isolated during testing.
Closes
Related to #999.
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run— 44 files, 842 testscd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servets dev proxyrendered a real selected APS creative with no/integrations/aps/rendererrequestChecklist
unwrap()in production code — useexpect("should ...")println!)