Skip to content

Make auction creative rewriting optional#916

Open
ChristianPavilonis wants to merge 3 commits into
mainfrom
feature/optional-creative-rewriting
Open

Make auction creative rewriting optional#916
ChristianPavilonis wants to merge 3 commits into
mainfrom
feature/optional-creative-rewriting

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Add a backward-compatible, default-true [auction].rewrite_creatives setting for winning-bid adm returned by POST /auction.
  • Keep server-side creative sanitization mandatory while allowing operators to skip first-party resource/click rewriting and creative TSJS injection.
  • Preserve independent /first-party/proxy HTML/CSS rewriting and document the behavior and privacy trade-offs.

Changes

File Change
crates/trusted-server-core/src/auction_config_types.rs Add rewrite_creatives with explicit serde and Rust defaults of true.
crates/trusted-server-core/src/auction/orchestrator.rs Update the exhaustive test configuration literal.
crates/trusted-server-core/src/auction/formats.rs Always sanitize auction creatives, conditionally rewrite them, add length-only mode logging, and test both modes.
crates/trusted-server-core/src/auction/endpoints.rs Document mandatory sanitization and optional rewriting for /auction responses.
crates/trusted-server-core/src/settings.rs Test omitted and explicitly disabled TOML settings.
crates/trusted-server-core/src/config_payload.rs Test legacy persisted JSON/blob payloads that omit the new field.
crates/trusted-server-core/src/proxy.rs Prove proxied HTML/CSS rewriting remains enabled when auction rewriting is disabled.
trusted-server.example.toml Add and explain the new operator setting.
docs/guide/configuration.md Add the setting, default, scope, and environment override.
docs/guide/auction-orchestration.md Document sanitize-only and sanitize-and-rewrite auction modes.
docs/guide/creative-processing.md Separate auction rewrite control from proxied response processing.
CHANGELOG.md Add an Unreleased entry for optional auction creative rewriting.

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:
    • Cloudflare and Spin native/WASM checks, clippy, tests, and Spin release build
    • CLI and OpenRTB codegen tests and clippy
    • Integration parity format, tests, and clippy
    • Core HTML processor benchmark smoke test
    • JS build and lint
    • Docs lint and production build
    • cargo doc --package trusted-server-core --no-deps --all-features (completed with existing unrelated rustdoc warnings)

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

Allow operators to retain sanitizer-accepted external URLs in POST /auction adm while preserving mandatory server-side sanitization and the existing default behavior.

@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

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

Comment thread crates/trusted-server-core/src/auction_config_types.rs Outdated
Comment thread docs/guide/configuration.md
Comment thread docs/guide/configuration.md
Comment thread docs/guide/creative-processing.md
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

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 prk-Jr 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 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

|| lower.starts_with("about:")
{
// Match exclusions against the same absolute URL used for rewriting.
if settings.rewrite.is_excluded(&absolute) {

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.

📌 out of scope — This changes to_abs() to build the absolute URL before calling settings.rewrite.is_excluded(), instead of checking exclusion on the raw token. That's a real behavior fix: is_excluded() calls url::Url::parse(), which fails on a schemeless string like //trusted-cdn.example.com/x.js, so previously an exclude_domains entry never actually excluded a creative URL that used the protocol-relative form — it got rewritten anyway despite being configured as excluded. It's well tested (new cases around line 1317+), but it's an independent bug fix bundled into the "make rewriting optional" PR and isn't mentioned in the Change table. Worth calling out explicitly in the PR description since it changes existing exclusion behavior on main for protocol-relative URLs.

let sanitized = creative::sanitize_creative_html(raw_creative);
let rewritten = creative::rewrite_creative_html(settings, &sanitized);
let sanitized_len = sanitized.len();
let rewrite_creatives = settings.auction.rewrite_creatives;

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.

🤔 thinkingsettings.auction.rewrite_creatives is read fresh on every iteration of the per-bid loop; it's loop-invariant for the whole response. Trivial cost, not blocking — just flagging that hoisting it above the for loop would be marginally cleaner.

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.

Make creative rewriting optional

3 participants