Suppress fabricated empty Prebid bidder params

[prk-Jr]

Summary

Stacked on #899 (feat/ssat-render-inline-creative) — base branch is that PR's head, not main.

A configured Prebid bidder with no inline params and no matching bid-param override was expanded into "bidder": {} in the OpenRTB request. PBS rejects an empty bidder object, surfacing as the intermittent auction error.

Fix

In to_openrtb (crates/trusted-server-core/src/integrations/prebid.rs):

• Track the bidders the publisher explicitly supplied (including an explicit empty {}).
• Reorder to: build bidder map → apply overrides → drop fabricated empties → decide stored-request.
• Drop any bidder still an empty object after overrides unless it was explicitly supplied.
• Recompute the PBS stored-request fallback after the drop, so a slot whose configured bidders all resolve to fabricated empties falls back to the stored-request/no-bid path instead of shipping invalid bidder objects.

Behavior contract:

• Absent bidder → dropped (never "bidder": {}).
• Publisher-supplied {} → preserved (misconfiguration stays visible).
• Bidder populated by a global/zone override → kept.
• Valid bidders still ship even when a sibling has no params.
• No eligible bidders → stored-request fallback.

The browser half of this defense (prevBidderParams retention on refresh) already shipped; this completes the server half.

Tests

New unit tests in prebid.rs:

• to_openrtb_drops_fabricated_empty_bidder_params
• to_openrtb_preserves_an_explicitly_empty_bidder
• to_openrtb_keeps_a_fabricated_bidder_that_an_override_populates
• to_openrtb_falls_back_to_stored_request_when_all_bidders_are_fabricated_empty

Verification

• cargo fmt --all -- --check ✓
• cargo clippy-fastly ✓
• cargo test-fastly ✓ (1628 core + 99 fastly, 0 failed)
• Other adapters (axum/cloudflare/spin) + clippy variants left to CI.

Test plan

• CI green across all adapters
• Manual: slot with a configured bidder lacking params no longer emits "bidder": {} to PBS

[ChristianPavilonis]

Review Summary

Approved with one non-blocking medium-severity finding documented inline. The primary fabricated-empty suppression, override ordering, and stored-request fallback behavior are covered by focused tests.

Please address the inline mixed-representation precedence case in follow-up work before relying on this path for clients that can send both direct bidder params and trustedServer.bidderParams.

[aram356]

Summary

The fabricated-empty suppression, override ordering, stored-request fallback, and focused tests are otherwise sound, but mixed bidder representations can still recreate the invalid empty bidder object nondeterministically.

Blocking

🔧 wrench

• Mixed bidder representations have nondeterministic precedence: direct bidder params can be overwritten by a fabricated empty object depending on HashMap iteration order (crates/trusted-server-core/src/integrations/prebid.rs:1440).

CI Status

• GitHub integration checks: PASS (4/4)
• fmt: PASS locally
• focused native core tests: PASS (44/44)
• Fastly test runtime: BLOCKED locally by unavailable macOS certificate keychain after successful compilation
• clippy-fastly: BLOCKED locally by a build-script tooling error (Unrecognized option: 'p')
• JS tests: not run; no JS/TS files changed

[aram356]

Summary

The or_insert fix in bfea103d9 does resolve the case flagged in the previous round — I confirmed the new 64-iteration test genuinely fails against the old extend and passes now. The reordering (build → override → drop → decide stored-request) is sound, and tracing the history shows this PR is finishing a migration #680 already intended rather than inventing a new policy: e06af4b0f abandoned "fill config.bidders with empty {}" in favour of the stored-request path a year ago, and that intent only leaked because expand_trusted_server_bidders fabricates the same empties one layer up. That evidence retired my initial concern that broadening the fallback was risky.

What holds the PR up is that the fix is one-directional, and that the central design choice re-creates the payload the PR exists to eliminate. PBS cannot distinguish a fabricated {} from an explicitly supplied one — they are identical bytes on the wire — so preserving explicit empties keeps emitting "bidder": {} through the primary shipped JS path, and parse_response turns the resulting PBS rejection into an auction-wide bid wipeout rather than a visible misconfiguration signal.

Blocking

🔧 wrench

• Preserving an explicit {} still ships the payload PBS rejects: provenance is invisible to PBS; reachable via the normal Prebid adapter path (params: {} → bidderParams: {kargo: {}}), and a PBS non-2xx zeroes every slot in the auction, not just the offending imp (prebid.rs:1480-1483, prebid.rs:2157-2166).
• The merge is asymmetric — an empty direct entry still wins over real params: the direct branch keeps an unconditional insert, so a direct {} deterministically discards trustedServer-supplied params and emits "bidder": {"kargo": {}} (prebid.rs:1453).

❓ question

• The premise underpinning the whole design is unverified in-repo: there is no test, fixture, or PBS mock anywhere backing either "PBS rejects "bidder": {}" or "an unknown stored-request ID yields a per-imp no-bid rather than a request-wide 400". Both exist only as prose in comments and commit messages. If a stored-request miss is in fact a 400, this trades one whole-auction failure mode for another instead of fixing it. Related: auction/endpoints.rs:60-63 already requires a stored request to exist for every slot code that omits inline params, and this PR widens that set to include all-fabricated-empty slots — operators with config.bidders set but no stored requests provisioned move from one broken state to another. Has the empty-bidder rejection been confirmed against the deployed PBS?

Non-blocking

🤔 thinking

• The new "cannot fire" comment is inaccurate: real inline params for a bidder outside config.bidders do fall back to the stored request, silently dropping those params (prebid.rs:1490-1493).
• Non-object params bypass the drop: null params ship as "bidder": {"kargo": null}; #[serde(default)] on BidConfig.params makes this reachable from a POST that omits params (prebid.rs:1481).

♻️ refactor

• Extract an is_empty_object helper: the emptiness predicate is inlined in the retain at prebid.rs:1481 and is needed again by both suggested fixes above. A shared fn is_empty_object(params: &Json) -> bool { params.as_object().is_some_and(serde_json::Map::is_empty) } keeps the rule in one place.

📌 out of scope

• zone leaks into every bidder's params: when a trustedServer entry has no bidderParams, expand_trusted_server_bidders (prebid.rs:942-957) hands each configured bidder the entire trustedServer object as its params, so a zone-only slot ships {"kargo": {"zone": "header"}, "triplelift": {"zone": "header"}} — a TS-internal orchestration key sent to PBS as a bidder param. These are non-empty, so the new drop never catches them; it's the third route by which fabricated params reach PBS. Pre-existing and not this PR's job, but it limits the "fabricated params never ship" contract and is worth a follow-up issue.

CI Status

• GitHub checks: PASS (4/4)
• fmt: PASS (verified locally)
• clippy (trusted-server-core, native host, -D warnings): PASS (verified locally)
• rust tests (trusted-server-core, native host): PASS (1631/1631, verified locally)
• js tests: not run — no JS/TS files changed

Note: #899 is still open, so this stays stacked and cannot merge first.

[aram356]

🔧 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?

[prk-Jr]

Resolved in 6a75cc4. Explicit empties are no longer preserved — the drop is now unconditional. is_unusable_bidder_params (empty object or non-object) runs after overrides and drops every remaining unusable value regardless of provenance, log::warn-ing each drop with slot + bidder so the misconfiguration is visible where an operator can act; the slot then falls back to its stored request. to_openrtb_preserves_an_explicitly_empty_bidder is now to_openrtb_drops_an_explicitly_empty_bidder (asserts drop + stored-request fallback).

On the premise question in the summary — whether a stored-request miss is a per-imp no-bid or a request-wide 400: I can't verify that from the repo; it is deployed-PBS behavior. What this change verifiably fixes is removing the definitely-rejected "bidder": {} / null from the wire, routed through the same stored-request fallback #680 already established for slots omitting inline params (endpoints.rs documents a stored request must exist for any such slot code). If a stored-request miss is itself a 400 on deployed PBS, that is a separate pre-existing exposure this does not widen for correctly-provisioned operators — but confirming it against deployed PBS would settle it. Do you have that confirmation, or should it be tracked?

Separately, the zone-leak out-of-scope item from the summary is filed as #927.

[aram356]

🔧 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.

[prk-Jr]

Resolved in 6a75cc4. The two sources are now collected separately — expanded (the trustedServer expansion) and direct (config-bidder entries) — then merged: a direct entry wins, except when an unusable direct value ({} or a non-object) would overwrite real params the expansion supplied. That closes the direct-{}-clobbers-real case (and, via the same predicate, direct-null-clobbers-real). Added to_openrtb_direct_empty_does_not_clobber_trusted_server_params — a 64-iteration loop mirroring the existing prefers-direct test.

[aram356]

🤔 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.

[prk-Jr]

Corrected in 6a75cc4. The comment no longer claims the fallback cannot fire for slots with real inline params; it now spells out that a slot whose only inline params name a bidder absent from config.bidders also lands in the stored-request fallback — that bidder is never expanded, the configured bidders fabricate empties, the drop clears them, and the real params are lost. Pinned by to_openrtb_falls_back_to_stored_request_for_real_params_of_unconfigured_bidder.

[aram356]

🤔 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.

[prk-Jr]

Resolved in 6a75cc4. The predicate now treats a non-object as unusable too — params.as_object().is_none_or(serde_json::Map::is_empty) — so null (reachable via a POST that omits params, since BidConfig.params is #[serde(default)]) is dropped rather than shipped as "bidder": {"kargo": null}. Added to_openrtb_drops_non_object_bidder_params. This also folds the is_empty_object helper the summary asked for into a single shared is_unusable_bidder_params used by both the merge and the drop.