Render APS creatives in opaque data frames - #1033
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Nested opaque data: documents with per-layer nonces and a transferred MessagePort are a genuine improvement over the single sandboxed route: the envelope, bid ID, price, and creative URL now never appear in any URL, and the browser tests assert on real compromise markers rather than the absence of an error. The containment reasoning holds up under review.
Three things block merge: the top-page mount leaves the slot in a state no later GAM refresh can recover from, the legacy fallback posts a message shape its only possible consumer rejects, and the browser-specific behavior the whole design rests on is not exercised by CI.
Blocking
🔧 wrench
- Universal-creative mount permanently forks the slot —
commit()hides the GAM/PUC iframe forever and nothing removes the mounted APS frame on a later refresh, so the slot ends up either blank or double-rendered, and ActiveView measures a hidden iframe (crates/trusted-server-js/lib/src/integrations/aps/render.ts:416, inline). - Legacy fallback message shape can never be accepted — the three-key message is rejected by the only document that path can reach, so the documented rollback never renders; the phase is also entirely untested (
crates/trusted-server-js/lib/src/integrations/aps/render.ts:428, inline). - Cross-browser containment is not gated by CI —
crates/trusted-server-integration-tests/browser/playwright.config.ts:16-21defines only achromiumproject, and.github/workflows/integration-tests.yml:201installs only chromium. The security model depends on browser-specific behavior this PR's own docs name explicitly: CSP inheritance intodata:frames (the containerframe-srcconfining the inner renderer) and WebKit opaque-origin propagation. The PR description reports Firefox and WebKit runs, but nothing in the repository executes them, so a regression in either behavior ships silently. Please addfirefoxandwebkitprojects — scoped toaps-renderer.spec.tsif full-suite runtime is a concern.
❓ question
- No operator kill switch for the new rendering mode —
mountApsRendererFramealways builds the bootstrap URL, so the mode is baked into the JS bundle. The docs warn that the exact-originframe-src"intentionally blocks an immediate creative-frame redirect" and ask operators to "validate real APS inventory for intermediate or redirect origins before rollout". If redirecting creatives turn up in production, is a JS rebuild and redeploy the only remedy? A config flag selecting legacy vs. data-bootstrap would make the rollout reversible at config-push speed.
Non-blocking
🤔 thinking
frame-ancestors 'self'is site-wide and coupled to APS enablement (crates/trusted-server-core/src/response_privacy.rs:169, inline).- Two documents on one path with no cache directives (
crates/trusted-server-core/src/integrations/aps.rs:1155, inline).
♻️ refactor
- Derive APS enablement from the typed config rather than raw JSON (
crates/trusted-server-core/src/response_privacy.rs:33, inline).
📌 out of scope
- The ancestor-allowlist feature the docs promise before publishers with trusted external embedders can enable APS deserves a tracked follow-up issue.
⛏ nitpick
- Rust test asserts on Prettier-formatted HTML (
crates/trusted-server-core/src/integrations/aps.rs:2406, inline).
📝 note
- Cross-crate
include_str!into the JS crate's source tree (crates/trusted-server-core/src/integrations/aps.rs:54, inline).
👍 praise
- Per-layer nonces plus a transferred
MessagePort, so the envelope, bid ID, price, and creative URL never appear in any URL (render.ts:341-360,renderer-container.html:51-65). - The one-shot mount capability carries an honest doc comment about why source-window equality cannot survive the PUC handoff (
render.ts:520, inline). Number.isInteger→Number.isSafeIntegerin both the TypeScript and in-document validators.- Browser tests assert on an actual compromise marker (
document.body.dataset.apsCompromised) instead of on the absence of an error.
CI Status
All 19 checks pass on 19c3709.
- fmt: PASS
- clippy / cargo check (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
- rust tests (core, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
- js tests (vitest) and format-typescript: PASS
- browser + integration tests: PASS (chromium only — see the blocking finding above)
- format-docs: PASS
| } else if (child instanceof HTMLElement) { | ||
| // Keep Universal Creative connected long enough to receive the success | ||
| // acknowledgement, but never leave two visible rendering surfaces. | ||
| child.style.display = 'none'; |
There was a problem hiding this comment.
🔧 wrench — Universal-creative mount permanently forks the slot.
commit() in mode: 'universal-creative' hides the GAM/PUC iframe and never restores or removes it. The mounted APS iframe is removed only when a newer APS mount commits (the dataset.tsApsRenderer branch above). Nothing hooks slot lifecycle — the slotRenderEnded listener in gpt/index.ts only runs the debug adm bypass.
Failure: the slot renders APS through PUC, then the publisher calls refresh() and GAM returns a non-TS creative for the same div.
- If GPT reuses the iframe, the new creative paints inside a
display: noneiframe → blank slot on a billed impression. - If GPT creates a fresh iframe, the stale APS creative and the new GAM creative both sit in the div.
Before this PR the renderer lived inside the PUC frame, so a refresh tore it down together with the GAM document. Moving the mount to the top page drops that cleanup.
Secondary: ActiveView now measures a hidden GAM iframe while the creative paints in a sibling, so viewability on these impressions reports ~0.
Fix: track the mounted frame per div and, on slotRenderEnded for that div, remove it and unhide the GAM iframe unless the new render is the TS APS render.
| fail(); | ||
| return; | ||
| } | ||
| target.postMessage({ nonce, publisherOrigin, renderer }, '*'); |
There was a problem hiding this comment.
🔧 wrench — The legacy fallback message shape can never be accepted.
After LEGACY_RENDERER_FALLBACK_MS without bootstrap-ready, this posts { nonce, publisherOrigin, renderer }. The only document reachable at ?mode=data-bootstrap that lacks the new handler is the pre-PR APS_RENDERER_DOCUMENT — the old Rust handler ignored the query string entirely and served the renderer for any query. That document requires exactly:
keys(message, ['nonce', 'renderer'])so the three-key message is dropped, renderer-ready never arrives, and the 10 s timeout removes the frame with no ad. The rollback path documented in docs/guide/integrations/aps.md does not work.
No test exercises phase === 'legacy' — there is no coverage in the unit specs or the browser spec.
Fix:
target.postMessage({ nonce, renderer }, '*');The new renderer.html accepts both the legacy and modern shapes, so the two-key message stays compatible in both directions. Please add a test for the fallback phase.
If the intended consumer is instead a cache that collapses the query string and serves the new document, that is worth stating explicitly — but the pre-PR rollback case still needs handling either way.
| // a publisher URL beneath the opaque renderer unless every publisher | ||
| // response rejects the cross-origin ancestor chain. Append this independent | ||
| // policy after operator headers so configuration cannot weaken it. | ||
| enforce_aps_publisher_frame_ancestors(settings, response); |
There was a problem hiding this comment.
🤔 thinking — This appends frame-ancestors 'self' to every Trusted Server response whenever integrations.aps.enabled = true, which silently changes the publisher's site-wide embedding policy: AMP caches, partner embeds, in-app webviews, and same-brand cross-origin framing all stop working the moment APS is turned on.
The containment argument for it is sound and the docs are honest about the trade-off, so this is not a request to change the behavior — but it deserves an explicit release note, since the blast radius reaches every page rather than the ad slot.
📌 out of scope — the docs promise "a separately reviewed ancestor-allowlist feature before enabling APS". Worth a tracked follow-up issue so that promise does not go stale.
| fn enforce_aps_publisher_frame_ancestors(settings: &Settings, response: &mut Response) { | ||
| let aps_enabled = settings | ||
| .integrations | ||
| .get(APS_INTEGRATION_ID) |
There was a problem hiding this comment.
♻️ refactor — This re-declares APS_INTEGRATION_ID and re-derives enablement from raw JSON. Settings::integration_config::<ApsConfig> and IntegrationConfig::is_enabled already exist and are the source of truth used by registration.
Using them keeps the two definitions from drifting if APS enablement ever gains another condition — otherwise a future change to ApsConfig::is_enabled leaves this header check silently out of sync with whether APS is actually running.
| message: "Failed to build APS not-found response".to_string(), | ||
| }); | ||
| } | ||
| let (renderer_document, renderer_csp) = match request.uri().query() { |
There was a problem hiding this comment.
🤔 thinking — Two different documents, with two different CSPs, are now served from one path and distinguished only by the query string, with no Cache-Control and no Vary on either response.
Any cache that drops or normalizes the query serves the wrong document to the wrong client: the bootstrap URL would return the legacy renderer (which the fallback then cannot talk to), or the query-free URL would return the bootstrap (which no legacy client understands).
Fix: set an explicit Cache-Control on both responses and confirm the query string is part of the Fastly cache key.
| assert!(APS_RENDERER_DOCUMENT.contains("message.nonce!==expected")); | ||
| assert!(APS_RENDERER_DOCUMENT.contains("event.source !== parent")); | ||
| assert!(APS_RENDERER_DOCUMENT.contains("message.nonce !== expected")); | ||
| assert!(APS_RENDERER_DOCUMENT.contains("['nonce', 'publisherOrigin', 'renderer']")); |
There was a problem hiding this comment.
⛏ nitpick — This asserts on Prettier's exact spacing and wrapping of a file Prettier owns. A printWidth or formatting-config change reflows the array and breaks a core Rust test for a purely cosmetic reason.
Prefer markers that survive reformatting — for example asserting on publisherOrigin and validPublisherOrigin separately, rather than on the rendered array literal.
| </script> | ||
| "#; | ||
| const APS_RENDERER_DOCUMENT: &str = | ||
| include_str!("../../../trusted-server-js/lib/src/integrations/aps/renderer.html"); |
There was a problem hiding this comment.
📝 note — Core now embeds a file from the JS crate's source tree (not dist/). This works and include_str! tracks it for rebuilds, so no change needed — but the cross-crate source coupling is easy to misread as a generated artifact. A one-line comment saying the HTML is hand-written source shared with the TS bundle would save the next reader a detour.
| * cannot survive the handoff. The 128-bit ID is disclosed only in that response, | ||
| * consumed once, and a newer registration revokes the same container's old ID. | ||
| */ | ||
| export function registerApsUniversalCreativeMount( |
There was a problem hiding this comment.
👍 praise — Good doc comment. It states plainly that real PUC asks from a nested frame rather than the controller that received the authenticated response, so source-window equality cannot survive the handoff, and then explains exactly what replaces it (128-bit ID, disclosed once, consumed once, revoked by re-registration). That is the kind of note that keeps someone from "hardening" this back into a broken source check later.
| const APS_INTEGRATION_ID: &str = "aps"; | ||
| const APS_PUBLISHER_FRAME_ANCESTORS_CSP: &str = "frame-ancestors 'self';"; | ||
|
|
||
| fn enforce_aps_publisher_frame_ancestors(settings: &Settings, response: &mut Response) { |
There was a problem hiding this comment.
🌱 seedling — This is a security-sensitive gate, but the only new coverage (aps_enabled_appends_an_independent_publisher_frame_policy) exercises the enabled path. There is no negative test asserting the header is not appended when APS is absent or enabled: false, so a regression that always-appends or always-skips would still pass CI. Consider adding a disabled-case test asserting no frame-ancestors header is added.
| const APS_PUBLISHER_FRAME_ANCESTORS_CSP: &str = "frame-ancestors 'self';"; | ||
|
|
||
| fn enforce_aps_publisher_frame_ancestors(settings: &Settings, response: &mut Response) { | ||
| let aps_enabled = settings |
There was a problem hiding this comment.
📝 note — This reads the raw integrations.aps.enabled bool directly (== Some(true)), while the rest of the codebase decides APS activation through integration_config::<ApsConfig>() / IntegrationConfig::is_enabled. They align today (the provider's is_enabled() is also just config.enabled), so behavior is correct — but it's a second source of truth for "is APS on" that could drift if APS ever gains an alternate enable path. A short comment noting the intentional coupling, or reusing the typed accessor, would guard against that.
| // a publisher URL beneath the opaque renderer unless every publisher | ||
| // response rejects the cross-origin ancestor chain. Append this independent | ||
| // policy after operator headers so configuration cannot weaken it. | ||
| enforce_aps_publisher_frame_ancestors(settings, response); |
There was a problem hiding this comment.
⛏ nitpick — The frame-ancestors 'self' policy is appended to every response type (JSON, images, the tsjs bundle), not just framed HTML. It's harmless (frame-ancestors is inert on non-framed resources) and the byte-exact dedup keeps it idempotent, but it's slightly broader than the stated "publisher document" intent. No change required — flagging for awareness.
|
Follow-up fix pushed in Real-origin reproduction and root causeI replayed captured real APS iframe descriptors through local Trusted Server WASM on Initial direct and PUC rendering both completed: one bootstrap request, one Amazon runner request, one creative request, and PUC The pre-fix refresh replay retained the APS frame and hidden controller. The post-fix replay removes the committed APS frame and restores the controller ( Fix
Validation
Independent final review found no blocker, high, or medium findings. |
3 participants
Summary
data:documents so HTTPS creative origins are retained without granting publisher access.frame-ancestors 'self'protection against recursive publisher framing.Changes
crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/response_privacy.rsframe-ancestors 'self'CSP policy.crates/trusted-server-js/lib/src/integrations/aps/crates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-js/lib/src/core/request.tscrates/trusted-server-js/lib/test/crates/trusted-server-integration-tests/browser/docs/guide/integrations/aps.mdCloses
Closes #999
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, adapter parity tests, Cloudflare/Spin Clippy targets,git diff --check, Chromium browser integration tests (Next.js and WordPress), and Firefox/WebKit browser integration tests (Next.js).Checklist
unwrap()in production code — useexpect("should ...")logmacros rather thanprintln!