Make creative sanitization opt-in and restore creative iframe origin isolation#956
Conversation
…isolation Creative sanitization ran unconditionally on every markup bid, stripping `script`/`object`/`embed`/`form` and friends together with their inner content. For script-based creatives — the majority of programmatic display — that leaves nothing renderable, and the slot goes blank with no error: the leftover markup is usually a tracking pixel, so the ad server reports a successful render. Measured over 291 creative deliveries on a live publisher: a median 43% of bytes removed, 29 creatives reduced by more than 80%, and 20 reduced below 500 bytes. One bidder lost 100% of every creative; another lost 76% across 43 of them. Add `auction.sanitize_creatives` so sanitization can be disabled where creatives render in a foreign-origin frame (the Prebid Universal Creative inside the ad server's iframe), and make both creative controls opt-in: `sanitize_creatives` and `rewrite_creatives` now default to false, so a creative ships exactly as the bidder returned it unless a publisher asks for processing. Removing `allow-same-origin` from the creative iframe sandbox is part of the same change rather than a follow-up. Sanitization was documented as "the primary defense against malicious markup", with the sandbox as defense-in-depth — but the sandbox granted `allow-same-origin` alongside `allow-scripts`, which removes its origin isolation entirely. With sanitization now optional, that pairing would leave creative markup able to reach publisher cookies, storage, and same-origin fetches. The two sibling sandboxes (APS_RENDERER_SANDBOX, ADM_IFRAME_SANDBOX) already omit the token for exactly this reason; this brings the third in line, so the origin boundary no longer depends on an optional transform. Note the default change alters behaviour for deployments that never set `rewrite_creatives`: creative URL rewriting is now off unless enabled explicitly. Verified end to end: creatives pass through byte-for-byte (triplelift 8902 -> 8902, openx 22069 -> 22069, previously 100% and 35% losses), page renders with ads serving and no hydration errors.
951799e to
b4f5def
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The creative-processing behavior change looks coherent: sanitization and rewriting are independently opt-in, and the creative iframe sandbox no longer combines allow-scripts with allow-same-origin. I did not find blocking issues in the runtime/config changes.
Non-blocking
📝 note
- Update stale auction docs/comments for the new creative-processing defaults:
crates/trusted-server-core/src/auction/README.mdstill describes the old contract: creative HTML is always sanitized, rewriting is enabled by default, and[auction].rewrite_creatives = falseskips rewriting but not sanitization. That is now stale with this PR: bothsanitize_creativesandrewrite_creativesdefault tofalse, and sanitization can be disabled independently. Please update the operator-facing README language aroundcrates/trusted-server-core/src/auction/README.md:252andcrates/trusted-server-core/src/auction/README.md:386to describe the new matrix: default raw bidderadm,sanitize_creatives = truestrips executable markup, andrewrite_creatives = trueapplies proxy/click/runtime rewriting to the current creative HTML.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- targeted local regression check: PASS (
cargo test --package trusted-server-cli --target aarch64-apple-darwin migrated_legacy_config_applies_rewrite_creatives_environment_override)
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I found one confirmed blocker in the opaque-origin creative runtime, along with high-risk upgrade and rollback compatibility problems. The sandbox isolation direction is sound, but rewritten clicks can currently fail, existing configurations silently change behavior, and the documented rollback sequence is now unsafe. Please address the inline findings before merge.
| // origin isolation and would let SSP-provided markup run with the publisher | ||
| // origin's privileges — cookies, storage, and same-origin fetches. The origin | ||
| // boundary must not depend on server-side sanitization, which is optional | ||
| // (`auction.sanitize_creatives`) and cannot run at all for renderer-based bids. |
There was a problem hiding this comment.
🔧 P0 — The opaque-origin sandbox breaks rewritten click recovery
Removing allow-same-origin correctly gives this srcdoc creative an opaque origin, but the injected click guard still sends an application/json POST to /first-party/proxy-rebuild. That request preflights with Origin: null, while the endpoint supplies no CORS response. On failure, the runtime navigates to GET /first-party/proxy-rebuild; core supports that GET as a 302 fallback, but every adapter registers the route for POST only. Mutated rewritten clicks can therefore fall through to the publisher origin and end at a 404/error instead of the advertiser. Dynamic image/iframe signing has the same CORS problem when renderGuard is enabled.
Suggested fix: Do not restore allow-same-origin. Register the existing GET rebuild handler in every adapter and make opaque-origin clicks use that navigation fallback. Use a narrowly validated same-origin parent postMessage broker for dynamic signing rather than blanket CORS for Origin: null. Add a real-browser regression test for a mutated rewritten click from this sandboxed srcdoc.
|
|
||
| fn default_rewrite_creatives() -> bool { | ||
| true | ||
| false |
There was a problem hiding this comment.
🔧 P1 — Omitted configuration fields silently disable existing processing on upgrade
Both creative controls now default to false. Every pre-existing deployment omits sanitize_creatives, and blobs produced under this PR's actual base commonly omit its default-true rewrite_creatives. Those blobs therefore deserialize with both operations disabled: mandatory sanitization becomes raw executable bidder markup, while proxy/click rewriting and runtime injection disappear. The sandbox change protects only Trusted Server's own iframe path; /auction is also consumed by programmatic renderers whose isolation is not enforced here.
Suggested fix: Keep sanitize_creatives defaulting to true so the risky behavior is an explicit opt-out, and preserve the base's default-true rewriting unless a breaking migration has been explicitly approved. If false defaults are retained, require a staged configuration migration and release-note the security and behavioral impact.
| serialized.get("rewrite_creatives"), | ||
| Some(&serde_json::Value::Bool(false)), | ||
| "should preserve an explicit rewrite opt-out" | ||
| Some(&serde_json::Value::Bool(true)), |
There was a problem hiding this comment.
🔧 P1 — The documented rollback procedure now creates blobs older binaries reject
The current operator guidance says rewrite_creatives = true is omitted and instructs operators to push true before rollback. This test proves the new serialization behavior is the inverse: true is emitted while false is omitted. Older auction schemas use deny_unknown_fields, and the immediate base similarly rejects a serialized sanitize_creatives. Following the published emergency rollback procedure can therefore make configuration loading fail.
Suggested fix: Add and test explicit downgrade sequences. Before reverting to the immediate base, push sanitize_creatives = false so that field is omitted. Before reverting to a binary knowing neither field, ensure both fields are omitted. Update the configuration guide, auction guide, and changelog accordingly.
| let sanitized = if sanitize_creatives { | ||
| creative::sanitize_creative_html(raw_creative) | ||
| } else { | ||
| raw_creative.clone() |
There was a problem hiding this comment.
🔧 P2 — Disabling sanitization also bypasses the creative-size safety limit
The deliberate 1 MiB MAX_CREATIVE_SIZE check exists only inside sanitize_creative_html, so both unsanitized modes bypass it. With rewriting enabled, the larger input is also passed into rewrite_creative_html; with rewriting disabled, this clone and subsequent JSON serialization still add allocations in constrained WASM memory. RTB responses have a 2 MiB aggregate cap, but this removes the lower per-creative invariant and can compound across providers.
Suggested fix: Enforce a processing-independent creative-size limit before branching on either flag, and test oversized creatives in every supported mode.
| // markup cannot reach the publisher origin — can opt out and deliver the | ||
| // creative exactly as the bidder returned it. | ||
| let mut settings = make_settings(); | ||
| settings.auction.sanitize_creatives = false; |
There was a problem hiding this comment.
🔧 P2 — The fourth processing mode and byte-for-byte contract are untested
The tests cover (sanitize, rewrite) combinations (true,true), (true,false), and (false,false), but not (false,true), which feeds executable bidder markup directly into the rewriter. This pass-through test also checks only that two markers remain rather than proving the stated byte-for-byte contract.
Suggested fix: Add a (false,true) test covering scripts, event handlers, resource/click rewriting, malformed input, and size limits. For (false,false), assert that the parsed response adm exactly equals the original bidder input.
| /// | ||
| /// Disable only when creatives render in a foreign-origin frame (for example the | ||
| /// Prebid Universal Creative inside the ad server's iframe), where the markup | ||
| /// cannot reach the publisher origin. Defaults to disabled. |
There was a problem hiding this comment.
📝 P2 — Security-critical API and client documentation still promises mandatory sanitization
Beyond the auction README already called out in another review, the endpoint Rustdoc, response-converter docs, configuration guide, creative-processing guide, auction-orchestration guide, changelog, and client comments still state that creative markup is always sanitized and rewriting defaults on. Client code also describes the incoming markup as already sanitized and non-executable. That guidance is now the opposite of the runtime default and can cause operators or future maintainers to treat executable bidder markup as trusted.
Suggested fix: Update all public guidance to document the four-mode matrix, defaults, sandbox requirements, direct-resource privacy implications, and migration/rollback behavior. Rename or clearly describe the client sanitizeCreativeHtml helper as validation-only.
3 participants
Fixes #955
Stacks on #916 — targets
feature/optional-creative-rewriting, extending the same "creative processing is opt-in" idea from rewriting to sanitization.Summary
Creative sanitization runs unconditionally on every markup bid, stripping
script/object/embed/formand friends together with their inner content. For script-based creatives — the majority of programmatic display — that leaves nothing renderable, and the slot goes blank with no error: what survives is usually a tracking pixel, soadmis non-empty and the ad server records a normal impression.Measured over 291 creative deliveries on a live publisher: a median 43% of bytes removed, 29 creatives reduced by more than 80%, and 20 reduced below 500 bytes. One bidder lost 100% of every creative; another lost 76% across 43 of them.
Changes
1.
auction.sanitize_creatives— gatessanitize_creative_html, independent ofrewrite_creatives. Lets a publisher disable stripping when creatives render in a foreign-origin frame (the Prebid Universal Creative inside the ad server's iframe — the contextrewrite_creative_htmlalready documents), where the markup cannot reach the publisher origin.2. Both creative controls are opt-in —
sanitize_creativesandrewrite_creativesdefault tofalse, so a creative ships exactly as the bidder returned it unless the publisher asks for processing.3.
allow-same-originremoved fromCREATIVE_SANDBOX_TOKENS— in this PR, not a follow-up.It also mirrors this branch's
skip_serializing_ifpattern onto the new field, so a blob only carries an explicit opt-in. Note the flipped default inverts which value is non-default:rewrite_creatives: trueis now the value worth preserving, sodisabled_rewrite_creatives_is_serializedbecameenabled_rewrite_creatives_is_serialized.Why the sandbox change belongs here
Sanitization is documented as "the primary defense against malicious markup", with the sandbox as defense-in-depth. But the sandbox granted
allow-same-originalongsideallow-scripts, which removes its origin isolation entirely — the codebase says so itself onADM_IFRAME_SANDBOX, andAPS_RENDERER_SANDBOXomits the token for the same reason.CREATIVE_SANDBOX_TOKENSwas the only one granting it.With sanitization now optional, that pairing would leave creative markup able to reach publisher cookies, storage, and same-origin fetches. The client-side
sanitizeCreativeHtmlis no backstop — it validates type and emptiness only and returns input unchanged (removedCountis always 0). So the origin boundary moves to isolation, where it no longer depends on an optional transform.Behaviour change to review
Flipping
rewrite_creativesinverts an existing backward-compatibility guarantee:legacy_blob_without_rewrite_creatives_preserves_rewritingasserted that deployments whose stored config blob predates the field keep rewriting enabled. That test is updated, and the practical effect is that any deployment which never setrewrite_creativesloses creative URL rewriting — first-party proxying, click conversion, and creative TSJS injection.If that is not wanted for other tenants,
rewrite_creativesshould keep defaulting totruewhilesanitize_creativesdefaults tofalse. One-line change on this branch — flagging explicitly for a decision.Verification
trusted-server-corelib suite 1663 passed, 0 failed; JS suite 411 passed;cargo fmtcleansanitize disabled, rewrite disabledwith raw == sanitized == output on every creative (8902 → 8902 and 22069 → 22069 for bidders previously losing 100% and 35%), page serving ads with no hydration errors