Skip to content

Commit 207795c

Browse files
authored
20260518-053619 Review fixes for #680 (#708)
* Tidy small review nits across the new auction surface Addresses review findings on #680: - P2-18 / price_bucket: reject NaN/Inf cpm up front before the (x * 100.0).floor() as u64 cast (Rust's NaN-cast behaviour is only safe by convention, not contract). Add test coverage. - P2-24 / publisher.rs::write_bids_to_state: drop the per-request log line from INFO to DEBUG so production logs don't spam a slot list on every page request. - P2-25 / publisher.rs::build_bids_script / build_ad_slots_script: serde_json::to_string of a Map / Vec is infallible -- use expect("should be infallible") instead of unwrap_or_else with a silent fallback that would mask any future bug. - P2-15 / prebid: drop the dead PrebidIntegrationConfig::suppress_nurl field -- declared but never read anywhere in the codebase. The no-op test it carried goes with it. * Extract GPT bootstrap script and guard double enableServices Addresses review findings on #680: - P2-2: the inline `__tsAdInit` bootstrap injected at <head> called googletag.pubads().enableSingleRequest() and googletag.enableServices() unconditionally. The TS bundle's later-installed version guards both with a `__tsServicesEnabled` flag — the inline version did not, so the publisher's own GPT init code (or an upgrade where the bundle loads before the bids script runs) caused double-enable + duplicate refresh(), producing duplicate ad requests on every load. Now both inline and bundle converge on the same flag and only invoke `refresh(newSlots)` for the slots this pass actually defined, never the global slot list. - P2-26: the bootstrap source moves out of a concat!() literal block in head_inserts() into a syntax-highlighted gpt_bootstrap.js file pulled in via include_str!. The Rust side keeps a single named constant, GPT_BOOTSTRAP_JS, so future edits diff cleanly. Adds a regression test that asserts the guard flag is present and that unbounded refresh() is gone. * Harden auction-orchestrator state cleanup Addresses review findings on #680: - P2-6: APS provider held its per-request slot_id_map across request boundaries when the same Wasm instance was reused (the mock provider already cleared its bid_index, APS did not). parse_response now `std::mem::take`s the map so it can never carry over to a subsequent request. - P2-7: apply_floor_prices used to silently pass bids with `price=None` through the floor filter. Today both production callers decode/skip None before calling, so the pass-through was dead code that would, if revived, cause winning_bids.len() to overcount what build_bid_map ships to the client. Drop the None branch and update the existing test to pin the new contract: callers must decode prices first. * Harden /__ts/page-bids and creative-opportunities loading Addresses review findings on #680: - P2-4: /__ts/page-bids ran the full SSP auction for every request without gating crawlers or prefetches the way the publisher path does, exposing partner request quota to client-side spraying. Apply the same is_bot / is_prefetch gate handle_publisher_request uses: slots are still returned (so HTML structure is unchanged) but the auction is short-circuited. New regression tests cover both gates. - P2-5 + P2-13: the adapter previously parsed CREATIVE_OPPORTUNITIES_TOML on every request and `expect("should parse...")` on failure, so a malformed embedded TOML (CI-bypassed binary patch, future schema change, anything build.rs didn't catch) would panic every request. Parse it lazily once per Wasm instance via LazyLock; on parse failure log an error and fall back to the documented "empty slots file = feature disabled" state instead of panicking. * Consolidate HTML stream processor + auction helpers Addresses review findings on #680: - P2-3 + P2-16: create_html_stream_processor previously built HtmlProcessorConfig inline, bypassing HtmlProcessorConfig::from_settings. A future edit to from_settings (e.g. to read a new flag from Settings) would silently miss the streaming-with-auction-hold path. Now goes through from_settings, with a new with_ad_state(...) builder method that layers the ad_slots_script / ad_bids_state fields on top. The `_settings` argument on create_html_stream_processor is now actually used and is no longer underscore-prefixed. - P2-17: the auction debug-comment prepend logic was duplicated between one_behind_loop (path=stream) and the BufferedProcessed arm (path=buffered). Extracted as `prepend_auction_debug_comment(label, result, state)` so the single source of truth gets one definition and the only difference between paths is the label string. - P2-14: build_auction_request was at the project's 7-argument cap. Bundled (matched_slots, request_path, co_config) into a new MatchedSlotsContext struct — the three fields always travel together anyway. The function now takes 5 args, leaving headroom for future per-request inputs without breaking the project rule. * Document AuctionContext request contract and pin mediator placeholder Addresses review finding P2-1 on #680: collect_dispatched_auction and the publisher-side collectors built fastly::Request::get("https://placeholder.invalid/") on the fly and stuffed it into AuctionContext.request before invoking the mediator. That worked today only because the current mediator (adserver_mock) does not read request headers. A future PBS-as-mediator change would silently lose DNT, client IP, UA, etc., because the placeholder has none of them. The structural fix that surfaces the contract: - AuctionContext::request now carries a thorough doc-comment that explicitly distinguishes the dispatch path (real client request, all headers available) from the collect path (synthetic placeholder, no client headers — mediators MUST snapshot at dispatch time if they need them). - MEDIATOR_PLACEHOLDER_URL is a single named const so the three inline `"https://placeholder.invalid/"` literals across orchestrator.rs and publisher.rs converge on one source of truth. Tests / debug-asserts can compare against it. - make_collect_context now debug_asserts that the placeholder argument matches the canonical URL, so any future caller that accidentally forwards a real client request through the collect path fails loudly in debug builds instead of triggering an invisible header-loss bug. A full snapshot-headers refactor (so mediators can read real client headers across the await) is tracked as follow-up; this PR makes the existing contract impossible to violate accidentally. * Apply cargo fmt to fix-up commits * Forward ts-eids cookie through /auction endpoint Addresses pass-2 review finding P3-1 on #680: handle_auction read the request's cookie jar for consent purposes but never threaded it into the AuctionRequest, so the Extended User IDs from the `ts-eids` cookie were dropped on the floor. The publisher-page and /__ts/page-bids paths both call parse_ts_eids_cookie(cookie_jar.as_ref()); the /auction endpoint now does the same so programmatic callers (slim-Prebid, native apps, server-to-server integrations) get parity instead of silently losing identity data. * Pass-3 review fixes for #680: bot helpers, dead arg, pre-compiled globs Addresses pass-2 review findings on #680: - Dedupe the bot-UA and prefetch checks. handle_publisher_request and handle_page_bids both inlined the same 5-entry crawler list and the sec-purpose/purpose header check. Promoted to two pub(crate) helpers in publisher.rs (is_bot_user_agent, is_prefetch_request) backed by a single BOT_USER_AGENT_FRAGMENTS const, so the two paths can't drift. - Drop the dead gam_network_id argument on CreativeOpportunitySlot::to_ad_slot. The parameter was already being discarded via `let _ = gam_network_id;`. Removing it also drops the now-unused co_config field from MatchedSlotsContext. - Pre-compile slot glob patterns once per Wasm instance. Each call to matches_path previously ran Pattern::new (per pattern, per slot, per request). The slots live in the adapter's LazyLock-cached CreativeOpportunitiesFile, so adding a #[serde(skip)] compiled_patterns cache populated by CreativeOpportunitiesFile::compile() at file-load time turns matches_path into a Vec<Pattern>::iter().any() lookup on the hot path. The fallback (compile-per-call) is preserved for hand-built slots in tests. Two new regression tests pin the cache. * Address deferred perf findings from PR #680 review P2-8: Promote STREAM_CHUNK_SIZE (8192) to module scope and use it for both the brotli Decompressor and CompressorWriter internal buffers, aligning them with the read buffer in one_behind_loop. P2-9: Replace RwLock<Option<String>> with Mutex<Option<String>> for ad_bids_state across publisher.rs and html_processor.rs. Single-threaded WASM gains no parallelism from RwLock; Mutex is simpler and avoids the reader-writer bookkeeping the workload cannot use. P2-10: Rewrite html_escape_for_script as a single-pass char loop with one pre-allocated String instead of seven sequential String::replace allocations. All existing escape tests pass unchanged.
1 parent 6c31b66 commit 207795c

12 files changed

Lines changed: 694 additions & 270 deletions

File tree

crates/trusted-server-adapter-fastly/src/main.rs

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,39 @@ use crate::platform::{build_runtime_services, open_kv_store, UnavailableKvStore}
4242

4343
const CREATIVE_OPPORTUNITIES_TOML: &str = include_str!("../../../creative-opportunities.toml");
4444

45+
/// Parses the embedded `creative-opportunities.toml` at most once per Wasm
46+
/// instance.
47+
///
48+
/// On parse failure, logs an error and falls back to an empty
49+
/// [`CreativeOpportunitiesFile`] — i.e. the documented "feature disabled"
50+
/// state — instead of panicking the request hot path. The build-time
51+
/// validator in `crates/trusted-server-core/build.rs` catches every realistic
52+
/// authoring mistake; this fallback exists so a CI-bypassed binary patch or a
53+
/// future schema change can't take the entire fleet down with a per-request
54+
/// panic.
55+
static SLOTS_FILE: std::sync::LazyLock<
56+
trusted_server_core::creative_opportunities::CreativeOpportunitiesFile,
57+
> = std::sync::LazyLock::new(|| {
58+
let mut file = match toml::from_str::<
59+
trusted_server_core::creative_opportunities::CreativeOpportunitiesFile,
60+
>(CREATIVE_OPPORTUNITIES_TOML)
61+
{
62+
Ok(file) => file,
63+
Err(err) => {
64+
log::error!(
65+
"creative-opportunities.toml failed to parse at startup; \
66+
falling back to an empty slots file (server-side ad-slot \
67+
templates disabled): {err}"
68+
);
69+
trusted_server_core::creative_opportunities::CreativeOpportunitiesFile::default()
70+
}
71+
};
72+
// Pre-compile glob patterns once so per-request `matches_path` doesn't
73+
// re-invoke `Pattern::new` on every page hit.
74+
file.compile();
75+
file
76+
});
77+
4578
/// Entry point for the Fastly Compute program.
4679
///
4780
/// Uses an undecorated `main()` with `Request::from_client()` instead of
@@ -94,9 +127,7 @@ fn main() {
94127
}
95128
};
96129

97-
let slots_file: trusted_server_core::creative_opportunities::CreativeOpportunitiesFile =
98-
toml::from_str(CREATIVE_OPPORTUNITIES_TOML)
99-
.expect("should parse creative-opportunities.toml");
130+
let slots_file = &*SLOTS_FILE;
100131

101132
let integration_registry = match IntegrationRegistry::new(&settings) {
102133
Ok(r) => r,
@@ -121,7 +152,7 @@ fn main() {
121152
&orchestrator,
122153
&integration_registry,
123154
&runtime_services,
124-
&slots_file,
155+
slots_file,
125156
req,
126157
)) {
127158
response.send_to_client();

crates/trusted-server-core/src/auction/endpoints.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use fastly::{Request, Response};
66
use crate::auction::formats::AdRequest;
77
use crate::compat;
88
use crate::consent;
9-
use crate::cookies::handle_request_cookies;
9+
use crate::cookies::{handle_request_cookies, parse_ts_eids_cookie};
1010
use crate::edge_cookie::get_or_generate_ec_id_from_http_request;
1111
use crate::error::TrustedServerError;
1212
use crate::platform::RuntimeServices;
@@ -125,8 +125,8 @@ pub async fn handle_auction(
125125
.map(|_| services.kv_store()),
126126
});
127127

128-
// Convert tsjs request format to auction request
129-
let auction_request = convert_tsjs_to_auction_request(
128+
// Convert tsjs request format to auction request.
129+
let mut auction_request = convert_tsjs_to_auction_request(
130130
&body,
131131
settings,
132132
services,
@@ -135,6 +135,10 @@ pub async fn handle_auction(
135135
&ec_id,
136136
geo,
137137
)?;
138+
// Forward Extended User IDs from the `ts-eids` cookie so programmatic
139+
// callers (slim-Prebid, native apps) get parity with the publisher /
140+
// page-bids paths, both of which already do this.
141+
auction_request.user.eids = parse_ts_eids_cookie(cookie_jar.as_ref());
138142

139143
// Create auction context
140144
let context = AuctionContext {

crates/trusted-server-core/src/auction/orchestrator.rs

Lines changed: 43 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -540,32 +540,29 @@ impl AuctionOrchestrator {
540540
}
541541

542542
let starting_count = winning_bids.len();
543-
winning_bids.retain(|slot_id, bid| match floor_prices.get(slot_id) {
544-
Some(floor) => {
545-
// price=None means the SSP returned an encoded price (e.g. APS amznbid).
546-
// In the parallel-only path this bid cannot yet be floor-checked; it passes
547-
// through and will be decoded (and re-checked) by the mediation layer.
548-
// In the mediation path, mediation decodes prices before calling this
549-
// function, so any bid still carrying price=None is dropped upstream.
550-
match bid.price {
551-
Some(price) if price >= *floor => true,
552-
Some(_) => {
553-
log::info!(
554-
"Dropping winning bid below floor price for slot '{}'",
555-
slot_id
556-
);
557-
false
558-
}
559-
None => {
560-
log::debug!(
561-
"Passing encoded-price bid for slot '{}' - price not yet decoded",
562-
slot_id
563-
);
564-
true
565-
}
566-
}
543+
winning_bids.retain(|slot_id, bid| match (floor_prices.get(slot_id), bid.price) {
544+
(Some(floor), Some(price)) if price >= *floor => true,
545+
(Some(_), Some(_)) => {
546+
log::info!(
547+
"Dropping winning bid below floor price for slot '{}'",
548+
slot_id
549+
);
550+
false
567551
}
568-
None => true,
552+
(_, None) => {
553+
// Any caller that needs to keep an undecoded (encoded-price)
554+
// bid must decode it *before* invoking this function — both
555+
// `select_winning_bids` and the mediator path already do.
556+
// Letting `None`-price bids through here would cause
557+
// `winning_bids.len()` to overcount what `build_bid_map`
558+
// downstream is willing to emit, so they get dropped instead.
559+
log::debug!(
560+
"Dropping bid for slot '{}' - no decoded price (caller must decode before apply_floor_prices)",
561+
slot_id
562+
);
563+
false
564+
}
565+
(None, Some(_)) => true,
569566
});
570567

571568
if winning_bids.len() != starting_count {
@@ -872,7 +869,14 @@ impl AuctionOrchestrator {
872869
remaining,
873870
mediator.timeout_ms(),
874871
);
875-
let placeholder = fastly::Request::get("https://placeholder.invalid/");
872+
// The mediator runs on the collect path. See the doc-comment on
873+
// `AuctionContext::request`: the real client request was already
874+
// consumed by `send_async` during dispatch, so we substitute a
875+
// canonical placeholder URL. Any future mediator that needs real
876+
// client headers must snapshot them at dispatch time onto
877+
// `DispatchedAuction` rather than reading `context.request` here.
878+
let placeholder =
879+
fastly::Request::get(crate::auction::types::MEDIATOR_PLACEHOLDER_URL);
876880
let mediator_context = AuctionContext {
877881
settings: context.settings,
878882
request: &placeholder,
@@ -1256,9 +1260,14 @@ mod tests {
12561260
}
12571261

12581262
#[test]
1259-
fn test_apply_floor_prices_allows_none_prices_for_encoded_bids() {
1260-
// Test that bids with None prices (APS-style) pass through floor pricing
1261-
// This is correct behavior for parallel-only strategy where mediation happens later
1263+
fn test_apply_floor_prices_drops_bids_with_undecoded_price() {
1264+
// Bids that reach apply_floor_prices with `price=None` cannot have a
1265+
// floor compared against them — and they would not survive downstream
1266+
// (build_bid_map filters them) — so apply_floor_prices drops them so
1267+
// the count it reports matches what eventually ships to the client.
1268+
// Both production paths (select_winning_bids and the mediator filter)
1269+
// already decode/skip None prices before calling this function; this
1270+
// test pins the contract.
12621271
let orchestrator = AuctionOrchestrator::new(AuctionConfig::default());
12631272
let mut floor_prices = HashMap::new();
12641273
floor_prices.insert("slot-1".to_string(), 1.00);
@@ -1268,7 +1277,7 @@ mod tests {
12681277
"slot-1".to_string(),
12691278
Bid {
12701279
slot_id: "slot-1".to_string(),
1271-
price: None, // APS bid with encoded price
1280+
price: None,
12721281
currency: "USD".to_string(),
12731282
creative: Some("<div>Ad</div>".to_string()),
12741283
adomain: None,
@@ -1289,25 +1298,15 @@ mod tests {
12891298
},
12901299
);
12911300

1292-
// Apply floor pricing - should pass through with None price
12931301
let filtered = orchestrator.apply_floor_prices(winning_bids, &floor_prices);
12941302

1295-
assert_eq!(
1296-
filtered.len(),
1297-
1,
1298-
"APS bid with None price should pass through floor check"
1299-
);
13001303
assert!(
1301-
filtered.contains_key("slot-1"),
1302-
"Slot-1 should still be present"
1304+
filtered.is_empty(),
1305+
"bid with None price should be dropped by apply_floor_prices"
13031306
);
13041307
assert!(
1305-
filtered
1306-
.get("slot-1")
1307-
.expect("slot-1 should be present")
1308-
.price
1309-
.is_none(),
1310-
"Price should still be None (not decoded yet)"
1308+
!filtered.contains_key("slot-1"),
1309+
"slot-1 should not survive when its bid has no decoded price"
13111310
);
13121311
}
13131312

crates/trusted-server-core/src/auction/types.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,29 @@ pub struct SiteInfo {
115115
}
116116

117117
/// Context passed to auction providers.
118+
///
119+
/// # The `request` field is path-dependent
120+
///
121+
/// `request` carries the **real downstream client request** in the dispatch
122+
/// path ([`AuctionOrchestrator::run_auction`][run] and
123+
/// [`dispatch_auction`][dispatch]). Providers there can read client headers
124+
/// (DNT, User-Agent, cookies, X-* customs) directly off it.
125+
///
126+
/// In the **collect path** ([`collect_dispatched_auction`][collect]) the
127+
/// mediator is invoked with a synthetic placeholder request
128+
/// (`https://placeholder.invalid/`), because the real client request has
129+
/// already been consumed by `send_async` during dispatch and the host pipeline
130+
/// can't lend it across the `.await`. **Mediators must not depend on reading
131+
/// client state from `context.request`** — the placeholder has none of the
132+
/// real headers. If a future mediator needs that data, snapshot it into a new
133+
/// field on this struct at dispatch time and stash it on the
134+
/// [`DispatchedAuction`] token so collect can attach it to the mediator's
135+
/// context. See <https://github.com/IABTechLab/trusted-server/issues/680>
136+
/// (P2-1) for the open follow-up.
137+
///
138+
/// [run]: crate::auction::AuctionOrchestrator::run_auction
139+
/// [dispatch]: crate::auction::AuctionOrchestrator::dispatch_auction
140+
/// [collect]: crate::auction::AuctionOrchestrator::collect_dispatched_auction
118141
pub struct AuctionContext<'a> {
119142
pub settings: &'a Settings,
120143
pub request: &'a Request,
@@ -127,6 +150,12 @@ pub struct AuctionContext<'a> {
127150
pub services: &'a RuntimeServices,
128151
}
129152

153+
/// URL used by the orchestrator when invoking a mediator from the collect
154+
/// path. Providers can `debug_assert` against this value to catch a mediator
155+
/// that has accidentally started depending on `context.request` carrying real
156+
/// client headers.
157+
pub const MEDIATOR_PLACEHOLDER_URL: &str = "https://placeholder.invalid/";
158+
130159
/// Response from a single auction provider.
131160
#[derive(Debug, Clone, Serialize, Deserialize)]
132161
pub struct AuctionResponse {

0 commit comments

Comments
 (0)