Skip to content

Prevent accidental Prebid Server stored requests - #1159

Open
ChristianPavilonis wants to merge 4 commits into
mainfrom
fix/pbs-stored-requests
Open

ChristianPavilonis wants to merge 4 commits into
mainfrom
fix/pbs-stored-requests

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Prevent generated browser auction slots from accidentally requesting Prebid Server (PBS) stored impressions. Previously, an empty bidder map could trigger a lookup using a dynamic slot code; a missing stored impression could reject the whole provider request, including valid sibling impressions.
  • Add optional trustedServer.params.storedRequest intent: false disables stored fallback, true permits it, and omission preserves legacy behavior. Filter demandless PBS impressions after server-side parameter overrides without suppressing eligible Amazon Publisher Services (APS) or other providers.
  • Preserve publisher intent across repeated and refresh auctions, with regression tests for the serialized browser payload and actual outbound provider request bytes.

PBS stored-request context

Stored requests are an existing optional Prebid Server feature. A caller normally opts in by referencing configuration already stored in PBS by ID. This PR does not introduce that feature or require new PBS configuration.

The defect was in Trusted Server's fallback behavior: when a slot had no usable inline bidder parameters, it could automatically use the dynamic slot code as a stored-impression ID. That accidentally opted generated slots into stored lookup. This fix makes newly generated envelopes explicitly opt out with storedRequest: false, while preserving intentional true and the legacy behavior for callers that omit the field.

Changes

Changed files
File Change
crates/trusted-server-core/src/auction/routing.rs Validates stored intent atomically, retains legacy admission facts, and carries intent into provider-local routing.
crates/trusted-server-core/src/auction/openrtb.rs Honors intent after bidder overrides, filters paired impressions safely, and skips empty requests before signing or transport.
crates/trusted-server-core/src/auction/openrtb/tests.rs Covers explicit, disabled, and legacy fallback, override-populated params, and retained impression identity.
crates/trusted-server-core/src/auction/orchestrator.rs Adds transport tests for zero, one, and two active PBS providers alongside APS, including valid bid preservation.
crates/trusted-server-core/src/auction/provider.rs Restricts response admission to emitted impressions and marks providers skipped after demand filtering.
crates/trusted-server-core/src/auction/formats.rs Documents the wire field and tests conversion through strict intent admission.
crates/trusted-server-js/lib/src/integrations/prebid/index.ts Defaults newly generated envelopes to false and preserves authored intent in live ad units and immutable refresh snapshots.
crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts Tests initial, repeated, and refresh serialization, including omitted and malformed intent and container-ID lookup.
crates/trusted-server-js/lib/test/core/auction.test.ts Confirms the shared serializer retains boolean values, omission, and invalid null for server validation.
crates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs Verifies generated and publisher-authored intent through the built adapter.
docs/guide/api-reference.md Defines the field, validation behavior, legacy compatibility, and request examples.
docs/guide/integrations/prebid.md Explains refresh behavior, server-first deployment, and rollback constraints.
docs/guide/auction-orchestration.md Describes provider routing and post-override impression filtering.
CHANGELOG.md Records the fix and deployment requirement.
docs/superpowers/plans/2026-09-10-pbs-stored-request-intent.md Records the approved plan, scope correction, regression evidence, and verification results.

Scope

This touches 15 files because the intent must survive browser generation, refresh reconstruction, server routing, final request construction, and response admission. Most additions are regression tests and documentation; changing only the router would leave a second stored-request fallback in the request builder.

No dependencies, adapter implementations, provider configuration, browser provider selectors, or explicit stored IDs are changed. Legacy inference and intentional stored-demand fanout remain supported. PBS all_eligible configuration remains rejected by the existing compiler, so its boundary test is retained rather than enabling a new configuration mode. Routing admits only slots with at least one positive banner format, and build_imp preserves one impression for every routed slot before provider-specific filtering, so paired impression and slot iteration cannot misalign.

Deployment

Deploy server support everywhere before serving the new JavaScript. The old router rejects unknown envelope fields and can drop valid inline demand when it receives storedRequest.

Rust artifacts embed the JavaScript bundles. First prepare a server-support-only build retaining old JS emission, then distribute the full build. Keep compatible server admission during rollback while browsers may retain the new JS. Until upgraded JavaScript reaches a publisher, an omitted storedRequest beside routed bidder entries that all contain empty parameter objects retains legacy slot-code fallback. Existing legacy callers and intentional stored requests can still reference missing slot-code IDs; this change does not eliminate unrelated PBS validation errors.

Closes

Closes #1086

Test plan

Meaningful failing-before assertions were captured for explicit intent admission, generated JS serialization, post-override fallback, filtered response admission, and no-usable-demand observability. They pass after the fix. Review findings are covered by focused regressions and the full validation below.

  • cargo test-fastly && cargo test-axum
  • cargo test-cloudflare && cargo test-spin
  • ./scripts/test-cli.sh
  • cargo clippy-fastly && cargo clippy-axum
  • cargo clippy-cloudflare && cargo clippy-cloudflare-wasm
  • cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run, 984 tests passed
  • JS lint/format/build: npm run lint, npm run format, and node build-all.mjs
  • Docs format/build: cd docs && npm run format && npm run build
  • git diff --check
  • Standalone release WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve

Fastly tests ran through Viceroy; Cloudflare and Spin production WASM paths passed Clippy. The full Rust suites passed with 10 pre-existing Fastly tests/doctests ignored. All six target-matched Clippy checks passed after the review fixes. The full JavaScript suite passed 984 tests across 45 files with no type errors, and the 13-module bundle build passed. No live PBS service was contacted.

Checklist

  • Changes follow the project coding conventions
  • No new unwrap() in production code; use expect("should ...")
  • Existing log instrumentation conventions preserved; no println! added
  • New behavior has regression tests
  • No secrets or credentials committed

ChristianPavilonis added a commit that referenced this pull request Sep 16, 2026
@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review September 16, 2026 15:08
@ChristianPavilonis
ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 September 16, 2026 15:08
@aram356 aram356 added this to the 202609 milestone Sep 17, 2026
aram356 added a commit that referenced this pull request Sep 18, 2026

@prk-Jr prk-Jr left a comment

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.

Summary

Reviewed commit 1e89a976a205bde3feaa9a599f725d99454afecf.

The explicit intent contract, provider-local filtering, and browser refresh preservation are well covered. One response-admission regression remains: PBS parse state still admits impressions removed from the outgoing request, allowing an unsolicited bid for an omitted slot to win.

Blocking

  • 🔧 Restrict response admission to impressions actually sent — see inline at crates/trusted-server-core/src/auction/openrtb.rs:515–516.

Validation

Local checks passed: 222 JS tests, 16 routing tests, 29 OpenRTB tests, one orchestrator transport test, and the 13-module bundle build. An additional scratch transport regression returned a bid for the omitted fictional-slot; the assertion that it cannot win failed, confirming the finding. Scratch changes were restored. No live PBS service was contacted; general adapter/lint gates rely on remote CI.

CI Status

Comment thread crates/trusted-server-core/src/auction/openrtb.rs

@aram356 aram356 left a comment

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.

Summary

The mechanism is correct and the tests are load-bearing rather than decorative. I verified locally against the PR head: 2675 core tests and 981 JS tests pass, cargo fmt --all -- --check clean, cargo clippy-axum clean, eslint/prettier clean. All 20 remote checks pass.

I checked the two hunks that read like regressions and both are safe. The AllEligible branch disappearing for prebid providers (routing.rs:376-382) is dead code, because plan.rs:307 rejects all_eligible + prebid-server at compile time. The zip over a filter_map-built impression list cannot misalign, because eligible_banner_slot guarantees build_imp never returns None for a routed slot. Neither is obvious from the diff, so both deserve a sentence in the PR description.

The deployment-ordering warning in the docs is accurate and load-bearing: I confirmed the base normalize_envelope rejects storedRequest as an unknown key, which drops bidderParams and zone for that slot. Old server plus new JS is a real demand-loss hazard, correctly identified.

One blocking finding: replacing a debug_assert! invariant with a routine production path that drops demand and emits no metadata, no log, and no counter.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff and can't be auto-applied.

Blocking

🔧 wrench

  • NoImpressions drops demand with no metadata, log, or counter — see Cross-cutting below

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note / ⛏ nitpick

  • The original defect is still live for un-upgraded publishers — see Cross-cutting below
  • storedRequestParams({ params: snapshot }) launders a snapshot through a fake bid — see inline at crates/trusted-server-js/lib/src/integrations/prebid/index.ts:842
  • undefined and null authored intent diverge silently — see inline at crates/trusted-server-js/lib/src/integrations/prebid/index.ts:707
  • auction.test.ts case passes with the entire PR reverted — see inline at crates/trusted-server-js/lib/test/core/auction.test.ts:94
  • allows_stored_fallback's has_candidates argument is dead in production — see inline at crates/trusted-server-core/src/auction/routing.rs:187

Cross-cutting / body-level findings

  • 🔧 NoImpressions drops demand with no metadata, log, or counter — This PR deletes debug_assert!(!prebid.is_empty(), "should never route a demandless slot to prebid-server") and turns that same condition into a routine production path via the new filter in apply_prebid plus the early return at openrtb.rs:266-268.

    That path lands on an unchanged call site, crates/trusted-server-core/src/auction/provider.rs:296-301, which returns a bare AuctionResponse::no_bid(self.provider_name(), 0) with empty metadata. To an operator that is indistinguishable from PBS legitimately returning no bids. Partial drops are invisible too: if 3 of 4 impressions are filtered but 1 survives, the request goes out as Ready and nothing records that 3 were dropped.

    The reachability change is what makes this matter. On base, NoImpressions could only fire when a provider had zero eligible slots from the start, and that case is already annotated upstream by provider_skipped_response (orchestrator.rs:218-223). This PR adds a second, semantically different trigger — demand filtered out after provider-local overrides — and routes it into the same metadata-free response. Once new TSJS ships storedRequest: false on every generated envelope, a misconfigured [auction.bidders] route produces no PBS request, no metadata, no log line, and no counter.

    The repo already has both idioms to fix this, so it is a small in-pattern change rather than a redesign. Minimum fix at provider.rs:296 (apply manually — provider.rs is outside this PR's diff, so it can't be a suggestion):

    OpenRtbBuildOutcome::NoImpressions => {
        return Ok(ProviderRequestOutcome::Immediate(
            AuctionResponse::no_bid(self.provider_name(), 0).with_metadata(
                "routing",
                serde_json::json!({"skipped_no_usable_demand": true}),
            ),
        ));
    }

    Better still, count the partial drops in apply_prebid using the existing RoutingDiagnostics saturating-counter pattern (routing.rs:70-105), which would restore the signal the debug_assert used to provide. pbs_disabled_empty_candidate_does_not_become_stored_demand and the new orchestrator transport test are the natural places to pin it.

  • 🤔 The original defect is still live for un-upgraded publishers — I verified this with a scratch test rather than inferring it. Routing the envelope {"bidderParams":{"exampleBidder":{}}} with no storedRequest key through build_request still produces:

    imp[0].ext.prebid = {"storedrequest":{"id":"fictional-slot"}}
    

    That is the reported defect — a stored-impression lookup keyed by the dynamic slot code — for any publisher who has not yet received the new JS.

    This is deliberate, not an oversight: base had || !slot.bidder_params().is_empty() in apply_prebid, and the PR faithfully preserves it as the has_candidates arm of StoredRequestIntent::Legacy. The pre-existing test openrtb/tests.rs:724 (pbs_empty_params_without_matching_override_fall_back_to_stored_request) pins exactly this shape, and creative_opportunities.rs:891 depends on legacy inference, so the compatibility choice is sound.

    The concern is only how it is described. The fix is effective once new JS reaches browsers, which the deployment section correctly says is gated on cached clients. "Closes #1086" reads as fully closed, and the escape hatch — any envelope with non-empty bidderParams whose entries are all empty objects — is not called out in the PR body or the api-reference table. Worth one sentence so operators know the server-side hole persists until client rollout completes.

CI Status

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS
  • CLAUDE.md symlink guard: PASS

Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/test/core/auction.test.ts Outdated
Comment thread crates/trusted-server-core/src/auction/routing.rs
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

Addressed the body-level review findings in 99743c09 and the PR description:

  • A PBS provider whose impressions are all removed after override processing now returns routing.skipped_no_usable_demand: true. The zero, one, and two-provider transport regression pins that metadata. Per the recorded decision, partial-drop counting is deferred until its field name and consumer are defined.
  • The PR description now explains the compiled rejection of PBS all_eligible, the routed-slot and build_imp pairing invariant, and the legacy empty-candidate fallback that remains until upgraded JavaScript reaches publishers.
  • The API guide documents that explicit JavaScript undefined serializes as omission and receives legacy inference.

Validation passed: all four adapter test aliases, ./scripts/test-cli.sh, all six target-matched Clippy aliases, Rust formatting, 984 JavaScript tests with no type errors, JS lint/format/build, docs format/build, and git diff --check.

@prk-Jr prk-Jr left a comment

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.

Summary

Solid, well-scoped fix. The StoredRequestIntent enum preserves every pre-existing legacy admission path while adding explicit opt-in/opt-out, and the post-override impression filter closes the second stored-request fallback in the request builder that a router-only change would have left behind. No blocking findings.

What I checked rather than assumed:

  • Compared the old and new admission truth tables path by path. They match on every legacy input (empty bidders map; valid envelope with missing/null/empty bidderParams; non-empty params; envelope absent with direct bidders). Only the two new explicit intents and one malformed-envelope case differ, and that difference is noted below.
  • Dropping routing == AllEligible from the PBS include arm is a dead path: plan.rs:307 rejects all_eligible for the prebid-server profile at compile time.
  • Server-generated opportunities are unaffected. creative_opportunities.rs:891 emits {"bidderParams":{}} with no storedRequest, which resolves to Legacy { empty_admission: true } and retains stored fallback.
  • filtered_slots feeds build_bid_dimension_index, so a bid for an impression that was filtered out resolves to BidRejectionReason::UnrequestedImpression rather than being silently admitted.

All four findings are non-blocking and prose-only; none is expressed as a one-click suggestion, because each either spans multiple files, changes behaviour, or is a design question rather than a defect.

Non-blocking

🤔 thinking

  • zip pairing guarded only by debug_assert, and the blast radius is now larger — see inline at crates/trusted-server-core/src/auction/openrtb.rs:491
  • Shim forwards a malformed authored storedRequest, which rejects the whole envelope server-side — see inline at crates/trusted-server-js/lib/src/integrations/prebid/index.ts:704

♻️ refactor

  • has_trusted_stored_request() no longer means what its name says — see inline at crates/trusted-server-core/src/auction/routing.rs:190

📝 note

  • Malformed envelope now hard-disables stored fallback even with independent direct demand — see inline at crates/trusted-server-core/src/auction/routing.rs:482

Cross-cutting / body-level findings

  • 👍 Test coverage is unusually strong — the matrices cover routing intent x params x direct demand, the orchestrator across zero, one, and two active PBS providers with APS participation preserved, and the fictional-slot case proves a bid for an unsent impression is rejected rather than admitted. Routing that rejection through the existing dimension index (UnrequestedImpression) instead of an ad-hoc check is the right mechanism.
  • 📝 Deploy ordering is already covered — the old-server-rejects-unknown-field hazard, and the fact that it drops valid inline demand rather than merely failing to fix the stored lookup, is documented in the PR body, the CHANGELOG entry, and docs/guide/integrations/prebid.md#stored-intent-deployment. Noting only that I verified it, not raising it as a finding.
  • 📝 TrustedProviderRoutes has no production caller — the new "server-owned routes do not authorize demandless PBS wire impressions" semantics, covered by pbs_filtering_keeps_slot_pairs_and_drops_demandless_trusted_routes, are exercised only by tests today. Pre-existing, not introduced here.

CI Status

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • CLAUDE.md symlink guard: PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • CodeQL: SKIPPED
  • cargo test (axum native): PENDING
  • Analyze (rust): PENDING
  • prepare integration artifacts: PENDING

No failing checks at the time of review.

// Filter paired impressions and slots together so later demand keeps its slot ID.
request.imp = std::mem::take(&mut request.imp)
.into_iter()
.zip(input.slots())

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 — build_common_request builds impressions with filter_map(build_imp) (line 319), so request.imp can in principle be shorter than input.slots(), and this zip would then pair each impression with the wrong slot.

Before this PR a misalignment only wrote the wrong imp.ext. Now it would also delete impressions here, and it would feed the wrong sent_impression_ids set in provider.rs:405 — so valid bids would come back and be rejected as unrequested_impression. The debug_assert_eq! above is a no-op in release builds.

I checked and this is currently unreachable: eligible_banner_slot in routing.rs admits only formats whose width and height are positive and fit in i32, while build_imp drops formats only when they exceed i32::MAX, so every routed slot yields Some. Raising it because the invariant now lives two modules away from the code that depends on it, and the failure mode changed from "wrong extension object" to "silently dropped demand plus rejected bids".

A self-enforcing version of the pairing:

// inside the filter_map closure, before building `bidder`
if imp.id.as_deref() != Some(slot.slot().id.as_str()) {
    log::error!("openrtb: impression/slot pairing drift; dropping imp");
    return None;
}

Apply manually — this changes runtime behaviour, and hardening build_common_request so the lengths cannot diverge in the first place may be the better of the two options. Worth a deliberate choice rather than a one-click commit.

None => diagnostics.record_malformed_envelope(),
None => {
diagnostics.record_malformed_envelope();
demand = NormalizedSlotDemand::default();

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.

📝 note — NormalizedSlotDemand::default() resolves to StoredRequestIntent::Disabled, which suppresses stored fallback unconditionally.

Before this PR, a malformed envelope left stored_request: false but openrtb.rs still permitted fallback through || !slot.bidder_params().is_empty(). So a slot with a malformed trustedServer envelope and an independent direct bidder whose params were emptied by provider-local overrides did fall back to storedrequest.id = <slot code>. It no longer does.

This is covered by malformed_stored_intent_rejects_envelope_atomically_but_preserves_direct_demand, and fail-closed is the right direction. The reason I am flagging it: the PR body, the CHANGELOG entry, and the docs all frame omission as "legacy inference preserved", and this is the one legacy path whose behaviour actually changed. A clause on the CHANGELOG entry noting that a malformed envelope now also disables fallback for that slot's direct demand would keep the record accurate.

}

#[cfg(test)]
pub(crate) fn has_trusted_stored_request(&self) -> bool {

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.

♻️ refactor — This used to return the trusted_stored_request field recorded at routing time. It is now #[cfg(test)] and derives a different predicate: bidder_params.is_empty() && allows_stored_fallback().

Five call sites still read as "was this slot routed as a trusted stored request" — here in routing.rs, plus formats.rs:786 and creative_opportunities.rs:2010. The assertions still catch regressions in allows_stored_fallback, so this is naming and indirection rather than a coverage hole, but the name now overstates what the helper knows.

Either rename it to something like stored_fallback_without_inline_demand(), or delete it and assert the two production predicates directly at the call sites:

assert!(slot.bidder_params().is_empty() && slot.allows_stored_fallback());

Apply manually — the rename touches routing.rs, formats.rs, and creative_opportunities.rs together, so it cannot be expressed as a single-file suggestion.


/** Capture immutable request-scoped bidder and zone data before the shim mutates an ad unit. */
/** Preserve authored presence and values, including invalid values for server validation. */
function storedRequestParams(bid: TrustedServerBid | undefined): { storedRequest?: unknown } {

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 deliberately forwards an authored non-boolean (null, "false", 0, and so on) verbatim, per the doc comment, so the server can validate it.

The server-side consequence is atomic rejection: normalize_envelope returns None for a non-boolean storedRequest, which discards the entire envelope — bidderParams and zone included. So a single publisher typo silently removes every inline PBS bid on that slot, and the only signal is the malformed_envelope_count diagnostic. There is no console warning and nothing visible in window.tsjs.

The shim already normalizes and filters other params before they reach the envelope, so an alternative is to treat a non-boolean as omission (legacy inference) and emit a console warning, which degrades to the pre-PR behaviour instead of dropping the publisher's demand:

function storedRequestParams(bid: TrustedServerBid | undefined): { storedRequest?: unknown } {
  if (!bid) return { storedRequest: false };
  if (!Object.prototype.hasOwnProperty.call(bid.params ?? {}, STORED_REQUEST_KEY)) return {};
  const value = bid.params?.[STORED_REQUEST_KEY];
  if (typeof value !== 'boolean') {
    console.warn('[tsjs] ignoring non-boolean trustedServer.params.storedRequest', value);
    return {};
  }
  return { storedRequest: value };
}

That trades the server-side diagnostic for client-side resilience, which is a genuine tradeoff rather than a clear win — hence a question, not a change request. Apply manually if you want it; it changes behaviour and the existing tests assert the current pass-through.

@aram356 aram356 left a comment

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.

Summary

All six findings from the previous pass are resolved, and the blocking one was fixed more thoroughly than requested. Approving.

I re-verified each fix against the code rather than from the replies:

  • NoImpressions silent drop — now carries routing.skipped_no_usable_demand. I traced the chain (types.rs:331 → ProviderSummary.metadata → provider_details) to confirm an operator actually sees it in the auction response, and the key matches the existing skipped_no_eligible_slots shape.
  • Response admission — f222033fd goes further than the review asked, deriving admission from the sent request.imp rather than the routed slot list. I confirmed request is not mutated between serialization at provider.rs:339 and the ID collection at provider.rs:403, so the gate is built from the same value that produced the wire bytes. I also checked the narrowing is safe for the one consumer that counts over slots(): unused_bidder_params_count early-returns 0 for PBS at openrtb.rs:895, so it never sees the shortened list.
  • The regression test is a real guard. I reverted filtered_slots to input.clone() in a scratch worktree and confirmed the test fails loudly at orchestrator.rs:6031 — winning_bids.len() becomes 2 instead of 1, because the 3.0 bid for the never-sent impression wins. It proves rejection, not merely losing.
  • JS snapshot read — applied as suggested. undefined coverage — added with not.toHaveProperty on parsed wire objects, which is the assertion form that can actually distinguish an absent key. Test rename and the allows_fallback comment are both in.

On the browser warning for non-boolean authored intent: the reasoning for declining it — that logging arbitrary authored values could expose publisher data to console collectors — is a stronger argument than the diagnosability concern that motivated it. Not re-raising.

Verification on this head: cargo test-axum -p trusted-server-core --lib 2675 passed, cargo test-cloudflare 22, cargo test-spin 49/37, parity 13, npx vitest run 984 passed / 45 files, cargo fmt --all -- --check and cargo clippy-axum clean. All 20 GitHub checks pass.

Two optional notes below. Neither affects behavior and neither needs to block merge — resolve them as you see fit.

Non-blocking

📝 note

  • materialize_planned_response no longer matches its own doc comment — see inline at crates/trusted-server-core/src/auction/provider.rs:299
  • Imp::id is Option<String>, and the invariant is now load-bearing — see inline at crates/trusted-server-core/src/auction/provider.rs:406

CI Status

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS
  • CLAUDE.md symlink guard: PASS

)));
return Ok(ProviderRequestOutcome::Immediate(
AuctionResponse::no_bid(self.provider_name(), 0)
.with_metadata("routing", json!({"skipped_no_usable_demand": true})),

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.

📝 note — Worth a look, but safe to resolve without changing anything if you disagree.

materialize_planned_response (orchestrator.rs:270-294) documents itself as "Skipped providers are routed separately and retain their exclusive skipped diagnostic", but the short-circuit at orchestrator.rs:277-282 only tests for skipped_no_eligible_slots. A response carrying the new skipped_no_usable_demand key falls through and picks up a sibling unused_bidder_params_count: 0.

Behavior is fine: as_object_mut().insert(...) merges rather than replaces, so the skip key survives, and orchestrator.rs:6046-6049 passes through this exact path and asserts it. The 0 is also accurate, since unused_bidder_params_count early-returns 0 for PBS.

It is only that the code now contradicts its stated contract — the new skip does not in fact retain an exclusive diagnostic. Either adding the new key to the line-277 check, or softening the comment to say only skipped_no_eligible_slots is exclusive, would resolve the mismatch. Pure cosmetics; no behavioral difference either way.

let sent_impression_ids = request
.imp
.iter()
.filter_map(|impression| impression.id.as_deref())

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.

📝 note — Not reachable today; flagging because this commit changes how much weight the invariant carries.

Imp::id is Option<String> (from the generated OpenRTB schema), so filter_map silently skips any impression whose id is None. Such an impression would be dropped from sent_impression_ids, and its slot would then be filtered out of the parse input even though it was sent — meaning a legitimate bid for it would be rejected as unrequested_impression.

That cannot happen right now. build_imp (openrtb.rs:446-447) is the only Imp construction site and sets id: Some(slot.slot().id.clone()) unconditionally, and neither apply_prebid nor finalize_request touches id. I verified this rather than assuming it.

The reason it seems worth a note: before this commit, a missing impression ID would have been inert, whereas sent_impression_ids is now the response-admission gate, so the invariant became load-bearing while remaining guaranteed only by convention. The generated type cannot be tightened locally, but a cheap assertion before the collect would pin it and mirror the existing style at openrtb.rs:483-487:

debug_assert!(
    request.imp.iter().all(|impression| impression.id.is_some()),
    "should populate every PBS impression ID"
);

Entirely optional — resolve if you would rather not add an assertion for an unreachable case.

aram356 added a commit that referenced this pull request Sep 24, 2026
# Conflicts:
#	crates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Distinguish empty PBS demand from stored-request intent

3 participants