From f59fd85a5830edcaab437603799702c34bb1c83b Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 13:24:31 +0530 Subject: [PATCH 01/21] Add gam_unit_path template parser --- .../src/creative_opportunities.rs | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 9ce741f3f..15bf95e69 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -14,6 +14,72 @@ use crate::auction::types::{AdFormat, AdSlot, MediaType}; use crate::price_bucket::PriceGranularity; use crate::settings::vec_from_seq_or_map; +/// A single parsed segment of a [`gam_unit_path`](CreativeOpportunitySlot::gam_unit_path) template. +#[derive(Debug, Clone)] +pub(crate) enum UnitTemplatePart { + /// Verbatim text between placeholders. + Literal(String), + /// `{network_id}` — replaced with the GAM network id. + NetworkId, + /// `{section}` — replaced with the request-derived section. + Section, + /// `{slot_id}` — replaced with the slot id. + SlotId, +} + +/// Parses a `gam_unit_path` template into an ordered list of parts. +/// +/// Supported placeholders: `{network_id}`, `{section}`, `{slot_id}`. A template +/// with no placeholders is a single [`UnitTemplatePart::Literal`] and renders +/// verbatim. +/// +/// # Errors +/// +/// Returns an error string for an empty template, an unmatched or nested `{`, +/// a stray `}`, or an unknown placeholder name. +fn parse_unit_template(raw: &str) -> Result, String> { + if raw.is_empty() { + return Err("gam_unit_path template must not be empty".to_string()); + } + let mut parts = Vec::new(); + let mut literal = String::new(); + let mut chars = raw.chars(); + while let Some(c) = chars.next() { + match c { + '{' => { + if !literal.is_empty() { + parts.push(UnitTemplatePart::Literal(std::mem::take(&mut literal))); + } + let mut name = String::new(); + loop { + match chars.next() { + Some('}') => break, + Some('{') => return Err(format!("nested '{{' in template `{raw}`")), + Some(ch) => name.push(ch), + None => return Err(format!("unmatched '{{' in template `{raw}`")), + } + } + match name.as_str() { + "network_id" => parts.push(UnitTemplatePart::NetworkId), + "section" => parts.push(UnitTemplatePart::Section), + "slot_id" => parts.push(UnitTemplatePart::SlotId), + other => { + return Err(format!( + "unknown placeholder `{{{other}}}` in template `{raw}`" + )); + } + } + } + '}' => return Err(format!("stray '}}' in template `{raw}`")), + other => literal.push(other), + } + } + if !literal.is_empty() { + parts.push(UnitTemplatePart::Literal(literal)); + } + Ok(parts) +} + /// Top-level configuration for the creative opportunities system. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(deny_unknown_fields)] @@ -554,6 +620,46 @@ mod tests { assert_eq!(slot.resolved_div_id(), "atf"); } + #[test] + fn parse_unit_template_accepts_known_placeholders() { + let parts = parse_unit_template("/{network_id}/autoblog/{section}") + .expect("should parse valid template"); + assert_eq!(parts.len(), 4, "should split into literal+ph+literal+ph"); + } + + #[test] + fn parse_unit_template_accepts_static_path() { + let parts = parse_unit_template("/88059007/autoblog/homepage") + .expect("should parse a static path as a single literal"); + assert!( + matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/88059007/autoblog/homepage"), + "should be one literal part" + ); + } + + #[test] + fn parse_unit_template_rejects_unknown_placeholder() { + let err = parse_unit_template("/{network_id}/{oops}") + .expect_err("should reject unknown placeholder"); + assert!(err.contains("oops"), "error should name the bad placeholder"); + } + + #[test] + fn parse_unit_template_rejects_unmatched_brace() { + parse_unit_template("/{network_id}/{section").expect_err("should reject unmatched '{'"); + parse_unit_template("/a}b").expect_err("should reject stray '}'"); + } + + #[test] + fn parse_unit_template_rejects_nested_brace() { + parse_unit_template("/{net{work}_id}").expect_err("should reject nested '{'"); + } + + #[test] + fn parse_unit_template_rejects_empty() { + parse_unit_template("").expect_err("should reject empty template"); + } + #[test] fn validate_runtime_rejects_empty_div_id_override() { // An empty/whitespace div_id would resolve every slot to the first From 9a71f556920215067c6f52cca12044945844a389 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 13:31:04 +0530 Subject: [PATCH 02/21] Add request-path section derivation --- .../src/creative_opportunities.rs | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 15bf95e69..0f5dc9acc 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -80,6 +80,39 @@ fn parse_unit_template(raw: &str) -> Result, String> { Ok(parts) } +/// Collapses each run of characters outside `[A-Za-z0-9_-]` to a single `_`. +/// +/// Returns a non-empty string for any non-empty input. +fn sanitize_section(segment: &str) -> String { + let mut out = String::with_capacity(segment.len()); + let mut in_bad_run = false; + for ch in segment.chars() { + if ch.is_ascii_alphanumeric() || ch == '_' || ch == '-' { + out.push(ch); + in_bad_run = false; + } else if !in_bad_run { + out.push('_'); + in_bad_run = true; + } + } + out +} + +/// Derives the `{section}` value from a request path. +/// +/// Uses the first non-empty path segment, sanitized to `[A-Za-z0-9_-]`. Falls +/// back to `section_root` when the path has no segment (`/`, repeated slashes). +/// +/// The path is used **raw** (not percent-decoded) so this stays consistent with +/// how [`page_patterns`](CreativeOpportunitySlot::page_patterns) glob-match the +/// same path — e.g. `/new%20s` yields `new_20s`, never the decoded `new_s`. +pub(crate) fn derive_section(path: &str, section_root: &str) -> String { + match path.split('/').find(|segment| !segment.is_empty()) { + Some(segment) => sanitize_section(segment), + None => section_root.to_string(), + } +} + /// Top-level configuration for the creative opportunities system. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(deny_unknown_fields)] @@ -660,6 +693,35 @@ mod tests { parse_unit_template("").expect_err("should reject empty template"); } + #[test] + fn derive_section_uses_first_segment() { + assert_eq!(derive_section("/news", "home"), "news"); + assert_eq!(derive_section("/news/gm-cadillac", "home"), "news"); + assert_eq!(derive_section("/car-research/x", "home"), "car-research"); + } + + #[test] + fn derive_section_uses_root_when_no_segment() { + assert_eq!(derive_section("/", "homepage"), "homepage"); + assert_eq!(derive_section("///", "homepage"), "homepage"); + } + + #[test] + fn derive_section_sanitizes_unsafe_runs_to_single_underscore() { + // Not decoded: in "new%20s" only '%' is disallowed ('2' and '0' are + // alphanumeric), so it collapses to a single '_' -> "new_20s". This is + // exactly the no-decode contract: had we decoded, %20 would be a space + // and yield "new_s"; we do NOT decode. + assert_eq!(derive_section("/new%20s", "home"), "new_20s"); + // A run of disallowed chars collapses to one '_'. + assert_eq!(derive_section("/a..b", "home"), "a_b"); + } + + #[test] + fn derive_section_is_non_empty_for_all_disallowed_segment() { + assert_eq!(derive_section("/%%%/x", "home"), "_"); + } + #[test] fn validate_runtime_rejects_empty_div_id_override() { // An empty/whitespace div_id would resolve every slot to the first From 75758730b7acfbd1e4f8779b43a90982538e8e9f Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 15:02:58 +0530 Subject: [PATCH 03/21] Add section_root, unit-template compile/render, and startup validation --- .../src/creative_opportunities.rs | 223 ++++++++++++++++-- crates/trusted-server-core/src/publisher.rs | 4 + 2 files changed, 208 insertions(+), 19 deletions(-) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 0f5dc9acc..59ce6d46b 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -139,6 +139,14 @@ pub struct CreativeOpportunitiesConfig { /// Price granularity for header-bidding price bucketing. Defaults to `Dense`. #[serde(default)] pub price_granularity: PriceGranularity, + /// Value substituted for `{section}` when the request path has no first + /// segment (e.g. `/`). + /// + /// Required when any slot's [`gam_unit_path`](CreativeOpportunitySlot::gam_unit_path) + /// template contains `{section}`. No default — a home-section name is + /// publisher-specific, so the URL→section convention stays in config, not core. + #[serde(default)] + pub section_root: Option, /// Slot templates. Empty vec = feature disabled (no auction fired, no globals injected). #[serde(default, deserialize_with = "vec_from_seq_or_map")] pub slot: Vec, @@ -152,15 +160,48 @@ impl CreativeOpportunitiesConfig { } } + /// Parse every slot's [`gam_unit_path`](CreativeOpportunitySlot::gam_unit_path) + /// template. Call once after deserialization, before [`validate_runtime`](Self::validate_runtime). + /// + /// # Errors + /// + /// Returns an error string when any slot's template is malformed. + pub fn compile_unit_templates(&mut self) -> Result<(), String> { + for slot in &mut self.slot { + slot.compile_unit_template()?; + } + Ok(()) + } + /// Validate all slot definitions after runtime preparation. /// /// # Errors /// /// Returns an error string when a slot has an invalid identifier, page - /// pattern set, format list, dimensions, or resolved GAM unit path. + /// pattern set, format list, or dimensions, or when a slot's `gam_unit_path` + /// template uses `{section}` without a valid [`section_root`](Self::section_root). pub fn validate_runtime(&self) -> Result<(), String> { for slot in &self.slot { - slot.validate_runtime(&self.gam_network_id)?; + slot.validate_runtime()?; + } + + if self + .slot + .iter() + .any(CreativeOpportunitySlot::template_uses_section) + { + match self.section_root.as_deref() { + Some(root) + if !root.is_empty() + && root + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') => {} + _ => { + return Err("section_root is required and must match [A-Za-z0-9_-]+ \ + when a gam_unit_path template uses {section}" + .to_string()); + } + } } Ok(()) @@ -205,6 +246,14 @@ pub struct CreativeOpportunitySlot { /// crate can construct slots via struct-literal syntax with an empty cache. #[serde(skip, default)] pub(crate) compiled_patterns: Vec, + /// Pre-parsed [`gam_unit_path`](Self::gam_unit_path) template, populated by + /// [`compile_unit_template`](Self::compile_unit_template) at startup. + /// + /// `None` when the slot has no explicit `gam_unit_path` (renders the default + /// `//`). `pub(crate)` so cross-module test helpers can build + /// slots via struct-literal syntax with an empty cache. + #[serde(skip, default)] + pub(crate) compiled_unit: Option>, } impl CreativeOpportunitySlot { @@ -214,7 +263,7 @@ impl CreativeOpportunitySlot { /// /// Returns an error string when required slot fields are empty, invalid, /// or semantically unusable at runtime. - pub fn validate_runtime(&self, gam_network_id: &str) -> Result<(), String> { + pub fn validate_runtime(&self) -> Result<(), String> { validate_slot_id(&self.id)?; if self.page_patterns.is_empty() { @@ -269,15 +318,14 @@ impl CreativeOpportunitySlot { )); } - if self - .resolved_gam_unit_path(gam_network_id) - .trim() - .is_empty() + // A present-but-blank `gam_unit_path` renders to an empty/whitespace + // unit path. An empty string also fails template parsing at startup; + // this keeps the slot-level check self-contained (tests call + // `validate_runtime` without compiling templates first). + if let Some(raw) = &self.gam_unit_path + && raw.trim().is_empty() { - return Err(format!( - "slot `{}` resolved GAM unit path must not be empty", - self.id - )); + return Err(format!("slot `{}` gam_unit_path must not be empty", self.id)); } Ok(()) @@ -364,6 +412,52 @@ impl CreativeOpportunitySlot { .unwrap_or_else(|| format!("/{}/{}", gam_network_id, self.id)) } + /// Parses [`gam_unit_path`](Self::gam_unit_path) into + /// [`compiled_unit`](Self::compiled_unit). Call once at startup via + /// [`CreativeOpportunitiesConfig::compile_unit_templates`]. + /// + /// # Errors + /// + /// Returns an error string (prefixed with the slot id) when the template is + /// malformed. See [`parse_unit_template`]. + pub fn compile_unit_template(&mut self) -> Result<(), String> { + self.compiled_unit = match &self.gam_unit_path { + Some(raw) => { + Some(parse_unit_template(raw).map_err(|e| format!("slot `{}`: {e}", self.id))?) + } + None => None, + }; + Ok(()) + } + + /// Renders the resolved GAM unit path for a given network id and section. + /// + /// Substitutes `{network_id}`, `{section}`, and `{slot_id}` in the parsed + /// template. Falls back to `//` when the slot has no template. + #[must_use] + pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { + match &self.compiled_unit { + Some(parts) => parts + .iter() + .map(|part| match part { + UnitTemplatePart::Literal(s) => s.as_str(), + UnitTemplatePart::NetworkId => gam_network_id, + UnitTemplatePart::Section => section, + UnitTemplatePart::SlotId => self.id.as_str(), + }) + .collect(), + None => format!("/{}/{}", gam_network_id, self.id), + } + } + + /// Returns `true` if this slot's compiled template contains `{section}`. + #[must_use] + pub(crate) fn template_uses_section(&self) -> bool { + self.compiled_unit + .as_ref() + .is_some_and(|parts| parts.iter().any(|p| matches!(p, UnitTemplatePart::Section))) + } + /// Returns the div element ID for this slot. /// /// Returns the [`div_id`](Self::div_id) override when set, otherwise returns [`id`](Self::id). @@ -554,6 +648,7 @@ mod tests { targeting: Default::default(), providers: Default::default(), compiled_patterns: Vec::new(), + compiled_unit: None, } } @@ -722,6 +817,96 @@ mod tests { assert_eq!(derive_section("/%%%/x", "home"), "_"); } + fn make_config_with_section_template(section_root: Option<&str>) -> CreativeOpportunitiesConfig { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + CreativeOpportunitiesConfig { + gam_network_id: "88059007".to_string(), + auction_timeout_ms: None, + price_granularity: PriceGranularity::default(), + section_root: section_root.map(str::to_string), + slot: vec![slot], + } + } + + #[test] + fn render_gam_unit_path_substitutes_placeholders() { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.compile_unit_template().expect("should compile template"); + assert_eq!( + slot.render_gam_unit_path("88059007", "news"), + "/88059007/autoblog/news" + ); + } + + #[test] + fn render_gam_unit_path_defaults_when_no_template() { + let mut slot = make_slot("sidebar", vec!["/*"]); + slot.gam_unit_path = None; + slot.compile_unit_template().expect("should compile (no template)"); + assert_eq!(slot.render_gam_unit_path("99999", "ignored"), "/99999/sidebar"); + } + + #[test] + fn render_gam_unit_path_uses_static_template_verbatim() { + let mut slot = make_slot("atf", vec!["/"]); + slot.gam_unit_path = Some("/99999/example/homepage".to_string()); + slot.compile_unit_template() + .expect("should compile static template"); + assert_eq!( + slot.render_gam_unit_path("99999", "news"), + "/99999/example/homepage" + ); + } + + #[test] + fn validate_runtime_requires_section_root_when_template_uses_section() { + let mut config = make_config_with_section_template(None); + config.compile_slots(); + config + .compile_unit_templates() + .expect("templates should compile"); + let err = config + .validate_runtime() + .expect_err("should require section_root"); + assert!(err.contains("section_root"), "error should mention section_root"); + } + + #[test] + fn validate_runtime_rejects_invalid_section_root() { + let mut config = make_config_with_section_template(Some("has space")); + config.compile_slots(); + config + .compile_unit_templates() + .expect("templates should compile"); + config + .validate_runtime() + .expect_err("should reject non [A-Za-z0-9_-] root"); + } + + #[test] + fn validate_runtime_accepts_section_template_with_valid_root() { + let mut config = make_config_with_section_template(Some("homepage")); + config.compile_slots(); + config + .compile_unit_templates() + .expect("templates should compile"); + config + .validate_runtime() + .expect("should accept valid section_root"); + } + + #[test] + fn compile_unit_templates_surfaces_parse_error() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("/{bad}".to_string()); + config.compile_slots(); + config + .compile_unit_templates() + .expect_err("should surface unknown-placeholder error"); + } + #[test] fn validate_runtime_rejects_empty_div_id_override() { // An empty/whitespace div_id would resolve every slot to the first @@ -731,19 +916,19 @@ mod tests { slot.div_id = Some(String::new()); assert!( - slot.validate_runtime("1234").is_err(), + slot.validate_runtime().is_err(), "empty div_id override should fail validation" ); slot.div_id = Some(" ".to_string()); assert!( - slot.validate_runtime("1234").is_err(), + slot.validate_runtime().is_err(), "whitespace-only div_id override should fail validation" ); slot.div_id = Some("div-ad-x".to_string()); assert!( - slot.validate_runtime("1234").is_ok(), + slot.validate_runtime().is_ok(), "a concrete div_id override should pass validation" ); } @@ -755,31 +940,31 @@ mod tests { slot.floor_price = Some(-0.01); assert!( - slot.validate_runtime("1234").is_err(), + slot.validate_runtime().is_err(), "negative floor_price should fail validation" ); slot.floor_price = Some(f64::NAN); assert!( - slot.validate_runtime("1234").is_err(), + slot.validate_runtime().is_err(), "NaN floor_price should fail validation" ); slot.floor_price = Some(f64::INFINITY); assert!( - slot.validate_runtime("1234").is_err(), + slot.validate_runtime().is_err(), "infinite floor_price should fail validation" ); slot.floor_price = Some(0.0); assert!( - slot.validate_runtime("1234").is_ok(), + slot.validate_runtime().is_ok(), "zero floor_price should pass validation" ); slot.floor_price = None; assert!( - slot.validate_runtime("1234").is_ok(), + slot.validate_runtime().is_ok(), "absent floor_price should pass validation" ); } diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index 34909efe7..7edd35cf2 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -4274,6 +4274,7 @@ mod tests { gam_network_id: "21765378893".to_string(), auction_timeout_ms: Some(500), price_granularity: PriceGranularity::Dense, + section_root: None, slot: Vec::new(), } } @@ -4295,6 +4296,7 @@ mod tests { .collect(), providers: Default::default(), compiled_patterns: Vec::new(), + compiled_unit: None, } } @@ -4942,6 +4944,7 @@ mod tests { targeting: Default::default(), providers: Default::default(), compiled_patterns: Vec::new(), + compiled_unit: None, }] } @@ -5444,6 +5447,7 @@ mod tests { targeting: Default::default(), providers: Default::default(), compiled_patterns: Vec::new(), + compiled_unit: None, }] } From 860ed0b263750be22f62fe2cdd13235726b00267 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 15:11:46 +0530 Subject: [PATCH 04/21] Render gam_unit_path template per request across initial and SPA paths --- crates/trusted-server-core/src/publisher.rs | 43 +++++++++++++++++---- crates/trusted-server-core/src/settings.rs | 9 ++++- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index 7edd35cf2..90bdc6c1d 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -1788,7 +1788,7 @@ pub async fn handle_publisher_request( settings .creative_opportunities .as_ref() - .map(|co_config| build_ad_slots_script(&matched_slots, co_config)) + .map(|co_config| build_ad_slots_script(&matched_slots, co_config, &request_path)) } else { None }; @@ -2201,11 +2201,18 @@ pub(crate) fn build_empty_bids_script() -> String { /// definition and the two paths cannot silently diverge. Property names match /// what the client-side TSJS bundle expects: `gam_unit_path`, `div_id`, /// `formats`, and `targeting`. -fn build_slot_json( +pub(crate) fn build_slot_json( slot: &crate::creative_opportunities::CreativeOpportunitySlot, co_config: &crate::creative_opportunities::CreativeOpportunitiesConfig, + request_path: &str, ) -> serde_json::Value { - let gam_path = slot.resolved_gam_unit_path(&co_config.gam_network_id); + // `{section}` derives from the same raw path `page_patterns` matched + // against; `section_root` covers the no-segment case (`/`). + let section = crate::creative_opportunities::derive_section( + request_path, + co_config.section_root.as_deref().unwrap_or_default(), + ); + let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, §ion); let div_id = slot.resolved_div_id(); let formats: Vec = slot .formats @@ -2233,10 +2240,11 @@ fn build_slot_json( pub(crate) fn build_ad_slots_script( matched_slots: &[crate::creative_opportunities::CreativeOpportunitySlot], co_config: &crate::creative_opportunities::CreativeOpportunitiesConfig, + request_path: &str, ) -> String { let slots: Vec = matched_slots .iter() - .map(|slot| build_slot_json(slot, co_config)) + .map(|slot| build_slot_json(slot, co_config, request_path)) .collect(); let json = serde_json::to_string(&slots) .expect("serde_json::to_string of Vec should be infallible"); @@ -2562,7 +2570,7 @@ pub async fn handle_page_bids( let slots_json: Vec = if ad_stack_enabled { matched_slots .iter() - .map(|slot| build_slot_json(slot, co_config)) + .map(|slot| build_slot_json(slot, co_config, &path_param)) .collect() } else { Vec::new() @@ -4331,7 +4339,7 @@ mod tests { fn ad_slots_script_contains_slot_data() { let slots = vec![make_slot()]; let config = make_config(); - let script = build_ad_slots_script(&slots, &config); + let script = build_ad_slots_script(&slots, &config, "/"); assert!( script.contains("window.tsjs=window.tsjs||{}"), "should initialise tsjs namespace" @@ -4352,7 +4360,7 @@ mod tests { fn ad_slots_script_is_xss_safe() { let slots = vec![make_slot()]; let config = make_config(); - let script = build_ad_slots_script(&slots, &config); + let script = build_ad_slots_script(&slots, &config, "/"); let inner = script .trim_start_matches(""); @@ -4360,6 +4368,27 @@ mod tests { assert!(!inner.contains('>'), "no unescaped > in script content"); } + #[test] + fn build_slot_json_renders_section_from_request_path() { + let mut config = make_config(); + config.section_root = Some("homepage".to_string()); + let mut slot = make_slot(); + slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.compile_unit_template().expect("template should compile"); + + let news = crate::publisher::build_slot_json(&slot, &config, "/news/gm-cadillac"); + assert_eq!( + news["gam_unit_path"], "/21765378893/autoblog/news", + "section should derive from the first path segment" + ); + + let home = crate::publisher::build_slot_json(&slot, &config, "/"); + assert_eq!( + home["gam_unit_path"], "/21765378893/autoblog/homepage", + "root path should use section_root" + ); + } + #[test] fn bid_map_includes_nurl_and_burl() { let mut winning_bids = HashMap::new(); diff --git a/crates/trusted-server-core/src/settings.rs b/crates/trusted-server-core/src/settings.rs index c49e99686..4514d12bd 100644 --- a/crates/trusted-server-core/src/settings.rs +++ b/crates/trusted-server-core/src/settings.rs @@ -2077,6 +2077,13 @@ impl Settings { if let Some(co) = &mut self.creative_opportunities { co.compile_slots(); + // Parse `gam_unit_path` templates once here (mirrors the compiled + // glob cache) so request-time rendering is substitution-only. + co.compile_unit_templates().map_err(|err| { + Report::new(TrustedServerError::Configuration { + message: format!("Invalid creative opportunity gam_unit_path template: {err}"), + }) + })?; // Slots flow into injected HTML/JS, provider payloads, and GPT // calls. Env/private config can bypass static review, so validate // the full runtime shape on every load path. @@ -5602,7 +5609,7 @@ gam_unit_path = "" page_patterns = ["/"] formats = [{ width = 300, height = 250 }] "#, - "resolved GAM unit path must not be empty", + "gam_unit_path template must not be empty", ); } From 21a35239b82ebe251ba2840075667f52b7fd3c20 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 15:13:39 +0530 Subject: [PATCH 05/21] Add spec and plan for per-section gam_unit_path --- .../2026-07-23-per-section-gam-unit-path.md | 746 ++++++++++++++++++ ...-07-23-per-section-gam-unit-path-design.md | 208 +++++ 2 files changed, 954 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md create mode 100644 docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md diff --git a/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md b/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md new file mode 100644 index 000000000..375795a6b --- /dev/null +++ b/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md @@ -0,0 +1,746 @@ +# Per-Section `gam_unit_path` Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `creative_opportunities.slot.gam_unit_path` a template with a +`{section}` placeholder derived from the request path, so one slot rule serves +all site sections instead of one rule per (slot × section). + +**Architecture:** Parse each slot's `gam_unit_path` into a cached template at +startup (alongside the existing compiled-glob cache); reject malformed templates +and a `{section}` template missing its `section_root`. At request time derive +`{section}` from the raw path (sanitized) and render the template inside +`build_slot_json`, which gains a `request_path` argument. Server-only — the +client keeps receiving a resolved `gam_unit_path` string, so no JS change. + +**Tech Stack:** Rust 2024, `trusted-server-core`. Tests via `cargo test_details` +(native host, `aarch64-apple-darwin`) for iteration and `cargo test-fastly` +(core + fastly on `wasm32-wasip1` via Viceroy) for the CI gate. + +**Spec:** `docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md` + +**Issue:** https://github.com/IABTechLab/trusted-server/issues/954 + +--- + +## File Structure + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs` + - new: `UnitTemplatePart` enum, `parse_unit_template`, `sanitize_section`, + `derive_section` + - new on `CreativeOpportunitySlot`: `compiled_unit` field, + `compile_unit_template`, `render_gam_unit_path`, `template_uses_section` + - new on `CreativeOpportunitiesConfig`: `section_root` field, + `compile_unit_templates`; extend `validate_runtime` + - unit tests in the existing `#[cfg(test)] mod tests` +- Modify: `crates/trusted-server-core/src/publisher.rs` + - `build_slot_json` gains `request_path: &str`; renders via `render_gam_unit_path` + - `build_ad_slots_script` gains `request_path: &str`; threads it through + - `handle_page_bids` passes its normalized `path` to `build_slot_json` +- Modify: `crates/trusted-server-core/src/settings.rs` + - `prepare_runtime` calls `compile_unit_templates` and surfaces parse errors +- Modify: `docs/guide/configuration.md` (add creative_opportunities section) +- Modify: `trusted-server.example.toml` and the live autoblog config + +Notes on lifecycle: `page_patterns` inheritance is **out of scope** (sibling +issue). Templates are parsed at startup and cached with `#[serde(skip)]`, +mirroring the existing `compiled_patterns` field. + +--- + +## Task 1: Template parser + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs` +- Test: same file, `#[cfg(test)] mod tests` + +- [ ] **Step 1: Write the failing tests** + +Add to `mod tests`: + +```rust +#[test] +fn parse_unit_template_accepts_known_placeholders() { + let parts = parse_unit_template("/{network_id}/autoblog/{section}") + .expect("should parse valid template"); + assert_eq!(parts.len(), 4, "should split into literal+ph+literal+ph"); +} + +#[test] +fn parse_unit_template_accepts_static_path() { + let parts = parse_unit_template("/88059007/autoblog/homepage") + .expect("should parse a static path as a single literal"); + assert!( + matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/88059007/autoblog/homepage"), + "should be one literal part" + ); +} + +#[test] +fn parse_unit_template_rejects_unknown_placeholder() { + let err = parse_unit_template("/{network_id}/{oops}").expect_err("should reject unknown placeholder"); + assert!(err.contains("oops"), "error should name the bad placeholder"); +} + +#[test] +fn parse_unit_template_rejects_unmatched_brace() { + parse_unit_template("/{network_id}/{section").expect_err("should reject unmatched '{'"); + parse_unit_template("/a}b").expect_err("should reject stray '}'"); +} + +#[test] +fn parse_unit_template_rejects_nested_brace() { + parse_unit_template("/{net{work}_id}").expect_err("should reject nested '{'"); +} + +#[test] +fn parse_unit_template_rejects_empty() { + parse_unit_template("").expect_err("should reject empty template"); +} +``` + +- [ ] **Step 2: Run tests, verify they fail** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests::parse_unit_template` +Expected: FAIL — `cannot find function parse_unit_template` / `UnitTemplatePart`. + +- [ ] **Step 3: Implement the enum + parser** + +Add near the top of the module body (after imports): + +```rust +/// A single parsed segment of a `gam_unit_path` template. +#[derive(Debug, Clone)] +pub(crate) enum UnitTemplatePart { + /// Verbatim text between placeholders. + Literal(String), + /// `{network_id}` — replaced with the GAM network id. + NetworkId, + /// `{section}` — replaced with the request-derived section. + Section, + /// `{slot_id}` — replaced with the slot id. + SlotId, +} + +/// Parses a `gam_unit_path` template into an ordered list of parts. +/// +/// # Errors +/// +/// Returns an error string for an empty template, an unmatched or nested `{`, +/// a stray `}`, or an unknown placeholder name. +fn parse_unit_template(raw: &str) -> Result, String> { + if raw.is_empty() { + return Err("gam_unit_path template must not be empty".to_string()); + } + let mut parts = Vec::new(); + let mut literal = String::new(); + let mut chars = raw.chars().peekable(); + while let Some(c) = chars.next() { + match c { + '{' => { + if !literal.is_empty() { + parts.push(UnitTemplatePart::Literal(std::mem::take(&mut literal))); + } + let mut name = String::new(); + loop { + match chars.next() { + Some('}') => break, + Some('{') => { + return Err(format!("nested '{{' in template `{raw}`")); + } + Some(ch) => name.push(ch), + None => return Err(format!("unmatched '{{' in template `{raw}`")), + } + } + match name.as_str() { + "network_id" => parts.push(UnitTemplatePart::NetworkId), + "section" => parts.push(UnitTemplatePart::Section), + "slot_id" => parts.push(UnitTemplatePart::SlotId), + other => { + return Err(format!( + "unknown placeholder `{{{other}}}` in template `{raw}`" + )); + } + } + } + '}' => return Err(format!("stray '}}' in template `{raw}`")), + other => literal.push(other), + } + } + if !literal.is_empty() { + parts.push(UnitTemplatePart::Literal(literal)); + } + Ok(parts) +} +``` + +- [ ] **Step 4: Run tests, verify they pass** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests::parse_unit_template` +Expected: PASS (6 tests). + +- [ ] **Step 5: Commit** + +```bash +git add crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Add gam_unit_path template parser" +``` + +--- + +## Task 2: Section derivation + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs` +- Test: same file + +- [ ] **Step 1: Write the failing tests** + +```rust +#[test] +fn derive_section_uses_first_segment() { + assert_eq!(derive_section("/news", "home"), "news"); + assert_eq!(derive_section("/news/gm-cadillac", "home"), "news"); + assert_eq!(derive_section("/car-research/x", "home"), "car-research"); +} + +#[test] +fn derive_section_uses_root_when_no_segment() { + assert_eq!(derive_section("/", "homepage"), "homepage"); + assert_eq!(derive_section("///", "homepage"), "homepage"); +} + +#[test] +fn derive_section_sanitizes_unsafe_runs_to_single_underscore() { + // Not decoded: in "new%20s" only '%' is disallowed ('2' and '0' are + // alphanumeric), so it collapses to a single '_' -> "new_20s". This is + // exactly the no-decode contract: had we decoded, %20 would be a space and + // yield "new_s"; we do NOT decode. + assert_eq!(derive_section("/new%20s", "home"), "new_20s"); + // A run of disallowed chars collapses to one '_'. + assert_eq!(derive_section("/a..b", "home"), "a_b"); +} + +#[test] +fn derive_section_is_non_empty_for_all_disallowed_segment() { + assert_eq!(derive_section("/%%%/x", "home"), "_"); +} +``` + +- [ ] **Step 2: Run tests, verify they fail** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests::derive_section` +Expected: FAIL — `cannot find function derive_section`. + +- [ ] **Step 3: Implement the two functions** + +```rust +/// Collapses each run of characters outside `[A-Za-z0-9_-]` to a single `_`. +/// +/// Returns a non-empty string for any non-empty input. +fn sanitize_section(segment: &str) -> String { + let mut out = String::with_capacity(segment.len()); + let mut in_bad_run = false; + for ch in segment.chars() { + if ch.is_ascii_alphanumeric() || ch == '_' || ch == '-' { + out.push(ch); + in_bad_run = false; + } else if !in_bad_run { + out.push('_'); + in_bad_run = true; + } + } + out +} + +/// Derives the `{section}` value from a request path. +/// +/// Uses the first non-empty path segment, sanitized to `[A-Za-z0-9_-]`. Falls +/// back to `section_root` when the path has no segment (`/`, repeated slashes). +/// The path is used **raw** (not percent-decoded) so this stays consistent with +/// how `page_patterns` glob-match the same path. +pub(crate) fn derive_section(path: &str, section_root: &str) -> String { + match path.split('/').find(|segment| !segment.is_empty()) { + Some(segment) => sanitize_section(segment), + None => section_root.to_string(), + } +} +``` + +- [ ] **Step 4: Run tests, verify they pass** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests::derive_section` +Expected: PASS (4 tests). + +- [ ] **Step 5: Commit** + +```bash +git add crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Add request-path section derivation" +``` + +--- + +## Task 3: Config field, template compile + render, startup validation + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs` +- Test: same file + +- [ ] **Step 1: Write the failing tests** + +```rust +// NOTE: the existing helper signature is `make_slot(id: &str, patterns: Vec<&str>)` +// (see creative_opportunities.rs:443) — pass `vec![...]`, not `&[...]`. +#[test] +fn render_gam_unit_path_substitutes_placeholders() { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.compile_unit_template().expect("should compile template"); + assert_eq!( + slot.render_gam_unit_path("88059007", "news"), + "/88059007/autoblog/news" + ); +} + +#[test] +fn render_gam_unit_path_defaults_when_no_template() { + let mut slot = make_slot("sidebar", vec!["/*"]); + slot.gam_unit_path = None; + slot.compile_unit_template().expect("should compile (no template)"); + assert_eq!(slot.render_gam_unit_path("99999", "ignored"), "/99999/sidebar"); +} + +#[test] +fn render_gam_unit_path_uses_static_template_verbatim() { + let mut slot = make_slot("atf", vec!["/"]); + slot.gam_unit_path = Some("/99999/example/homepage".to_string()); + slot.compile_unit_template().expect("should compile static template"); + assert_eq!(slot.render_gam_unit_path("99999", "news"), "/99999/example/homepage"); +} + +#[test] +fn validate_runtime_requires_section_root_when_template_uses_section() { + let mut config = make_config_with_section_template(None); // section_root = None + config.compile_slots(); + config.compile_unit_templates().expect("templates compile"); + let err = config.validate_runtime().expect_err("should require section_root"); + assert!(err.contains("section_root"), "error should mention section_root"); +} + +#[test] +fn validate_runtime_rejects_invalid_section_root() { + let mut config = make_config_with_section_template(Some("has space")); + config.compile_slots(); + config.compile_unit_templates().expect("templates compile"); + config.validate_runtime().expect_err("should reject non [A-Za-z0-9_-] root"); +} + +#[test] +fn validate_runtime_accepts_section_template_with_valid_root() { + let mut config = make_config_with_section_template(Some("homepage")); + config.compile_slots(); + config.compile_unit_templates().expect("templates compile"); + config.validate_runtime().expect("should accept valid section_root"); +} + +#[test] +fn compile_unit_templates_surfaces_parse_error() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("/{bad}".to_string()); + config.compile_slots(); + config.compile_unit_templates().expect_err("should surface unknown-placeholder error"); +} +``` + +Add test helpers to `mod tests` if not present (adapt to the existing helper +style in this module): + +```rust +fn make_config_with_section_template(section_root: Option<&str>) -> CreativeOpportunitiesConfig { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + CreativeOpportunitiesConfig { + gam_network_id: "88059007".to_string(), + auction_timeout_ms: None, + price_granularity: PriceGranularity::default(), + section_root: section_root.map(str::to_string), + slot: vec![slot], + } +} +``` + +The `make_slot(id: &str, patterns: Vec<&str>)` helper **already exists** at +`creative_opportunities.rs:443` and constructs a `CreativeOpportunitySlot` via +struct-literal syntax. Because the struct uses `#[serde(deny_unknown_fields)]` +and the helper names every field explicitly, adding `compiled_unit` to the +struct makes this helper fail to compile until updated — see Step 3's helper-fix +sub-step. + +- [ ] **Step 2: Run tests, verify they fail** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests` +Expected: FAIL — missing `section_root`, `compiled_unit`, `compile_unit_template`, +`render_gam_unit_path`, `compile_unit_templates`. + +- [ ] **Step 3: Add the field, cache, methods, and validation** + +On `CreativeOpportunitiesConfig` (add field): + +```rust +/// Value substituted for `{section}` when the request path has no first +/// segment (e.g. `/`). Required when any slot's `gam_unit_path` template +/// contains `{section}`. No default — a home-section name is publisher-specific. +#[serde(default)] +pub section_root: Option, +``` + +On `CreativeOpportunitySlot` (add cached template, parallel to `compiled_patterns`): + +```rust +/// Pre-parsed [`gam_unit_path`](Self::gam_unit_path) template, populated by +/// [`compile_unit_template`](Self::compile_unit_template) at startup. `None` +/// when the slot has no explicit `gam_unit_path` (uses the default path). +#[serde(skip, default)] +pub(crate) compiled_unit: Option>, +``` + +Slot methods: + +```rust +/// Parses [`gam_unit_path`](Self::gam_unit_path) into [`compiled_unit`](Self::compiled_unit). +/// +/// # Errors +/// +/// Returns an error string when the template is malformed (see +/// [`parse_unit_template`]). +pub fn compile_unit_template(&mut self) -> Result<(), String> { + self.compiled_unit = match &self.gam_unit_path { + Some(raw) => Some(parse_unit_template(raw).map_err(|e| format!("slot `{}`: {e}", self.id))?), + None => None, + }; + Ok(()) +} + +/// Renders the resolved GAM unit path for a given network id and section. +/// +/// Uses the parsed template when present, otherwise the default +/// `//`. +#[must_use] +pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { + match &self.compiled_unit { + Some(parts) => parts + .iter() + .map(|part| match part { + UnitTemplatePart::Literal(s) => s.as_str(), + UnitTemplatePart::NetworkId => gam_network_id, + UnitTemplatePart::Section => section, + UnitTemplatePart::SlotId => self.id.as_str(), + }) + .collect(), + None => format!("/{}/{}", gam_network_id, self.id), + } +} + +/// Returns `true` if this slot's compiled template contains `{section}`. +#[must_use] +pub(crate) fn template_uses_section(&self) -> bool { + self.compiled_unit + .as_ref() + .is_some_and(|parts| parts.iter().any(|p| matches!(p, UnitTemplatePart::Section))) +} +``` + +On `CreativeOpportunitiesConfig` (compile all templates + extend validation): + +```rust +/// Parse every slot's `gam_unit_path` template. Call once after deserialization. +/// +/// # Errors +/// +/// Returns an error string when any slot's template is malformed. +pub fn compile_unit_templates(&mut self) -> Result<(), String> { + for slot in &mut self.slot { + slot.compile_unit_template()?; + } + Ok(()) +} +``` + +In `validate_runtime`, after the existing per-slot loop, add: + +```rust +if self.slot.iter().any(CreativeOpportunitySlot::template_uses_section) { + match self.section_root.as_deref() { + Some(root) + if !root.is_empty() + && root.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') => {} + _ => { + return Err( + "section_root is required and must match [A-Za-z0-9_-]+ when a \ + gam_unit_path template uses {section}" + .to_string(), + ); + } + } +} +``` + +Remove the old path-render emptiness check in `validate_runtime` +(the block calling `resolved_gam_unit_path(...).trim().is_empty()`); malformed or +empty templates are now caught at parse time by `compile_unit_templates`, and a +rendered result is non-empty by construction. + +**Update the existing test helper (required — adding `compiled_unit` breaks it):** +Add `compiled_unit: None` to the `CreativeOpportunitySlot` struct-literal in +`make_slot` at `crates/trusted-server-core/src/creative_opportunities.rs:443`. +The struct uses `#[serde(deny_unknown_fields)]` and the helper names every field, +so a missing field is a compile error, not a `#[serde(default)]` fill-in. + +- [ ] **Step 4: Run tests, verify they pass** + +Run: `cargo test_details -p trusted-server-core creative_opportunities::tests` +Expected: PASS (Task 1–3 tests). + +- [ ] **Step 5: Commit** + +```bash +git add crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Add section_root, unit-template compile/render, and startup validation" +``` + +--- + +## Task 4: Render at request time (thread the path through publisher.rs) + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs` (remove now-unused `resolved_gam_unit_path`, or keep if other callers remain — grep first) +- Modify: `crates/trusted-server-core/src/publisher.rs` +- Modify: `crates/trusted-server-core/src/settings.rs` +- Test: `crates/trusted-server-core/src/publisher.rs` `#[cfg(test)] mod tests` + +- [ ] **Step 0: Update publisher.rs struct-literal test helpers (required — new fields break them)** + +Adding `section_root` to `CreativeOpportunitiesConfig` and `compiled_unit` to +`CreativeOpportunitySlot` breaks every hand-built literal in `publisher.rs` +tests. Add the new fields to each: + +- `crates/trusted-server-core/src/publisher.rs:4272` — `make_config()`: add `section_root: None`. +- `crates/trusted-server-core/src/publisher.rs:4282` — `make_slot()`: add `compiled_unit: None`. +- `crates/trusted-server-core/src/publisher.rs:4931` — `article_slot()`: add `compiled_unit: None`. +- `crates/trusted-server-core/src/publisher.rs:5433` — `article_slot()` (second module): add `compiled_unit: None`. + +Run: `cargo test_details -p trusted-server-core publisher:: --no-run` +Expected: compiles (no `missing field` errors) before writing the new test. + +- [ ] **Step 1: Write the failing test (equivalence + per-section)** + +In `publisher.rs` tests, add (adapt to the existing test helpers/config builders +in that module): + +```rust +#[test] +fn build_slot_json_renders_section_from_request_path() { + let config = creative_opportunities_config_with_template(); // gam_unit_path = "/{network_id}/autoblog/{section}", section_root = "homepage" + let slot = &config.slot[0]; + + let news = build_slot_json(slot, &config, "/news/gm-cadillac"); + assert_eq!(news["gam_unit_path"], "/88059007/autoblog/news"); + + let home = build_slot_json(slot, &config, "/"); + assert_eq!(home["gam_unit_path"], "/88059007/autoblog/homepage"); +} +``` + +- [ ] **Step 2: Run test, verify it fails** + +Run: `cargo test_details -p trusted-server-core publisher::tests::build_slot_json_renders_section` +Expected: FAIL — `build_slot_json` takes 2 args / wrong unit value. + +- [ ] **Step 3: Thread `request_path` and render** + +In `build_slot_json` (`crates/trusted-server-core/src/publisher.rs` ~2204): + +```rust +fn build_slot_json( + slot: &crate::creative_opportunities::CreativeOpportunitySlot, + co_config: &crate::creative_opportunities::CreativeOpportunitiesConfig, + request_path: &str, +) -> serde_json::Value { + let section = crate::creative_opportunities::derive_section( + request_path, + co_config.section_root.as_deref().unwrap_or_default(), + ); + let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, §ion); + // ...rest unchanged (div_id, formats, targeting, json!)... +} +``` + +In `build_ad_slots_script` (~2233) add `request_path: &str` and pass it: + +```rust +pub(crate) fn build_ad_slots_script( + matched_slots: &[crate::creative_opportunities::CreativeOpportunitySlot], + co_config: &crate::creative_opportunities::CreativeOpportunitiesConfig, + request_path: &str, +) -> String { + let slots: Vec = matched_slots + .iter() + .map(|slot| build_slot_json(slot, co_config, request_path)) + .collect(); + // ...unchanged... +} +``` + +At the initial-render caller (~publisher.rs:1791) pass `&request_path`: + +```rust +.map(|co_config| build_ad_slots_script(&matched_slots, co_config, &request_path)) +``` + +In `handle_page_bids` (~2562) pass the already-normalized path +(`path_param` / the value from `normalize_page_bids_path`) to `build_slot_json`: + +```rust +.map(|slot| build_slot_json(slot, co_config, &path_param)) +``` + +Update any existing `build_ad_slots_script(...)` / `build_slot_json(...)` test +call sites in `publisher.rs` to pass a path argument (e.g. `"/"`). + +- [ ] **Step 4: Update `settings.rs::prepare_runtime`** + +In `crates/trusted-server-core/src/settings.rs` (~2078), compile templates and +surface parse errors: + +```rust +if let Some(co) = &mut self.creative_opportunities { + co.compile_slots(); + co.compile_unit_templates().map_err(|err| { + Report::new(TrustedServerError::Configuration { + message: format!("Invalid creative opportunity gam_unit_path template: {err}"), + }) + })?; + co.validate_runtime().map_err(|err| { + Report::new(TrustedServerError::Configuration { + message: format!("Invalid creative opportunity slot config: {err}"), + }) + })?; +} +``` + +- [ ] **Step 5: Run tests, verify they pass** + +Run: `cargo test_details -p trusted-server-core publisher::tests` +Expected: PASS. + +- [ ] **Step 6: Fix the existing empty-`gam_unit_path` settings test if needed** + +`settings.rs::settings_rejects_creative_opportunity_slot_with_empty_gam_unit_path` +now fails at template-parse (empty template) rather than the render check. Verify +it still asserts rejection; update the expected error substring if it pins a +message. + +Run: `cargo test_details -p trusted-server-core settings::` +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add crates/trusted-server-core/src/publisher.rs crates/trusted-server-core/src/creative_opportunities.rs crates/trusted-server-core/src/settings.rs +git commit -m "Render gam_unit_path template per request across initial and SPA paths" +``` + +--- + +## Task 5: Docs + config + +**Files:** + +- Modify: `docs/guide/configuration.md` +- Modify: `trusted-server.example.toml` +- Modify: the live autoblog `trusted-server.toml` (operator-owned, gitignored — update locally, do not commit) + +- [ ] **Step 1: Add a creative_opportunities section to configuration.md** + +Cover: the placeholder set (`{network_id}`, `{section}`, `{slot_id}`); section +derivation (first path segment, sanitized to `[A-Za-z0-9_-]`, raw/undecoded); +`section_root` requirement and validation; behavior on an unmatched route (no +slot, template never rendered); back-compat (static path used verbatim; no +`gam_unit_path` → `//`). Use fictional values +(`example.com`, network `99999`) per the repo's docs rule. + +- [ ] **Step 2: Update `trusted-server.example.toml`** + +Show one templated slot with `section_root` and a `{section}` `gam_unit_path`, +using fictional values. + +- [ ] **Step 3: Docs format check** + +Run: `cd docs && npm run format` +Expected: no diff / formatting clean. + +- [ ] **Step 4: Commit** + +```bash +git add docs/guide/configuration.md trusted-server.example.toml +git commit -m "Document per-section gam_unit_path templating" +``` + +--- + +## Task 6: Full verification (CI gate) + +- [ ] **Step 1: Format** + +Run: `cargo fmt --all -- --check` +Expected: clean. + +- [ ] **Step 2: Core + Fastly tests under Viceroy (full module, not filtered)** + +Run: `cargo test-fastly` +Expected: PASS. (Runs the full creative_opportunities + publisher test modules on +`wasm32-wasip1`; a format-changing edit can hide later failures when filtered, so +run the whole suite here.) + +- [ ] **Step 3: Other adapters (no behavior change expected, guard against signature breaks)** + +Run: `cargo test-axum && cargo test-cloudflare && cargo test-spin` +Expected: PASS. + +- [ ] **Step 4: Clippy across adapter targets** + +Run: `cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm` +Expected: no warnings. + +- [ ] **Step 5: JS unaffected (sanity)** + +Run: `cd crates/trusted-server-js/lib && npx vitest run` +Expected: PASS (no JS change; confirms wire shape unbroken). + +- [ ] **Step 6: Final commit if any fixups** + +```bash +git add -A +git commit -m "Fix clippy/fmt for per-section gam_unit_path" +``` + +--- + +## Acceptance criteria mapping + +- **N slots × M sections without N×M rules** — Task 1–4 (one templated slot rule + serves all sections). +- **Resolution tested (`/`, single/multi-segment, no-match, encoded)** — Task 2 + tests + Task 4 equivalence + the unmatched-route case (no slot matched → no + `build_slot_json` call; covered by existing `match_slots` empty tests). +- **Existing static configs unchanged** — Task 3 `render_gam_unit_path` verbatim + - default tests. +- **Startup catches empty/unknown/malformed template + missing/invalid + `section_root`** — Task 1 + Task 3 validation tests. +- **`{section}` sanitized, raw path** — Task 2 tests. +- **Documented** — Task 5. diff --git a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md new file mode 100644 index 000000000..39e25d42f --- /dev/null +++ b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md @@ -0,0 +1,208 @@ +# Per-Section `gam_unit_path` Design + +**Date:** 2026-07-23 + +**Status:** Proposed + +**Issue:** [IABTechLab/trusted-server#954](https://github.com/IABTechLab/trusted-server/issues/954) + +## Summary + +`creative_opportunities.slot.gam_unit_path` is a static string, so a publisher +whose GAM ad unit varies by site section cannot express that in one rule. The +only way to model it today is one slot rule per (slot × section), which +multiplies out fast: 3 slots across 10 sections needs 30 near-identical rules. + +This design makes `gam_unit_path` a **template** with a small, fixed placeholder +set — `{network_id}`, `{section}`, `{slot_id}` — where `{section}` is derived +from the request path at render time. One slot rule then covers all sections. + +The derivation policy that matters (`{section}` for the site root) lives in +config via a required `section_root`, not in core — honoring the issue's +constraint that the URL→section convention is publisher-specific. + +Scope is deliberately narrow: **only** `gam_unit_path` templating. Sharing +`page_patterns`/`gam_unit_path` defaults across slots is a related but distinct +duplication problem, tracked as a sibling issue, not built here. + +## Goals + +1. A publisher with N slots across M sections expresses per-section ad units + without N×M rules. +2. `{section}` is derived from the request path with a config-supplied value for + the site root; no URL convention is hardcoded in core. +3. Existing static `gam_unit_path` configs keep working, byte-for-byte + unchanged. +4. Startup rejects unresolvable configuration: unknown placeholders, malformed + templates, and a `{section}` template missing its `section_root`. +5. Resolution is covered by tests including `/`, single- and multi-segment + paths, unsafe/encoded segments, and paths matching no slot. +6. Documented in `docs/guide/configuration.md`, which currently has no + creative_opportunities section. + +## Non-goals + +Documented here so onboarding publishers know the boundary. Each is an additive +extension that does **not** change the config shape below. + +1. **Locale offset** — deriving `{section}` from a segment other than the first + (e.g. `/en/news` → `news`). `{section}` is the first path segment. A + `section_segment` index knob can be added later. +2. **Full-path mirror** — `{section}` spanning multiple segments (`/a/b` → + `a/b`). Real GAM trees bucket by section, not per-article, so this is rare; + use a static per-slot `gam_unit_path` for the exception. +3. **Named per-section overrides** — mapping an irregular section to a renamed + unit (`/reviews` → `editorial/reviews-v2`). Set that one slot's + `gam_unit_path` explicitly, or add named overrides later. +4. **Host- or query-derived sections** — path-only. Out of scope entirely. +5. **Slot-defaults inheritance** — sharing `page_patterns`/`gam_unit_path` at + the `[creative_opportunities]` level. Separate issue; has a startup-lifecycle + concern this design intentionally avoids. + +## Background: how `gam_unit_path` is used + +- `gam_unit_path` is **client-side only**. The resolved string reaches + `googletag.defineSlot(path, sizes, div)` in + `crates/trusted-server-js/lib/src/integrations/gpt/index.ts`. It is **not** in + the OpenRTB bid request — `CreativeOpportunitySlot::to_ad_slot` never emits it. + Therefore this change is **server-only; no JS wire change**. The client keeps + receiving a resolved `gam_unit_path` string. +- Today's resolver is literal-or-default and path-independent + (`crates/trusted-server-core/src/creative_opportunities.rs`): + + ```rust + pub fn resolved_gam_unit_path(&self, gam_network_id: &str) -> String { + self.gam_unit_path + .clone() + .unwrap_or_else(|| format!("/{}/{}", gam_network_id, self.id)) + } + ``` + +- The value is emitted in `build_slot_json` + (`crates/trusted-server-core/src/publisher.rs`), shared by two paths: + - initial render via `build_ad_slots_script` (called where `request_path` is + in scope); + - SPA navigation via `handle_page_bids` (has the normalized `path` param). + + Neither currently passes the path into `build_slot_json`. + +## Design + +### Config shape + +```toml +[creative_opportunities] +gam_network_id = "88059007" +auction_timeout_ms = 2000 +price_granularity = "dense" +section_root = "homepage" # required when a template uses {section} + +[[creative_opportunities.slot]] +id = "ad-header-0" +gam_unit_path = "/{network_id}/autoblog/{section}" +page_patterns = ["/", "/news/*", "/reviews/*", "/deals/*"] +formats = [{ width = 970, height = 90 }, { width = 728, height = 90 }] +[creative_opportunities.slot.providers.prebid] +bidders = {} +``` + +### Placeholders + +| placeholder | resolves to | +| -------------- | ----------------------------------------------------- | +| `{network_id}` | `gam_network_id` | +| `{slot_id}` | slot `id` | +| `{section}` | first path segment; `section_root` when path has none | + +### Resolution model + +```text +startup (prepare_runtime, once): + for each slot: + parse slot.gam_unit_path (if Some) into a template: + reject unknown placeholder, unmatched/nested brace, empty template + cache the parsed template (serde-skipped, like compiled_patterns) + if any slot's template contains {section}: + require section_root present AND matching ^[A-Za-z0-9_-]+$ + +request (per matched slot, path known): + if slot has a parsed template: + section = first non-empty segment of the RAW path, + runs of [^A-Za-z0-9_-] replaced with a single '_'; + section_root when the path has no segment ("/", repeated slashes) + render template + else: + "/{network_id}/{slot_id}" # existing default (back-compat) +``` + +### Section derivation rules (deterministic) + +- Extract the **first non-empty** path segment. +- Replace each run of disallowed characters (`[^A-Za-z0-9_-]`) with a single + `_`. Guarantees a non-empty result for any non-empty segment. Because the path + is **not** decoded, `new%20s` → `new_20s` (only `%` is disallowed; `2` and `0` + are alphanumeric) — never silently `news`, and never the decoded `new_s`. +- Use `section_root` **only** when there is no segment (`/`, repeated slashes). +- Derive from the **raw, undecoded** path — the same string `page_patterns` + glob-match against — so matching and derivation never disagree. Percent-encoded + segments are **not** decoded. +- `section_root` validated at startup: non-empty, entirely `[A-Za-z0-9_-]`. + +### Back-compat + +- No template placeholders in a slot's `gam_unit_path` → used verbatim. +- No `gam_unit_path` set on a slot → `/{network_id}/{slot_id}` (unchanged). +- A config with no `{section}` anywhere never requires `section_root`. + +### Validation moves from render to parse + +`validate_runtime` currently calls `resolved_gam_unit_path` and rejects an empty +result. That check becomes path-dependent under templating, so it is replaced by +**startup template validation**: the template parses, all placeholders are +known, and `section_root` is present when `{section}` is used. The rendered +result is non-empty by construction (literals plus non-empty substitutions, or +the `/{network_id}/{slot_id}` default), so no per-request emptiness check is +needed. + +## Alternatives considered + +- **Named sections** (`[section.NAME]` blocks carrying patterns + unit): more + general (expresses irregular units) but forces enumerating every section, and + centralizes patterns — a bigger change that overlaps the deferred + slot-defaults concern. Rejected as the base; the `unit`-override variant is a + possible future extension. +- **Explicit `unit_by_pattern` map per slot** (issue option 2): fully + data-driven but repeats the section→unit table inside every slot, so adding a + section still edits all N slots. Rejected. +- **Hardcoded first-segment derivation** (issue option 3, literal): smallest, + but bakes one site's URL convention into core, which the issue forbids. The + chosen design keeps the one publisher-specific knob (`section_root`) in config. + +## Risks + +- **Client-influenced path.** `{section}` is derived from a request path the + client controls (especially the SPA `path` param). Mitigated by: sanitizing to + `[A-Za-z0-9_-]`; deriving only for paths that already matched a slot's + `page_patterns`; and the fact that `gam_unit_path` is not in the bid request, + so a crafted section only affects the caller's own `defineSlot`. +- **Two render paths drift.** Initial-render and SPA must produce identical + units for the same path. Covered by an equivalence test. + +## Acceptance criteria + +- [ ] N slots × M sections without N×M rules. +- [ ] Resolution tested: `/`, single-segment, multi-segment, no-match, encoded + segment. +- [ ] Existing static `gam_unit_path` configs unchanged. +- [ ] `validate()` (startup) catches empty/unknown/malformed template and a + `{section}` template with missing/invalid `section_root`. +- [ ] `{section}` sanitized to `[A-Za-z0-9_-]`, derived from the raw path. +- [ ] Documented in `docs/guide/configuration.md`, including unmatched-route + behavior and the no-decode rule; example and live autoblog configs updated. + +## Sibling issue (not built here) + +"creative_opportunities: support shared slot defaults for `page_patterns` and +`gam_unit_path`." Inheritance of `page_patterns` must materialize onto each slot +at startup **before** `compile_slots()` (because `match_slots` never sees the +top-level config), which is the lifecycle subtlety this scoped design avoids. From 070fd944b6cb08a667d885fc1277adb4aaa33ca3 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 15:13:39 +0530 Subject: [PATCH 06/21] Document per-section gam_unit_path templating --- docs/guide/configuration.md | 75 +++++++++++++++++++++++++++++++++++++ trusted-server.example.toml | 22 +++++++++++ 2 files changed, 97 insertions(+) diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index b975590c6..f5b778a15 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -1229,6 +1229,81 @@ TRUSTED_SERVER__AUCTION__TIMEOUT_MS=2000 TRUSTED_SERVER__AUCTION__CREATIVE_STORE=creative_store ``` +## Creative Opportunities Configuration + +### `[creative_opportunities]` + +Defines the ad slots the trusted server offers on a page: which pages each slot +appears on (`page_patterns`), its supported sizes (`formats`), and the GAM ad +unit it maps to (`gam_unit_path`). + +```toml +[creative_opportunities] +gam_network_id = "123456789" +price_granularity = "dense" + +# Shared placeholder value for the site root ("/") — see {section} below. +section_root = "home" + +[[creative_opportunities.slot]] +id = "ad-header" +gam_unit_path = "/{network_id}/example/{section}" +page_patterns = ["/", "/news/*", "/reviews/*"] +formats = [{ width = 728, height = 90 }] +``` + +### `gam_unit_path` templating + +`gam_unit_path` is a template. A publisher whose ad unit varies by site section +expresses that in **one** slot rule instead of one rule per (slot × section). + +Supported placeholders: + +| Placeholder | Resolves to | +| -------------- | -------------------------------------------------------------- | +| `{network_id}` | `gam_network_id` | +| `{slot_id}` | the slot's `id` | +| `{section}` | first path segment of the request (see derivation rules below) | + +A template with **no** placeholders is used verbatim. A slot with **no** +`gam_unit_path` falls back to `//`. Both preserve the +pre-templating behavior, so existing static configs are unchanged. + +### `{section}` derivation + +`{section}` is derived from the request path at request time: + +- It is the **first non-empty path segment**. `/news/article-123` → `news`. +- It is sanitized: each run of characters outside `[A-Za-z0-9_-]` becomes a + single `_`. +- The path is used **raw — it is not percent-decoded**. So `/new%20s` → + `new_20s` (only `%` is disallowed; `2` and `0` are kept), never the decoded + `new_s`. This keeps `{section}` consistent with how `page_patterns` match the + same raw path. +- When the path has no segment (`/`, or repeated slashes), `{section}` is + `section_root`. + +`section_root` is **required** whenever any slot's template uses `{section}`, +and must match `[A-Za-z0-9_-]+`. There is no default: the home-section name is +publisher-specific, so the URL→section convention lives in config, not core. +Startup fails if `{section}` is used without a valid `section_root`. + +Example resolution for `gam_unit_path = "/{network_id}/example/{section}"` with +`gam_network_id = "123456789"` and `section_root = "home"`: + +| Request path | `gam_unit_path` | +| --------------- | ---------------------------- | +| `/` | `/123456789/example/home` | +| `/news` | `/123456789/example/news` | +| `/news/article` | `/123456789/example/news` | +| `/reviews/x` | `/123456789/example/reviews` | + +An **unmatched route** — a path matched by no slot's `page_patterns` — produces +no slot at all, so no template is rendered for it. + +Startup validation rejects a malformed template: an unknown placeholder (e.g. +`{oops}`), an unmatched or nested `{`, a stray `}`, or an empty `gam_unit_path`. + ## Fastly Runtime Config Store After the EdgeZero cutover, the Fastly adapter always dispatches through the diff --git a/trusted-server.example.toml b/trusted-server.example.toml index 26d95d681..e2f8994f6 100644 --- a/trusted-server.example.toml +++ b/trusted-server.example.toml @@ -157,7 +157,29 @@ gam_network_id = "123456789" auction_timeout_ms = 500 # override via TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__AUCTION_TIMEOUT_MS price_granularity = "dense" +# `gam_unit_path` may be a template. Supported placeholders: +# {network_id} -> gam_network_id +# {slot_id} -> the slot's id +# {section} -> first path segment of the request, sanitized to +# [A-Za-z0-9_-]; `section_root` below is used for "/". +# A template with no placeholders (or an absent gam_unit_path) keeps the old +# behavior: verbatim path, or the default `//`. +# +# `section_root` is REQUIRED when any slot's template uses {section}. There is no +# default — the home-section name is publisher-specific. Must be [A-Za-z0-9_-]+. +section_root = "home" + # No slot templates are enabled in the checked-in default config. Add # `[[creative_opportunities.slot]]` entries via private config or override the # entire array via: # TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT='[{"id":"...","gam_unit_path":"...",...}]' +# +# Example templated slot (one rule serves every section): +# [[creative_opportunities.slot]] +# id = "ad-header" +# gam_unit_path = "/{network_id}/example/{section}" +# page_patterns = ["/", "/news/*", "/reviews/*"] +# formats = [{ width = 728, height = 90 }] +# "/" -> /123456789/example/home +# "/news/x" -> /123456789/example/news +# "/reviews/y" -> /123456789/example/reviews From 20f5f8e3a6b4ac4d4e7711f1f5c0d7ee08bea3a2 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 23 Jul 2026 15:23:17 +0530 Subject: [PATCH 07/21] Use fictional example values in tests and docs --- .../src/creative_opportunities.rs | 50 ++++++++++++------- crates/trusted-server-core/src/publisher.rs | 17 ++++--- .../2026-07-23-per-section-gam-unit-path.md | 32 ++++++------ ...-07-23-per-section-gam-unit-path-design.md | 6 +-- 4 files changed, 61 insertions(+), 44 deletions(-) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 59ce6d46b..cc1ac67a3 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -325,7 +325,10 @@ impl CreativeOpportunitySlot { if let Some(raw) = &self.gam_unit_path && raw.trim().is_empty() { - return Err(format!("slot `{}` gam_unit_path must not be empty", self.id)); + return Err(format!( + "slot `{}` gam_unit_path must not be empty", + self.id + )); } Ok(()) @@ -750,17 +753,17 @@ mod tests { #[test] fn parse_unit_template_accepts_known_placeholders() { - let parts = parse_unit_template("/{network_id}/autoblog/{section}") + let parts = parse_unit_template("/{network_id}/example/{section}") .expect("should parse valid template"); assert_eq!(parts.len(), 4, "should split into literal+ph+literal+ph"); } #[test] fn parse_unit_template_accepts_static_path() { - let parts = parse_unit_template("/88059007/autoblog/homepage") + let parts = parse_unit_template("/99999/example/homepage") .expect("should parse a static path as a single literal"); assert!( - matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/88059007/autoblog/homepage"), + matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/99999/example/homepage"), "should be one literal part" ); } @@ -769,7 +772,10 @@ mod tests { fn parse_unit_template_rejects_unknown_placeholder() { let err = parse_unit_template("/{network_id}/{oops}") .expect_err("should reject unknown placeholder"); - assert!(err.contains("oops"), "error should name the bad placeholder"); + assert!( + err.contains("oops"), + "error should name the bad placeholder" + ); } #[test] @@ -791,8 +797,8 @@ mod tests { #[test] fn derive_section_uses_first_segment() { assert_eq!(derive_section("/news", "home"), "news"); - assert_eq!(derive_section("/news/gm-cadillac", "home"), "news"); - assert_eq!(derive_section("/car-research/x", "home"), "car-research"); + assert_eq!(derive_section("/news/article-123", "home"), "news"); + assert_eq!(derive_section("/my-section/x", "home"), "my-section"); } #[test] @@ -817,11 +823,13 @@ mod tests { assert_eq!(derive_section("/%%%/x", "home"), "_"); } - fn make_config_with_section_template(section_root: Option<&str>) -> CreativeOpportunitiesConfig { + fn make_config_with_section_template( + section_root: Option<&str>, + ) -> CreativeOpportunitiesConfig { let mut slot = make_slot("ad-header-0", vec!["/news/*"]); - slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); CreativeOpportunitiesConfig { - gam_network_id: "88059007".to_string(), + gam_network_id: "99999".to_string(), auction_timeout_ms: None, price_granularity: PriceGranularity::default(), section_root: section_root.map(str::to_string), @@ -832,11 +840,12 @@ mod tests { #[test] fn render_gam_unit_path_substitutes_placeholders() { let mut slot = make_slot("ad-header-0", vec!["/news/*"]); - slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); - slot.compile_unit_template().expect("should compile template"); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); + slot.compile_unit_template() + .expect("should compile template"); assert_eq!( - slot.render_gam_unit_path("88059007", "news"), - "/88059007/autoblog/news" + slot.render_gam_unit_path("99999", "news"), + "/99999/example/news" ); } @@ -844,8 +853,12 @@ mod tests { fn render_gam_unit_path_defaults_when_no_template() { let mut slot = make_slot("sidebar", vec!["/*"]); slot.gam_unit_path = None; - slot.compile_unit_template().expect("should compile (no template)"); - assert_eq!(slot.render_gam_unit_path("99999", "ignored"), "/99999/sidebar"); + slot.compile_unit_template() + .expect("should compile (no template)"); + assert_eq!( + slot.render_gam_unit_path("99999", "ignored"), + "/99999/sidebar" + ); } #[test] @@ -870,7 +883,10 @@ mod tests { let err = config .validate_runtime() .expect_err("should require section_root"); - assert!(err.contains("section_root"), "error should mention section_root"); + assert!( + err.contains("section_root"), + "error should mention section_root" + ); } #[test] diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index 90bdc6c1d..e8de8582c 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -4304,7 +4304,7 @@ mod tests { .collect(), providers: Default::default(), compiled_patterns: Vec::new(), - compiled_unit: None, + compiled_unit: None, } } @@ -4373,18 +4373,19 @@ mod tests { let mut config = make_config(); config.section_root = Some("homepage".to_string()); let mut slot = make_slot(); - slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); - slot.compile_unit_template().expect("template should compile"); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); + slot.compile_unit_template() + .expect("template should compile"); - let news = crate::publisher::build_slot_json(&slot, &config, "/news/gm-cadillac"); + let news = crate::publisher::build_slot_json(&slot, &config, "/news/article-123"); assert_eq!( - news["gam_unit_path"], "/21765378893/autoblog/news", + news["gam_unit_path"], "/21765378893/example/news", "section should derive from the first path segment" ); let home = crate::publisher::build_slot_json(&slot, &config, "/"); assert_eq!( - home["gam_unit_path"], "/21765378893/autoblog/homepage", + home["gam_unit_path"], "/21765378893/example/homepage", "root path should use section_root" ); } @@ -4973,7 +4974,7 @@ mod tests { targeting: Default::default(), providers: Default::default(), compiled_patterns: Vec::new(), - compiled_unit: None, + compiled_unit: None, }] } @@ -5476,7 +5477,7 @@ mod tests { targeting: Default::default(), providers: Default::default(), compiled_patterns: Vec::new(), - compiled_unit: None, + compiled_unit: None, }] } diff --git a/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md b/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md index 375795a6b..cbd4a2ed1 100644 --- a/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md +++ b/docs/superpowers/plans/2026-07-23-per-section-gam-unit-path.md @@ -40,7 +40,7 @@ client keeps receiving a resolved `gam_unit_path` string, so no JS change. - Modify: `crates/trusted-server-core/src/settings.rs` - `prepare_runtime` calls `compile_unit_templates` and surfaces parse errors - Modify: `docs/guide/configuration.md` (add creative_opportunities section) -- Modify: `trusted-server.example.toml` and the live autoblog config +- Modify: `trusted-server.example.toml` and the live example config Notes on lifecycle: `page_patterns` inheritance is **out of scope** (sibling issue). Templates are parsed at startup and cached with `#[serde(skip)]`, @@ -62,17 +62,17 @@ Add to `mod tests`: ```rust #[test] fn parse_unit_template_accepts_known_placeholders() { - let parts = parse_unit_template("/{network_id}/autoblog/{section}") + let parts = parse_unit_template("/{network_id}/example/{section}") .expect("should parse valid template"); assert_eq!(parts.len(), 4, "should split into literal+ph+literal+ph"); } #[test] fn parse_unit_template_accepts_static_path() { - let parts = parse_unit_template("/88059007/autoblog/homepage") + let parts = parse_unit_template("/99999/example/homepage") .expect("should parse a static path as a single literal"); assert!( - matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/88059007/autoblog/homepage"), + matches!(parts.as_slice(), [UnitTemplatePart::Literal(s)] if s == "/99999/example/homepage"), "should be one literal part" ); } @@ -202,8 +202,8 @@ git commit -m "Add gam_unit_path template parser" #[test] fn derive_section_uses_first_segment() { assert_eq!(derive_section("/news", "home"), "news"); - assert_eq!(derive_section("/news/gm-cadillac", "home"), "news"); - assert_eq!(derive_section("/car-research/x", "home"), "car-research"); + assert_eq!(derive_section("/news/article-123", "home"), "news"); + assert_eq!(derive_section("/my-section/x", "home"), "my-section"); } #[test] @@ -298,11 +298,11 @@ git commit -m "Add request-path section derivation" #[test] fn render_gam_unit_path_substitutes_placeholders() { let mut slot = make_slot("ad-header-0", vec!["/news/*"]); - slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); slot.compile_unit_template().expect("should compile template"); assert_eq!( - slot.render_gam_unit_path("88059007", "news"), - "/88059007/autoblog/news" + slot.render_gam_unit_path("99999", "news"), + "/99999/example/news" ); } @@ -362,9 +362,9 @@ style in this module): ```rust fn make_config_with_section_template(section_root: Option<&str>) -> CreativeOpportunitiesConfig { let mut slot = make_slot("ad-header-0", vec!["/news/*"]); - slot.gam_unit_path = Some("/{network_id}/autoblog/{section}".to_string()); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); CreativeOpportunitiesConfig { - gam_network_id: "88059007".to_string(), + gam_network_id: "99999".to_string(), auction_timeout_ms: None, price_granularity: PriceGranularity::default(), section_root: section_root.map(str::to_string), @@ -545,14 +545,14 @@ in that module): ```rust #[test] fn build_slot_json_renders_section_from_request_path() { - let config = creative_opportunities_config_with_template(); // gam_unit_path = "/{network_id}/autoblog/{section}", section_root = "homepage" + let config = creative_opportunities_config_with_template(); // gam_unit_path = "/{network_id}/example/{section}", section_root = "homepage" let slot = &config.slot[0]; - let news = build_slot_json(slot, &config, "/news/gm-cadillac"); - assert_eq!(news["gam_unit_path"], "/88059007/autoblog/news"); + let news = build_slot_json(slot, &config, "/news/article-123"); + assert_eq!(news["gam_unit_path"], "/99999/example/news"); let home = build_slot_json(slot, &config, "/"); - assert_eq!(home["gam_unit_path"], "/88059007/autoblog/homepage"); + assert_eq!(home["gam_unit_path"], "/99999/example/homepage"); } ``` @@ -663,7 +663,7 @@ git commit -m "Render gam_unit_path template per request across initial and SPA - Modify: `docs/guide/configuration.md` - Modify: `trusted-server.example.toml` -- Modify: the live autoblog `trusted-server.toml` (operator-owned, gitignored — update locally, do not commit) +- Modify: the live example `trusted-server.toml` (operator-owned, gitignored — update locally, do not commit) - [ ] **Step 1: Add a creative_opportunities section to configuration.md** diff --git a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md index 39e25d42f..b18c1bcb0 100644 --- a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md +++ b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md @@ -92,14 +92,14 @@ extension that does **not** change the config shape below. ```toml [creative_opportunities] -gam_network_id = "88059007" +gam_network_id = "99999" auction_timeout_ms = 2000 price_granularity = "dense" section_root = "homepage" # required when a template uses {section} [[creative_opportunities.slot]] id = "ad-header-0" -gam_unit_path = "/{network_id}/autoblog/{section}" +gam_unit_path = "/{network_id}/example/{section}" page_patterns = ["/", "/news/*", "/reviews/*", "/deals/*"] formats = [{ width = 970, height = 90 }, { width = 728, height = 90 }] [creative_opportunities.slot.providers.prebid] @@ -198,7 +198,7 @@ needed. `{section}` template with missing/invalid `section_root`. - [ ] `{section}` sanitized to `[A-Za-z0-9_-]`, derived from the raw path. - [ ] Documented in `docs/guide/configuration.md`, including unmatched-route - behavior and the no-decode rule; example and live autoblog configs updated. + behavior and the no-decode rule; example and live example configs updated. ## Sibling issue (not built here) From f0a275b06fc3659055373e4744b9d723ec41f3e2 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Fri, 24 Jul 2026 15:02:06 +0530 Subject: [PATCH 08/21] Address review feedback on gam_unit_path templating - Remove now-dead resolved_gam_unit_path (superseded by render_gam_unit_path) and its two tests; the default // path now lives solely in render_gam_unit_path - Tighten compile_unit_template and render_gam_unit_path to pub(crate) - Document that validate_runtime must run after compile_unit_templates for the {section} -> section_root check to fire - Use fictional gam_network_id 99999 in the new build_slot_json test --- .../src/creative_opportunities.rs | 40 ++++--------------- crates/trusted-server-core/src/publisher.rs | 5 ++- 2 files changed, 11 insertions(+), 34 deletions(-) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index cc1ac67a3..723c22960 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -175,6 +175,12 @@ impl CreativeOpportunitiesConfig { /// Validate all slot definitions after runtime preparation. /// + /// Call [`compile_unit_templates`](Self::compile_unit_templates) first: the + /// `{section}` → [`section_root`](Self::section_root) requirement is keyed off + /// each slot's compiled template, so an uncompiled config silently skips that + /// check. [`Settings::prepare_runtime`](crate::settings::Settings) enforces + /// this order. + /// /// # Errors /// /// Returns an error string when a slot has an invalid identifier, page @@ -404,17 +410,6 @@ impl CreativeOpportunitySlot { .collect(); } - /// Returns the GAM ad unit path for this slot. - /// - /// Uses the explicit [`gam_unit_path`](Self::gam_unit_path) override when set, - /// otherwise constructs `//`. - #[must_use] - pub fn resolved_gam_unit_path(&self, gam_network_id: &str) -> String { - self.gam_unit_path - .clone() - .unwrap_or_else(|| format!("/{}/{}", gam_network_id, self.id)) - } - /// Parses [`gam_unit_path`](Self::gam_unit_path) into /// [`compiled_unit`](Self::compiled_unit). Call once at startup via /// [`CreativeOpportunitiesConfig::compile_unit_templates`]. @@ -423,7 +418,7 @@ impl CreativeOpportunitySlot { /// /// Returns an error string (prefixed with the slot id) when the template is /// malformed. See [`parse_unit_template`]. - pub fn compile_unit_template(&mut self) -> Result<(), String> { + pub(crate) fn compile_unit_template(&mut self) -> Result<(), String> { self.compiled_unit = match &self.gam_unit_path { Some(raw) => { Some(parse_unit_template(raw).map_err(|e| format!("slot `{}`: {e}", self.id))?) @@ -438,7 +433,7 @@ impl CreativeOpportunitySlot { /// Substitutes `{network_id}`, `{section}`, and `{slot_id}` in the parsed /// template. Falls back to `//` when the slot has no template. #[must_use] - pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { + pub(crate) fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { match &self.compiled_unit { Some(parts) => parts .iter() @@ -726,25 +721,6 @@ mod tests { assert!(validate_slot_id("has space").is_err(), "spaces should fail"); } - #[test] - fn resolved_gam_unit_path_uses_default_when_absent() { - let slot = make_slot("atf", vec!["/"]); - assert_eq!( - slot.resolved_gam_unit_path("21765378893"), - "/21765378893/atf" - ); - } - - #[test] - fn resolved_gam_unit_path_uses_override_when_set() { - let mut slot = make_slot("atf", vec!["/"]); - slot.gam_unit_path = Some("/21765378893/publisher/atf-sidebar".to_string()); - assert_eq!( - slot.resolved_gam_unit_path("21765378893"), - "/21765378893/publisher/atf-sidebar" - ); - } - #[test] fn resolved_div_id_defaults_to_slot_id() { let slot = make_slot("atf", vec!["/"]); diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index e8de8582c..2f3f55504 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -4371,6 +4371,7 @@ mod tests { #[test] fn build_slot_json_renders_section_from_request_path() { let mut config = make_config(); + config.gam_network_id = "99999".to_string(); config.section_root = Some("homepage".to_string()); let mut slot = make_slot(); slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); @@ -4379,13 +4380,13 @@ mod tests { let news = crate::publisher::build_slot_json(&slot, &config, "/news/article-123"); assert_eq!( - news["gam_unit_path"], "/21765378893/example/news", + news["gam_unit_path"], "/99999/example/news", "section should derive from the first path segment" ); let home = crate::publisher::build_slot_json(&slot, &config, "/"); assert_eq!( - home["gam_unit_path"], "/21765378893/example/homepage", + home["gam_unit_path"], "/99999/example/homepage", "root path should use section_root" ); } From 4bedcd73b2cd74b171021964795af31392e6d17b Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Wed, 29 Jul 2026 12:35:39 +0530 Subject: [PATCH 09/21] Address review feedback on per-section gam_unit_path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the review findings on the gam_unit_path templating PR. - Make section derivation fully publisher-configurable with `section_segment` (0-based, default 0), so a locale-prefixed site (/en/news/article) selects `news` rather than `en`. Paths with no segment at that index fall back to `section_root`. Adds `CreativeOpportunitiesConfig::section_for_path` so a call site cannot apply one policy knob and forget the other. - Skip serializing `section_root` and `section_segment` when unset. `ts config push` re-serializes the typed config into the pushed blob, and these structs use `deny_unknown_fields`, so emitting a null key would make a binary rollback fail at config load. Comment both keys out of the example config for the same reason. - Stop conflating "no template" with "not compiled". `compiled_unit: None` also covers a slot deserialized or built without `compile_unit_templates`, and treating it as absent silently routed such slots to // — bidding against the wrong inventory. `render_gam_unit_path` now falls back to the raw template, and `template_uses_section` reads it too so validation cannot skip the `section_root` requirement. - Reject a blank `gam_network_id` at startup when slots are configured, closing the case where `gam_unit_path = "{network_id}"` passes validation and renders an empty path into googletag.defineSlot. An empty slot list disables the feature, so the id stays unchecked there. - Fix the documented page_patterns: `/news/*` does not match `/news`, so the example lost every section landing page it claimed to serve. `resolved_gam_unit_path` is deliberately not restored as a deprecated wrapper: it is the path-independent resolver this issue filed as the bug, so a shim would silently return the untemplated path. The crate is `publish = false`; `render_gam_unit_path` and `derive_section` are now `pub` as the supported replacement, making a build break the failure mode instead of wrong inventory. --- CHANGELOG.md | 1 + .../src/creative_opportunities.rs | 383 ++++++++++++++++-- crates/trusted-server-core/src/publisher.rs | 35 +- docs/guide/configuration.md | 44 +- ...-07-23-per-section-gam-unit-path-design.md | 35 +- trusted-server.example.toml | 22 +- 6 files changed, 461 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f19e0ee2b..8053bf6b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `creative_opportunities.slot.gam_unit_path` is now a template supporting `{network_id}`, `{slot_id}`, and `{section}`, so a publisher whose ad unit varies by site section expresses it in one slot rule instead of one per (slot × section). `{section}` derives from the request path: `[creative_opportunities].section_segment` selects which path segment names the section (0-based, default `0`; set `1` for locale-prefixed URLs), and `section_root` supplies the value for paths with no such segment. `section_root` is required when a template uses `{section}`. Existing static and absent `gam_unit_path` configs are unchanged. Startup now also rejects a blank `gam_network_id` when slots are configured. Note that a config setting `section_root` or `section_segment` requires a binary that knows those keys — rolling the binary back below this release while the keys are present fails config load; configs that omit them roll back cleanly. - Added Osano consent mirror integration docs and public enablement guidance. - Implemented basic authentication for configurable endpoint paths (#73) - Added integrations guide with example `testlight` integration diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 723c22960..7df15932a 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -100,14 +100,25 @@ fn sanitize_section(segment: &str) -> String { /// Derives the `{section}` value from a request path. /// -/// Uses the first non-empty path segment, sanitized to `[A-Za-z0-9_-]`. Falls -/// back to `section_root` when the path has no segment (`/`, repeated slashes). +/// Takes the non-empty path segment at `section_segment` (0-based, counting +/// only non-empty segments), sanitized to `[A-Za-z0-9_-]`. Falls back to +/// `section_root` when the path has no such segment — the site root (`/`, +/// repeated slashes), or a path shorter than the configured index. +/// +/// `section_segment` exists because the URL→section convention is +/// publisher-specific: a site that prefixes a locale (`/en/news/article`) sets +/// `section_segment = 1` to get `news` rather than `en`. /// /// The path is used **raw** (not percent-decoded) so this stays consistent with /// how [`page_patterns`](CreativeOpportunitySlot::page_patterns) glob-match the /// same path — e.g. `/new%20s` yields `new_20s`, never the decoded `new_s`. -pub(crate) fn derive_section(path: &str, section_root: &str) -> String { - match path.split('/').find(|segment| !segment.is_empty()) { +#[must_use] +pub fn derive_section(path: &str, section_root: &str, section_segment: usize) -> String { + match path + .split('/') + .filter(|segment| !segment.is_empty()) + .nth(section_segment) + { Some(segment) => sanitize_section(segment), None => section_root.to_string(), } @@ -144,15 +155,54 @@ pub struct CreativeOpportunitiesConfig { /// /// Required when any slot's [`gam_unit_path`](CreativeOpportunitySlot::gam_unit_path) /// template contains `{section}`. No default — a home-section name is - /// publisher-specific, so the URL→section convention stays in config, not core. - #[serde(default)] + /// publisher-specific, so it stays in config, not core. + /// + /// Skipped when absent so a config blob pushed by a newer binary stays + /// readable by an older one: these structs use `deny_unknown_fields`, so + /// emitting `"section_root": null` would make a rollback fail at startup. + #[serde(default, skip_serializing_if = "Option::is_none")] pub section_root: Option, + /// Index of the path segment `{section}` is taken from, 0-based over + /// non-empty segments. Defaults to `0` (the first segment). + /// + /// The URL→section convention is publisher-specific: a site that prefixes a + /// locale (`/en/news/article`) sets `section_segment = 1` to select `news` + /// instead of `en`. Paths with no segment at this index fall back to + /// [`section_root`](Self::section_root), so on `/en` a config with + /// `section_segment = 1` renders the root section. + /// + /// `Option` rather than a plain `usize` with a serde default for the same + /// rollback reason as [`section_root`](Self::section_root): an unset key + /// must not appear in the serialized config blob. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub section_segment: Option, /// Slot templates. Empty vec = feature disabled (no auction fired, no globals injected). #[serde(default, deserialize_with = "vec_from_seq_or_map")] pub slot: Vec, } impl CreativeOpportunitiesConfig { + /// Derives the `{section}` value for `path` under this config's section + /// policy ([`section_root`](Self::section_root) and + /// [`section_segment`](Self::section_segment)). + /// + /// Prefer this over calling [`derive_section`] directly: it keeps both + /// policy knobs together so a call site cannot apply one and forget the + /// other. + /// + /// An unset [`section_root`](Self::section_root) yields an empty section for + /// a path with no matching segment. [`validate_runtime`](Self::validate_runtime) + /// rejects that combination for any template that uses `{section}`, so it + /// cannot reach a rendered unit path. + #[must_use] + pub fn section_for_path(&self, path: &str) -> String { + derive_section( + path, + self.section_root.as_deref().unwrap_or_default(), + self.section_segment.unwrap_or(0), + ) + } + /// Pre-compile glob patterns for all slots. Call once after deserialization. pub fn compile_slots(&mut self) { for slot in &mut self.slot { @@ -183,10 +233,24 @@ impl CreativeOpportunitiesConfig { /// /// # Errors /// - /// Returns an error string when a slot has an invalid identifier, page - /// pattern set, format list, or dimensions, or when a slot's `gam_unit_path` - /// template uses `{section}` without a valid [`section_root`](Self::section_root). + /// Returns an error string when [`gam_network_id`](Self::gam_network_id) is + /// blank while slots are configured, when a slot has an invalid identifier, + /// page pattern set, format list, or dimensions, or when a slot's + /// `gam_unit_path` template uses `{section}` without a valid + /// [`section_root`](Self::section_root). pub fn validate_runtime(&self) -> Result<(), String> { + // Every rendered unit path either substitutes `{network_id}` or uses the + // `//` default, so a blank network id produces a + // path GAM cannot resolve (`""` or `//slot`). Reject at startup rather + // than shipping it to `googletag.defineSlot`. + // + // Gated on a non-empty slot list: an empty list disables the feature, so + // no path is ever rendered and the id is inert. Failing startup there + // would take a site down over a value it does not use. + if !self.slot.is_empty() && self.gam_network_id.trim().is_empty() { + return Err("gam_network_id must not be empty".to_string()); + } + for slot in &self.slot { slot.validate_runtime()?; } @@ -255,9 +319,15 @@ pub struct CreativeOpportunitySlot { /// Pre-parsed [`gam_unit_path`](Self::gam_unit_path) template, populated by /// [`compile_unit_template`](Self::compile_unit_template) at startup. /// - /// `None` when the slot has no explicit `gam_unit_path` (renders the default - /// `//`). `pub(crate)` so cross-module test helpers can build - /// slots via struct-literal syntax with an empty cache. + /// `None` means *not compiled* — either the slot has no explicit + /// `gam_unit_path`, or it was deserialized/built without running + /// [`CreativeOpportunitiesConfig::compile_unit_templates`]. Callers must + /// therefore fall back to [`gam_unit_path`](Self::gam_unit_path) rather than + /// treating `None` as "no template"; see + /// [`render_gam_unit_path`](Self::render_gam_unit_path). + /// + /// `pub(crate)` so cross-module test helpers can build slots via + /// struct-literal syntax with an empty cache. #[serde(skip, default)] pub(crate) compiled_unit: Option>, } @@ -431,11 +501,24 @@ impl CreativeOpportunitySlot { /// Renders the resolved GAM unit path for a given network id and section. /// /// Substitutes `{network_id}`, `{section}`, and `{slot_id}` in the parsed - /// template. Falls back to `//` when the slot has no template. + /// template. Falls back to `//` only when the slot has no + /// [`gam_unit_path`](Self::gam_unit_path) at all. + /// + /// This is the path-aware replacement for the pre-templating + /// `resolved_gam_unit_path(&self, gam_network_id)`. + /// + /// # Performance + /// + /// The hot path reads the [`compiled_unit`](Self::compiled_unit) cache. A + /// slot with an explicit `gam_unit_path` but no cache (built by hand, or + /// deserialized without [`CreativeOpportunitiesConfig::compile_unit_templates`]) + /// re-parses its template on every call — same fallback shape as + /// [`matches_path`](Self::matches_path). It must never silently degrade to + /// the default path, which would bid against the wrong inventory. #[must_use] - pub(crate) fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { - match &self.compiled_unit { - Some(parts) => parts + pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { + let render = |parts: &[UnitTemplatePart]| -> String { + parts .iter() .map(|part| match part { UnitTemplatePart::Literal(s) => s.as_str(), @@ -443,17 +526,37 @@ impl CreativeOpportunitySlot { UnitTemplatePart::Section => section, UnitTemplatePart::SlotId => self.id.as_str(), }) - .collect(), - None => format!("/{}/{}", gam_network_id, self.id), + .collect() + }; + + match (&self.compiled_unit, &self.gam_unit_path) { + (Some(parts), _) => render(parts), + // A malformed template cannot reach a compiled config (startup + // rejects it), so on this path use the raw string verbatim — the + // pre-templating behaviour — instead of dropping to the default. + (None, Some(raw)) => parse_unit_template(raw) + .map(|parts| render(&parts)) + .unwrap_or_else(|_| raw.clone()), + (None, None) => format!("/{}/{}", gam_network_id, self.id), } } - /// Returns `true` if this slot's compiled template contains `{section}`. + /// Returns `true` if this slot's `gam_unit_path` template contains `{section}`. + /// + /// Reads the raw template when [`compiled_unit`](Self::compiled_unit) is + /// empty so validation cannot silently skip the + /// [`section_root`](CreativeOpportunitiesConfig::section_root) requirement + /// for an uncompiled config. #[must_use] pub(crate) fn template_uses_section(&self) -> bool { - self.compiled_unit - .as_ref() - .is_some_and(|parts| parts.iter().any(|p| matches!(p, UnitTemplatePart::Section))) + let uses_section = |parts: &[UnitTemplatePart]| { + parts.iter().any(|p| matches!(p, UnitTemplatePart::Section)) + }; + match (&self.compiled_unit, &self.gam_unit_path) { + (Some(parts), _) => uses_section(parts), + (None, Some(raw)) => parse_unit_template(raw).is_ok_and(|parts| uses_section(&parts)), + (None, None) => false, + } } /// Returns the div element ID for this slot. @@ -772,15 +875,32 @@ mod tests { #[test] fn derive_section_uses_first_segment() { - assert_eq!(derive_section("/news", "home"), "news"); - assert_eq!(derive_section("/news/article-123", "home"), "news"); - assert_eq!(derive_section("/my-section/x", "home"), "my-section"); + assert_eq!(derive_section("/news", "home", 0), "news"); + assert_eq!(derive_section("/news/article-123", "home", 0), "news"); + assert_eq!(derive_section("/my-section/x", "home", 0), "my-section"); + } + + #[test] + fn derive_section_uses_configured_segment_index() { + // A locale-prefixed site sets section_segment = 1. + assert_eq!(derive_section("/en/news/article", "home", 1), "news"); + assert_eq!(derive_section("/en/news", "home", 1), "news"); + // Repeated separators are not counted as segments. + assert_eq!(derive_section("//en//news//x", "home", 1), "news"); + } + + #[test] + fn derive_section_uses_root_when_segment_index_out_of_range() { + // Section landing page of a locale-prefixed site: no segment 1 exists, + // so the root value stands in rather than reusing the locale. + assert_eq!(derive_section("/en", "home", 1), "home"); + assert_eq!(derive_section("/", "home", 1), "home"); } #[test] fn derive_section_uses_root_when_no_segment() { - assert_eq!(derive_section("/", "homepage"), "homepage"); - assert_eq!(derive_section("///", "homepage"), "homepage"); + assert_eq!(derive_section("/", "homepage", 0), "homepage"); + assert_eq!(derive_section("///", "homepage", 0), "homepage"); } #[test] @@ -789,14 +909,48 @@ mod tests { // alphanumeric), so it collapses to a single '_' -> "new_20s". This is // exactly the no-decode contract: had we decoded, %20 would be a space // and yield "new_s"; we do NOT decode. - assert_eq!(derive_section("/new%20s", "home"), "new_20s"); + assert_eq!(derive_section("/new%20s", "home", 0), "new_20s"); // A run of disallowed chars collapses to one '_'. - assert_eq!(derive_section("/a..b", "home"), "a_b"); + assert_eq!(derive_section("/a..b", "home", 0), "a_b"); + } + + #[test] + fn section_for_path_applies_both_policy_knobs() { + let mut config = make_config_with_section_template(Some("home")); + assert_eq!( + config.section_for_path("/en/news/article"), + "en", + "should default to the first segment when section_segment is unset" + ); + + config.section_segment = Some(1); + assert_eq!( + config.section_for_path("/en/news/article"), + "news", + "should honour the configured segment index" + ); + assert_eq!( + config.section_for_path("/en"), + "home", + "should fall back to section_root when the index is out of range" + ); + } + + #[test] + fn section_segment_is_omitted_from_serialized_config_when_unset() { + // Same rollback contract as section_root: `deny_unknown_fields` on the + // previous binary rejects a blob carrying keys it does not know. + let config = make_config_with_section_template(None); + let value = serde_json::to_value(&config).expect("should serialize config"); + assert!( + value.get("section_segment").is_none(), + "unset section_segment should not be serialized, got {value}" + ); } #[test] fn derive_section_is_non_empty_for_all_disallowed_segment() { - assert_eq!(derive_section("/%%%/x", "home"), "_"); + assert_eq!(derive_section("/%%%/x", "home", 0), "_"); } fn make_config_with_section_template( @@ -809,6 +963,7 @@ mod tests { auction_timeout_ms: None, price_granularity: PriceGranularity::default(), section_root: section_root.map(str::to_string), + section_segment: None, slot: vec![slot], } } @@ -889,6 +1044,174 @@ mod tests { .expect("should accept valid section_root"); } + #[test] + fn render_gam_unit_path_honours_raw_template_without_compiled_cache() { + // A slot deserialized straight from JSON (or built by a test helper) + // never ran `compile_unit_templates`. It must still render its explicit + // path — dropping to `//` would bid the wrong inventory. + let slot: CreativeOpportunitySlot = serde_json::from_value(serde_json::json!({ + "id": "ad-header-0", + "gam_unit_path": "/{network_id}/example/{section}", + "page_patterns": ["/news/*"], + "formats": [{ "width": 728, "height": 90 }], + })) + .expect("should deserialize slot"); + assert!( + slot.compiled_unit.is_none(), + "direct deserialization should leave the template cache empty" + ); + assert_eq!( + slot.render_gam_unit_path("99999", "news"), + "/99999/example/news", + "uncompiled slot should still substitute placeholders" + ); + } + + #[test] + fn render_gam_unit_path_honours_static_path_without_compiled_cache() { + let mut slot = make_slot("atf", vec!["/"]); + slot.gam_unit_path = Some("/99999/example/homepage".to_string()); + assert_eq!( + slot.render_gam_unit_path("99999", "news"), + "/99999/example/homepage", + "uncompiled static path should render verbatim, not the default" + ); + } + + #[test] + fn validate_runtime_requires_section_root_for_uncompiled_template() { + // `template_uses_section` must read the raw template, otherwise an + // uncompiled config silently skips the section_root requirement. + let mut config = make_config_with_section_template(None); + config.compile_slots(); + assert!( + config.slot[0].compiled_unit.is_none(), + "test precondition: template cache is empty" + ); + let err = config + .validate_runtime() + .expect_err("should require section_root even without compiled templates"); + assert!( + err.contains("section_root"), + "error should mention section_root" + ); + } + + #[test] + fn validate_runtime_rejects_blank_network_id() { + // `gam_unit_path = "{network_id}"` renders to an empty string with a + // blank network id, which reaches googletag.defineSlot as an invalid path. + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("{network_id}".to_string()); + config.gam_network_id = String::new(); + config.compile_slots(); + config + .compile_unit_templates() + .expect("templates should compile"); + let err = config + .validate_runtime() + .expect_err("blank gam_network_id should fail startup validation"); + assert!( + err.contains("gam_network_id"), + "error should name gam_network_id, got: {err}" + ); + } + + #[test] + fn validate_runtime_allows_blank_network_id_when_no_slots_configured() { + // An empty slot list disables the feature, so the id is never rendered. + // Failing startup there would break a deploy over an unused value. + let mut config = make_config_with_section_template(Some("home")); + config.gam_network_id = String::new(); + config.slot.clear(); + config + .validate_runtime() + .expect("a disabled creative_opportunities stack should not fail on a blank id"); + } + + #[test] + fn section_root_is_omitted_from_serialized_config_when_unset() { + // Older binaries deserialize this struct with `deny_unknown_fields`, so + // a pushed config blob must not carry `"section_root": null`. + let config = CreativeOpportunitiesConfig { + gam_network_id: "99999".to_string(), + auction_timeout_ms: None, + price_granularity: PriceGranularity::default(), + section_root: None, + section_segment: None, + slot: Vec::new(), + }; + let value = serde_json::to_value(&config).expect("should serialize config"); + assert!( + value.get("section_root").is_none(), + "unset section_root should not be serialized, got {value}" + ); + + let with_root = CreativeOpportunitiesConfig { + section_root: Some("home".to_string()), + ..config + }; + assert_eq!( + serde_json::to_value(&with_root) + .expect("should serialize config") + .get("section_root") + .and_then(serde_json::Value::as_str), + Some("home"), + "a set section_root should still round-trip" + ); + } + + #[test] + fn documented_page_patterns_match_and_render_their_documented_paths() { + // Mirrors the example in docs/guide/configuration.md. `/news/*` alone + // does NOT match `/news` (the glob needs the trailing separator), so the + // documented config must list the section landing pages explicitly. + let mut slot = make_slot( + "ad-header", + vec!["/", "/news", "/news/*", "/reviews", "/reviews/*"], + ); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); + slot.compile_patterns(); + slot.compile_unit_template() + .expect("should compile template"); + let slots = vec![slot]; + + for (path, expected) in [ + ("/", "/123456789/example/home"), + ("/news", "/123456789/example/news"), + ("/news/article", "/123456789/example/news"), + ("/reviews/x", "/123456789/example/reviews"), + ] { + let matched = match_slots(&slots, path); + assert_eq!( + matched.len(), + 1, + "`{path}` should match the documented slot" + ); + assert_eq!( + matched[0].render_gam_unit_path("123456789", &derive_section(path, "home", 0)), + expected, + "`{path}` should render the documented unit path" + ); + } + } + + #[test] + fn bare_section_pattern_does_not_match_without_trailing_separator() { + // Guards the docs fix above: a `"/news/*"`-only config loses the section + // landing page entirely. + let mut slot = make_slot("ad-header", vec!["/news/*"]); + slot.compile_patterns(); + assert!( + !slot.matches_path("/news"), + "`/news/*` must not match `/news`" + ); + assert!( + slot.matches_path("/news/article"), + "`/news/*` should match descendants" + ); + } + #[test] fn compile_unit_templates_surfaces_parse_error() { let mut config = make_config_with_section_template(Some("home")); diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index afff8085d..079c8d9fc 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -3341,11 +3341,9 @@ pub(crate) fn build_slot_json( request_path: &str, ) -> serde_json::Value { // `{section}` derives from the same raw path `page_patterns` matched - // against; `section_root` covers the no-segment case (`/`). - let section = crate::creative_opportunities::derive_section( - request_path, - co_config.section_root.as_deref().unwrap_or_default(), - ); + // against; `section_root` covers the no-segment case (`/`) and paths shorter + // than the configured `section_segment`. + let section = co_config.section_for_path(request_path); let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, §ion); let div_id = slot.resolved_div_id(); let formats: Vec = slot @@ -7099,6 +7097,7 @@ mod tests { auction_timeout_ms: Some(500), price_granularity: PriceGranularity::Dense, section_root: None, + section_segment: None, slot: Vec::new(), } } @@ -7207,6 +7206,32 @@ mod tests { ); } + #[test] + fn build_slot_json_honours_configured_section_segment() { + // Locale-prefixed publisher: `/en/news/article` must resolve to the + // `news` unit, not `en`. + let mut config = make_config(); + config.gam_network_id = "99999".to_string(); + config.section_root = Some("homepage".to_string()); + config.section_segment = Some(1); + let mut slot = make_slot(); + slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); + slot.compile_unit_template() + .expect("template should compile"); + + let news = crate::publisher::build_slot_json(&slot, &config, "/en/news/article-123"); + assert_eq!( + news["gam_unit_path"], "/99999/example/news", + "section should derive from the configured segment index" + ); + + let locale_root = crate::publisher::build_slot_json(&slot, &config, "/en"); + assert_eq!( + locale_root["gam_unit_path"], "/99999/example/homepage", + "a path with no segment at the configured index should use section_root" + ); + } + #[test] fn bid_map_includes_nurl_and_burl() { let mut winning_bids = HashMap::new(); diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index 206fc4b4a..84305660b 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -1256,11 +1256,16 @@ price_granularity = "dense" # Shared placeholder value for the site root ("/") — see {section} below. section_root = "home" +# Which path segment names the section, 0-based. Default 0 (first segment). +# Set to 1 for locale-prefixed URLs such as "/en/news/article". +# section_segment = 0 [[creative_opportunities.slot]] id = "ad-header" gam_unit_path = "/{network_id}/example/{section}" -page_patterns = ["/", "/news/*", "/reviews/*"] +# List each section landing page as well as its subtree: `/news/*` matches +# `/news/article` but NOT `/news` — the glob requires the trailing separator. +page_patterns = ["/", "/news", "/news/*", "/reviews", "/reviews/*"] formats = [{ width = 728, height = 90 }] ``` @@ -1285,23 +1290,39 @@ pre-templating behavior, so existing static configs are unchanged. `{section}` is derived from the request path at request time: -- It is the **first non-empty path segment**. `/news/article-123` → `news`. +- It is the non-empty path segment at `section_segment` (0-based, default `0`). + With the default, `/news/article-123` → `news`. A site that prefixes a locale + sets `section_segment = 1`, so `/en/news/article` → `news` rather than `en`. - It is sanitized: each run of characters outside `[A-Za-z0-9_-]` becomes a single `_`. - The path is used **raw — it is not percent-decoded**. So `/new%20s` → `new_20s` (only `%` is disallowed; `2` and `0` are kept), never the decoded `new_s`. This keeps `{section}` consistent with how `page_patterns` match the same raw path. -- When the path has no segment (`/`, or repeated slashes), `{section}` is - `section_root`. +- When the path has no segment at that index — the site root (`/`, or repeated + slashes), or a path shorter than `section_segment` — `{section}` is + `section_root`. So with `section_segment = 1`, the path `/en` renders the root + section rather than reusing the locale. `section_root` is **required** whenever any slot's template uses `{section}`, and must match `[A-Za-z0-9_-]+`. There is no default: the home-section name is -publisher-specific, so the URL→section convention lives in config, not core. -Startup fails if `{section}` is used without a valid `section_root`. +publisher-specific. Startup fails if `{section}` is used without a valid +`section_root`, and also if `gam_network_id` is blank while any slot is +configured (a template referencing `{network_id}` would otherwise render an +unusable path). A `[creative_opportunities]` block with no slots is disabled, so +its `gam_network_id` is not checked. + +Both knobs are config-driven, so the URL→section convention stays with the +publisher: `section_segment` selects which segment names the section, and +`section_root` names the section when there is none. + +Leave both keys out when no template uses `{section}`. The pushed config blob +carries only the keys your config sets, and a binary older than these keys +rejects a blob containing them — so an unused key would block a rollback. Example resolution for `gam_unit_path = "/{network_id}/example/{section}"` with -`gam_network_id = "123456789"` and `section_root = "home"`: +`gam_network_id = "123456789"`, `section_root = "home"`, and the +`page_patterns` shown above: | Request path | `gam_unit_path` | | --------------- | ---------------------------- | @@ -1310,6 +1331,15 @@ Example resolution for `gam_unit_path = "/{network_id}/example/{section}"` with | `/news/article` | `/123456789/example/news` | | `/reviews/x` | `/123456789/example/reviews` | +The same config with `section_segment = 1` and locale-prefixed patterns +(`["/en", "/en/news", "/en/news/*"]`): + +| Request path | `gam_unit_path` | +| ------------------ | ------------------------- | +| `/en` | `/123456789/example/home` | +| `/en/news` | `/123456789/example/news` | +| `/en/news/article` | `/123456789/example/news` | + An **unmatched route** — a path matched by no slot's `page_patterns` — produces no slot at all, so no template is rendered for it. diff --git a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md index b18c1bcb0..cdfb61cad 100644 --- a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md +++ b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md @@ -17,9 +17,15 @@ This design makes `gam_unit_path` a **template** with a small, fixed placeholder set — `{network_id}`, `{section}`, `{slot_id}` — where `{section}` is derived from the request path at render time. One slot rule then covers all sections. -The derivation policy that matters (`{section}` for the site root) lives in -config via a required `section_root`, not in core — honoring the issue's -constraint that the URL→section convention is publisher-specific. +The derivation policy lives in config, not core — honoring the issue's +constraint that the URL→section convention is publisher-specific: +`section_segment` selects which path segment names the section, and a required +`section_root` supplies the value when the path has no such segment. + +**Revision (2026-07-29, PR review):** `section_segment` was originally listed as +a non-goal (below) with first-segment derivation hardcoded. Review found that +this left the convention only half configurable, so `section_segment` was pulled +into scope. Scope is deliberately narrow: **only** `gam_unit_path` templating. Sharing `page_patterns`/`gam_unit_path` defaults across slots is a related but distinct @@ -45,9 +51,9 @@ duplication problem, tracked as a sibling issue, not built here. Documented here so onboarding publishers know the boundary. Each is an additive extension that does **not** change the config shape below. -1. **Locale offset** — deriving `{section}` from a segment other than the first - (e.g. `/en/news` → `news`). `{section}` is the first path segment. A - `section_segment` index knob can be added later. +1. ~~**Locale offset**~~ — **now in scope.** Deriving `{section}` from a segment + other than the first (e.g. `/en/news` → `news`) is configured with + `section_segment` (0-based, default `0`). 2. **Full-path mirror** — `{section}` spanning multiple segments (`/a/b` → `a/b`). Real GAM trees bucket by section, not per-article, so this is rare; use a static per-slot `gam_unit_path` for the exception. @@ -96,6 +102,7 @@ gam_network_id = "99999" auction_timeout_ms = 2000 price_granularity = "dense" section_root = "homepage" # required when a template uses {section} +section_segment = 0 # which segment names the section (default 0) [[creative_opportunities.slot]] id = "ad-header-0" @@ -112,7 +119,7 @@ bidders = {} | -------------- | ----------------------------------------------------- | | `{network_id}` | `gam_network_id` | | `{slot_id}` | slot `id` | -| `{section}` | first path segment; `section_root` when path has none | +| `{section}` | segment at `section_segment`; `section_root` when absent | ### Resolution model @@ -127,9 +134,10 @@ startup (prepare_runtime, once): request (per matched slot, path known): if slot has a parsed template: - section = first non-empty segment of the RAW path, + section = non-empty segment #section_segment of the RAW path, runs of [^A-Za-z0-9_-] replaced with a single '_'; - section_root when the path has no segment ("/", repeated slashes) + section_root when the path has no segment at that index + ("/", repeated slashes, or a path shorter than the index) render template else: "/{network_id}/{slot_id}" # existing default (back-compat) @@ -137,12 +145,14 @@ request (per matched slot, path known): ### Section derivation rules (deterministic) -- Extract the **first non-empty** path segment. +- Extract the non-empty path segment at `section_segment` (0-based, default + `0`), counting only non-empty segments. - Replace each run of disallowed characters (`[^A-Za-z0-9_-]`) with a single `_`. Guarantees a non-empty result for any non-empty segment. Because the path is **not** decoded, `new%20s` → `new_20s` (only `%` is disallowed; `2` and `0` are alphanumeric) — never silently `news`, and never the decoded `new_s`. -- Use `section_root` **only** when there is no segment (`/`, repeated slashes). +- Use `section_root` **only** when there is no segment at that index (`/`, + repeated slashes, or a path with fewer segments than the index). - Derive from the **raw, undecoded** path — the same string `page_patterns` glob-match against — so matching and derivation never disagree. Percent-encoded segments are **not** decoded. @@ -176,7 +186,8 @@ needed. section still edits all N slots. Rejected. - **Hardcoded first-segment derivation** (issue option 3, literal): smallest, but bakes one site's URL convention into core, which the issue forbids. The - chosen design keeps the one publisher-specific knob (`section_root`) in config. + chosen design keeps both publisher-specific knobs (`section_segment`, + `section_root`) in config. ## Risks diff --git a/trusted-server.example.toml b/trusted-server.example.toml index e2f8994f6..0d18ada7a 100644 --- a/trusted-server.example.toml +++ b/trusted-server.example.toml @@ -160,26 +160,38 @@ price_granularity = "dense" # `gam_unit_path` may be a template. Supported placeholders: # {network_id} -> gam_network_id # {slot_id} -> the slot's id -# {section} -> first path segment of the request, sanitized to -# [A-Za-z0-9_-]; `section_root` below is used for "/". +# {section} -> a path segment of the request, sanitized to [A-Za-z0-9_-]; +# `section_segment` picks which one, `section_root` covers +# paths that have no such segment. # A template with no placeholders (or an absent gam_unit_path) keeps the old # behavior: verbatim path, or the default `//`. # # `section_root` is REQUIRED when any slot's template uses {section}. There is no # default — the home-section name is publisher-specific. Must be [A-Za-z0-9_-]+. -section_root = "home" +# +# `section_segment` is the 0-based index of the segment that names the section; +# it defaults to 0 (the first segment). Set it to 1 for locale-prefixed URLs, so +# "/en/news/article" resolves to "news" instead of "en". +# +# Both are left commented out: no slot below uses {section}, and an unused key +# still ships in the pushed config blob. +# section_root = "home" +# section_segment = 0 # No slot templates are enabled in the checked-in default config. Add # `[[creative_opportunities.slot]]` entries via private config or override the # entire array via: # TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT='[{"id":"...","gam_unit_path":"...",...}]' # -# Example templated slot (one rule serves every section): +# Example templated slot (one rule serves every section). Uncomment +# `section_root` above when enabling it. Note that "/news/*" does not match +# "/news" — list the section landing page separately. # [[creative_opportunities.slot]] # id = "ad-header" # gam_unit_path = "/{network_id}/example/{section}" -# page_patterns = ["/", "/news/*", "/reviews/*"] +# page_patterns = ["/", "/news", "/news/*", "/reviews", "/reviews/*"] # formats = [{ width = 728, height = 90 }] # "/" -> /123456789/example/home +# "/news" -> /123456789/example/news # "/news/x" -> /123456789/example/news # "/reviews/y" -> /123456789/example/reviews From 367dc0415ecd01896f5fdb2976a8369646877781 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Wed, 29 Jul 2026 12:47:55 +0530 Subject: [PATCH 10/21] Fix doc lint --- .../specs/2026-07-23-per-section-gam-unit-path-design.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md index cdfb61cad..d124b38fd 100644 --- a/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md +++ b/docs/superpowers/specs/2026-07-23-per-section-gam-unit-path-design.md @@ -115,10 +115,10 @@ bidders = {} ### Placeholders -| placeholder | resolves to | -| -------------- | ----------------------------------------------------- | -| `{network_id}` | `gam_network_id` | -| `{slot_id}` | slot `id` | +| placeholder | resolves to | +| -------------- | -------------------------------------------------------- | +| `{network_id}` | `gam_network_id` | +| `{slot_id}` | slot `id` | | `{section}` | segment at `section_segment`; `section_root` when absent | ### Resolution model From 7a3dec4784dff8add354895cc03960155a3e5b82 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 3 Aug 2026 23:38:42 +0530 Subject: [PATCH 11/21] Document PR 957 review resolution design --- ...6-08-03-pr-957-review-resolution-design.md | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md diff --git a/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md b/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md new file mode 100644 index 000000000..f54abc0cb --- /dev/null +++ b/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md @@ -0,0 +1,175 @@ +# PR #957 Review Resolution Design + +**Date:** 2026-08-03 + +**Status:** Approved + +**PR:** [IABTechLab/trusted-server#957](https://github.com/IABTechLab/trusted-server/pull/957) + +## Summary + +Resolve the outstanding review feedback on per-section `gam_unit_path` +templates without changing the behavior of legacy static or absent paths. The +changes make dynamic-template rollbacks fail loudly on older binaries, validate +`gam_network_id` only when it is consumed, bound request-controlled rendering, +and correct the documentation and small API issues identified during review. + +## Goals + +1. A config blob containing any dynamic placeholder must be rejected by the + legacy schema rather than accepted with placeholder braces rendered + literally. +2. A blank `gam_network_id` must remain valid when every configured slot uses + an explicit static path that does not consume it. +3. A request path must not amplify repeated `{section}` placeholders into an + unbounded allocation. +4. Initial and SPA rendering must continue to use the same bounded slot-building + path. +5. Documentation and visibility must describe the implemented behavior + accurately. + +## Non-goals + +- Introduce a public template-version setting. +- Change the serialized shape of static or absent `gam_unit_path` configs. +- Change the supported placeholder set. +- Change section casing. Google Ad Manager documents ad-unit codes as + case-insensitive, so the review claim that casing routes to different + inventory does not justify a behavior change. +- Refactor creative-opportunity configuration outside the reviewed code. + +## Design + +### Automatic rollback marker + +`CreativeOpportunitiesConfig::compile_unit_templates` continues to parse and +cache every explicit template. After parsing, it detects whether any slot uses +at least one dynamic placeholder. If so, and `section_segment` is absent, it +materializes the existing default as `Some(0)`. + +The typed CLI deserializes `TrustedServerAppConfig` through +`Settings::finalize_deserialized`, which runs runtime preparation before the +wrapper is serialized for a config push. The materialized `section_segment` +therefore appears in every pushed dynamic-template blob. A binary predating +`section_segment` rejects that blob through its `deny_unknown_fields` schema. + +Static templates and slots without `gam_unit_path` do not materialize the +marker, so their serialized shape remains compatible with the legacy schema. +An existing nonzero `section_segment` remains unchanged. + +Using the existing field is preferable to a new +`gam_unit_path_template_version` field: it creates the required fail-loud +behavior with no additional public configuration surface, and `Some(0)` is +semantically identical to the existing default. + +### Scoped network-ID validation + +Add a `template_uses_network_id` helper parallel to +`template_uses_section`. It checks the compiled cache when present and parses +the raw template as a fallback when the cache is absent. + +`gam_network_id` is required only when at least one slot either: + +- omits `gam_unit_path`, which uses the `//` default; or +- contains `{network_id}`. + +Explicit static paths and templates using only `{slot_id}` and/or `{section}` +do not consume the network ID and remain valid when it is blank. + +### Bounded dynamic rendering + +Define a 100-character limit for a rendered dynamic GAM unit path, matching the +documented Google Ad Manager ad-unit-code limit. The compatibility promise for +pre-existing static and absent paths takes precedence, so those two legacy +cases keep their current string-returning behavior even if their configured +values exceed the new dynamic limit. + +Section sanitization keeps at most 100 ASCII output characters. This bounds the +first request-controlled allocation and is safe at byte boundaries because the +sanitized output is ASCII-only. + +Before allocating a rendered dynamic path, the renderer computes the exact +length of literals and substituted values with checked arithmetic. If addition +overflows or the total exceeds 100 characters, rendering returns `None`. +Otherwise it allocates once with the computed capacity and appends each part. + +Startup validation renders each dynamic template with `section_root` when it +uses `{section}`, or with an empty section when it does not. This rejects +configurations whose fixed values or repeated root substitution already exceed +the limit. A non-root request can still derive a longer section; that request's +slot is recoverably omitted before the final path allocation. + +The raw-template fallback applies the same checked renderer, so callers that +construct or deserialize slots without runtime preparation cannot bypass the +bound. Malformed templates remain a startup error in the normal finalized +settings path; the existing raw fallback remains only for compatibility with +direct callers. + +### Publisher data flow + +Change `build_slot_json` to accept a derived `&str` section and return +`Option`. Both production callers derive the section once +per request and use `filter_map` while constructing the slot list. An +over-limit dynamic path therefore omits only that slot; it does not fail the +page response or SPA endpoint. + +This preserves the existing single slot-wire-shape implementation shared by +initial and SPA rendering while removing repeated section allocation per slot. + +### Focused cleanup and documentation + +- Extract `is_section_char` and use it in both sanitization and + `section_root` validation. +- Make `derive_section` private because only `section_for_path` and same-module + tests use it. +- Correct `section_root` rustdoc to refer to the configured segment rather than + the first segment. +- Clarify that raw-template fallback enforces placeholder-dependent validation, + while compilation is still required to reject malformed templates. +- Update the placeholder table, validation wording, changelog, and rollback + guidance to describe the automatic compatibility marker. +- Prepare a sourced technical reply instead of changing casing behavior. + +## Error handling + +- Malformed templates, invalid roots, blank consumed network IDs, and dynamic + paths that are already over limit with configured values fail settings + preparation with the existing configuration error flow. +- A request-derived over-limit dynamic path returns `None` and omits that slot + from the generated JSON. No partial path reaches `googletag.defineSlot`. +- Checked arithmetic prevents integer overflow before allocation. + +## Testing + +Implementation follows test-driven development. + +1. Add a push-shaped test that deserializes/finalizes + `TrustedServerAppConfig`, serializes it, and passes the creative-opportunity + value to a test-only legacy schema with `deny_unknown_fields`: + - `{network_id}` and `{slot_id}` templates are rejected because the automatic + marker is present; + - static and absent paths are accepted because the marker is absent. +2. Verify blank network IDs are accepted for explicit static paths and rejected + for absent paths or `{network_id}` templates, including raw-cache fallback. +3. Verify section sanitization is capped at 100 ASCII characters. +4. Verify checked rendering handles repeated placeholders, rejects overflow or + over-limit output before allocation, and preserves static/absent behavior. +5. Verify publisher slot building omits only an over-limit dynamic slot and that + initial/SPA paths continue to share the same section and slot builder. +6. Run core/adapter tests, formatting, clippy, documentation formatting, and the + repository's full CI-equivalent verification before handoff. + +## Acceptance criteria + +- [ ] Every pushed dynamic-template blob fails legacy-schema deserialization. +- [ ] Pushed static and absent-path blobs remain legacy-schema compatible. +- [ ] Blank `gam_network_id` is rejected exactly when a rendered slot consumes + it. +- [ ] Dynamic rendered length is checked before final allocation and cannot + exceed 100 characters. +- [ ] Request-time overflow omits the affected slot without failing the + response. +- [ ] Static and absent paths retain their pre-template behavior. +- [ ] All sound review comments are implemented and the casing comment has a + sourced technical response. +- [ ] Relevant tests and CI-equivalent checks pass. From 2e0acb3b6ebc773bafc28b264296ca899a4af6e3 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 3 Aug 2026 23:50:00 +0530 Subject: [PATCH 12/21] Plan PR 957 review resolution --- .../2026-08-03-pr-957-review-resolution.md | 738 ++++++++++++++++++ ...6-08-03-pr-957-review-resolution-design.md | 10 +- 2 files changed, 744 insertions(+), 4 deletions(-) create mode 100644 docs/superpowers/plans/2026-08-03-pr-957-review-resolution.md diff --git a/docs/superpowers/plans/2026-08-03-pr-957-review-resolution.md b/docs/superpowers/plans/2026-08-03-pr-957-review-resolution.md new file mode 100644 index 000000000..03b390cad --- /dev/null +++ b/docs/superpowers/plans/2026-08-03-pr-957-review-resolution.md @@ -0,0 +1,738 @@ +# PR #957 Review Resolution Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Resolve every technically sound review comment on PR #957 while preserving legacy static/absent `gam_unit_path` behavior and making dynamic paths rollback-safe and allocation-bounded. + +**Architecture:** Extend the existing startup compilation pass to materialize the default `section_segment` as a rollback marker and to validate only values consumed by parsed templates. Dynamic rendering becomes a checked `Option` path capped at 100 UTF-8 bytes; both initial and SPA slot builders derive the section once and omit only an over-limit dynamic slot. Static and absent paths bypass the new dynamic limit to preserve pre-template behavior. + +**Tech Stack:** Rust 2024, serde/TOML/JSON, `error-stack` through existing settings preparation, native and WASM adapter test aliases, Markdown documentation. + +--- + +## File map + +- Modify `crates/trusted-server-core/src/config.rs`: add push-shaped legacy-schema compatibility tests around `TrustedServerAppConfig` serialization. +- Modify `crates/trusted-server-core/src/creative_opportunities.rs`: add rollback-marker detection, scoped placeholder consumption, bounded rendering, shared section validation, and unit tests. +- Modify `crates/trusted-server-core/src/publisher.rs`: derive one section per request, propagate recoverable slot omission, and update publisher tests. +- Modify `docs/guide/configuration.md`: document configured-segment behavior, dynamic rollback marker, precise validation, and rendered-path bound. +- Modify `CHANGELOG.md`: replace the misleading rollback and blanket network-ID statements. +- Modify `docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md`: retain the independent review clarification that the structural bound is measured in UTF-8 bytes. + +### Task 1: Make pushed dynamic templates fail legacy deserialization + +**Files:** + +- Modify: `crates/trusted-server-core/src/config.rs:213-253` +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs:17-28, 213-224, 483-560` +- Test: `crates/trusted-server-core/src/config.rs` + +- [ ] **Step 1: Add a test-only legacy schema and pushed-config helper** + +In `config.rs`'s test module, add a minimal legacy creative-opportunities schema. It deliberately knows the old top-level fields but not `section_root` or `section_segment`: + +```rust +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct LegacyCreativeOpportunitiesConfig { + gam_network_id: String, + #[serde(default)] + auction_timeout_ms: Option, + #[serde(default)] + price_granularity: serde_json::Value, + #[serde(default)] + slot: Vec, +} + +fn pushed_creative_opportunities(gam_unit_path: Option<&str>) -> serde_json::Value { + let gam_unit_path = gam_unit_path + .map(|path| format!("gam_unit_path = {path:?}")) + .unwrap_or_default(); + let toml = format!( + r#"{} +[creative_opportunities] +gam_network_id = "99999" + +[[creative_opportunities.slot]] +id = "ad-header" +{gam_unit_path} +page_patterns = ["/"] +formats = [{{ width = 728, height = 90 }}] +"#, + crate_test_settings_str() + ); + let app_config: TrustedServerAppConfig = + toml::from_str(&toml).expect("should finalize typed app config"); + serde_json::to_value(app_config) + .expect("should serialize pushed app config") + .get("creative_opportunities") + .cloned() + .expect("should contain creative opportunities") +} +``` + +Use field reads or `#[allow(dead_code)]` if rustc flags the intentionally deserialize-only legacy fields. + +- [ ] **Step 2: Write failing push-shaped rollback tests** + +Add three focused tests: + +```rust +#[test] +fn pushed_dynamic_templates_are_rejected_by_legacy_schema() { + for template in ["/{network_id}/example", "/example/{slot_id}"] { + let value = pushed_creative_opportunities(Some(template)); + let error = serde_json::from_value::(value) + .expect_err("legacy schema should reject automatic compatibility marker"); + assert!( + error.to_string().contains("section_segment"), + "error should name compatibility marker: {error}" + ); + } +} + +#[test] +fn pushed_static_template_is_accepted_by_legacy_schema() { + let value = pushed_creative_opportunities(Some("/99999/example/home")); + serde_json::from_value::(value) + .expect("legacy schema should accept static path"); +} + +#[test] +fn pushed_absent_template_is_accepted_by_legacy_schema() { + let value = pushed_creative_opportunities(None); + serde_json::from_value::(value) + .expect("legacy schema should accept absent path"); +} +``` + +- [ ] **Step 3: Run the dynamic rollback test and verify RED** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin pushed_dynamic_templates_are_rejected_by_legacy_schema +``` + +Expected: FAIL because the finalized dynamic config still omits `section_segment`, so the legacy schema accepts it. + +- [ ] **Step 4: Implement placeholder detection and automatic marker materialization** + +Add a placeholder predicate to `UnitTemplatePart`: + +```rust +impl UnitTemplatePart { + fn is_placeholder(&self) -> bool { + !matches!(self, Self::Literal(_)) + } +} +``` + +Add `template_is_dynamic` beside the existing placeholder-use helpers. It must read `compiled_unit` when present and parse the raw template when absent: + +```rust +fn template_is_dynamic(&self) -> bool { + self.template_parts() + .is_some_and(|parts| parts.iter().any(UnitTemplatePart::is_placeholder)) +} +``` + +Introduce a small private `template_parts` helper that returns borrowed compiled parts when available and parsed owned parts for the raw fallback without changing malformed-template startup behavior. If a `Cow<'_, [UnitTemplatePart]>` keeps the implementation clearer, import `std::borrow::Cow`; otherwise keep the existing match shape and avoid a new abstraction. + +After compiling every slot in `compile_unit_templates`, materialize the existing default only for dynamic templates: + +```rust +if self.section_segment.is_none() + && self + .slot + .iter() + .any(CreativeOpportunitySlot::template_is_dynamic) +{ + self.section_segment = Some(0); +} +``` + +Do not mark static or absent paths, and do not overwrite an explicitly configured segment. + +- [ ] **Step 5: Run the three rollback tests and verify GREEN** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin pushed_dynamic_templates_are_rejected_by_legacy_schema +cargo test --package trusted-server-core --target aarch64-apple-darwin pushed_static_template_is_accepted_by_legacy_schema +cargo test --package trusted-server-core --target aarch64-apple-darwin pushed_absent_template_is_accepted_by_legacy_schema +``` + +Expected: all PASS. + +- [ ] **Step 6: Run the complete core test target** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin +``` + +Expected: PASS with no warnings. + +- [ ] **Step 7: Commit the rollback marker** + +```bash +git add crates/trusted-server-core/src/config.rs crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Make dynamic GAM templates fail legacy rollback" +``` + +### Task 2: Scope network-ID validation to actual consumers + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs:226-278, 544-560, 1100-1130` +- Test: `crates/trusted-server-core/src/creative_opportunities.rs` + +- [ ] **Step 1: Add failing compatibility tests** + +Add tests covering explicit static and `{slot_id}`-only paths with a blank network ID: + +```rust +#[test] +fn validate_runtime_allows_blank_network_id_for_static_paths() { + let mut config = make_config_with_section_template(None); + config.gam_network_id.clear(); + config.slot[0].gam_unit_path = Some("/example/static".to_string()); + config.compile_slots(); + config.compile_unit_templates().expect("should compile static path"); + config + .validate_runtime() + .expect("static path should not consume network id"); +} + +#[test] +fn validate_runtime_allows_blank_network_id_for_slot_id_template() { + let mut config = make_config_with_section_template(None); + config.gam_network_id.clear(); + config.slot[0].gam_unit_path = Some("/example/{slot_id}".to_string()); + config.compile_slots(); + config + .compile_unit_templates() + .expect("should compile slot-id template"); + config + .validate_runtime() + .expect("slot-id template should not consume network id"); +} +``` + +Also retain the existing compiled `{network_id}` rejection and add an uncompiled/raw-cache-fallback test so validation cannot skip consumption detection. + +Add an explicit absent-path rejection because the default path consumes the +network ID: + +```rust +#[test] +fn validate_runtime_rejects_blank_network_id_for_absent_path() { + let mut config = make_config_with_section_template(None); + config.gam_network_id.clear(); + config.slot[0].gam_unit_path = None; + config.compile_slots(); + config + .compile_unit_templates() + .expect("should compile absent path"); + config + .validate_runtime() + .expect_err("absent path should consume network id"); +} +``` + +- [ ] **Step 2: Run the new compatibility test and verify RED** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin validate_runtime_allows_blank_network_id_for_static_paths +``` + +Expected: FAIL with `gam_network_id must not be empty`. + +- [ ] **Step 3: Implement `template_uses_network_id` and scoped validation** + +Add a sibling of `template_uses_section` using the same compiled/raw fallback: + +```rust +fn template_uses_network_id(&self) -> bool { + self.template_parts().is_some_and(|parts| { + parts + .iter() + .any(|part| matches!(part, UnitTemplatePart::NetworkId)) + }) +} +``` + +Replace the blanket slot-list check with: + +```rust +let network_id_consumed = self.slot.iter().any(|slot| { + slot.gam_unit_path.is_none() || slot.template_uses_network_id() +}); +if network_id_consumed && self.gam_network_id.trim().is_empty() { + return Err("gam_network_id must not be empty".to_string()); +} +``` + +Update the nearby comments and rustdoc to say “consumed by a slot,” not “when slots are configured.” + +- [ ] **Step 4: Run scoped validation tests and verify GREEN** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin validate_runtime_allows_blank_network_id +cargo test --package trusted-server-core --target aarch64-apple-darwin validate_runtime_rejects_blank_network_id +``` + +Expected: static, slot-ID-only, and empty-slot cases PASS; absent-path and `{network_id}` cases reject as asserted. + +- [ ] **Step 5: Run complete core tests** + +Run `cargo test --package trusted-server-core --target aarch64-apple-darwin`. + +Expected: PASS. + +- [ ] **Step 6: Commit scoped validation** + +```bash +git add crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Validate GAM network IDs only when consumed" +``` + +### Task 3: Bound dynamic rendering before allocation + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs:83-125, 226-278, 483-560` +- Test: `crates/trusted-server-core/src/creative_opportunities.rs:876-1080` + +- [ ] **Step 1: Add failing section and renderer tests** + +Add these behaviors as separate tests: + +```rust +#[test] +fn derive_section_caps_sanitized_output_at_100_ascii_bytes() { + let path = format!("/{}", "a".repeat(150)); + assert_eq!(derive_section(&path, "home", 0), "a".repeat(100)); +} + +#[test] +fn render_gam_unit_path_omits_over_limit_repeated_section() { + let mut slot = make_slot("ad-header", vec!["/*"]); + slot.gam_unit_path = Some("/{section}/{section}".to_string()); + slot.compile_unit_template().expect("should compile template"); + assert_eq!( + slot.render_gam_unit_path("99999", &"a".repeat(60)), + None, + "dynamic output over 100 bytes should be omitted" + ); +} + +#[test] +fn render_gam_unit_path_preserves_over_limit_static_path() { + let mut slot = make_slot("ad-header", vec!["/*"]); + let static_path = format!("/{}", "a".repeat(120)); + slot.gam_unit_path = Some(static_path.clone()); + slot.compile_unit_template().expect("should compile static path"); + assert_eq!( + slot.render_gam_unit_path("99999", "ignored"), + Some(static_path), + "legacy static paths should retain existing behavior" + ); +} +``` + +Add an equivalent raw/uncompiled repeated-placeholder test and a startup test where repeated `{section}` with `section_root = "homepage"` already exceeds the bound. + +- [ ] **Step 2: Run the new renderer test and verify RED** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin render_gam_unit_path_omits_over_limit_repeated_section +``` + +Expected: compile failure or assertion failure because the renderer still returns an unbounded `String`. + +- [ ] **Step 3: Share the section-character predicate and cap sanitization** + +Add constants and a predicate near `sanitize_section`: + +```rust +const MAX_DYNAMIC_GAM_UNIT_PATH_BYTES: usize = 100; +const MAX_SECTION_BYTES: usize = 100; + +fn is_section_char(ch: char) -> bool { + ch.is_ascii_alphanumeric() || ch == '_' || ch == '-' +} +``` + +Use `is_section_char` in sanitization and `section_root` validation. Build the sanitized ASCII result with capacity capped at `MAX_SECTION_BYTES`, and stop once the output reaches that byte count. Preserve the “one underscore per disallowed run” behavior. + +- [ ] **Step 4: Implement checked dynamic rendering** + +Add a private renderer that selects each part's `&str`, computes the exact UTF-8 byte count with `checked_add`, rejects totals over `MAX_DYNAMIC_GAM_UNIT_PATH_BYTES`, then performs a single allocation: + +```rust +fn render_dynamic_parts( + parts: &[UnitTemplatePart], + gam_network_id: &str, + section: &str, + slot_id: &str, +) -> Option { + let value = |part: &UnitTemplatePart| match part { + UnitTemplatePart::Literal(value) => value.as_str(), + UnitTemplatePart::NetworkId => gam_network_id, + UnitTemplatePart::Section => section, + UnitTemplatePart::SlotId => slot_id, + }; + let rendered_len = parts.iter().try_fold(0usize, |len, part| { + len.checked_add(value(part).len()) + })?; + if rendered_len > MAX_DYNAMIC_GAM_UNIT_PATH_BYTES { + return None; + } + let mut rendered = String::with_capacity(rendered_len); + for part in parts { + rendered.push_str(value(part)); + } + Some(rendered) +} +``` + +Change `render_gam_unit_path` to `Option`: + +- compiled/raw dynamic template → `render_dynamic_parts`; +- compiled/raw static template → `Some(raw.clone())` without the dynamic bound; +- absent path → `Some(format!(...))` without the dynamic bound. + +Keep raw malformed templates literal for existing direct-caller compatibility, wrapped in `Some`. + +- [ ] **Step 5: Reject configured dynamic paths that already exceed the bound** + +After slot shape, network-ID, and section-root validation, validate dynamic templates by rendering with the configured `section_root` (or `""` when `{section}` is unused). Return a slot-specific startup error when that fixed/configured rendering is `None`. + +This catches repeated-root templates at startup while leaving request-specific long sections to recoverable omission. + +- [ ] **Step 6: Update existing renderer assertions for `Option`** + +Wrap expected paths in `Some(...)` or call `.expect("should render ...")` where the test subsequently indexes/compares a concrete string. Do not weaken malformed-template or static/absent compatibility assertions. + +- [ ] **Step 7: Run focused and complete core tests** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin derive_section_caps_sanitized_output +cargo test --package trusted-server-core --target aarch64-apple-darwin render_gam_unit_path +cargo test --package trusted-server-core --target aarch64-apple-darwin validate_runtime +cargo test --package trusted-server-core --target aarch64-apple-darwin +``` + +Expected: PASS with no warnings. + +- [ ] **Step 8: Commit bounded rendering** + +```bash +git add crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Bound dynamic GAM unit path rendering" +``` + +### Task 4: Omit over-limit slots consistently and derive section once + +**Files:** + +- Modify: `crates/trusted-server-core/src/publisher.rs:3331-3387, 3790-3799, 7293-7340` +- Test: `crates/trusted-server-core/src/publisher.rs` + +- [ ] **Step 1: Add a failing publisher omission test** + +Add a test beside the existing `build_slot_json` section tests: + +```rust +#[test] +fn ad_slots_script_omits_over_limit_dynamic_slot() { + let mut config = make_config(); + config.section_root = Some("home".to_string()); + let mut over_limit = make_slot(); + over_limit.gam_unit_path = Some("/{section}/{section}".to_string()); + over_limit + .compile_unit_template() + .expect("should compile over-limit template"); + let mut valid = make_slot(); + valid.id = "valid-slot".to_string(); + valid.gam_unit_path = Some("/99999/example/valid".to_string()); + valid + .compile_unit_template() + .expect("should compile valid static path"); + let request_path = format!("/{}", "a".repeat(60)); + + let script = build_ad_slots_script(&[over_limit, valid], &config, &request_path); + + assert!( + !script.contains("atf_sidebar_ad"), + "should omit over-limit dynamic slot" + ); + assert!(script.contains("valid-slot"), "should preserve valid sibling slot"); +} +``` + +If script escaping changes the exact literal, parse/extract the JSON using the existing test helper rather than asserting a brittle escaped substring. + +- [ ] **Step 2: Run the omission test and verify RED** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin ad_slots_script_omits_over_limit_dynamic_slot +``` + +Expected: compile failure because `build_slot_json` does not yet handle `Option`, or an assertion failure because the slot is still emitted. + +- [ ] **Step 3: Change the shared builder to accept a derived section** + +Change the signature and early-return on renderer omission: + +```rust +fn build_slot_json( + slot: &CreativeOpportunitySlot, + co_config: &CreativeOpportunitiesConfig, + section: &str, +) -> Option { + let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, section)?; + Some(serde_json::json!({ /* existing wire shape */ })) +} +``` + +Keep the function `pub(crate)` if cross-module tests still call it. + +- [ ] **Step 4: Derive once and `filter_map` in both production callers** + +In `build_ad_slots_script`: + +```rust +let section = co_config.section_for_path(request_path); +let slots = matched_slots + .iter() + .filter_map(|slot| build_slot_json(slot, co_config, §ion)) + .collect::>(); +``` + +In `handle_page_bids`, derive `section` once from `path_param` before the ad-stack gate, then use the same `filter_map`. Do not create a separate SPA rendering implementation. + +- [ ] **Step 5: Update existing publisher unit tests** + +Derive the section explicitly in tests that call `build_slot_json`, then call `.expect("should render slot")` before indexing. Preserve assertions for default segment, configured segment, and root fallback. + +- [ ] **Step 6: Run publisher and core tests** + +Run: + +```bash +cargo test --package trusted-server-core --target aarch64-apple-darwin ad_slots_script +cargo test --package trusted-server-core --target aarch64-apple-darwin build_slot_json +cargo test --package trusted-server-core --target aarch64-apple-darwin page_bids +cargo test --package trusted-server-core --target aarch64-apple-darwin +``` + +Expected: PASS; both initial and SPA tests use the shared bounded builder. + +- [ ] **Step 7: Commit publisher propagation** + +```bash +git add crates/trusted-server-core/src/publisher.rs +git commit -m "Omit over-limit dynamic GAM slots" +``` + +### Task 5: Correct API visibility and documentation + +**Files:** + +- Modify: `crates/trusted-server-core/src/creative_opportunities.rs:101-240, 501-560` +- Modify: `docs/guide/configuration.md:1297-1350` +- Modify: `CHANGELOG.md:23` +- Modify: `docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md` + +- [ ] **Step 1: Make focused rustdoc and visibility edits** + +- Make `derive_section` private; same-module tests remain valid. +- Describe `section_root` as fallback when the configured segment is absent, not when there is no first segment. +- State that raw-template fallback enforces placeholder-dependent requirements even without the cache, while `compile_unit_templates` remains required to reject malformed templates. +- Document `render_gam_unit_path`'s `None` result and the 100-byte dynamic bound. +- Update validation rustdoc to describe network-ID consumption rather than any configured slot. + +- [ ] **Step 2: Correct the configuration guide** + +Update the placeholder row to: + +```markdown +| `{section}` | non-empty path segment at `section_segment` (default: first; see below) | +``` + +Update validation and rollback paragraphs to say: + +- blank `gam_network_id` is rejected only if an absent path or `{network_id}` template consumes it; +- every dynamic template causes typed serialization to materialize + `section_segment = 0` when it was omitted; +- legacy binaries therefore reject dynamic blobs loudly; +- only static and absent paths retain legacy-schema compatibility; +- dynamic rendering is capped at 100 UTF-8 bytes and over-limit request slots are omitted. + +Do not add the reviewer's case-sensitive warning or lowercase sections: Google documents GAM ad-unit codes as case-insensitive. + +- [ ] **Step 3: Correct the changelog** + +Replace “startup rejects a blank network ID when slots are configured” with the scoped consumption rule. Replace “configs that omit the new keys roll back cleanly” with the automatic-marker behavior and the static/absent compatibility guarantee. + +- [ ] **Step 4: Format and inspect documentation** + +Run: + +```bash +cd docs && npm run format +git diff --check +``` + +Expected: formatter exits 0; no whitespace errors or unrelated documentation changes. Revert only formatter-induced unrelated edits with a non-destructive patch if any appear. + +- [ ] **Step 5: Run rustdoc-sensitive lint and focused tests** + +Run: + +```bash +cargo fmt --all -- --check +cargo clippy-axum +cargo test --package trusted-server-core --target aarch64-apple-darwin +``` + +Expected: PASS. + +- [ ] **Step 6: Commit documentation and visibility cleanup** + +```bash +git add CHANGELOG.md docs/guide/configuration.md docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md crates/trusted-server-core/src/creative_opportunities.rs +git commit -m "Document dynamic GAM template safeguards" +``` + +### Task 6: Run complete PR verification and review the final diff + +**Files:** + +- Verify all modified files; no planned code changes. + +- [ ] **Step 1: Run Rust and parity formatting gates** + +```bash +cargo fmt --all -- --check +cargo fmt --manifest-path crates/trusted-server-integration-tests/Cargo.toml -- --check +``` + +Expected: both PASS and `git status --short` shows no formatter changes. + +- [ ] **Step 2: Run adapter checks and release builds** + +From repository root: + +```bash +cargo build -p trusted-server-adapter-axum +cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1 +cargo check -p trusted-server-adapter-cloudflare +cargo check-cloudflare +cargo check -p trusted-server-adapter-spin +cargo check-spin +TRUSTED_SERVER__PUBLISHER__ORIGIN_URL=http://127.0.0.1:8080 \ +TRUSTED_SERVER__PUBLISHER__PROXY_SECRET=integration-test-proxy-secret \ +TRUSTED_SERVER__EC__PASSPHRASE=integration-test-ec-secret-padded-32 \ +TRUSTED_SERVER__PROXY__CERTIFICATE_CHECK=false \ +cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --release +``` + +Expected: all PASS. + +- [ ] **Step 3: Run Rust test and benchmark gates** + +From repository root: + +```bash +cargo test-fastly +cargo test-axum +cargo bench -p trusted-server-core --bench html_processor_bench -- --test +cargo test-cloudflare +cargo test-spin +cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity +./scripts/test-cli.sh +cargo test --package trusted-server-openrtb-codegen --target aarch64-apple-darwin +``` + +Expected: all PASS. + +- [ ] **Step 4: Run every PR clippy gate** + +From repository root: + +```bash +cargo clippy-fastly +cargo clippy-axum +cargo clippy-cloudflare +cargo clippy-cloudflare-wasm +cargo clippy-spin-native +cargo clippy-spin-wasm +cargo clippy --package trusted-server-cli --target aarch64-apple-darwin --all-targets --all-features -- -D warnings +cargo clippy --package trusted-server-openrtb-codegen --target aarch64-apple-darwin --all-targets -- -D warnings +cargo clippy --manifest-path crates/trusted-server-integration-tests/Cargo.toml --all-targets -- -D warnings +``` + +Expected: all PASS with `-D warnings`. + +- [ ] **Step 5: Run JavaScript build, test, lint, and format gates** + +```bash +cd crates/trusted-server-js/lib +npm run build +npm test -- --run +npm run lint +npm run format +``` + +Expected: all PASS. + +- [ ] **Step 6: Run documentation lint, format, and build checks** + +From the repository root: + +```bash +cd docs +npm run lint +npm run format +npm run build +``` + +Expected: all PASS. The build is included because this review changes the +published configuration guide, even though docs deployment itself runs only on +`main`. + +- [ ] **Step 7: Inspect the complete branch diff** + +```bash +git status --short --branch +git diff --check main...HEAD +git diff --stat main...HEAD +git log --oneline --decorate -10 +``` + +Confirm: + +- only planned runtime, tests, docs, spec, and plan files changed in this review round; +- no placeholder, debug output, unrelated refactor, or sensitive real-world data was added; +- static and absent paths retain their compatibility tests; +- every dynamic rendering call handles `None`. + +- [ ] **Step 8: Prepare GitHub thread resolutions** + +Draft one concise technical reply per current review thread, naming the implemented behavior and test. For the casing thread, cite Google's official “Ad units: Name vs. code” documentation and explain that no lowercasing or warning was added because codes are case-insensitive. Do not post or resolve GitHub threads without explicit user authorization. diff --git a/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md b/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md index f54abc0cb..41b162f53 100644 --- a/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md +++ b/docs/superpowers/specs/2026-08-03-pr-957-review-resolution-design.md @@ -78,8 +78,10 @@ do not consume the network ID and remain valid when it is blank. ### Bounded dynamic rendering -Define a 100-character limit for a rendered dynamic GAM unit path, matching the -documented Google Ad Manager ad-unit-code limit. The compatibility promise for +Define a 100-byte limit for a rendered dynamic GAM unit path, conservatively +enforcing the documented 100-character Google Ad Manager ad-unit-code limit. +The byte limit directly bounds allocation; validated substitutions are ASCII, +and non-ASCII template literals consume their UTF-8 byte length. The compatibility promise for pre-existing static and absent paths takes precedence, so those two legacy cases keep their current string-returning behavior even if their configured values exceed the new dynamic limit. @@ -90,7 +92,7 @@ sanitized output is ASCII-only. Before allocating a rendered dynamic path, the renderer computes the exact length of literals and substituted values with checked arithmetic. If addition -overflows or the total exceeds 100 characters, rendering returns `None`. +overflows or the total exceeds 100 bytes, rendering returns `None`. Otherwise it allocates once with the computed capacity and appends each part. Startup validation renders each dynamic template with `section_root` when it @@ -166,7 +168,7 @@ Implementation follows test-driven development. - [ ] Blank `gam_network_id` is rejected exactly when a rendered slot consumes it. - [ ] Dynamic rendered length is checked before final allocation and cannot - exceed 100 characters. + exceed 100 bytes. - [ ] Request-time overflow omits the affected slot without failing the response. - [ ] Static and absent paths retain their pre-template behavior. From 3cb971da696f2f24fadca79de9514795929d9042 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 3 Aug 2026 23:54:26 +0530 Subject: [PATCH 13/21] Make dynamic GAM templates fail legacy rollback --- crates/trusted-server-core/src/config.rs | 71 +++++++++++++++++++ .../src/creative_opportunities.rs | 24 +++++++ 2 files changed, 95 insertions(+) diff --git a/crates/trusted-server-core/src/config.rs b/crates/trusted-server-core/src/config.rs index 7bbecd747..35658b253 100644 --- a/crates/trusted-server-core/src/config.rs +++ b/crates/trusted-server-core/src/config.rs @@ -215,6 +215,46 @@ mod tests { use super::*; use crate::test_support::tests::crate_test_settings_str; + #[derive(Debug, Deserialize)] + #[serde(deny_unknown_fields)] + #[allow(dead_code)] + struct LegacyCreativeOpportunitiesConfig { + gam_network_id: String, + #[serde(default)] + auction_timeout_ms: Option, + #[serde(default)] + price_granularity: serde_json::Value, + #[serde(default)] + slot: Vec, + } + + fn serialized_creative_opportunities(gam_unit_path: Option<&str>) -> serde_json::Value { + let mut toml = crate_test_settings_str(); + toml.push_str( + r#" + +[creative_opportunities] +gam_network_id = "99999" + +[[creative_opportunities.slot]] +id = "example-slot" +page_patterns = ["/*"] +formats = [{ width = 300, height = 250 }] +"#, + ); + if let Some(gam_unit_path) = gam_unit_path { + toml.push_str(&format!("gam_unit_path = {gam_unit_path:?}\n")); + } + + let app_config: TrustedServerAppConfig = + toml::from_str(&toml).expect("should deserialize app config wrapper"); + serde_json::to_value(app_config) + .expect("should serialize app config wrapper") + .get("creative_opportunities") + .cloned() + .expect("should contain creative opportunities") + } + fn valid_settings() -> Settings { let mut settings = Settings::from_toml(&crate_test_settings_str()).expect("should parse test settings"); @@ -251,6 +291,37 @@ mod tests { ); } + #[test] + fn dynamic_gam_unit_templates_are_rejected_by_legacy_schema() { + for gam_unit_path in ["/{network_id}/example", "/example/{slot_id}"] { + let creative_opportunities = serialized_creative_opportunities(Some(gam_unit_path)); + let err = + serde_json::from_value::(creative_opportunities) + .expect_err("should reject dynamic GAM unit template"); + + assert!( + err.to_string().contains("section_segment"), + "legacy error should name section_segment: {err}" + ); + } + } + + #[test] + fn static_gam_unit_template_is_accepted_by_legacy_schema() { + let creative_opportunities = serialized_creative_opportunities(Some("/99999/example/home")); + + serde_json::from_value::(creative_opportunities) + .expect("should accept static GAM unit template"); + } + + #[test] + fn absent_gam_unit_template_is_accepted_by_legacy_schema() { + let creative_opportunities = serialized_creative_opportunities(None); + + serde_json::from_value::(creative_opportunities) + .expect("should accept absent GAM unit template"); + } + #[test] fn deploy_validation_rejects_placeholders() { let settings = Settings::from_toml( diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 7df15932a..aba745331 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -27,6 +27,12 @@ pub(crate) enum UnitTemplatePart { SlotId, } +impl UnitTemplatePart { + fn is_placeholder(&self) -> bool { + !matches!(self, Self::Literal(_)) + } +} + /// Parses a `gam_unit_path` template into an ordered list of parts. /// /// Supported placeholders: `{network_id}`, `{section}`, `{slot_id}`. A template @@ -220,6 +226,14 @@ impl CreativeOpportunitiesConfig { for slot in &mut self.slot { slot.compile_unit_template()?; } + if self.section_segment.is_none() + && self + .slot + .iter() + .any(CreativeOpportunitySlot::template_is_dynamic) + { + self.section_segment = Some(0); + } Ok(()) } @@ -498,6 +512,16 @@ impl CreativeOpportunitySlot { Ok(()) } + fn template_is_dynamic(&self) -> bool { + let is_dynamic = + |parts: &[UnitTemplatePart]| parts.iter().any(UnitTemplatePart::is_placeholder); + match (&self.compiled_unit, &self.gam_unit_path) { + (Some(parts), _) => is_dynamic(parts), + (None, Some(raw)) => parse_unit_template(raw).is_ok_and(|parts| is_dynamic(&parts)), + (None, None) => false, + } + } + /// Renders the resolved GAM unit path for a given network id and section. /// /// Substitutes `{network_id}`, `{section}`, and `{slot_id}` in the parsed From b71d16dbbfcf00103e2e2315b98a873b13d46d18 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 4 Aug 2026 00:06:57 +0530 Subject: [PATCH 14/21] Validate GAM network IDs only when consumed --- .../src/creative_opportunities.rs | 119 +++++++++++++++--- 1 file changed, 101 insertions(+), 18 deletions(-) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index aba745331..09ceb5658 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -239,29 +239,27 @@ impl CreativeOpportunitiesConfig { /// Validate all slot definitions after runtime preparation. /// - /// Call [`compile_unit_templates`](Self::compile_unit_templates) first: the - /// `{section}` → [`section_root`](Self::section_root) requirement is keyed off - /// each slot's compiled template, so an uncompiled config silently skips that - /// check. [`Settings::prepare_runtime`](crate::settings::Settings) enforces - /// this order. + /// Call [`compile_unit_templates`](Self::compile_unit_templates) first so + /// malformed templates fail at startup. Validation also reads raw templates + /// when their cache is absent. [`Settings::prepare_runtime`](crate::settings::Settings) + /// enforces this order. /// /// # Errors /// - /// Returns an error string when [`gam_network_id`](Self::gam_network_id) is - /// blank while slots are configured, when a slot has an invalid identifier, - /// page pattern set, format list, or dimensions, or when a slot's + /// Returns an error string when a consumed [`gam_network_id`](Self::gam_network_id) + /// is blank, when a slot has an invalid identifier, page pattern set, format + /// list, or dimensions, or when a slot's /// `gam_unit_path` template uses `{section}` without a valid /// [`section_root`](Self::section_root). pub fn validate_runtime(&self) -> Result<(), String> { - // Every rendered unit path either substitutes `{network_id}` or uses the - // `//` default, so a blank network id produces a - // path GAM cannot resolve (`""` or `//slot`). Reject at startup rather - // than shipping it to `googletag.defineSlot`. - // - // Gated on a non-empty slot list: an empty list disables the feature, so - // no path is ever rendered and the id is inert. Failing startup there - // would take a site down over a value it does not use. - if !self.slot.is_empty() && self.gam_network_id.trim().is_empty() { + // A network ID is required only when a slot renders the default + // `//` path or substitutes `{network_id}`. Static + // and `{slot_id}`/`{section}`-only templates leave it inert. + let network_id_consumed = self + .slot + .iter() + .any(|slot| slot.gam_unit_path.is_none() || slot.template_uses_network_id()); + if network_id_consumed && self.gam_network_id.trim().is_empty() { return Err("gam_network_id must not be empty".to_string()); } @@ -583,6 +581,26 @@ impl CreativeOpportunitySlot { } } + /// Returns `true` if this slot's `gam_unit_path` template contains `{network_id}`. + /// + /// Reads the raw template when [`compiled_unit`](Self::compiled_unit) is + /// empty so validation cannot silently skip the network ID requirement for + /// an uncompiled config. + fn template_uses_network_id(&self) -> bool { + let uses_network_id = |parts: &[UnitTemplatePart]| { + parts + .iter() + .any(|part| matches!(part, UnitTemplatePart::NetworkId)) + }; + match (&self.compiled_unit, &self.gam_unit_path) { + (Some(parts), _) => uses_network_id(parts), + (None, Some(raw)) => { + parse_unit_template(raw).is_ok_and(|parts| uses_network_id(&parts)) + } + (None, None) => false, + } + } + /// Returns the div element ID for this slot. /// /// Returns the [`div_id`](Self::div_id) override when set, otherwise returns [`id`](Self::id). @@ -1122,7 +1140,53 @@ mod tests { } #[test] - fn validate_runtime_rejects_blank_network_id() { + fn validate_runtime_allows_blank_network_id_with_static_paths() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("/12345/example/homepage".to_string()); + config.gam_network_id = " ".to_string(); + config.compile_slots(); + config + .compile_unit_templates() + .expect("should compile static template"); + config + .validate_runtime() + .expect("should allow a blank unused network id"); + } + + #[test] + fn validate_runtime_allows_blank_network_id_with_slot_id_template() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("/example/{slot_id}".to_string()); + config.gam_network_id = String::new(); + config.compile_slots(); + config + .compile_unit_templates() + .expect("should compile slot-id template"); + config + .validate_runtime() + .expect("should allow a blank unused network id"); + } + + #[test] + fn validate_runtime_rejects_blank_network_id_when_default_path_uses_it() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = None; + config.gam_network_id = String::new(); + config.compile_slots(); + config + .compile_unit_templates() + .expect("should compile default path"); + let err = config + .validate_runtime() + .expect_err("blank network id should fail when the default path uses it"); + assert_eq!( + err, "gam_network_id must not be empty", + "should report the blank network id" + ); + } + + #[test] + fn validate_runtime_rejects_blank_network_id_when_compiled_template_uses_it() { // `gam_unit_path = "{network_id}"` renders to an empty string with a // blank network id, which reaches googletag.defineSlot as an invalid path. let mut config = make_config_with_section_template(Some("home")); @@ -1141,6 +1205,25 @@ mod tests { ); } + #[test] + fn validate_runtime_rejects_blank_network_id_when_raw_template_uses_it() { + let mut config = make_config_with_section_template(Some("home")); + config.slot[0].gam_unit_path = Some("/{network_id}/example".to_string()); + config.gam_network_id = String::new(); + config.compile_slots(); + assert!( + config.slot[0].compiled_unit.is_none(), + "test precondition: template cache is empty" + ); + let err = config + .validate_runtime() + .expect_err("blank network id should fail when a raw template uses it"); + assert_eq!( + err, "gam_network_id must not be empty", + "should report the blank network id" + ); + } + #[test] fn validate_runtime_allows_blank_network_id_when_no_slots_configured() { // An empty slot list disables the feature, so the id is never rendered. From b7f491c76ab7b53c2469b62376b60045bde591f1 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 4 Aug 2026 00:22:50 +0530 Subject: [PATCH 15/21] Bound dynamic GAM slot rendering --- .../src/creative_opportunities.rs | 244 +++++++++++++++--- crates/trusted-server-core/src/publisher.rs | 109 ++++++-- 2 files changed, 305 insertions(+), 48 deletions(-) diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index 09ceb5658..3c18a35bc 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -14,6 +14,9 @@ use crate::auction::types::{AdFormat, AdSlot, MediaType}; use crate::price_bucket::PriceGranularity; use crate::settings::vec_from_seq_or_map; +const MAX_DYNAMIC_GAM_UNIT_PATH_BYTES: usize = 100; +const MAX_SECTION_BYTES: usize = 100; + /// A single parsed segment of a [`gam_unit_path`](CreativeOpportunitySlot::gam_unit_path) template. #[derive(Debug, Clone)] pub(crate) enum UnitTemplatePart { @@ -86,14 +89,62 @@ fn parse_unit_template(raw: &str) -> Result, String> { Ok(parts) } +fn resolved_unit_template_part<'a>( + part: &'a UnitTemplatePart, + gam_network_id: &'a str, + section: &'a str, + slot_id: &'a str, +) -> &'a str { + match part { + UnitTemplatePart::Literal(value) => value, + UnitTemplatePart::NetworkId => gam_network_id, + UnitTemplatePart::Section => section, + UnitTemplatePart::SlotId => slot_id, + } +} + +fn render_dynamic_unit_path( + parts: &[UnitTemplatePart], + gam_network_id: &str, + section: &str, + slot_id: &str, +) -> Option { + let rendered_len = parts.iter().try_fold(0usize, |len, part| { + let value = resolved_unit_template_part(part, gam_network_id, section, slot_id); + len.checked_add(value.len()) + })?; + if rendered_len > MAX_DYNAMIC_GAM_UNIT_PATH_BYTES { + return None; + } + + let mut rendered = String::with_capacity(rendered_len); + for part in parts { + rendered.push_str(resolved_unit_template_part( + part, + gam_network_id, + section, + slot_id, + )); + } + Some(rendered) +} + +fn is_section_char(ch: char) -> bool { + ch.is_ascii_alphanumeric() || ch == '_' || ch == '-' +} + /// Collapses each run of characters outside `[A-Za-z0-9_-]` to a single `_`. /// /// Returns a non-empty string for any non-empty input. fn sanitize_section(segment: &str) -> String { - let mut out = String::with_capacity(segment.len()); + let mut out = String::with_capacity(segment.len().min(MAX_SECTION_BYTES)); let mut in_bad_run = false; - for ch in segment.chars() { - if ch.is_ascii_alphanumeric() || ch == '_' || ch == '-' { + let mut chars = segment.chars(); + while out.len() < MAX_SECTION_BYTES { + let Some(ch) = chars.next() else { + break; + }; + if is_section_char(ch) { out.push(ch); in_bad_run = false; } else if !in_bad_run { @@ -273,11 +324,7 @@ impl CreativeOpportunitiesConfig { .any(CreativeOpportunitySlot::template_uses_section) { match self.section_root.as_deref() { - Some(root) - if !root.is_empty() - && root - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') => {} + Some(root) if !root.is_empty() && root.chars().all(is_section_char) => {} _ => { return Err("section_root is required and must match [A-Za-z0-9_-]+ \ when a gam_unit_path template uses {section}" @@ -286,6 +333,21 @@ impl CreativeOpportunitiesConfig { } } + let configured_section = self.section_root.as_deref().unwrap_or_default(); + for slot in &self.slot { + if slot.template_is_dynamic() + && slot + .render_gam_unit_path(&self.gam_network_id, configured_section) + .is_none() + { + return Err(format!( + "slot `{}` dynamic gam_unit_path must render to at most \ + {MAX_DYNAMIC_GAM_UNIT_PATH_BYTES} UTF-8 bytes using configured values", + slot.id + )); + } + } + Ok(()) } } @@ -526,6 +588,10 @@ impl CreativeOpportunitySlot { /// template. Falls back to `//` only when the slot has no /// [`gam_unit_path`](Self::gam_unit_path) at all. /// + /// Returns `None` when a dynamic template would render beyond the 100-byte + /// GAM unit-path limit. Explicit static paths and the default path retain + /// their pre-template behavior and are not subject to this dynamic limit. + /// /// This is the path-aware replacement for the pre-templating /// `resolved_gam_unit_path(&self, gam_network_id)`. /// @@ -537,29 +603,36 @@ impl CreativeOpportunitySlot { /// re-parses its template on every call — same fallback shape as /// [`matches_path`](Self::matches_path). It must never silently degrade to /// the default path, which would bid against the wrong inventory. + /// Dynamic templates compute their exact UTF-8 byte length with checked + /// arithmetic before allocating the final string, then allocate once at + /// the exact capacity. #[must_use] - pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> String { - let render = |parts: &[UnitTemplatePart]| -> String { - parts - .iter() - .map(|part| match part { - UnitTemplatePart::Literal(s) => s.as_str(), - UnitTemplatePart::NetworkId => gam_network_id, - UnitTemplatePart::Section => section, - UnitTemplatePart::SlotId => self.id.as_str(), - }) - .collect() - }; - + pub fn render_gam_unit_path(&self, gam_network_id: &str, section: &str) -> Option { + let is_dynamic = + |parts: &[UnitTemplatePart]| parts.iter().any(UnitTemplatePart::is_placeholder); match (&self.compiled_unit, &self.gam_unit_path) { - (Some(parts), _) => render(parts), + (Some(parts), _) if is_dynamic(parts) => { + render_dynamic_unit_path(parts, gam_network_id, section, &self.id) + } + (Some(_), Some(raw)) => Some(raw.clone()), + (Some(parts), None) => Some( + parts + .iter() + .map(|part| { + resolved_unit_template_part(part, gam_network_id, section, &self.id) + }) + .collect(), + ), // A malformed template cannot reach a compiled config (startup // rejects it), so on this path use the raw string verbatim — the // pre-templating behaviour — instead of dropping to the default. - (None, Some(raw)) => parse_unit_template(raw) - .map(|parts| render(&parts)) - .unwrap_or_else(|_| raw.clone()), - (None, None) => format!("/{}/{}", gam_network_id, self.id), + (None, Some(raw)) => match parse_unit_template(raw) { + Ok(parts) if is_dynamic(&parts) => { + render_dynamic_unit_path(&parts, gam_network_id, section, &self.id) + } + Ok(_) | Err(_) => Some(raw.clone()), + }, + (None, None) => Some(format!("/{}/{}", gam_network_id, self.id)), } } @@ -956,6 +1029,25 @@ mod tests { assert_eq!(derive_section("/a..b", "home", 0), "a_b"); } + #[test] + fn derive_section_caps_safe_segment_at_one_hundred_ascii_bytes() { + let path = format!("/{}", "a".repeat(150)); + + let section = derive_section(&path, "home", 0); + + assert_eq!( + section, + "a".repeat(100), + "should cap a safe request segment at 100 ASCII bytes" + ); + assert!(section.is_ascii(), "section output should remain ASCII"); + assert_eq!( + section.len(), + 100, + "section should contain exactly 100 bytes" + ); + } + #[test] fn section_for_path_applies_both_policy_knobs() { let mut config = make_config_with_section_template(Some("home")); @@ -1018,7 +1110,39 @@ mod tests { .expect("should compile template"); assert_eq!( slot.render_gam_unit_path("99999", "news"), - "/99999/example/news" + Some("/99999/example/news".to_string()) + ); + } + + #[test] + fn render_gam_unit_path_omits_over_limit_compiled_dynamic_template() { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{section}/{section}".to_string()); + slot.compile_unit_template() + .expect("should compile template"); + + let rendered = slot.render_gam_unit_path("99999", &"a".repeat(60)); + + assert_eq!( + rendered, None, + "a compiled dynamic path over 100 bytes should be omitted" + ); + } + + #[test] + fn render_gam_unit_path_omits_over_limit_raw_dynamic_template() { + let mut slot = make_slot("ad-header-0", vec!["/news/*"]); + slot.gam_unit_path = Some("/{section}/{section}".to_string()); + assert!( + slot.compiled_unit.is_none(), + "test should exercise the raw parsing fallback" + ); + + let rendered = slot.render_gam_unit_path("99999", &"a".repeat(60)); + + assert_eq!( + rendered, None, + "a raw dynamic path over 100 bytes should be omitted" ); } @@ -1030,7 +1154,8 @@ mod tests { .expect("should compile (no template)"); assert_eq!( slot.render_gam_unit_path("99999", "ignored"), - "/99999/sidebar" + Some("/99999/sidebar".to_string()), + "an absent template should retain the default path behavior" ); } @@ -1042,7 +1167,24 @@ mod tests { .expect("should compile static template"); assert_eq!( slot.render_gam_unit_path("99999", "news"), - "/99999/example/homepage" + Some("/99999/example/homepage".to_string()) + ); + } + + #[test] + fn render_gam_unit_path_preserves_over_limit_static_template() { + let static_path = format!("/{}", "a".repeat(100)); + let mut slot = make_slot("atf", vec!["/"]); + slot.gam_unit_path = Some(static_path.clone()); + slot.compile_unit_template() + .expect("should compile static template"); + + let rendered = slot.render_gam_unit_path("99999", "news"); + + assert_eq!( + rendered, + Some(static_path), + "an explicit static path should retain pre-template behavior" ); } @@ -1086,6 +1228,30 @@ mod tests { .expect("should accept valid section_root"); } + #[test] + fn validate_runtime_rejects_dynamic_template_over_limit_with_configured_root() { + let root = "a".repeat(60); + let mut config = make_config_with_section_template(Some(&root)); + config.slot[0].gam_unit_path = Some("/{section}/{section}".to_string()); + config.compile_slots(); + config + .compile_unit_templates() + .expect("templates should compile"); + + let err = config + .validate_runtime() + .expect_err("should reject a configured dynamic path over 100 bytes"); + + assert!( + err.contains("ad-header-0"), + "error should identify the over-limit slot, got: {err}" + ); + assert!( + err.contains("100"), + "error should identify the dynamic path byte limit, got: {err}" + ); + } + #[test] fn render_gam_unit_path_honours_raw_template_without_compiled_cache() { // A slot deserialized straight from JSON (or built by a test helper) @@ -1104,7 +1270,7 @@ mod tests { ); assert_eq!( slot.render_gam_unit_path("99999", "news"), - "/99999/example/news", + Some("/99999/example/news".to_string()), "uncompiled slot should still substitute placeholders" ); } @@ -1115,11 +1281,25 @@ mod tests { slot.gam_unit_path = Some("/99999/example/homepage".to_string()); assert_eq!( slot.render_gam_unit_path("99999", "news"), - "/99999/example/homepage", + Some("/99999/example/homepage".to_string()), "uncompiled static path should render verbatim, not the default" ); } + #[test] + fn render_gam_unit_path_preserves_malformed_raw_template() { + let mut slot = make_slot("atf", vec!["/"]); + slot.gam_unit_path = Some("/{unknown}".to_string()); + + let rendered = slot.render_gam_unit_path("99999", "news"); + + assert_eq!( + rendered, + Some("/{unknown}".to_string()), + "a malformed raw template should retain direct-caller compatibility" + ); + } + #[test] fn validate_runtime_requires_section_root_for_uncompiled_template() { // `template_uses_section` must read the raw template, otherwise an @@ -1297,7 +1477,7 @@ mod tests { ); assert_eq!( matched[0].render_gam_unit_path("123456789", &derive_section(path, "home", 0)), - expected, + Some(expected.to_string()), "`{path}` should render the documented unit path" ); } diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index 76e365771..e0b8180e2 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -3334,17 +3334,14 @@ pub(crate) fn build_empty_bids_script() -> String { /// [`handle_page_bids`] (SPA navigation) so the slot wire shape has a single /// definition and the two paths cannot silently diverge. Property names match /// what the client-side TSJS bundle expects: `gam_unit_path`, `div_id`, -/// `formats`, and `targeting`. +/// `formats`, and `targeting`. Returns `None` when the slot's dynamic GAM unit +/// path exceeds its rendering limit. pub(crate) fn build_slot_json( slot: &crate::creative_opportunities::CreativeOpportunitySlot, co_config: &crate::creative_opportunities::CreativeOpportunitiesConfig, - request_path: &str, -) -> serde_json::Value { - // `{section}` derives from the same raw path `page_patterns` matched - // against; `section_root` covers the no-segment case (`/`) and paths shorter - // than the configured `section_segment`. - let section = co_config.section_for_path(request_path); - let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, §ion); + section: &str, +) -> Option { + let gam_path = slot.render_gam_unit_path(&co_config.gam_network_id, section)?; let div_id = slot.resolved_div_id(); let formats: Vec = slot .formats @@ -3356,13 +3353,13 @@ pub(crate) fn build_slot_json( .iter() .map(|(k, v)| (k.clone(), serde_json::Value::String(v.clone()))) .collect(); - serde_json::json!({ + Some(serde_json::json!({ "id": slot.id, "gam_unit_path": gam_path, "div_id": div_id, "formats": formats, "targeting": targeting, - }) + })) } /// Build the `tsjs.adSlots` `