Improve Prebid auction diagnostics#671
Conversation
| .chars() | ||
| .take(PROVIDER_ERROR_MESSAGE_CHARS) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🔧 wrench — Information disclosure: format!("{error:?}") on a Report<TrustedServerError> emits error-stack's full Debug stack, which includes at <file>:<line>:<col> location entries (and backtrace frames if compiled with them). That string flows through AuctionResponse.metadata["message"] → ProviderSummary.metadata → public JSON body of POST /auction, exposing internal source paths to any caller.
This contradicts the explicit policy in error.rs:18-22 ("message field is included in client-facing HTTP responses … keep it free of internal implementation details") and the IntoHttpResponse::user_message() pattern ("Full error details are preserved in server logs").
Fix — use Display on the current context so the user-safe #[display(...)] text is preserved but file/line/stack info is dropped:
fn provider_error_message(error: &Report<TrustedServerError>) -> String {
error
.current_context()
.to_string()
.chars()
.take(PROVIDER_ERROR_MESSAGE_CHARS)
.collect()
}The detailed {error:?} is still emitted to logs via the existing log::warn!("…: {:?}", e) calls.
| if !body_preview.is_empty() { | ||
| auction_response = | ||
| auction_response.with_metadata("body_preview", serde_json::json!(body_preview)); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — The first 1000 chars of the upstream Prebid Server's error body are placed in metadata["body_preview"] and shipped to whoever called /auction. Upstream Prebid error bodies routinely contain bidder-config echoes, internal request IDs, and account hints.
The trace-level body logging at line 1146 is gated on self.config.debug && trace_enabled; consider gating body_preview similarly (debug flag or request-time header) so the default /auction response stays free of upstream content. The warn! log line keeps the operator-side signal regardless.
| let mut backend_to_provider: HashMap<String, (&str, Instant, &dyn AuctionProvider)> = | ||
| HashMap::new(); | ||
| let mut pending_requests: Vec<PlatformPendingRequest> = Vec::new(); | ||
| let mut responses = Vec::new(); |
There was a problem hiding this comment.
🤔 thinking — Moving let mut responses = Vec::new(); above the launch loop means launch failures are appended before any awaited responses. Previously the list was ordered by completion. select_winning_bids is order-insensitive (HashMap), but external consumers reading provider_details will see a different ordering than they did pre-PR. Worth either calling out in the PR description or sorting deterministically before serialization.
| log::warn!("Prebid returned non-success status: {status} body: {body_preview}"); | ||
| } | ||
|
|
||
| let mut auction_response = AuctionResponse::error("prebid", response_time_ms) |
There was a problem hiding this comment.
♻️ refactor — Use PREBID_INTEGRATION_ID (defined at line 36) instead of the literal "prebid". Same applies to the no_bid("prebid", …) call below at line 1130. Other call sites in this file already use the constant.
| .chars() | ||
| .take(PREBID_ERROR_BODY_PREVIEW_CHARS) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — No tests pin the truncation behavior (>1000 chars) or non-UTF-8 input handling for prebid_body_preview, nor for provider_error_message (500-char cap). Cheap to add and would lock in the security boundary that today's reviewers are relying on.
SummaryRicher Blocking🔧 wrench
Non-blocking🤔 thinking
♻️ refactor
🌱 seedling
📌 out of scope
⛏ nitpick
CI Status
|
2 participants
Summary
/auctionprovider metadata.204 No Contentor empty200) asnobidinstead of providererror.client_side_bidderssample config so it is not enabled by default.Changes
crates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/auction/orchestrator.rs/auctionresponses include actionable diagnostics.trusted-server.tomlclient_side_biddersfrom["rubicon"]to[].Closes
N/A — no issue filed.
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test -p trusted-server-core prebid -- --nocapture;cargo test -p trusted-server-core auction -- --nocaptureChecklist
unwrap()in production code — useexpect("should ...")logmacros per CLAUDE.md (template saystracing, but this repo standard islog)