Address creative rewriting review feedback

Author: ChristianPavilonis

crates/trusted-server-cli/tests/config_env_overlay.rs

@@ -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}"
+    );
+}

crates/trusted-server-core/src/auction/README.md

@@ -136,7 +136,8 @@ When a request arrives at the `/auction` endpoint, it goes through the following
 ┌──────────────────────────────────────────────────────────────────────┐
 │  11. Transform to OpenRTB Response (mod.rs:274-322)                  │
 │      - Build seatbid array (one per winning bid)                     │
-│      - Rewrite creative HTML for first-party proxy                   │
+│      - Always sanitize creative HTML                                 │
+│      - Rewrite creative HTML when enabled (default)                  │
 │      - Add orchestrator metadata (timing, strategy, bid count)       │
 └──────────────────────────────────────────────────────────────────────┘
                               │
@@ -248,7 +249,10 @@ The orchestrator collects all bids and creates an OpenRTB response:
 }
 ```
 
-Note that creative HTML is rewritten to use the first-party proxy (`/first-party/proxy`) for privacy and security.
+Creative HTML is always sanitized. By default, it is then rewritten to use the
+first-party proxy (`/first-party/proxy`) and the creative runtime is injected.
+Setting `[auction].rewrite_creatives = false` skips only that rewrite and
+injection pass.
 
 ## Route Registration & Endpoints
 
@@ -379,7 +383,8 @@ The `/auction` endpoint is the primary entry point for auctions:
 **Key Transformations:**
 - `adUnits[].code` → `seatbid[].bid[].impid` (slot identifier)
 - `mediaTypes.banner.sizes` → evaluated by providers, winning size in `bid.w` and `bid.h`
-- Creative HTML is rewritten to use `/first-party/proxy` URLs
+- Creative HTML is always sanitized, then rewritten to use `/first-party/proxy` URLs by default
+- `[auction].rewrite_creatives = false` skips rewriting and creative runtime injection, not sanitization
 - Multiple bids per slot become separate `seatbid` entries
 - Orchestrator metadata added in `ext.orchestrator`
 

crates/trusted-server-core/src/auction_config_types.rs

@@ -12,7 +12,10 @@ pub struct AuctionConfig {
     pub enabled: bool,
 
     /// Rewrite sanitized winning-bid creative HTML to first-party endpoints.
-    #[serde(default = "default_rewrite_creatives")]
+    #[serde(
+        default = "default_rewrite_creatives",
+        skip_serializing_if = "is_default_rewrite_creatives"
+    )]
     pub rewrite_creatives: bool,
 
     /// Provider names that participate in bidding
@@ -63,6 +66,10 @@ fn default_rewrite_creatives() -> bool {
     true
 }
 
+fn is_default_rewrite_creatives(value: &bool) -> bool {
+    *value == default_rewrite_creatives()
+}
+
 fn default_creative_store() -> String {
     "creative_store".to_owned()
 }
@@ -95,9 +102,38 @@ mod tests {
 
     #[test]
     fn rewrite_creatives_defaults_to_true() {
+        let config: AuctionConfig =
+            serde_json::from_value(serde_json::json!({})).expect("should deserialize defaults");
+
         assert!(
-            AuctionConfig::default().rewrite_creatives,
+            config.rewrite_creatives,
             "should enable creative rewriting by default"
         );
     }
+
+    #[test]
+    fn default_rewrite_creatives_is_not_serialized() {
+        let serialized =
+            serde_json::to_value(AuctionConfig::default()).expect("should serialize defaults");
+
+        assert!(
+            serialized.get("rewrite_creatives").is_none(),
+            "should omit the default rewrite setting"
+        );
+    }
+
+    #[test]
+    fn disabled_rewrite_creatives_is_serialized() {
+        let config = AuctionConfig {
+            rewrite_creatives: false,
+            ..AuctionConfig::default()
+        };
+        let serialized = serde_json::to_value(config).expect("should serialize disabled rewriting");
+
+        assert_eq!(
+            serialized.get("rewrite_creatives"),
+            Some(&serde_json::Value::Bool(false)),
+            "should preserve an explicit rewrite opt-out"
+        );
+    }
 }

crates/trusted-server-core/src/config_payload.rs

@@ -46,6 +46,24 @@ mod tests {
     use super::*;
     use crate::redacted::Redacted;
     use crate::test_support::tests::crate_test_settings_str;
+    use serde::Deserialize;
+
+    #[derive(Deserialize)]
+    #[serde(deny_unknown_fields)]
+    struct LegacyAuctionConfig {
+        #[serde(rename = "enabled")]
+        _enabled: bool,
+        #[serde(rename = "providers")]
+        _providers: Vec<String>,
+        #[serde(rename = "mediator")]
+        _mediator: Option<String>,
+        #[serde(rename = "timeout_ms")]
+        _timeout_ms: u32,
+        #[serde(rename = "creative_store")]
+        _creative_store: String,
+        #[serde(rename = "allowed_context_keys")]
+        _allowed_context_keys: std::collections::HashSet<String>,
+    }
 
     fn test_settings() -> Settings {
         Settings::from_toml(&crate_test_settings_str()).expect("should parse test settings")
@@ -80,15 +98,15 @@ mod tests {
 
     #[test]
     fn legacy_blob_without_rewrite_creatives_preserves_rewriting() {
-        let mut data =
+        let data =
             serde_json::to_value(test_settings()).expect("should serialize settings to JSON");
         let auction = data
-            .get_mut("auction")
-            .and_then(serde_json::Value::as_object_mut)
+            .get("auction")
+            .and_then(serde_json::Value::as_object)
             .expect("should serialize auction settings as an object");
         assert!(
-            auction.remove("rewrite_creatives").is_some(),
-            "should remove the newly serialized setting"
+            !auction.contains_key("rewrite_creatives"),
+            "should omit the default rewrite setting from the payload"
         );
         let envelope = BlobEnvelope::new(data, "2026-01-01T00:00:00Z".to_string());
         let envelope_json = serde_json::to_string(&envelope).expect("should serialize envelope");
@@ -102,6 +120,33 @@ mod tests {
         );
     }
 
+    #[test]
+    fn default_auction_payload_is_accepted_by_legacy_schema() {
+        let data =
+            serde_json::to_value(test_settings()).expect("should serialize settings to JSON");
+        let auction = data
+            .get("auction")
+            .cloned()
+            .expect("should serialize auction settings");
+
+        serde_json::from_value::<LegacyAuctionConfig>(auction)
+            .expect("should deserialize the default payload with the legacy schema");
+    }
+
+    #[test]
+    fn disabled_rewrite_creatives_survives_blob_round_trip() {
+        let mut original = test_settings();
+        original.auction.rewrite_creatives = false;
+
+        let reconstructed = settings_from_config_blob(&envelope_json(&original))
+            .expect("should reconstruct disabled rewriting");
+
+        assert!(
+            !reconstructed.auction.rewrite_creatives,
+            "should preserve the explicit rewrite opt-out"
+        );
+    }
+
     #[test]
     fn strings_that_look_like_json_scalars_round_trip_as_strings() {
         let mut original = test_settings();

crates/trusted-server-core/src/creative.rs

@@ -56,30 +56,21 @@ pub(super) fn to_abs(settings: &Settings, u: &str) -> Option<String> {
         return None;
     }
 
-    // Skip if excluded from rewrites in settings
-    if settings.rewrite.is_excluded(t) {
+    let lower = t.to_ascii_lowercase();
+    let absolute = if t.starts_with("//") {
+        format!("https:{t}")
+    } else if lower.starts_with("http://") || lower.starts_with("https://") {
+        t.to_owned()
+    } else {
         return None;
-    }
+    };
 
-    // Skip non-network schemes commonly found in creatives
-    let lower = t.to_ascii_lowercase();
-    if lower.starts_with("data:")
-        || lower.starts_with("javascript:")
-        || lower.starts_with("mailto:")
-        || lower.starts_with("tel:")
-        || lower.starts_with("blob:")
-        || lower.starts_with("about:")
-    {
+    // Match exclusions against the same absolute URL used for rewriting.
+    if settings.rewrite.is_excluded(&absolute) {
         return None;
     }
 
-    if t.starts_with("//") {
-        Some(format!("https:{t}"))
-    } else if lower.starts_with("http://") || lower.starts_with("https://") {
-        Some(t.to_owned())
-    } else {
-        None
-    }
+    Some(absolute)
 }
 
 // Helper: rewrite url(...) occurrences inside a CSS style string to first-party proxy.
@@ -1323,11 +1314,22 @@ mod tests {
             None
         );
 
+        assert_eq!(
+            to_abs(&settings, "//trusted-cdn.example.com/lib.js"),
+            None,
+            "should exclude a protocol-relative URL by exact domain"
+        );
+
         // Non-excluded domain should return Some
         assert_eq!(
             to_abs(&settings, "https://other-cdn.example.com/lib.js"),
             Some("https://other-cdn.example.com/lib.js".to_owned())
         );
+        assert_eq!(
+            to_abs(&settings, "//other-cdn.example.com/lib.js"),
+            Some("https://other-cdn.example.com/lib.js".to_owned()),
+            "should normalize a non-excluded protocol-relative URL"
+        );
     }
 
     #[test]
@@ -1343,6 +1345,16 @@ mod tests {
             to_abs(&settings, "https://cdnjs.cloudflare.com/lib.js"),
             None
         );
+        assert_eq!(
+            to_abs(&settings, "//cloudflare.com/cdn.js"),
+            None,
+            "should exclude a protocol-relative wildcard base domain"
+        );
+        assert_eq!(
+            to_abs(&settings, "//cdnjs.cloudflare.com/lib.js"),
+            None,
+            "should exclude a protocol-relative wildcard subdomain"
+        );
 
         // Should not exclude different domain
         assert_eq!(
@@ -1358,6 +1370,7 @@ mod tests {
 
         let html = r#"
             <img src="https://trusted-cdn.example.com/logo.png">
+            <img src="//trusted-cdn.example.com/protocol-relative.png">
             <img src="https://other-cdn.example.com/banner.jpg">
         "#;
 
@@ -1366,6 +1379,11 @@ mod tests {
         // Excluded domain should NOT be rewritten
         assert!(out.contains(r#"src="https://trusted-cdn.example.com/logo.png"#));
 
+        assert!(
+            out.contains(r#"src="//trusted-cdn.example.com/protocol-relative.png""#),
+            "excluded protocol-relative URL should remain direct: {out}"
+        );
+
         // Non-excluded domain SHOULD be rewritten
         assert!(out.contains("/first-party/proxy?tsurl="));
         assert!(out.contains("other-cdn.example.com"));