-
Notifications
You must be signed in to change notification settings - Fork 12
Make auction creative rewriting optional #916
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: main
Are you sure you want to change the base?
Changes from all commits
6b4e82e
6fcbee0
f341a17
9456b95
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 |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| //! Regression coverage for typed app-config environment overlays. | ||
|
|
||
| use std::fs; | ||
| use std::process::{Command, Output}; | ||
|
|
||
| use tempfile::TempDir; | ||
| use toml_edit::{DocumentMut, value}; | ||
|
|
||
| const LEGACY_CONFIG: &str = include_str!( | ||
| "../../trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml" | ||
| ); | ||
| const MANIFEST: &str = r#" | ||
| [app] | ||
| name = "trusted-server" | ||
|
|
||
| [adapters.axum.adapter] | ||
| crate = "crates/trusted-server-adapter-axum" | ||
|
|
||
| [adapters.axum.commands] | ||
| build = "echo" | ||
| deploy = "echo" | ||
| serve = "echo" | ||
|
|
||
| [stores.config] | ||
| ids = ["trusted_server_config"] | ||
|
|
||
| [stores.secrets] | ||
| ids = ["trusted_server_secrets"] | ||
| "#; | ||
| const REWRITE_ENV: &str = "TRUSTED_SERVER__AUCTION__REWRITE_CREATIVES"; | ||
|
|
||
| struct MigratedProject { | ||
| directory: TempDir, | ||
| config_path: std::path::PathBuf, | ||
| manifest_path: std::path::PathBuf, | ||
| } | ||
|
|
||
| fn migrated_legacy_project() -> MigratedProject { | ||
| let directory = tempfile::tempdir().expect("should create temporary config directory"); | ||
| let config_path = directory.path().join("trusted-server.toml"); | ||
| let manifest_path = directory.path().join("edgezero.toml"); | ||
| let mut document = LEGACY_CONFIG | ||
| .parse::<DocumentMut>() | ||
| .expect("should parse legacy integration config"); | ||
| document["auction"]["rewrite_creatives"] = value(true); | ||
| fs::write(&config_path, document.to_string()).expect("should write migrated config"); | ||
| fs::write(&manifest_path, MANIFEST).expect("should write test manifest"); | ||
| MigratedProject { | ||
| directory, | ||
| config_path, | ||
| manifest_path, | ||
| } | ||
| } | ||
|
|
||
| fn validate_with_overlay(project: &MigratedProject, raw_value: &str) -> Output { | ||
| Command::new(env!("CARGO_BIN_EXE_ts")) | ||
| .args(["config", "validate", "--manifest"]) | ||
| .arg(&project.manifest_path) | ||
| .arg("--app-config") | ||
| .arg(&project.config_path) | ||
| .env(REWRITE_ENV, raw_value) | ||
| .output() | ||
| .expect("should run ts config validate") | ||
| } | ||
|
|
||
| #[test] | ||
| fn migrated_legacy_config_applies_rewrite_creatives_environment_override() { | ||
| let project = migrated_legacy_project(); | ||
| let output = Command::new(env!("CARGO_BIN_EXE_ts")) | ||
| .args(["config", "push", "--adapter", "axum", "--manifest"]) | ||
| .arg(&project.manifest_path) | ||
| .arg("--app-config") | ||
| .arg(&project.config_path) | ||
| .args(["--yes", "--no-diff"]) | ||
| .env(REWRITE_ENV, "false") | ||
| .output() | ||
| .expect("should run ts config push"); | ||
|
|
||
| assert!( | ||
| output.status.success(), | ||
| "valid boolean overlay should push successfully: {}", | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
|
|
||
| let local_store_path = project | ||
| .directory | ||
| .path() | ||
| .join(".edgezero/local-config-trusted_server_config.json"); | ||
| let local_store: serde_json::Value = serde_json::from_str( | ||
| &fs::read_to_string(local_store_path).expect("should read pushed local config"), | ||
| ) | ||
| .expect("should parse local config store"); | ||
| let envelope_json = local_store | ||
| .as_object() | ||
| .and_then(|entries| entries.values().next()) | ||
| .and_then(serde_json::Value::as_str) | ||
| .expect("should contain a blob envelope"); | ||
| let envelope: serde_json::Value = | ||
| serde_json::from_str(envelope_json).expect("should parse blob envelope"); | ||
|
|
||
| assert_eq!( | ||
| envelope["data"]["auction"]["rewrite_creatives"], | ||
| serde_json::Value::Bool(false), | ||
| "pushed config should contain the environment override" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn migrated_legacy_config_rejects_invalid_rewrite_creatives_environment_override() { | ||
| let project = migrated_legacy_project(); | ||
| let output = validate_with_overlay(&project, "not-a-boolean"); | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
|
|
||
| assert!( | ||
| !output.status.success(), | ||
| "invalid boolean overlay should fail validation" | ||
| ); | ||
| assert!( | ||
| stderr.contains(REWRITE_ENV) && stderr.contains("boolean"), | ||
| "error should identify the invalid boolean overlay: {stderr}" | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,7 +217,8 @@ pub fn convert_tsjs_to_auction_request( | |
|
|
||
| /// Convert `OrchestrationResult` to `OpenRTB` response format. | ||
| /// | ||
| /// Returns rewritten creative HTML directly in the `adm` field for inline delivery. | ||
| /// Always sanitizes creative HTML in the `adm` field and optionally rewrites it | ||
| /// according to the auction configuration. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
|
|
@@ -250,21 +251,34 @@ 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 - — sanitize dangerous markup first, then rewrite URLs. | ||
| // Process creative HTML if present — always sanitize dangerous markup first. | ||
| let creative_html = if let Some(ref raw_creative) = bid.creative { | ||
| let sanitized = creative::sanitize_creative_html(raw_creative); | ||
| let rewritten = creative::rewrite_creative_html(settings, &sanitized); | ||
| let sanitized_len = sanitized.len(); | ||
| let rewrite_creatives = settings.auction.rewrite_creatives; | ||
|
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. 🤔 thinking —
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. ♻️ refactor — Disabling this flag turns off the product's core first-party mediation promise, but the only signal is a Fix: emit a one-shot |
||
| let processed = if rewrite_creatives { | ||
| creative::rewrite_creative_html(settings, &sanitized) | ||
| } else { | ||
| sanitized | ||
|
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. ♻️ refactor — "Always sanitize, conditionally rewrite" is a policy, but it lives as an inline Fix: move it into // creative.rs
/// Sanitize auction creative markup, then rewrite it to first-party endpoints
/// unless `[auction].rewrite_creatives` is disabled. Sanitization is mandatory
/// in both modes.
pub fn process_auction_creative(settings: &Settings, raw: &str) -> String {
let sanitized = sanitize_creative_html(raw);
if settings.auction.rewrite_creatives {
rewrite_creative_html(settings, &sanitized)
} else {
sanitized
}
}That also collapses the |
||
| }; | ||
| let rewrite_mode = if rewrite_creatives { | ||
| "enabled" | ||
| } else { | ||
| "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. ⛏ nitpick — |
||
|
|
||
| log::debug!( | ||
| "Processed creative for auction {} slot {} ({} → {} → {} bytes)", | ||
| "Processed creative for auction {} slot {} bidder {} (rewrite {}, raw {} bytes, sanitized {} bytes, output {} bytes)", | ||
| auction_request.id, | ||
| slot_id, | ||
| bid.bidder, | ||
| rewrite_mode, | ||
| raw_creative.len(), | ||
| sanitized.len(), | ||
| rewritten.len() | ||
| sanitized_len, | ||
| processed.len() | ||
| ); | ||
|
|
||
| rewritten | ||
| processed | ||
| } else { | ||
| // No creative provided (e.g., from mediation layer that returns iframe URLs) | ||
| log::warn!( | ||
|
|
@@ -445,6 +459,15 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| fn make_complete_creative_bid() -> Bid { | ||
| let mut bid = make_bid("div-gpt-top", "appnexus", Some(2.75)); | ||
| bid.creative = Some( | ||
| r#"<html><body><a href="https://advertiser.example.com/landing"><img src="https://cdn.example.com/ad.png" style="background-image:url(https://styles.example.com/bg.png)" onerror="auction-handler-marker()"></a><script>auction-script-marker</script></body></html>"# | ||
| .to_string(), | ||
| ); | ||
| bid | ||
| } | ||
|
|
||
| fn make_result(bid: Bid) -> OrchestrationResult { | ||
| OrchestrationResult { | ||
| provider_responses: vec![AuctionResponse { | ||
|
|
@@ -466,6 +489,13 @@ mod tests { | |
| .expect("should parse JSON response") | ||
| } | ||
|
|
||
| fn response_adm(response: Response<EdgeBody>) -> String { | ||
| response_json(response)["seatbid"][0]["bid"][0]["adm"] | ||
| .as_str() | ||
| .expect("should serialize adm as a string") | ||
| .to_string() | ||
| } | ||
|
|
||
| fn make_banner_body(config: Option<JsonValue>) -> AdRequest { | ||
| AdRequest { | ||
| ad_units: vec![AdUnit { | ||
|
|
@@ -932,6 +962,103 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_rewrites_sanitized_creative_by_default() { | ||
| let settings = make_settings(); | ||
| 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 rewriting enabled"); | ||
| let adm = response_adm(response); | ||
|
|
||
| assert!( | ||
| adm.matches("/first-party/proxy?tsurl=").count() >= 2, | ||
| "should rewrite image and inline CSS URLs through the proxy: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains("/first-party/click?tsurl="), | ||
| "should rewrite click URLs: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains("data-tsclick"), | ||
| "should add the click guard attribute: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains("tsjs-unified.min.js"), | ||
| "should inject the unified creative runtime: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains(r#"src="https://cdn.example.com/ad.png""#), | ||
| "should not retain the image URL as a direct attribute: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains(r#"href="https://advertiser.example.com/landing""#), | ||
| "should not retain the click URL as a direct attribute: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("url(https://styles.example.com/bg.png)"), | ||
| "should not retain the CSS URL as a direct value: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("auction-script-marker"), | ||
| "should remove malicious script content before rewriting: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("auction-handler-marker") && !adm.contains("onerror"), | ||
| "should remove event handlers before rewriting: {adm}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_can_skip_rewriting_but_not_sanitization() { | ||
| let mut settings = make_settings(); | ||
| 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 rewriting disabled"); | ||
| let adm = response_adm(response); | ||
|
|
||
| assert!( | ||
| adm.contains(r#"src="https://cdn.example.com/ad.png""#), | ||
| "should retain the sanitizer-accepted image URL: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains(r#"href="https://advertiser.example.com/landing""#), | ||
| "should retain the sanitizer-accepted click URL: {adm}" | ||
| ); | ||
| assert!( | ||
| adm.contains("url(https://styles.example.com/bg.png)"), | ||
| "should retain the sanitizer-accepted CSS URL: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("/first-party/proxy"), | ||
| "should not rewrite resource URLs: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("/first-party/click"), | ||
| "should not rewrite click URLs: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("data-tsclick"), | ||
| "should not add the click guard attribute: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("tsjs-unified.min.js"), | ||
| "should not inject the unified creative runtime: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("auction-script-marker"), | ||
| "should still remove malicious script content: {adm}" | ||
| ); | ||
| assert!( | ||
| !adm.contains("auction-handler-marker") && !adm.contains("onerror"), | ||
| "should still remove event handlers: {adm}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_serializes_missing_creative_as_empty_adm() { | ||
| let settings = make_settings(); | ||
|
|
||
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.
🤔 thinking — Neither this helper nor the
config pushinvocation pins a working directory, soCommandinherits the crate's cwd while the assertion reads.edgezero/local-config-trusted_server_config.jsonfrom the tempdir. It passes on CI, so the store evidently resolves relative to--manifest— but that is an edgezero-cli implementation detail, and if it ever changes the test either fails confusingly or writes.edgezero/into the repo.Fix: add
.current_dir(project.directory.path())to both commands.