Skip to content

Suppress fabricated empty Prebid bidder params #910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prk-Jr wants to merge 3 commits into
base: feat/ssat-render-inline-creative
Choose a base branch
from
Open

Suppress fabricated empty Prebid bidder params #910

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef61953
Suppress fabricated empty Prebid bidder params
prk-Jr Jul 14, 2026
bfea103
Prefer direct bidder params over fabricated-empty in mixed slots
prk-Jr Jul 16, 2026
ef7298c
Merge remote-tracking branch 'origin/feat/ssat-render-inline-creative…
prk-Jr Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
241 changes: 224 additions & 17 deletions crates/trusted-server-core/src/integrations/prebid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::sync::Arc;
use std::time::Duration;

Expand Down Expand Up @@ -1423,10 +1423,33 @@ impl PrebidAuctionProvider {
// Only pass through keys that are known PBS bidders — skip provider-specific
// keys like "aps" which belong to their own separate auction provider.
let mut bidder: HashMap<String, Json> = HashMap::new();
// Bidders the publisher explicitly supplied — including an
// explicit empty `{}`. A configured bidder that exists only
// because `expand_trusted_server_bidders` fabricated an empty
// params object is NOT explicit and must not ship as
// `"bidder": {}` (which PBS rejects).
let mut explicit_bidders: HashSet<String> = HashSet::new();
for (name, params) in &slot.bidders {
if name == TRUSTED_SERVER_BIDDER {
bidder.extend(expand_trusted_server_bidders(&self.config.bidders, params));
if let Some(per_bidder) =
params.get(BIDDER_PARAMS_KEY).and_then(Json::as_object)
{
explicit_bidders.extend(per_bidder.keys().cloned());
}
// Fill in trusted-server-expanded bidders without
// clobbering a direct bidder entry. `slot.bidders` is a
// HashMap, so this branch can run after a direct entry;
// `extend` would then overwrite valid direct params with a
// fabricated empty `{}`, which `explicit_bidders` would
// wrongly preserve past the drop below. `or_insert` makes
// direct params win regardless of iteration order.
for (bidder_name, bidder_params) in
expand_trusted_server_bidders(&self.config.bidders, params)
{
bidder.entry(bidder_name).or_insert(bidder_params);
}
} else if self.config.bidders.iter().any(|b| b == name) {
explicit_bidders.insert(name.clone());
bidder.insert(name.clone(), params.clone());

@aram356 aram356 Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 wrench — The merge is asymmetric: an empty direct entry still wins over real params.

bfea103d9 fixed the trustedServer branch with or_insert, but this direct branch still uses an unconditional insert. A slot carrying a direct kargo: {} and a trustedServer entry whose bidderParams supplies real kargo params deterministically discards the real params and ships "bidder": {"kargo": {}} — the exact invalid payload this PR exists to remove, now on every run rather than intermittently. I verified it against this head:

PROBE direct-empty + ts-real => {"bidder":{"kargo":{}}}
PROBE LOST PARAMS on iter 0: {}

Note the comment at line 1444-1446 claims or_insert makes direct params win "regardless of iteration order". That's true but incomplete, and no reordering of the current single loop fixes it: if the direct branch runs first it inserts {} into a vacant entry, and or_insert then skips. The two sources have to be separated before merging:

// Merge: start from the expansion, then let direct entries win — but never
// let an empty `{}` clobber real params from the other source.
let mut bidder = expanded;
for (name, params) in direct {
    let keep_existing = is_empty_object(&params)
        && bidder.get(&name).is_some_and(|existing| !is_empty_object(existing));
    if !keep_existing {
        bidder.insert(name, params);
    }
}

In fairness on reachability: shipped JS never emits a direct entry and a trustedServer entry on the same slot — the adapter folds and filters at index.ts:576-591 — so this needs a hand-crafted POST to the public /auction endpoint. That is the same reachability as the mirror-image case already fixed in this PR, which is why I'd rather see both closed than one.

} else if name != "aps" {
// `aps` is intentionally handled by its own provider. Any
Expand All @@ -1442,16 +1465,32 @@ impl PrebidAuctionProvider {
}
}

// When no inline PBS bidder params exist (e.g. creative-opportunity slots
// whose PBS params live in stored requests), tell PBS to resolve bidder
// config from the stored request keyed by this slot ID.
// Apply canonical and compatibility-derived rules in normalized
// order. An override rule can populate a bidder that arrived with
// empty params, promoting a fabricated empty into a valid bidder.
for (name, params) in &mut bidder {
self.bid_param_override_engine
.apply(BidParamOverrideFacts { bidder: name, zone }, params);
}

// Drop bidders that are still an empty object after overrides and
// were not explicitly supplied. Shipping `"bidder": {}` makes PBS
// reject the imp; an explicit empty object is preserved so genuine
// publisher misconfiguration stays visible.
bidder.retain(|name, params| {
let is_empty_object = params.as_object().is_some_and(serde_json::Map::is_empty);

@aram356 aram356 Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking — Non-object params bypass the drop entirely.

as_object().is_some_and(is_empty) only catches empty objects. A Json::Null params value is not an object, so it survives and ships as "bidder": {"kargo": null} — presumably just as invalid to PBS as {}. Verified against this head:

PROBE direct-null-params => {"bidder":{"kargo":null}}

This is reachable without a malformed request: BidConfig.params carries #[serde(default)] (crates/trusted-server-core/src/auction/formats.rs:71-72), so a POST that simply omits params deserializes to Null rather than {}. If the intent is "anything that isn't a usable params object shouldn't ship", the predicate probably wants to treat a non-object as ineligible too — though that's a slightly wider behavior change, so I'd rather flag it than prescribe it.

!is_empty_object || explicit_bidders.contains(name)

@aram356 aram356 Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 wrench — Preserving an explicit {} still ships the payload PBS rejects.

PBS cannot see provenance: "bidder": {"kargo": {}} is byte-identical on the wire whether the empty object was fabricated by expand_trusted_server_bidders or supplied by the publisher. This retain rule drops the first and ships the second, but PBS rejects both — so the PR's stated goal ("never emit \"bidder\": {}") is only half-enforced. The test at prebid.rs:6077 asserts this shape ships.

This is reachable from the primary shipped path, not a corner case. The Prebid adapter folds every server-side bid into bidderParams[bid.bidder] = bid.params ?? {} (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:584), so any publisher ad unit with bids: [{bidder: 'kargo', params: {}}] marks kargo explicit and ships {}. The JS test at index.test.ts:541 pins that shape as { appnexus: {} }.

The blast radius is wider than one imp: parse_response (prebid.rs:2157-2166) collapses any non-2xx into AuctionResponse::error, zeroing bids for every slot in the auction. One publisher's empty params can take down otherwise-healthy slots.

The "misconfiguration stays visible" rationale doesn't hold in practice. When PBS rejects, the operator sees only Prebid returned non-success status: 400 — the response body is discarded unless trace logging is enabled. That is precisely the opaque intermittent error this PR set out to eliminate. Dropping it and logging makes the misconfiguration visible where someone can actually act on it:

bidder.retain(|name, params| {
    if is_empty_object(params) {
        log::warn!(
            "prebid: dropping bidder '{}' on slot '{}' — empty params",
            name,
            slot.id
        );
        return false;
    }
    true
});

If preserving explicit empties is a deliberate contract with PBS that I'm missing, could you spell out what makes the explicit case safe on the wire when the fabricated one isn't?

});

// When no eligible PBS bidder params remain (e.g. creative-opportunity
// slots whose PBS params live in stored requests, or a slot whose
// configured bidders all resolved to fabricated empties), tell PBS to
// resolve bidder config from the stored request keyed by this slot ID.
//
// This cannot fire for the client /auction path: the JS adapter
// injects a `trustedServer` entry into every ad unit, so `bidder`
// is only empty for server-side creative-opportunity slots with
// no inline provider params (or when `config.bidders` is empty,
// where PBS previously received an empty bidder map and returned
// no bids — a stored-request miss is the same no-bid outcome).
// This cannot fire for a client /auction slot that carries real

@aram356 aram356 Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking — This "cannot fire" claim isn't accurate.

A client /auction slot can carry real inline params and still hit the stored-request fallback: when the params name a bidder absent from config.bidders, expand_trusted_server_bidders fabricates {} for each configured bidder (prebid.rs:945-954), the retain drops them all, and bidder ends up empty. Verified against this head with config.bidders = ["appnexus"] and inline {"kargo": {"placementId": "kn1"}}:

PROBE real-inline-params-unconfigured-bidder => {"storedrequest":{"id":"ad-header-0"}}

The real params are silently discarded and the slot falls back to a stored request. The param loss itself is pre-existing (the log::debug! drop above), so this isn't a regression — but the comment is new and load-bearing, since it's the stated justification that the fallback can't over-broaden. Worth correcting and pinning with a test, as to_openrtb_falls_back_to_stored_request_when_all_bidders_are_fabricated_empty covers only the empty-bidderParams variant.

// inline params: the JS adapter injects a `trustedServer` entry, and
// any bidder with params survives the drop above. It falls back only
// when nothing eligible remains — the same no-bid outcome as before.
let storedrequest = if bidder.is_empty() {
Some(ImpStoredRequest {
id: slot.id.clone(),
Expand All @@ -1460,12 +1499,6 @@ impl PrebidAuctionProvider {
None
};

// Apply canonical and compatibility-derived rules in normalized order.
for (name, params) in &mut bidder {
self.bid_param_override_engine
.apply(BidParamOverrideFacts { bidder: name, zone }, params);
}

Some(Imp {
id: Some(slot.id.clone()),
banner: Some(Banner {
Expand Down Expand Up @@ -5952,6 +5985,180 @@ set = { placementId = "explicit_header" }
);
}

#[test]
fn to_openrtb_drops_fabricated_empty_bidder_params() {
// config.bidders lists three, but the slot supplies inline params only
// for kargo. Without a matching override, triplelift and criteo would
// expand to empty `{}` objects — invalid bidder entries PBS rejects.
// They must be dropped; the valid kargo bidder must still ship.
let config = parse_prebid_toml(
r#"
[integrations.prebid]
enabled = true
server_url = "https://prebid.example"
bidders = ["kargo", "triplelift", "criteo"]
"#,
);

let slot = make_ts_slot(
"ad-header-0",
&json!({ "kargo": { "placementId": "kn1" } }),
None,
);
let request = make_auction_request(vec![slot]);

let ortb = call_to_openrtb(config, &request);
let params = bidder_params(&ortb);

assert_eq!(
params["kargo"]["placementId"], "kn1",
"should keep the valid inline bidder"
);
assert!(
!params.contains_key("triplelift"),
"should drop a fabricated empty bidder with no inline params or override"
);
assert!(
!params.contains_key("criteo"),
"should drop a fabricated empty bidder with no inline params or override"
);
}

#[test]
fn to_openrtb_prefers_direct_bidder_params_over_fabricated_empty() {
// A slot can carry BOTH a direct bidder entry (valid inline params) and a
// `trustedServer` entry whose `bidderParams` omits that bidder. Since
// `slot.bidders` is a HashMap, the trustedServer expansion — which
// fabricates an empty `{}` for the omitted bidder — could run after the
// direct entry and overwrite its valid params; the direct entry marks the
// bidder explicit, so the empty would survive the drop and PBS would reject
// the imp. Direct params must win regardless of iteration order.
//
// Looped because the HashMap iteration order is randomized per map, so a
// single run could miss the overwrite ordering; the fix must hold every
// time.
let config = parse_prebid_toml(
r#"
[integrations.prebid]
enabled = true
server_url = "https://prebid.example"
bidders = ["kargo", "triplelift"]
"#,
);

for _ in 0..64 {
let slot = make_slot(
"ad-header-0",
HashMap::from([
("kargo".to_string(), json!({ "placementId": "direct-1" })),
(
TRUSTED_SERVER_BIDDER.to_string(),
json!({ BIDDER_PARAMS_KEY: { "triplelift": { "inventoryCode": "tl-1" } } }),
),
]),
);
let request = make_auction_request(vec![slot]);

let ortb = call_to_openrtb(config.clone(), &request);
let params = bidder_params(&ortb);

assert_eq!(
params["kargo"]["placementId"], "direct-1",
"direct bidder params must win over a fabricated empty from trustedServer expansion"
);
assert_eq!(
params["triplelift"]["inventoryCode"], "tl-1",
"trustedServer-supplied bidder params must still ship"
);
}
}

#[test]
fn to_openrtb_preserves_an_explicitly_empty_bidder() {
// A publisher-supplied empty `{}` is a real (if misconfigured) signal and
// must survive so the misconfiguration stays visible — unlike a fabricated
// empty, which is dropped.
let config = parse_prebid_toml(
r#"
[integrations.prebid]
enabled = true
server_url = "https://prebid.example"
bidders = ["kargo"]
"#,
);

let slot = make_ts_slot("ad-header-0", &json!({ "kargo": {} }), None);
let request = make_auction_request(vec![slot]);

let ortb = call_to_openrtb(config, &request);
let params = bidder_params(&ortb);

assert_eq!(
params["kargo"],
json!({}),
"should preserve an explicitly supplied empty bidder object"
);
}

#[test]
fn to_openrtb_keeps_a_fabricated_bidder_that_an_override_populates() {
// criteo has no inline params (fabricated empty), but an override rule
// fills it — so it is valid and must ship, not be dropped.
let config = parse_prebid_toml(
r#"
[integrations.prebid]
enabled = true
server_url = "https://prebid.example"
bidders = ["criteo"]

[integrations.prebid.bid_param_overrides.criteo]
networkId = 99999
"#,
);

let slot = make_ts_slot("ad-header-0", &json!({}), None);
let request = make_auction_request(vec![slot]);

let ortb = call_to_openrtb(config, &request);
let params = bidder_params(&ortb);

assert_eq!(
params["criteo"]["networkId"], 99999,
"override should populate the fabricated empty bidder, keeping it"
);
}

#[test]
fn to_openrtb_falls_back_to_stored_request_when_all_bidders_are_fabricated_empty() {
// config.bidders present, but the slot supplies no inline params and no
// override matches — every configured bidder resolves to a fabricated
// empty and is dropped, leaving PBS to resolve via the stored request.
let config = parse_prebid_toml(
r#"
[integrations.prebid]
enabled = true
server_url = "https://prebid.example"
bidders = ["kargo", "triplelift"]
"#,
);

let slot = make_ts_slot("ad-header-0", &json!({}), None);
let request = make_auction_request(vec![slot]);

let ortb = call_to_openrtb(config, &request);
let ext = ortb.imp[0].ext.as_ref().expect("should have imp ext");
let prebid = ext.get("prebid").expect("should have prebid in ext");

assert!(
prebid.get("bidder").is_none(),
"should drop all fabricated empty bidders"
);
assert_eq!(
prebid["storedrequest"]["id"], "ad-header-0",
"should fall back to stored request when no eligible bidders remain"
);
}

#[test]
fn to_openrtb_skips_aps_key_from_slot_bidders_in_pbs_request() {
let slot = make_slot(
Expand Down
Loading