Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 63 additions & 6 deletions crates/trusted-server-core/src/auction/formats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Copy link
Copy Markdown
Collaborator

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_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.

};
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,
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 (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.

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());

Expand Down
1 change: 1 addition & 0 deletions crates/trusted-server-core/src/auction/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,7 @@ mod tests {
futures::executor::block_on(async {
let config = AuctionConfig {
enabled: true,
sanitize_creatives: true,
rewrite_creatives: true,
providers: vec![],
mediator: None,
Expand Down
74 changes: 65 additions & 9 deletions crates/trusted-server-core/src/auction_config_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 sanitizeCreativeHtml helper as validation-only.

#[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",
Expand Down Expand Up @@ -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,
Expand All @@ -62,14 +79,22 @@ fn default_timeout() -> u32 {
2000
}

fn default_sanitize_creatives() -> bool {
false
}

fn default_rewrite_creatives() -> bool {
true
false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 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.

}

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()
}
Expand Down Expand Up @@ -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"
);
}

Expand All @@ -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)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 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.

"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"
);
}
}
8 changes: 4 additions & 4 deletions crates/trusted-server-core/src/config_payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ mod tests {
}

#[test]
fn legacy_blob_without_rewrite_creatives_preserves_rewriting() {
let data =
fn legacy_blob_without_rewrite_creatives_leaves_rewriting_disabled() {
let mut data =
serde_json::to_value(test_settings()).expect("should serialize settings to JSON");
let auction = data
.get("auction")
Expand All @@ -115,8 +115,8 @@ mod tests {
settings_from_config_blob(&envelope_json).expect("should reconstruct legacy settings");

assert!(
reconstructed.auction.rewrite_creatives,
"should enable creative rewriting for legacy blobs"
!reconstructed.auction.rewrite_creatives,
"creative rewriting is opt-in: a blob without the field leaves it disabled"
);
}

Expand Down
10 changes: 7 additions & 3 deletions crates/trusted-server-core/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4380,7 +4380,7 @@ origin_host_header_overide = "www.example.com""#,
}

#[test]
fn test_auction_rewrite_creatives_defaults_to_true_when_omitted() {
fn test_auction_creative_processing_defaults_to_false_when_omitted() {
let toml_str = crate_test_settings_str()
+ r#"
[auction]
Expand All @@ -4391,8 +4391,12 @@ origin_host_header_overide = "www.example.com""#,
let settings = Settings::from_toml(&toml_str).expect("should parse valid TOML");

assert!(
settings.auction.rewrite_creatives,
"should preserve creative rewriting when the setting is omitted"
!settings.auction.rewrite_creatives,
"creative rewriting is opt-in when the setting is omitted"
);
assert!(
!settings.auction.sanitize_creatives,
"creative sanitization is opt-in when the setting is omitted"
);
}

Expand Down
14 changes: 10 additions & 4 deletions crates/trusted-server-js/lib/src/core/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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.

// 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;
Expand Down
6 changes: 5 additions & 1 deletion crates/trusted-server-js/lib/test/core/render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ describe('render', () => {
expect(sandbox).toContain('allow-popups');
expect(sandbox).toContain('allow-popups-to-escape-sandbox');
expect(sandbox).toContain('allow-top-navigation-by-user-activation');
expect(sandbox).toContain('allow-same-origin');
expect(sandbox).toContain('allow-scripts');
// `allow-scripts` + `allow-same-origin` together defeat the sandbox: creative
// markup would run with the publisher origin's privileges (cookies, storage,
// same-origin fetches). Matches APS_RENDERER_SANDBOX and ADM_IFRAME_SANDBOX,
// which already omit it.
expect(sandbox).not.toContain('allow-same-origin');
});

it('preserves dollar sequences when building the creative document', async () => {
Expand Down
22 changes: 17 additions & 5 deletions trusted-server.example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,23 @@ rewrite_script = true

[auction]
enabled = false
# Defaults to true. Keep this leaf present when using the EdgeZero v0.0.4
# environment override. Set false to return sanitized but unre-written winning-bid
# adm, skipping proxy/click URL conversion and creative TSJS injection.
# Sanitization is always applied. Restore and push true before an older-binary rollback.
rewrite_creatives = true
# Defaults to false. Keep this leaf present when using the EdgeZero v0.0.4
# environment override. Set true to rewrite winning-bid adm to first-party
# endpoints, converting proxy/click URLs and injecting the creative TSJS runtime.
# Sanitization is controlled separately by `sanitize_creatives` below.
rewrite_creatives = false
# Strip executable markup (script/object/embed/form/...) from winning-bid adm,
# removing those elements together with their inner content.
#
# Defaults to false: creatives are delivered exactly as the bidder returned them.
# Enable whenever creatives can render in a context that shares the publisher's
# origin — it is the primary defence there.
#
# Leave disabled 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. Sanitization removes script-based creatives entirely,
# so enabling it on a script-heavy demand stack silently blanks those slots.
sanitize_creatives = false
providers = []
timeout_ms = 2000
allowed_context_keys = []
Expand Down
Loading