Prevent accidental Prebid Server stored requests - #1159
ChristianPavilonis wants to merge 3 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- format-docs: PASS (required)
- Analyze (actions): PASS
- cargo test (axum native): PASS
- vitest: PASS
- CLAUDE.md symlink guard: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- format-typescript: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- cargo test: PASS (required)
- cargo fmt: PASS (required)
| if prebid.is_empty() { | ||
| return None; |
There was a problem hiding this comment.
🔧 [P2] Restrict response admission to impressions actually sent
Dropping an impression here does not remove it from the response-admission input. GenericOpenRtbProvider::request_bids_routed still clones the original ProviderAuctionInput into PBS parse state (provider.rs:401-405), and PBS builds its allowed impression/dimension index from that input (integrations/prebid.rs:2066). With one disabled empty candidate beside a valid inline sibling, the wire request contains only the sibling, but a PBS response naming the dropped candidate is accepted and can win the auction. This breaks the existing rejection of unrequested impressions precisely for the new post-override filtering path.
Carry the emitted impression membership into the provider's response state, and exclude dropped slots before building its dimension index. Add a mixed-request regression that returns both a valid sibling bid and a bid for the omitted impression; only the sibling should survive and the omitted impression should count as unrequested_impression.
Proposed implementation shape (manual, illustrative API):
let sent_impression_ids = request.imp.iter()
.filter_map(|imp| imp.id.as_deref())
.collect::<HashSet<_>>();
let response_input = input.filtered_slots(|slot| {
sent_impression_ids.contains(slot.slot().id.as_str())
});
// Store response_input, rather than input.clone(), in the PBS parse state.Apply manually — this needs a filtered-input helper or equivalent parse-state change across routing/provider code plus a regression test, outside the changed OpenRTB hunk. No one-click suggestion is eligible; the illustrative API above is not an implemented or verified replacement.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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
NoImpressionsdrops 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 atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:842undefinedandnullauthored intent diverge silently — see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:707auction.test.tscase passes with the entire PR reverted — see inline atcrates/trusted-server-js/lib/test/core/auction.test.ts:94allows_stored_fallback'shas_candidatesargument is dead in production — see inline atcrates/trusted-server-core/src/auction/routing.rs:187
Cross-cutting / body-level findings
-
🔧
NoImpressionsdrops demand with no metadata, log, or counter — This PR deletesdebug_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 inapply_prebidplus the early return atopenrtb.rs:266-268.That path lands on an unchanged call site,
crates/trusted-server-core/src/auction/provider.rs:296-301, which returns a bareAuctionResponse::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 asReadyand nothing records that 3 were dropped.The reachability change is what makes this matter. On base,
NoImpressionscould only fire when a provider had zero eligible slots from the start, and that case is already annotated upstream byprovider_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 shipsstoredRequest: falseon 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.rsis 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_prebidusing the existingRoutingDiagnosticssaturating-counter pattern (routing.rs:70-105), which would restore the signal thedebug_assertused to provide.pbs_disabled_empty_candidate_does_not_become_stored_demandand 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 nostoredRequestkey throughbuild_requeststill 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()inapply_prebid, and the PR faithfully preserves it as thehas_candidatesarm ofStoredRequestIntent::Legacy. The pre-existing testopenrtb/tests.rs:724(pbs_empty_params_without_matching_override_fall_back_to_stored_request) pins exactly this shape, andcreative_opportunities.rs:891depends 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
bidderParamswhose 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
| const snapshot = findRefreshSnapshot(candidateCodes); | ||
| return snapshot ? storedRequestParams({ params: snapshot }) : { storedRequest: false }; | ||
| } |
There was a problem hiding this comment.
♻️ refactor — This launders a PublisherAdUnitSnapshot through a synthetic { params: ... } bid to reuse storedRequestParams. It is correct today, but only because of an invisible coupling.
It works because this PR hoisted storedRequest to the snapshot's top level (index.ts:385, as a sibling of bidderParams / clientSideBids / zone), so hasOwnProperty.call(bid.params ?? {}, STORED_REQUEST_KEY) at index.ts:706 happens to read the right field. TypeScript cannot catch a violation, because PublisherAdUnitSnapshot is structurally assignable to Record<string, unknown> and the value type is unknown.
The failure mode is silent and lands exactly on this PR's purpose. If someone later restructures the snapshot to { params: {...}, clientSideBids: [...] } — a natural refactor — this line starts returning {} for every snapshot-backed refresh, flipping those slots from "stored fallback disabled" to legacy inference and re-enabling the accidental stored requests this PR exists to prevent. No type error, and no existing test fails: I checked, and every current test moves the write site (index.ts:743) and this read site together, so they only break on an inconsistent change.
Reading the field directly at the point of use makes the dependency explicit:
| const snapshot = findRefreshSnapshot(candidateCodes); | |
| return snapshot ? storedRequestParams({ params: snapshot }) : { storedRequest: false }; | |
| } | |
| const snapshot = findRefreshSnapshot(candidateCodes); | |
| if (!snapshot) return { storedRequest: false }; | |
| return Object.prototype.hasOwnProperty.call(snapshot, STORED_REQUEST_KEY) | |
| ? { storedRequest: copyParamValue(snapshot[STORED_REQUEST_KEY]) } | |
| : {}; | |
| } |
Scratch-verified on the PR head: npx vitest run 981 passed / 45 files, npm run lint (eslint --max-warnings=0) clean, prettier --check clean, and tsc --noEmit reports nothing for this file. Byte-exact pre/post patch comparison showed no drift.
| function storedRequestParams(bid: TrustedServerBid | undefined): { storedRequest?: unknown } { | ||
| if (!bid) return { storedRequest: false }; | ||
| return Object.prototype.hasOwnProperty.call(bid.params ?? {}, STORED_REQUEST_KEY) | ||
| ? { storedRequest: copyParamValue(bid.params?.[STORED_REQUEST_KEY]) } |
There was a problem hiding this comment.
🤔 thinking — undefined and null are both "the publisher wrote nonsense here", but they take completely different paths, and the difference is invisible at this line.
I traced both through the real serialization path (buildRequests → buildAdRequest → JSON.stringify) and confirmed by running it:
storedRequest: undefined—hasOwnPropertyistruefor an explicitly-assignedundefined, so this returns{ storedRequest: undefined }.JSON.stringifythen drops the key, the server sees omission, and the slot gets legacy inference.storedRequest: null— serializes as"storedRequest": null, which hitsSome(_) => return Noneinnormalize_envelope. The envelope is rejected atomically, so that slot loses itsbidderParamsand itszonefor PBS.
So a publisher writing storedRequest: config.enableStored with a missing config key silently gets legacy behavior, while storedRequest: config.enableStored ?? null silently loses all of that slot's PBS demand. The only evidence of the second case is a server-side record_malformed_envelope counter the publisher cannot see.
Forwarding invalid values verbatim for server validation is a defensible design and the docstring says so. Being silent about it in the browser is the part worth reconsidering — a one-time log.warn when the authored value is not a boolean would turn a silent demand drop into something diagnosable:
const authored = bid.params?.[STORED_REQUEST_KEY];
if (typeof authored !== 'boolean') {
log.warn('[tsjs-prebid] non-boolean storedRequest will be rejected by the server', authored);
}Separately, undefined is not covered by either it.each list (index.test.ts:1253, index.test.ts:2793) or by auction.test.ts:93. Given it is the one non-boolean that behaves differently from all the others, it is the most valuable missing case. Note that asserting it needs not.toHaveProperty on the parsed wire object rather than toEqual, since toEqual treats {a:1, b:undefined} and {a:1} as equal.
Apply manually — the warn helper and the added test case live outside this hunk.
| }); | ||
|
|
||
| it.each([{}, { storedRequest: false }, { storedRequest: true }, { storedRequest: null }])( | ||
| 'retains stored intent presence in serialized shared requests: %j', |
There was a problem hiding this comment.
⛏ nitpick — This case passes with the entire PR reverted, so the title credits it with coverage it does not provide.
buildAdRequest (src/core/auction.ts:92-104) copies params by reference and neither filters nor rewrites keys, and src/core/auction.ts is not in this PR's diff. The test therefore pins a pre-existing pass-through property rather than anything this change introduces.
That is still reasonable regression insurance — it proves the shared serializer will not strip or mangle the new key — but "retains stored intent presence in serialized shared requests" reads like coverage of the new feature, and a reviewer scanning test names would count it as such. Renaming to something like buildAdRequest passes bid params through verbatim keeps the value and drops the implication.
Also note the toEqual assertion here cannot distinguish "key absent" from "key present with value undefined", which is the distinction that matters for the omission case. prebid-artifact-integration.test.mjs:229 gets this right with not.toHaveProperty on the post-JSON.parse wire object.
Apply manually — rename only, outside the suggestion-eligible range.
| self.bidder_params.is_empty() && self.allows_stored_fallback() | ||
| } | ||
|
|
||
| pub(crate) fn allows_stored_fallback(&self) -> bool { |
There was a problem hiding this comment.
📝 note — The has_candidates parameter is dead at the only production call site.
allows_stored_fallback passes !self.bidder_params.is_empty(), but the one non-test caller is the else if slot.allows_stored_fallback() arm in apply_prebid (openrtb.rs:512), which is reached only when the computed bidder map came out empty. So has_candidates == true is only reachable there when the slot has routed bidder params that all failed the post-override usability filter.
That is not a bug — it is exactly how base's || !slot.bidder_params().is_empty() behaved, and preserving it is the point. Worth a short comment on StoredRequestIntent::allows_fallback recording why Legacy consults has_candidates, since the parameter otherwise looks vestigial to a future reader and is an easy thing to "simplify" away — doing so would change the legacy fallback semantics that openrtb/tests.rs:724 depends on.
Apply manually — a doc comment above allows_fallback in routing.rs:256-264, outside this hunk.
3 participants
Summary
trustedServer.params.storedRequestintent:falsedisables stored fallback,truepermits 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.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 intentionaltrueand the legacy behavior for callers that omit the field.Changes
Changed files
crates/trusted-server-core/src/auction/routing.rscrates/trusted-server-core/src/auction/openrtb.rscrates/trusted-server-core/src/auction/openrtb/tests.rscrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-js/lib/src/integrations/prebid/index.tsfalseand preserves authored intent in live ad units and immutable refresh snapshots.crates/trusted-server-js/lib/test/integrations/prebid/index.test.tscrates/trusted-server-js/lib/test/core/auction.test.tsnullfor server validation.crates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjsdocs/guide/api-reference.mddocs/guide/integrations/prebid.mddocs/guide/auction-orchestration.mdCHANGELOG.mddocs/superpowers/plans/2026-09-10-pbs-stored-request-intent.mdScope
This touches 14 files because the intent must survive browser generation, refresh reconstruction, server routing, and final request construction. 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_eligibleconfiguration remains rejected by the existing compiler, so its boundary test is retained rather than enabling a new configuration mode.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. 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, and the remaining post-override fallback. They pass after the fix. Independent correctness review found no issues.
cargo test-fastly && cargo test-axumcargo test-cloudflare && cargo test-spin./scripts/test-cli.shcargo clippy-fastly && cargo clippy-axumcargo clippy-cloudflare && cargo clippy-cloudflare-wasmcargo clippy-spin-native && cargo clippy-spin-wasmcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run, 923 tests passednpm run lint,npm run format, andnode build-all.mjscd docs && npm run format && npm run buildgit diff --checkcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveFastly 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. After a mechanical Clippy fix and a test-only example-domain correction, all six Clippy checks and focused routing/OpenRTB tests were rerun successfully; the full suites were not repeated after those two edits. No live PBS service was contacted.
Checklist
unwrap()in production code; useexpect("should ...")loginstrumentation conventions preserved; noprintln!added