Skip to content

Make the SSAT debug comment configurable - #943

Open
prk-Jr wants to merge 26 commits into
mainfrom
feat/ssat-debug-comment-config
Open

Make the SSAT debug comment configurable#943
prk-Jr wants to merge 26 commits into
mainfrom
feat/ssat-debug-comment-config

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds [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.
  • Provides three explicit sensitivity levels: redacted (default, schema-validated server-owned diagnostics), upstream (six named provider-controlled diagnostics), and full (raw response metadata and untruncated creatives for local investigation).
  • Preserves unconditional safeguards in every mode and format: HTML-comment terminator neutralization and a 256 KiB total dump cap.

Configuration

[debug]
auction_html_comment = true

[debug.auction_html_comment_options]
include_provider_responses = true
include_mediator_response = true
include_bids = true
metadata_keys = ["error_type", "http_status", "message"]
verbosity = "redacted" # redacted | upstream | full
format = "compact"     # compact | pretty

format = "pretty" indents the outer auction dump for easier navigation. Nested JSON-looking values such as PBS requestbody and responsebody remain strings exactly as captured.

upstream and full can expose request or identity data and must not be enabled in production.

Security model

  • In redacted, metadata_keys can only narrow the fixed Rust allowlist; it cannot expose arbitrary keys such as PBS debug.
  • error_type and http_status are schema-validated, and message is generated from fixed server-owned wording rather than copied from a provider.
  • Provider-controlled errors, warnings, timings, bid statuses, and upstream messages require upstream or full.
  • Raw PBS debug.httpcalls and resolvedrequest require full.
  • Pretty formatting changes whitespace only; it does not change the dump schema or sensitivity boundary.

Main changes

File Change
crates/trusted-server-core/src/settings.rs Adds configurable section toggles, metadata subset, AuctionDebugCommentVerbosity, and AuctionDebugCommentFormat with backward-compatible defaults
crates/trusted-server-core/src/publisher.rs Applies mode-specific metadata/creative handling and compact/pretty serialization while retaining the unconditional cap and terminator neutralization
trusted-server.example.toml Documents all options, accepted modes/formats, and sensitive-mode warnings
docs/guide/auction-orchestration.md Explains how to enable, push, inspect, and safely use the SSAT HTML debug comment
docs/superpowers/specs/ Includes reviewed designs for configuration/security and output formatting
docs/superpowers/plans/ Includes reviewed test-driven implementation plans

Closes

Closes #935

Test plan

  • cargo fmt --all -- --check
  • cargo test-fastly
  • cargo test-axum
  • cargo test-cloudflare
  • cargo test-spin
  • cargo clippy-fastly
  • cargo clippy-axum
  • cargo clippy-cloudflare
  • cargo clippy-cloudflare-wasm
  • cargo clippy-spin-native
  • cargo clippy-spin-wasm
  • Focused red/green tests cover config defaults and invalid values, compact/pretty semantic equivalence, nested-string fidelity, the total cap, and comment-terminator safety
  • Manual local validation with Full output confirmed provider HTTP calls, resolved requests, bids, prices, and currency are visible according to section toggles
  • GitHub Actions (running after the latest push)

Checklist

  • Changes follow CLAUDE.md conventions
  • Compact remains the default for backward compatibility
  • Unknown verbosity and format values fail configuration loading
  • New behavior has focused regression tests
  • No secrets or credentials committed

@prk-Jr prk-Jr self-assigned this Jul 21, 2026
@aram356 aram356 added this to the 202608 milestone Aug 13, 2026
…nt-config

# Conflicts:
#	crates/trusted-server-core/src/publisher.rs
#	crates/trusted-server-core/src/settings.rs
@aram356
aram356 marked this pull request as draft August 17, 2026 15:26
@prk-Jr
prk-Jr marked this pull request as ready for review August 18, 2026 07:16

@ChristianPavilonis ChristianPavilonis 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

🔧 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,

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.

🔧 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 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

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_keys entries are silently dead config: validate membership in AUCTION_DEBUG_METADATA_ALLOWLIST at config load (crates/trusted-server-core/src/settings.rs:2007)

Non-blocking

♻️ refactor

  • error_type literals duplicated from orchestrator.rs: reuse the ERROR_TYPE_* constants to prevent drift (crates/trusted-server-core/src/publisher.rs:1895)

⛏ nitpick

  • metadata_keys docs omit its Upstream-mode behavior: it still gates the three base keys in upstream mode (crates/trusted-server-core/src/settings.rs:1966, docs/guide/auction-orchestration.md:863)
  • Guide recipe pairs metadata_keys with verbosity = "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) {

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.

🔧 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(

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.

♻️ refactorvalidated_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>,

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.

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"]

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.

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.

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.

For SSAT, make debug comment configurable

3 participants