Make the SSAT debug comment configurable - #943
Conversation
…edundant closures)
…nt-config # Conflicts: # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-core/src/settings.rs
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
🔧 Requesting changes because the new default configuration serialization breaks rollback and mixed-version deployment compatibility. One P1 inline finding is included.
| /// Content and verbosity of the `auction_html_comment` dump. Ignored | ||
| /// when `auction_html_comment` is false. | ||
| #[serde(default)] | ||
| pub auction_html_comment_options: AuctionDebugCommentOptions, |
There was a problem hiding this comment.
🔧 P1 — Preserve rollback compatibility for default config blobs.
ts config push serializes the complete Settings blob. Because this field has no skip_serializing_if, an omitted/default options table is emitted in every blob. The previous binary's DebugConfig uses deny_unknown_fields, so it rejects that blob during a mixed-version deployment or emergency rollback. This is the compatibility contract already enforced for default AuctionConfig additions.
Add a default predicate (or PartialEq-based helper) and skip_serializing_if so default options are omitted while non-default options still serialize. Please add a config_payload regression test that deserializes the default debug payload through a legacy DebugConfig schema with deny_unknown_fields.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds [debug.auction_html_comment_options] — section toggles, a metadata_keys subset selector, a redacted/upstream/full verbosity ladder, and compact/pretty formatting — replacing the old single hardcoded allowlist. The security design is sound: the redacted-mode schema validation matches exactly what orchestrator.rs and prebid.rs actually write, all six upstream keys have real writers, the dropped "status" allowlist key has no production writer, and terminator neutralization plus the 256 KiB cap are exercised across every verbosity × format combination. One blocking config-validation gap and three smaller items, all inline.
Blocking
🔧 wrench
- Unknown
metadata_keysentries are silently dead config: validate membership inAUCTION_DEBUG_METADATA_ALLOWLISTat config load (crates/trusted-server-core/src/settings.rs:2007)
Non-blocking
♻️ refactor
error_typeliterals duplicated fromorchestrator.rs: reuse theERROR_TYPE_*constants to prevent drift (crates/trusted-server-core/src/publisher.rs:1895)
⛏ nitpick
metadata_keysdocs omit its Upstream-mode behavior: it still gates the three base keys inupstreammode (crates/trusted-server-core/src/settings.rs:1966,docs/guide/auction-orchestration.md:863)- Guide recipe pairs
metadata_keyswithverbosity = "full", where the field has no effect (docs/guide/auction-orchestration.md:842)
CI Status
- GitHub checks: pending at review time (fresh push)
- fmt (local): PASS
- targeted rust tests, new settings + publisher tests via
cargo test-fastly(local): PASS (11/11)
| } | ||
|
|
||
| impl AuctionDebugCommentOptions { | ||
| pub(crate) fn normalize(&mut self) { |
There was a problem hiding this comment.
🔧 wrench — Unknown metadata_keys entries are silently dead config. normalize() trims and drops empty strings but never checks membership in AUCTION_DEBUG_METADATA_ALLOWLIST. The struct's own doc comment justifies deny_unknown_fields with "an operator typo must fail config load loudly, not be silently ignored" — yet metadata_keys = ["http_staus"] (typo) or ["errors"] (a plausible expectation, since errors was in the pre-PR default allowlist) silently renders metadata: {} with no signal anywhere. Since upstream keys are unlocked by verbosity, not by metadata_keys, and full ignores the field, its meaningful domain is exactly the three allowlist keys — any other entry is dead config.
Fix: reject keys outside the allowlist during config load (e.g. in prepare_runtime, which already returns Result), keeping the render-time intersection as defense-in-depth. This doesn't contradict the spec's silent-drop security invariant — that boundary stays; this adds the loud-failure UX the config layer already promises. Please add a test alongside, and one for deny_unknown_fields itself (e.g. metadata_key = [...]), which is currently untested.
| /// dump: only [`DEBUG_DUMP_METADATA_ALLOWLIST`] metadata keys survive, and each | ||
| /// bid's creative is previewed to [`MAX_BID_CREATIVE_DUMP_BYTES`]. | ||
| /// Return a recognized server-owned provider error classification. | ||
| fn validated_error_type( |
There was a problem hiding this comment.
♻️ refactor — validated_error_type and safe_error_message hardcode "parse_response" | "launch_failed" | "transport" | "timeout" | "http_status", duplicating the ERROR_TYPE_* constants at auction/orchestrator.rs:112-120 (mostly private today). A future error classification added in the orchestrator silently vanishes from redacted dumps via the _ => None arm, and no test would catch the drift.
Fix: make the constants pub(crate) and use them as match patterns (&'static str consts are valid patterns), or centralize the classification→message mapping next to the constants.
| /// fixed allowlist are always dropped, config or not. This selector cannot | ||
| /// unlock provider diagnostics. Ignored when `verbosity` is `Full`. | ||
| #[serde(default = "default_auction_debug_metadata_keys")] | ||
| pub metadata_keys: Vec<String>, |
There was a problem hiding this comment.
⛏ nitpick — The docstring says the subset applies in Redacted mode and is "Ignored when verbosity is Full", but in Upstream mode it still gates the three base keys (Upstream builds on redacted_metadata_for_dump in publisher.rs). The guide table in docs/guide/auction-orchestration.md has the same gap. One sentence in each would make the Upstream behavior explicit.
| include_provider_responses = true | ||
| include_mediator_response = false | ||
| include_bids = false | ||
| metadata_keys = ["error_type", "http_status", "message"] |
There was a problem hiding this comment.
⛏ nitpick — This copy-paste recipe sets metadata_keys = [...] together with verbosity = "full", where the field has no effect. Operators tweaking the recipe may expect it to filter. Drop the line from this example or annotate it.
Summary
[debug.auction_html_comment_options]for controlling SSAT<!-- ts-debug: ... -->auction comments: provider/mediator/bid section toggles, a fail-closed metadata subset, sensitivity modes, and compact or pretty outer JSON formatting.redacted(default, schema-validated server-owned diagnostics),upstream(six named provider-controlled diagnostics), andfull(raw response metadata and untruncated creatives for local investigation).Configuration
format = "pretty"indents the outer auction dump for easier navigation. Nested JSON-looking values such as PBSrequestbodyandresponsebodyremain strings exactly as captured.upstreamandfullcan expose request or identity data and must not be enabled in production.Security model
redacted,metadata_keyscan only narrow the fixed Rust allowlist; it cannot expose arbitrary keys such as PBSdebug.error_typeandhttp_statusare schema-validated, andmessageis generated from fixed server-owned wording rather than copied from a provider.upstreamorfull.debug.httpcallsandresolvedrequestrequirefull.Main changes
crates/trusted-server-core/src/settings.rsAuctionDebugCommentVerbosity, andAuctionDebugCommentFormatwith backward-compatible defaultscrates/trusted-server-core/src/publisher.rstrusted-server.example.tomldocs/guide/auction-orchestration.mddocs/superpowers/specs/docs/superpowers/plans/Closes
Closes #935
Test plan
cargo fmt --all -- --checkcargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincargo clippy-fastlycargo clippy-axumcargo clippy-cloudflarecargo clippy-cloudflare-wasmcargo clippy-spin-nativecargo clippy-spin-wasmChecklist
CLAUDE.mdconventions