-
Notifications
You must be signed in to change notification settings - Fork 12
Make creative sanitization opt-in and restore creative iframe origin isolation #956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/optional-creative-rewriting
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,27 +251,39 @@ pub fn convert_to_openrtb_response( | |
| let width = to_openrtb_i32(bid.width, "width", &bid_context); | ||
| let height = to_openrtb_i32(bid.height, "height", &bid_context); | ||
|
|
||
| // Process creative HTML if present — always sanitize dangerous markup first. | ||
| // Process creative HTML if present. Sanitization is opt-in: when disabled | ||
| // the creative ships exactly as the bidder returned it. | ||
| let creative_html = if let Some(ref raw_creative) = bid.creative { | ||
| let sanitized = creative::sanitize_creative_html(raw_creative); | ||
| let sanitize_creatives = settings.auction.sanitize_creatives; | ||
| let sanitized = if sanitize_creatives { | ||
| creative::sanitize_creative_html(raw_creative) | ||
| } else { | ||
| raw_creative.clone() | ||
| }; | ||
| let sanitized_len = sanitized.len(); | ||
| let rewrite_creatives = settings.auction.rewrite_creatives; | ||
| let processed = if rewrite_creatives { | ||
| creative::rewrite_creative_html(settings, &sanitized) | ||
| } else { | ||
| sanitized | ||
| }; | ||
| let sanitize_mode = if sanitize_creatives { | ||
| "enabled" | ||
| } else { | ||
| "disabled" | ||
| }; | ||
| let rewrite_mode = if rewrite_creatives { | ||
| "enabled" | ||
| } else { | ||
| "disabled" | ||
| }; | ||
|
|
||
| log::debug!( | ||
| "Processed creative for auction {} slot {} bidder {} (rewrite {}, raw {} bytes, sanitized {} bytes, output {} bytes)", | ||
| "Processed creative for auction {} slot {} bidder {} (sanitize {}, rewrite {}, raw {} bytes, sanitized {} bytes, output {} bytes)", | ||
| auction_request.id, | ||
| slot_id, | ||
| bid.bidder, | ||
| sanitize_mode, | ||
| rewrite_mode, | ||
| raw_creative.len(), | ||
| sanitized_len, | ||
|
|
@@ -963,8 +975,10 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_rewrites_sanitized_creative_by_default() { | ||
| let settings = make_settings(); | ||
| fn convert_to_openrtb_response_rewrites_sanitized_creative_when_enabled() { | ||
| let mut settings = make_settings(); | ||
| settings.auction.sanitize_creatives = true; | ||
| settings.auction.rewrite_creatives = true; | ||
| let auction_request = make_auction_request(); | ||
| let result = make_result(make_complete_creative_bid()); | ||
|
|
||
|
|
@@ -1011,9 +1025,52 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_can_skip_rewriting_but_not_sanitization() { | ||
| fn convert_to_openrtb_response_can_skip_sanitization_when_disabled() { | ||
| // Sanitization strips every executable element with its inner content, which | ||
| // destroys script-based creatives (the majority of programmatic display). | ||
| // Publishers whose creatives render in a foreign-origin frame — where the | ||
| // 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P2 — The fourth processing mode and byte-for-byte contract are untested The tests cover Suggested fix: Add a |
||
| settings.auction.rewrite_creatives = false; | ||
| let auction_request = make_auction_request(); | ||
| let result = make_result(make_complete_creative_bid()); | ||
|
|
||
| let response = convert_to_openrtb_response(&result, &settings, &auction_request, false) | ||
| .expect("should convert creative with sanitization disabled"); | ||
| let adm = response_adm(response); | ||
|
|
||
| assert!( | ||
| adm.contains("auction-script-marker"), | ||
| "should retain script content when sanitization is disabled: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains("auction-handler-marker"), | ||
| "should retain event handlers when sanitization is disabled: {adm}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn sanitize_creatives_defaults_to_disabled() { | ||
| let config = crate::auction_config_types::AuctionConfig::default(); | ||
| assert!( | ||
| !config.sanitize_creatives, | ||
| "creatives are delivered as the bidder returned them unless a publisher opts in" | ||
| ); | ||
| assert!( | ||
| !config.rewrite_creatives, | ||
| "creative URL rewriting is opt-in" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_can_skip_rewriting_while_sanitizing() { | ||
| // The two controls are independent: sanitization can stay on while URL | ||
| // rewriting is off. | ||
| let mut settings = make_settings(); | ||
| settings.auction.rewrite_creatives = false; | ||
| settings.auction.sanitize_creatives = true; | ||
| let auction_request = make_auction_request(); | ||
| let result = make_result(make_complete_creative_bid()); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,22 @@ pub struct AuctionConfig { | |
| #[serde(default)] | ||
| pub enabled: bool, | ||
|
|
||
| /// Strip executable markup from winning-bid creative HTML before delivery. | ||
| /// | ||
| /// Sanitization removes `script`/`object`/`embed`/`form`/etc. **with their inner | ||
| /// content**, which blanks script-based creatives — the majority of programmatic | ||
| /// display. It is the primary defence when the creative renders in a context that | ||
| /// shares the publisher's origin. | ||
| /// | ||
| /// 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 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 |
||
| #[serde( | ||
| default = "default_sanitize_creatives", | ||
| skip_serializing_if = "is_default_sanitize_creatives" | ||
| )] | ||
| pub sanitize_creatives: bool, | ||
|
|
||
| /// Rewrite sanitized winning-bid creative HTML to first-party endpoints. | ||
| #[serde( | ||
| default = "default_rewrite_creatives", | ||
|
|
@@ -48,6 +64,7 @@ impl Default for AuctionConfig { | |
| fn default() -> Self { | ||
| Self { | ||
| enabled: false, | ||
| sanitize_creatives: default_sanitize_creatives(), | ||
| rewrite_creatives: default_rewrite_creatives(), | ||
| providers: Vec::new(), | ||
| mediator: None, | ||
|
|
@@ -62,14 +79,22 @@ fn default_timeout() -> u32 { | |
| 2000 | ||
| } | ||
|
|
||
| fn default_sanitize_creatives() -> bool { | ||
| false | ||
| } | ||
|
|
||
| fn default_rewrite_creatives() -> bool { | ||
| true | ||
| false | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P1 — Omitted configuration fields silently disable existing processing on upgrade Both creative controls now default to Suggested fix: Keep |
||
| } | ||
|
|
||
| fn is_default_rewrite_creatives(value: &bool) -> bool { | ||
| *value == default_rewrite_creatives() | ||
| } | ||
|
|
||
| fn is_default_sanitize_creatives(value: &bool) -> bool { | ||
| *value == default_sanitize_creatives() | ||
| } | ||
|
|
||
| fn default_creative_store() -> String { | ||
| "creative_store".to_owned() | ||
| } | ||
|
|
@@ -101,13 +126,17 @@ mod tests { | |
| use super::*; | ||
|
|
||
| #[test] | ||
| fn rewrite_creatives_defaults_to_true() { | ||
| fn creative_processing_defaults_to_disabled() { | ||
| let config: AuctionConfig = | ||
| serde_json::from_value(serde_json::json!({})).expect("should deserialize defaults"); | ||
|
|
||
| assert!( | ||
| config.rewrite_creatives, | ||
| "should enable creative rewriting by default" | ||
| !config.rewrite_creatives, | ||
| "creative rewriting is opt-in: creatives ship as the bidder returned them" | ||
| ); | ||
| assert!( | ||
| !config.sanitize_creatives, | ||
| "creative sanitization is opt-in: it strips executable markup with its content" | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -123,17 +152,44 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn disabled_rewrite_creatives_is_serialized() { | ||
| fn enabled_rewrite_creatives_is_serialized() { | ||
| let config = AuctionConfig { | ||
| rewrite_creatives: false, | ||
| rewrite_creatives: true, | ||
| ..AuctionConfig::default() | ||
| }; | ||
| let serialized = serde_json::to_value(config).expect("should serialize disabled rewriting"); | ||
| let serialized = serde_json::to_value(config).expect("should serialize enabled rewriting"); | ||
|
|
||
| assert_eq!( | ||
| serialized.get("rewrite_creatives"), | ||
| Some(&serde_json::Value::Bool(false)), | ||
| "should preserve an explicit rewrite opt-out" | ||
| Some(&serde_json::Value::Bool(true)), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P1 — The documented rollback procedure now creates blobs older binaries reject The current operator guidance says Suggested fix: Add and test explicit downgrade sequences. Before reverting to the immediate base, push |
||
| "should preserve an explicit rewrite opt-in" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn default_sanitize_creatives_is_not_serialized() { | ||
| let serialized = | ||
| serde_json::to_value(AuctionConfig::default()).expect("should serialize defaults"); | ||
|
|
||
| assert!( | ||
| serialized.get("sanitize_creatives").is_none(), | ||
| "should omit the default sanitize setting" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn enabled_sanitize_creatives_is_serialized() { | ||
| let config = AuctionConfig { | ||
| sanitize_creatives: true, | ||
| ..AuctionConfig::default() | ||
| }; | ||
| let serialized = | ||
| serde_json::to_value(config).expect("should serialize enabled sanitization"); | ||
|
|
||
| assert_eq!( | ||
| serialized.get("sanitize_creatives"), | ||
| Some(&serde_json::Value::Bool(true)), | ||
| "should preserve an explicit sanitize opt-in" | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,15 +7,21 @@ import NORMALIZE_CSS from './styles/normalize.css?inline'; | |
| import IFRAME_TEMPLATE from './templates/iframe.html?raw'; | ||
|
|
||
| // Sandbox permissions granted to creative iframes. | ||
| // | ||
| // Ad creatives routinely contain scripts for tracking, click handling, and | ||
| // viewability measurement, so allow-scripts and allow-same-origin are required | ||
| // for creatives to render correctly. Server-side sanitization is the primary | ||
| // defense against malicious markup; the sandbox provides defense-in-depth. | ||
| // viewability measurement, so `allow-scripts` is required for them to render. | ||
| // | ||
| // `allow-same-origin` is deliberately excluded: combined with `allow-scripts` on | ||
| // srcdoc (or first-party src) content, that pair effectively removes the sandbox's | ||
| // 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P0 — The opaque-origin sandbox breaks rewritten click recovery Removing Suggested fix: Do not restore |
||
| // Matches APS_RENDERER_SANDBOX and ADM_IFRAME_SANDBOX, which already omit it. | ||
| const CREATIVE_SANDBOX_TOKENS = [ | ||
| 'allow-forms', | ||
| 'allow-popups', | ||
| 'allow-popups-to-escape-sandbox', | ||
| 'allow-same-origin', | ||
| 'allow-scripts', | ||
| 'allow-top-navigation-by-user-activation', | ||
| ] as const; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 P2 — Disabling sanitization also bypasses the creative-size safety limit
The deliberate 1 MiB
MAX_CREATIVE_SIZEcheck exists only insidesanitize_creative_html, so both unsanitized modes bypass it. With rewriting enabled, the larger input is also passed intorewrite_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.