Make auction creative rewriting optional

[ChristianPavilonis]

Summary

• Make winning-bid creative rewriting configurable while keeping server-side sanitization mandatory.
• Preserve rollback compatibility and make the operator migration, privacy, and rendering consequences explicit.
• Fix protocol-relative rewrite-exclusion matching and record the associated first-party signing validation behavior.

Changes

File	Change
CHANGELOG.md	Document optional rewriting and protocol-relative exclusion behavior.
crates/trusted-server-cli/tests/config_env_overlay.rs	Cover migrated overlays, default blob omission, and clean local config diffs.
crates/trusted-server-core/src/auction/README.md	Distinguish mandatory sanitization from configurable rewriting.
crates/trusted-server-core/src/auction/endpoints.rs	Document /auction sanitization and rewrite behavior.
crates/trusted-server-core/src/auction/formats.rs	Use the centralized auction creative processing policy and cover both modes.
crates/trusted-server-core/src/auction/orchestrator.rs	Update the exhaustive configuration test literal.
crates/trusted-server-core/src/auction_config_types.rs	Add the default-true setting and document rollback-safe serialization.
crates/trusted-server-core/src/config_payload.rs	Test and label the legacy blob schema snapshot.
crates/trusted-server-core/src/creative.rs	Normalize protocol-relative exclusions and centralize sanitize-then-rewrite processing.
crates/trusted-server-core/src/proxy.rs	Prove proxy response rewriting remains independent and reject excluded signing targets.
crates/trusted-server-core/src/publisher.rs	Apply the setting to production SSAT inline creatives and cover sanitize-only delivery.
crates/trusted-server-core/src/settings.rs	Parse the setting and warn when enabled auctions disable rewriting.
docs/guide/auction-orchestration.md	Document sanitize-only and sanitize-and-rewrite modes.
docs/guide/cli.md	Document typed config validation, diff, and push flow.
docs/guide/configuration.md	Document setting scope, migration, rollback, and environment overrides.
docs/guide/creative-processing.md	Explain privacy, direct-resource, runtime, and rendering effects.
trusted-server.example.toml	Add and explain the operator setting.

Closes

Closes #914

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: cargo test-cloudflare, cargo test-spin, all target-matched clippy aliases, and ./scripts/test-cli.sh

Checklist

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

[aram356]

Summary

The runtime branch itself is well scoped: sanitization remains mandatory, rewriting is gated after sanitization, both modes have substantive tests, and /first-party/proxy remains independent. I am requesting changes for rollback compatibility and two operator-facing configuration/privacy contracts described in the inline comments.

Non-blocking

🏕 camp site

• Update the internal auction README: crates/trusted-server-core/src/auction/README.md:139, :251, and :382 still describe creative rewriting as unconditional. Please consider updating those passages to distinguish mandatory sanitization from default-on, configurable rewriting.

📌 out of scope

• Track the pre-existing iframe[srcdoc] sanitizer gap: the generic handler at crates/trusted-server-core/src/creative.rs:385 neither removes nor recursively sanitizes srcdoc, while the normal renderer grants allow-scripts and allow-same-origin at crates/trusted-server-js/lib/src/core/render.ts:14. This exists at the base SHA and should not block this PR, but it deserves a security follow-up that removes srcdoc and adds regression coverage through both rewrite modes.

👍 praise

• The implementation keeps sanitization strictly before the configuration branch, avoids logging creative contents, covers default/disabled behavior and legacy blob loading, and verifies that proxy HTML/CSS rewriting is independent. No new dependency, OS API, or WASM-incompatible construct is introduced.

CI Status

• fmt and all adapter/target clippy checks: PASS
• Rust tests and builds (Fastly, Axum, Cloudflare, Spin, parity, CLI): PASS
• JS formatting and Vitest: PASS
• integration, browser, Fastly EC lifecycle, and CodeQL checks: PASS

[ChristianPavilonis]

Implemented and pushed the requested review fixes in 6fcbee0d.

Also addressed the non-blocking internal auction README cleanup. The pre-existing nested iframe[srcdoc] sanitizer gap remains out of scope for this PR and is now tracked in #929.

Validation completed locally:

• all Fastly/Axum/Cloudflare/Spin test aliases
• native CLI tests and cross-adapter parity
• all target-matched clippy aliases plus CLI clippy
• Rust/JS/docs formatting and Vitest

[prk-Jr]

Summary

Adds a default-true [auction].rewrite_creatives setting: creative sanitization for POST /auction stays mandatory, while first-party resource/click rewriting and TSJS injection become optional. Backward-compatibility handling (default omitted from serialized/legacy blobs, explicit false preserved, legacy schema round-trip, EdgeZero env-overlay pre-existing-leaf quirk) is thorough and well tested. No blocking issues found.

Non-blocking

⛏ nitpick

• "unre-written" wording: docs/guide/configuration.md and docs/guide/creative-processing.md repeat the hyphenation "unre-written" (e.g. "sanitized but unre-written HTML") several times. Suggest "not rewritten" or "unrewritten" for readability — no behavior impact.

👍 praise

• Backward-compatibility test coverage: the skip_serializing_if default-omission design plus tests across settings.rs (TOML omitted/explicit-false), config_payload.rs (legacy JSON blob round-trip and legacy-schema deserialization), and the new CLI integration test (config_env_overlay.rs, proving the EdgeZero "env overlay only overrides pre-existing leaves" quirk is handled) directly cover the real rollback/migration edge cases operators will hit. The proxy.rs test proving /first-party/proxy rewriting stays independent of the new flag is a good isolation check too.

📝 note

• Change table is stale: the PR description's file table lists 12 files; the actual diff touches 16, including creative.rs (see inline comment — an independent exclusion-matching bug fix bundled here), crates/trusted-server-cli/tests/config_env_overlay.rs, crates/trusted-server-core/src/auction/README.md, and docs/guide/cli.md. Worth syncing the table with the real diff before merge.

CI Status

• fmt: PASS
• clippy (fastly/axum/cloudflare native+wasm/spin native+wasm): PASS
• rust tests (fastly/axum/cloudflare/spin/cross-adapter parity/CLI): PASS
• js tests (vitest): PASS
• js/docs format: PASS

[aram356]

Summary

Narrowly-scoped, backward-compatible feature with strong test coverage (default, explicit false, TOML parse, blob round-trip, legacy-schema acceptance, CLI env overlay, and a regression test proving /first-party/proxy rewriting is unaffected) and careful documentation. Two items need attention before merge: an undeclared drive-by behavior change in to_abs, and the fact that the rollback-critical skip_serializing_if carries no in-code rationale. Both are inline.

Blocking

🔧 wrench

• Undeclared behavior change in to_abs: moving the exclusion check onto the normalized URL is a real fix — is_excluded never matched protocol-relative URLs because url::Url::parse fails on //host/path — but it is unrelated to rewrite_creatives and appears in neither the PR description nor the CHANGELOG. It also silently tightens the /first-party/proxy tsurl validator (proxy.rs:1633), which is untested. (crates/trusted-server-core/src/creative.rs:53-73)
• skip_serializing_if is load-bearing but unexplained: AuctionConfig is deny_unknown_fields, so omitting the default is what keeps pushed blobs readable by an older binary during rollback. That rationale exists only in docs and a TOML comment, so a future cleanup of the attribute silently breaks rollback. (crates/trusted-server-core/src/auction_config_types.rs:15-19)

❓ question

• Client-side behavior with rewrite_creatives = false: disabling also drops data-tsclick and the tsjs-unified.min.js runtime injection. The docs frame this purely as a privacy trade-off. Is the creative render bridge tolerant of a missing runtime, or does the creative fail to size/render? If rendering depends on it, that is a functional consequence and belongs in the warning block in docs/guide/creative-processing.md.
• ts config diff with the leaf present: raised inline on trusted-server.example.toml:119.

Non-blocking

♻️ refactor

• Gate the flag inside creative: crates/trusted-server-core/src/auction/formats.rs:256-268 (inline).
• Warn when the privacy default is off: crates/trusted-server-core/src/auction/formats.rs:258 (inline).

🤔 thinking

• Hand-mirrored LegacyAuctionConfig will drift: crates/trusted-server-core/src/config_payload.rs:52-72 (inline).
• CLI test does not pin the working directory: crates/trusted-server-cli/tests/config_env_overlay.rs:36-63 (inline).

📌 out of scope

• /first-party/proxy sign handler bypasses to_abs for protocol-relative URLs: the //-prefixed branch builds the absolute URL inline and never calls to_abs, so it skips the exclusion check entirely — the exact inconsistency this PR fixes on the creative path. Pre-existing; worth a follow-up issue now that the sibling path was corrected. (crates/trusted-server-core/src/proxy.rs:1624-1632)
• [debug].inject_adm_for_testing embeds raw, unsanitized adm: correctly documented here as debug-only, but it remains the one creative path with neither sanitization nor rewriting. Follow-up issue. (crates/trusted-server-core/src/publisher.rs:2144-2147)

⛏ nitpick

• rewrite_mode string is unnecessary: crates/trusted-server-core/src/auction/formats.rs:264-268 (inline).

Verification notes

• The docs change from "env vars baked at build time" to "typed CLI overlay" was verified accurate: no TRUSTED_SERVER__ handling remains in crates/trusted-server-core/build.rs.
• [debug].inject_adm_for_testing referenced in the new docs exists as described (settings.rs:1923).
• convert_to_openrtb_response has exactly two production callers, both in auction/endpoints.rs (219, 340), so the new flag's blast radius matches what the docs claim.

CI Status

Taken from the PR's own checks — not re-run locally.

• fmt: PASS
• clippy / CodeQL (Analyze (rust), Analyze (javascript-typescript), Analyze (actions)): PASS
• rust tests (cargo test, axum native, cloudflare, spin, ts CLI native, cross-adapter parity): PASS
• js tests (vitest), format-typescript, format-docs: PASS
• integration tests, Fastly EC lifecycle, browser integration tests: still pending at review time

[aram356]

Summary

Re-review after the "Address creative rewriting review feedback" commits. Every finding from the previous review was addressed and verified (including that the new config-load log::warn! fires on the production blob path, not just TOML parse). One new blocking gap remains: the /first-party/sign protocol-relative branch does not honor exclude_domains, which is inconsistent with this PR's own "Fixed" changelog entry. Details inline on crates/trusted-server-core/src/proxy.rs.

Blocking

🔧 wrench

• /first-party/sign bypasses exclude_domains for protocol-relative URLs (crates/trusted-server-core/src/proxy.rs:1642-1648)

  The CHANGELOG adds: "Protocol-relative creative URLs now honor rewrite.exclude_domains … excluded absolute URLs submitted to /first-party/sign are rejected." But the sign handler's // branch builds abs inline (format!("{}:{}", default_scheme, trimmed)) and never runs the exclusion check, while the else branch routes through to_abs, which now does. Net result:

  • https://cdn.example/asset.js on an excluded host → rejected (proxy_sign_rejects_excluded_absolute_url proves it);
  • //cdn.example/asset.js on the same excluded host → still signed and proxied.

  build_proxy_url_with_extras → build_signed_url_for performs no exclusion check either, so nothing downstream catches it. The two changelog clauses are each individually accurate (one is scoped to "absolute"), but the asymmetry undercuts the protocol-relative headline.

  The // branch can't simply call to_abs — it intentionally preserves the request scheme rather than hardcoding https: — so the fix is a uniform post-check after abs is computed, before signing:

  if settings.rewrite.is_excluded(&abs) {
      return Err(Report::new(TrustedServerError::Proxy {
          message: "unsupported url".to_string(),
      }));
  }

  Either apply that (and add a //-host case to proxy_sign_rejects_excluded_absolute_url), or narrow the changelog to state the sign path enforces exclusions only for absolute URLs and explain why protocol-relative is intentionally exempt.

Non-blocking

🌱 seedling

• [debug].inject_adm_for_testing remains the one creative path with neither sanitization nor rewriting: pre-existing and correctly documented here as debug-only. Now that auction adm is uniformly sanitized, this raw-adm debug path is the remaining asymmetry — worth a follow-up issue so it is not mistaken for a production-safe toggle. (crates/trusted-server-core/src/publisher.rs:2144-2147)

📝 note

• Prior review resolutions confirmed: to_abs behavior change now carries a ### Fixed CHANGELOG entry plus proxy-validator test coverage; skip_serializing_if has an explanatory doc comment; config diff no-op is asserted; the sanitize+conditional-rewrite policy is centralized in creative::process_auction_creative; the disabled-rewrite warn fires via finalize_deserialized on both TOML and blob load paths; LegacyAuctionConfig carries a drift-guard comment; the CLI test pins its working directory.
• New public API surface is intentional: creative is pub mod, so process_auction_creative is public — consistent with its pub siblings sanitize_creative_html / rewrite_creative_html.

CI Status

Taken from the PR's own checks (full run, all green):

• fmt: PASS
• clippy / CodeQL (Analyze (rust), Analyze (javascript-typescript), Analyze (actions)): PASS
• rust tests (cargo test, axum native, cloudflare, spin, ts CLI native, cross-adapter parity): PASS
• integration tests, Fastly EC lifecycle, browser integration tests: PASS
• js tests (vitest), format-typescript, format-docs: PASS

[aram356]

Inline version of the blocking finding from the prior change-request review (that finding could not be anchored inline because the sign-handler lines are unchanged by this PR; anchoring it here on the new test, which is what needs extending).

[prk-Jr]

Summary

Makes winning-bid creative rewriting configurable via a default-true [auction].rewrite_creatives, keeps sanitization mandatory, and fixes protocol-relative URL matching against rewrite.exclude_domains. The serialization design (default omitted, explicit false retained) and its rollback rationale are solid and well covered by tests. Three blocking issues: the setting does not reach the server-side auction (SSAT) inline adm render path, two guides describe that path incorrectly, and the protocol-relative exclusion fix does not reach /first-party/sign.

Blocking

🔧 wrench

• [auction].rewrite_creatives is not honored on the SSAT inline adm path: build_bid_map calls sanitize_creative_html then rewrite_inline_creative_html unconditionally (crates/trusted-server-core/src/publisher.rs:3259-3274). That is the production server-side-auction render path since #899 and it is what populates window.tsjs.bids. With rewrite_creatives = false, POST /auction returns sanitized-only markup while page-rendered SSAT creatives still receive first-party proxy/click URLs and data-tsclick. Either gate that rewrite call on the setting (keeping sanitization unconditional), or scope the setting name and docs explicitly to POST /auction. No test covers the SSAT path under the new flag.

• Two guides describe the page-bids adm path incorrectly (docs/guide/creative-processing.md:80-82, docs/guide/auction-orchestration.md:583-584): both state the path is "debug-only [debug].inject_adm_for_testing" and "may include raw adm". inject_adm_for_testing is passed as include_debug_bid (publisher.rs:3698) and gates only the debug_bid diagnostic blob (publisher.rs:3281). The adm insertion at publisher.rs:3259-3274 is unconditional, and the value is always sanitized and rewritten — never raw.

• Protocol-relative exclusion fix does not reach /first-party/sign (crates/trusted-server-core/src/proxy.rs:1642-1655): the trimmed.starts_with("//") branch builds the absolute URL from the request scheme and bypasses to_abs entirely, so no is_excluded check runs. https://excluded.example/a.png is now rejected (covered by the new test), but //excluded.example/a.png is still signed. Creative HTML leaves that URL direct while the signing endpoint mints a proxy token for it. Add the exclusion check to that branch and a matching protocol-relative test.

Non-blocking

🤔 thinking

• 502 for client-supplied URLs at /first-party/sign: unsupported url maps to BAD_GATEWAY; 400 fits client input better. Pre-existing, but the new test now pins 502 as the contract.

👍 praise

• Rollback-safe serialization: skip_serializing_if plus the deny_unknown_fields LegacyAuctionConfig mirror test is the right way to prove a defaulted field stays readable by the previous binary schema.
• to_abs exclusion fix addresses the actual root cause — is_excluded parses with url::Url::parse, which fails on //host/path and silently returned false. Dropping the explicit data:/javascript:/blob: skip-list is safe: the final else returns None for all of them.

CI Status

All 19 checks pass (fmt, clippy/check across fastly, axum, cloudflare, spin, parity, CLI tests, vitest, docs format, integration and browser suites).

[ChristianPavilonis]

Implemented and pushed the remaining review fixes in 710bf493.

• [auction].rewrite_creatives now controls the production SSAT/page-bids inline adm path while sanitization remains mandatory.
• /first-party/sign now rejects excluded protocol-relative targets as well as excluded absolute targets.
• The creative-processing, configuration, orchestration, auction README, and changelog wording now describes both auction delivery contexts accurately.
• The pre-existing TrustedServerError::Proxy → 502 mapping is intentionally unchanged in this PR.

Validation completed locally:

• cargo test-fastly, cargo test-axum, cargo test-cloudflare, cargo test-spin
• cross-adapter parity and native CLI tests
• all target-matched Fastly/Axum/Cloudflare/Spin clippy aliases
• Rust, JS, and docs formatting
• Vitest (425 tests)

[aram356]

🌱 seedling — process_auction_creative is pub but has no caller outside trusted-server-core (only auction::formats uses it), while its sibling process_inline_auction_creative just below is deliberately pub(crate). CLAUDE.md advises avoiding unnecessary pub, so pub(crate) would be tighter and match the inline sibling.

Non-blocking: the other creative entry points (sanitize_creative_html, rewrite_creative_html, rewrite_inline_creative_html) are all pub too, so leaving this as-is is a defensible consistency call. Flagging only for your judgment.