Dump full SSAT prebid responses in ts-debug auction comment

[prk-Jr]

Summary

• The server-side auction (SSAT) stream path only emitted a summary line in the ts-debug HTML comment (ssp/mediator/winning/time), so an operator seeing winning=0 could not tell whether prebid returned nothing, errored, or bid below the floor.
• Dump the full provider_responses (and mediator_response) into the comment so the SSAT surfaces the same prebid server response detail an operator gets from the /auction endpoint — status, every bid, and metadata (which carries PBS ext.errors / ext.debug.httpcalls when prebid debug=true).
• Bid creative and provider metadata are attacker/partner-influenced, so the embedded JSON has its HTML-comment terminators neutralized to keep the dump inside the comment and out of the live DOM.

Changes

File	Change
crates/trusted-server-core/src/publisher.rs	prepend_auction_debug_comment now serializes provider_responses + mediator_response as JSON into the ts-debug comment; neutralizes --> / --!> in the payload; adds a unit test asserting status is surfaced and terminators are neutralized (exactly one closing -->).

Closes

Closes #889

Test plan

• cargo fmt --all -- --check
• cargo test-fastly (new test auction_debug_comment_dumps_provider_status_and_neutralises_terminators passes; cargo check-fastly clean)
• cargo test-axum — deferred to CI
• cargo clippy-fastly && cargo clippy-axum — deferred to CI
• JS tests / format — n/a (no JS change)
• Docs format — n/a (no docs change)

Notes

• The dump is gated behind the existing [debug].auction_html_comment flag — off by default.
• On a winning auction the dump includes Bid.creative (full ad markup) inline and can be large; intended for debugging, not steady-state production.

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses log macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

The HTML-comment side of this PR is sound — I fuzzed the terminator neutralisation against a real HTML5 parser (11 escape vectors, including the <!--> / <!--!> nested-comment paths) and nothing escaped the comment. The problem is the other half: the new prebid error metadata is not confined to the debug comment. It flows into the public POST /auction response, which breaks an invariant the codebase documents explicitly.

Blocking

🔧 wrench

• Upstream PBS error body is echoed into the public /auction response (integrations/prebid.rs:2143): .with_metadata("body", …) lands in ProviderSummary.metadata (auction/types.rs:252) → ext.orchestrator.provider_details[] (auction/formats.rs:305), ungated by any debug flag. auction/orchestrator.rs:100 carries a // SECURITY: comment forbidding exactly this: "Providers MUST NOT interpolate upstream-controlled content (response bodies, parse errors, headers)… Use static text and log details server-side with log::warn! instead." Verified by serializing the resulting summary — the body snippet appears on the wire. See inline comment for the fix.
• error_type: "http_status" is mislabelled as a transport error in telemetry (integrations/prebid.rs:2141): it isn't one of the four ERROR_TYPE_* consts (auction/orchestrator.rs:95-98), so provider_status() (auction/telemetry.rs:795) hits its _ => "transport_error" fallback. Every PBS 4xx/5xx is now bucketed with genuine connection failures — which defeats this PR's own goal on the telemetry path.
• [debug].auction_html_comment doc no longer describes what the flag does (settings.rs:1904): it still promises "auction pipeline stats (SSP count, mediator status, winning bid count)", but the comment now embeds every bid's full raw SSP creative markup. Operators make the enable/disable call from that sentence. The sibling adm_in_bids flag already carries the right warning ("Never enable in production — injects raw HTML from SSPs") — please mirror it here, and update the prepend_auction_debug_comment doc at publisher.rs:858, which still says "summarising".

Non-blocking

🤔 thinking

• Unbounded dump size (publisher.rs:885): every bid's creative — including losers — pretty-printed into every page render. See inline.

♻️ refactor

• A single replace("--", "- -") would replace the two-terminator blacklist (publisher.rs:884) — equivalent, trivially auditable, and XHTML-safe. Inline comment has the fuzz results.
• Test coverage gap: --!> and the nested-comment paths are handled but unpinned (publisher.rs:2502).
• Local imports inside test fn bodies violate CLAUDE.md (publisher.rs:2465, integrations/prebid.rs:2355).

⛏ nitpick

• mediator_response=null is dumped when there is no mediator, duplicating mediator=none (publisher.rs:888).

🌱 seedling

• path_label only ever receives "stream" (publisher.rs:1265), though the doc says it differentiates stream from buffered. The buffered path produces no dump at all, so an operator debugging that path gets nothing from this feature. Pre-existing, not introduced here — worth a follow-up.

CI Status

All 19 checks pass on 08de9c4 (fmt, clippy, cargo test across fastly/axum/cloudflare/spin/parity/CLI, vitest, docs format, integration + browser tests, CodeQL).

[ChristianPavilonis]

Summary

Approved after reviewing the current head against main. The HTML-comment containment and public-response protections appear correct. I noted two follow-up findings inline: one telemetry aggregation gap and one edge-runtime resource concern.

[ChristianPavilonis]

🔧 P1 / High — PBS HTTP failures are excluded from provider-health error metrics

The new http_status_error value is emitted here, but tinybird/pipes/auction_provider_stats_mv.pipe:15 only counts launch_error, parse_error, and transport_error as errors. These requests therefore increase requests without increasing errors, timeouts, or another outcome counter, and provider_health.error_rate under-reports PBS 4xx/5xx failures. The documented status contract at docs/superpowers/specs/2026-06-22-auction-prebid-metrics-tinybird-grafana-design.md:347 is also stale.

Suggested fix: Add http_status_error to the Tinybird rollup's error set, update the documented status list, and add coverage for both the Rust mapping and downstream aggregation classification.

[ChristianPavilonis]

🤔 P2 / Medium — The 256 KiB cap does not bound serialization work or transient allocations

serde_json::to_string materializes the complete provider response before this closure runs, and the chained replace calls then allocate and scan full-size strings before truncation. Provider responses can each reach 2 MiB, and mediator serialization repeats the process separately. On Wasm edge runtimes this can create substantial transient heap pressure and latency despite the final output cap; with a mediator, the final comment can also exceed 512 KiB because each section receives its own budget.

Suggested fix: Serialize through a bounded writer, apply one explicit total-comment budget, then neutralize the bounded buffer. Add an oversized multibyte provider-plus-mediator test verifying bounded output, valid UTF-8, the truncation marker, and comment containment.

[aram356]

Summary

Second, deeper pass across the surfaces the first review didn't reach (privacy/PII content of the dump, every egress channel, runtime/memory/caching, and the test surface). It turned up two problems that outrank everything in my previous review.

The short version: with prebid debug = true, this dump renders the visitor's identity graph — EC ID, EIDs, TC consent string, client IP, and geo lat/lon — into the publisher's page source, where any third-party tag on the page can read it. And because the dump is unbounded, it can push the page over the buffered-body cap and turn a working page into a 500.

Blocking

🔧 wrench

• The dump renders the visitor's identity graph into the page HTML (publisher.rs:874-890). AuctionResponse.metadata carries PBS's entire ext.debug subtree, copied verbatim at integrations/prebid.rs:1831-1836 when [integration.prebid].debug = true — that is resolvedrequest (the full OpenRTB request TS sent) and httpcalls (the outbound body sent to each bidder). That request is built at integrations/prebid.rs:1514-1600 and contains user.id (EC ID), user.ext.eids, the raw TC string, device.ip, and device.geo.lat/lon. Serializing metadata into a DOM comment node hands the consent-gated identity graph straight back to the client-side script environment that server-side EID resolution exists to keep it out of. Reproduction below. See inline.
• The unbounded dump can convert a working page into a 500 (publisher.rs:892). Output is written through a BoundedWriter capped at publisher.max_buffered_body_bytes (default 16 MiB, settings.rs:71-73); write() returns Err past the cap (publisher.rs:574-583) and that surfaces as TrustedServerError::Proxy → 5xx. The whole comment goes out in one end_tag.before() write. With UPSTREAM_RTB_MAX_RESPONSE_BYTES at 2 MiB per provider (integrations/mod.rs:163) and httpcalls carrying full per-bidder request/response bodies, multi-MB dumps are realistic — so the debug flag can break the page it was enabled to debug. Every other partner-influenced payload here is bounded (MAX_AUCTION_BODY_SIZE 256 KiB, MAX_CREATIVE_SIZE 1 MiB — and this PR's own 512-byte snippet). This dump is the one that isn't. See inline.
• The new prebid test asserts the leak as intended behaviour (integrations/prebid.rs:2383-2389). Fixing the /auction body exposure from my previous review means this assertion has to be deleted or inverted — flagging so it isn't mistaken for a regression. See inline.

Non-blocking

♻️ refactor

• The new error response omits message (integrations/prebid.rs:2140), the one key every other error path sets (auction/orchestrator.rs:123, 129, 141, 147). The prebid HTTP-error shape now diverges from every other provider error. See inline.
• --!> neutralisation is untested. Delete .replace("--!>", "-- !>") from publisher.rs:884 and the suite stays green — the fixture only plants --> in Bid.creative. Half the claimed protection is unguarded. Also worth planting a terminator in metadata (which the code comment at publisher.rs:879-882 explicitly calls out as attacker-influenced, but the test never exercises) and using a Some(_) mediator so the second neutralise call at publisher.rs:888-890 is covered at all.

⛏ nitpick

• The serialize-error fallback bypasses the neutraliser (publisher.rs:887, 890) — .map() applies only to the Ok branch. Unreachable in practice, but it's a latent seam in a security-relevant path. See inline.
• Non-deterministic output. AuctionResponse.metadata and winning_bids are HashMaps, so key order varies per render and two page sources of the same auction won't diff cleanly.

📌 out of scope

• auction_html_comment is undocumented. Zero hits across docs/, and it is absent from the [debug] block in trusted-server.example.toml (which lists only ja4_endpoint_enabled). There is no operator-facing warning anywhere for a flag that now publishes creatives and, with prebid debug on, user identifiers.

Reproduction — identity graph in page source

Drop into publisher.rs's test module and run cargo test -p trusted-server-core --target wasm32-wasip1 --lib -- probe_identity_graph --nocapture. It passes on this branch — every needle is present in the injected HTML:

#[test]
fn probe_identity_graph_lands_in_page_html() {
    // Shape of PBS `ext.debug` as prebid.rs:1831-1836 stores it verbatim.
    let prebid = crate::auction::types::AuctionResponse::error("prebid", 12).with_metadata(
        "debug",
        serde_json::json!({
            "resolvedrequest": {
                "user": {
                    "id": "EC-ID-abc123",
                    "consent": "CPtc-TCSTRING-xyz",
                    "ext": { "eids": [{ "source": "example.com",
                                        "uids": [{ "id": "EID-USER-999" }] }] }
                },
                "device": { "ip": "203.0.113.77",
                            "geo": { "lat": 37.7749, "lon": -122.4194 } }
            }
        }),
    );
    let result = crate::auction::orchestrator::OrchestrationResult {
        provider_responses: vec![prebid],
        mediator_response: None,
        winning_bids: std::collections::HashMap::new(),
        total_time_ms: 12,
        metadata: std::collections::HashMap::new(),
    };
    let state = Arc::new(Mutex::new(Some("<script>BIDS</script>".to_string())));
    prepend_auction_debug_comment("stream", &result, &state);
    let page = state.lock().expect("should lock").clone().expect("should have html");
    for needle in ["EC-ID-abc123", "EID-USER-999", "CPtc-TCSTRING-xyz", "203.0.113.77", "37.7749"] {
        assert!(page.contains(needle), "leaked into page HTML: {needle}");
    }
}

Checked and not a problem

Cross-user cache poisoning. publisher.rs:1671-1683 forces Cache-Control: private, max-age=0 and strips Surrogate-Control on any ad-stack HTML, which is exactly the condition under which this comment can exist — so one visitor's dump cannot be shared-cached and served to another. Flagging explicitly so nobody spends time on it. The exposure is same-page JS, not the CDN.

Also clean: GET /__ts/page-bids, window.tsjs.bids, the telemetry event rows, and the mediator request all read Bid fields rather than AuctionResponse.metadata, so none of them carry the new data.

Why CI's 19 green checks caught none of this

Both this and the /auction exposure from my previous review live in cross-module data-flow contracts with no test at any boundary. Every formats.rs fixture hardcodes metadata: HashMap::new() and BidStatus::Success (auction/formats.rs:418, 448), so no error response with metadata is ever pushed through convert_to_openrtb_response; the one exact-JSON wire test hardcodes provider_details: vec![] (openrtb.rs:212); and provider_status() has a _ => catch-all (auction/telemetry.rs:803) which by construction cannot fail on an unrecognised error_type. It's an assertion gap, not an execution gap — the tests do run.

Suggested minimal fix

Whitelist what goes into the dump instead of serializing the whole metadata map: keep error_type, status, responsetimemillis, errors, warnings, bidstatus; exclude debug outright (or scrub resolvedrequest + httpcalls[].requestbody). Then cap the serialized payload — 256 KiB matching MAX_AUCTION_BODY_SIZE, with a <truncated N bytes> marker, and truncate Bid.creative per bid. That closes the identity exposure, the 5xx risk, and the memory pressure in one change, and it is consistent with the 512-byte cap this same PR already applies at integrations/prebid.rs:2139.

[aram356]

🔧 wrench — This comment advertises ext.debug.httpcalls as the feature, and that is exactly the problem: it renders the visitor's identity graph into the publisher's page source.

integrations/prebid.rs:1831-1836 copies PBS's entire ext.debug subtree into AuctionResponse.metadata verbatim when [integration.prebid].debug = true:

if self.config.debug {
    if let Some(debug) = ext.and_then(|e| e.get("debug")) {
        auction_response.metadata.insert("debug".to_string(), debug.clone());

That subtree is resolvedrequest (the full OpenRTB request Trusted Server sent) and httpcalls (the outbound body sent to each bidder). That request is built at integrations/prebid.rs:1514-1600 and carries:

Field	Source	Content
user.id	prebid.rs:1515	EC ID
user.ext.eids	prebid.rs:1528	the consent-gated identity graph
user.consent	prebid.rs:1516-1519	raw TCF TC string
device.ip	prebid.rs:1583	real client IP
device.geo.lat/lon	prebid.rs:1584-1600	precise geo

Before this PR the comment held four counters. Now it serializes metadata, so all of the above lands in a DOM comment node — readable by every third-party tag already running on the page via document.documentElement.innerHTML, and present in view-source, scrapers, and archives.

This inverts the purpose of server-side EID resolution: the identity graph is resolved and consent-gated on the edge specifically so it does not reach the client script environment, and this hands it straight back. Note the codebase already redacts in the equivalent situation — auction/telemetry.rs:115-116 ships consent_present: bool, never the raw TC string.

I verified this rather than inferring it; a runnable reproduction is in the review body — it passes on this branch.

Fix — whitelist keys instead of dumping the map. Keep error_type / status / responsetimemillis / errors / warnings / bidstatus; exclude debug entirely, or scrub resolvedrequest and httpcalls[].requestbody before embedding.

[aram356]

🔧 wrench — The unbounded dump can turn a working page into a 500.

The whole comment is emitted in a single end_tag.before(&bids_script, ContentType::Html) (html_processor.rs:365), and that write lands in a BoundedWriter:

// publisher.rs:507
let mut output = BoundedWriter::new(settings.publisher.max_buffered_body_bytes);

// publisher.rs:574-583
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
    if self.inner.len() + buf.len() > self.limit {
        return Err(std::io::Error::other("publisher body exceeded maximum buffered size"));

Default limit is 16 MiB (settings.rs:71-73). The error propagates through write_processed_chunk as TrustedServerError::Proxy → 5xx. So enabling this flag can break the very page it was enabled to debug — and it fails at the </body> hold, after the page has already begun streaming.

The size is realistic, not theoretical: UPSTREAM_RTB_MAX_RESPONSE_BYTES allows 2 MiB per provider (integrations/mod.rs:163), Bid.creative is the raw adm retained for every bid from every provider (losers included), to_string_pretty adds indentation and escapes every " in that HTML, and with prebid debug on, metadata.debug.httpcalls carries the full request and response body for every bidder. The two chained String::replace calls then keep ~3 full copies of the payload live simultaneously, against a Wasm heap this repo's own design notes put at ~16 MB with a 3-4 MB baseline.

Every other partner-influenced payload in this codebase is bounded — MAX_AUCTION_BODY_SIZE (256 KiB, auction/endpoints.rs:42), MAX_CREATIVE_SIZE (1 MiB, creative.rs:310), SIGN_MAX_BODY_BYTES (64 KiB, proxy.rs:38) — and this PR itself caps the error snippet at 512 bytes. This dump is the one thing that isn't capped.

Fix — truncate Bid.creative per bid and cap the total serialized dump (256 KiB, matching MAX_AUCTION_BODY_SIZE) with a <truncated N bytes> marker.

[aram356]

🔧 wrench — Heads-up: this assertion pins the security bug in place.

It asserts that the raw upstream response body is retained in metadata — and metadata is cloned verbatim into ext.orchestrator.provider_details[].metadata of the public POST /auction response (see my comment on line 2143). So fixing that exposure requires deleting or inverting this assertion.

Calling it out so the change doesn't read like a regression when you make it. The replacement test worth having is the inverse:

#[test]
fn parse_response_omits_upstream_body_from_public_metadata() {
    // … 403 with a body …
    assert!(
        !result.metadata.contains_key("body"),
        "upstream response body must not reach ProviderSummary.metadata — \
         it is serialized into the public /auction response"
    );
}

Worth noting the surrounding module already has the gating pattern this path should have followed: enrich_response_metadata_includes_debug_fields_when_enabled (prebid.rs:5877) tests exactly that shape for the config.debug-gated keys.

[aram356]

♻️ refactor — This is the only provider error response in the codebase that omits message.

All four orchestrator error constructors set it (auction/orchestrator.rs:123, 129, 141, 147), so every consumer of provider_details[].metadata can currently rely on error_type + message being present. This path breaks that shape.

Adding a static message (per the SECURITY: note at auction/orchestrator.rs:100-105 — static text only, no upstream content) restores parity and gives the operator a human-readable line without the leak:

.with_metadata("message", serde_json::json!("Prebid returned non-success status"))

[aram356]

⛏ nitpick — .map(neutralise_comment_terminators) applies only to the Ok branch, so the unwrap_or_else fallback string on the next line bypasses the neutraliser entirely.

Unreachable in practice (serde_json won't fail on these types), so this is not a live bug — but it's an un-neutralised seam in a path whose entire safety property is "nothing reaches the DOM un-neutralised." Applying the neutraliser after the fallback instead of inside the Ok branch removes the seam and the need to reason about it:

let providers_dump = neutralise_comment_terminators(
    serde_json::to_string_pretty(&result.provider_responses)
        .unwrap_or_else(|e| format!("<provider_responses serialize error: {e}>")),
);