Skip to content

Align signature verification with Trusted Server v1.1#80

Open
ChristianPavilonis wants to merge 9 commits intomainfrom
fix/request-verification-issue
Open

Align signature verification with Trusted Server v1.1#80
ChristianPavilonis wants to merge 9 commits intomainfrom
fix/request-verification-issue

Conversation

@ChristianPavilonis
Copy link
Contributor

@ChristianPavilonis ChristianPavilonis commented Mar 3, 2026

Summary

  • Align request signature verification with Trusted Server signing v1.1 so signed requests are validated against the canonical payload expected by upstream.
  • Require and parse all v1.1 ext.trusted_server fields (version, kid, request_host, request_scheme, ts) to prevent partial or ambiguous verification inputs.
  • Use a fixed CPM bid value ($0.01) for generated OpenRTB and APS responses, and remove request-level bid override behavior from generated ad markup.
  • Keep signature visibility in creatives consistent while simplifying badge text to signature status only, and document both the v1.1 signing payload and fixed pricing behavior for integrators.

Changes

Crate / File Change
crates/mocktioneer-core/src/verification.rs Reworked verification to build and verify canonical JSON payload (version, kid, host, scheme, id, ts), required v1.1 ext fields, enforced signing version 1.1, and added payload/version tests.
crates/mocktioneer-core/src/render.rs Added coverage asserting the signature badge is always rendered in creative HTML.
crates/mocktioneer-core/static/templates/creative.html.hbs Updated badge behavior so it is always shown and displays signature status only (verified/failed/not_present).
crates/mocktioneer-core/src/auction.rs Switched generated OpenRTB and APS bids to fixed CPM ($0.01), removed request-supplied bid overrides from response generation, and updated related tests.
docs/api/openrtb-auction.md Updated Trusted Server v1.1 signature docs and aligned pricing docs/examples with fixed CPM behavior.

Closes

Closes #79
Closes #81

Test plan

  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo check --workspace --all-targets --features "fastly cloudflare"
  • Manual testing via cargo run -p mocktioneer-adapter-axum
  • Playwright e2e tests (cd tests/playwright && npm test)
  • Other: cargo fmt --all -- --check; cargo test -p mocktioneer-core

Checklist

  • Changes follow CLAUDE.md conventions
  • Business logic lives in mocktioneer-core, not in adapter crates
  • New routes added to both routes.rs and edgezero.toml
  • Determinism preserved — no randomness or time-dependent logic
  • Ad markup rendered via templates in render.rs, not inlined in handlers
  • New code has tests (colocated #[cfg(test)] or in tests/)
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis self-assigned this Mar 3, 2026
Copy link
Contributor

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

The v1.1 signing protocol and SigningPayload canonicalization are solid structural improvements. The fixed-pricing simplification makes sense for a mock bidder. A few things need attention before merge — mostly around the trust model for signed fields and consistency across docs.

Summary

Emoji Count Meaning
🔧 4 Needs change
♻️ 1 Refactoring suggestion
🌱 1 Future consideration
📌 1 Out-of-scope follow-up
💯 1 Positive

Non-inline notes

test_price_from_ext_and_iframe_bid_param (auction.rs:474) — the test name still implies we're verifying that ext.bid drives the price. It now tests the opposite. Rename to something like test_ext_bid_override_is_ignored.

log::info!("URI: {}", uri) at verification.rs:73 looks like a debug leftover from development. The line above already logs at debug level. Demote or remove.

📌 The request example in openrtb-auction.md (lines 52–55) still includes "ext": {"mocktioneer": {"bid": 2.5}} in the impression — contradicts the new Pricing section that says bid overrides are ignored. Remove it from the example to avoid confusion.

ChristianPavilonis added a commit that referenced this pull request Mar 6, 2026
… with fixed pricing

- Cross-check ext.trusted_server host/scheme against server-observed values
- Enforce timestamp freshness window (±5 min) to prevent replay attacks
- Add field-order warning comment on SigningPayload struct
- Fix ts unit mismatch: align tests with docs (milliseconds)
- Add positive-path Ed25519 round-trip test with deterministic keypair
- Remove debug log leftover in fetch_jwks
- Simplify APS size selection to area-based ranking (replaces CPM)
- Remove CPM from /sizes endpoint response
- Rename test to test_ext_bid_override_is_ignored
- Docs sweep: remove bid override references across all doc pages
@ChristianPavilonis
Copy link
Contributor Author

ChristianPavilonis commented Mar 6, 2026

Review feedback addressed in 3bbbbc7

🔧 Trust model for signed fields (verification.rs)

  • verify_request_id_signature now accepts trusted_host and trusted_scheme parameters (server-observed values)
  • After extracting request_host/request_scheme from ext.trusted_server, they are compared against the trusted values — mismatches return VerificationError::InvalidSignature
  • The handler in routes.rs passes ForwardedHost and extracts scheme from the X-Forwarded-Proto header (defaulting to "https")
  • Timestamp freshness: enforced via a ±5 minute window (TS_FRESHNESS_WINDOW_MS). Stale or future timestamps are rejected with a descriptive error including the drift amount.
  • Added tests: verify_host_mismatch_rejected, verify_scheme_mismatch_rejected, verify_stale_timestamp_rejected, verify_future_timestamp_rejected, check_timestamp_freshness_within_window

🌱 Field order comment on SigningPayload

  • Added a prominent comment above the struct explaining that field order defines the canonical signing payload and reordering will silently break verification.

🔧 ts unit mismatch

  • Aligned on milliseconds (matching docs and Trusted Server spec). All test values updated from 17069000001706900000000. The canonical payload example in docs already used millis.

🔧 Positive-path E2E test

  • Added verify_ed25519_roundtrip_with_known_keypair: generates a deterministic Ed25519 keypair from a fixed seed, builds a canonical payload, signs it, and verifies the round-trip. Also asserts that a tampered payload produces SignatureVerificationFailed.

🔧 Docs sweep for pricing

  • openrtb-auction.md: Removed ext.mocktioneer.bid from the Full Request example and removed the "With Price Override" cURL section
  • prebidjs.md: Removed bid parameter from table, removed Price Override section, removed bid values from all examples, removed High CPM Testing section
  • prebid-server.md: Removed bid parameter from table, removed With Price Override section, removed bid values from all examples, updated response price to 0.01
  • aps-bid.md: Updated decoded price example to 0.01, updated amznbid/amznp values to match $0.01 encoding, updated size selection description to area-based
  • docs/index.md: Changed "price overrides" to "fixed pricing"
  • docs/api/index.md: Removed CPM column from sizes table, updated /sizes endpoint response docs

♻️ APS size selection simplification (auction.rs)

  • Replaced get_cpm() ranking with w * h area-based ranking. Since all bids use FIXED_BID_CPM, CPM-based ranking was effectively arbitrary. Largest area is now the explicit selection criterion.
  • Updated related tests in both auction.rs and tests/aps_endpoints.rs

/sizes endpoint consistency (routes.rs)

  • Removed cpm from the /sizes response since it contradicted fixed pricing. The endpoint now returns only width/height per size.

⛏ Quick fixes

  • Renamed test_price_from_ext_and_iframe_bid_paramtest_ext_bid_override_is_ignored
  • Removed log::info!("URI: {}", uri) debug leftover from fetch_jwks

💯 Sig allowlist

  • No changes needed — kept as-is.

All CI gates pass: cargo test --workspace --all-targets (100 tests), cargo fmt, cargo clippy.

ChristianPavilonis added a commit that referenced this pull request Mar 6, 2026
Align remaining docs with fixed /bin/zsh.01 bidding and update Playwright /_/sizes assertions to match the cpm-free response, so review cleanups and CI checks reflect the current API behavior.
Copy link
Contributor

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Round 2

The follow-up commits addressed the first review well — host/scheme cross-checks, timestamp freshness, v1.1 canonical payload, E2E roundtrip test, docs sweep, and APS size selection are all in. Good work.

Remaining issues below, grouped by severity.

What's done well

  • Check ordering — cheap validations (field extraction, host/scheme cross-check, timestamp freshness) all run before the expensive JWKS fetch + crypto. Good.
  • Roundtrip crypto testverify_ed25519_roundtrip_with_known_keypair proves the full sign→verify pipeline and catches tampered payloads. Most valuable test in the file.
  • Creative sig param whitelist — sanitizes sig query param against known-safe values instead of trusting arbitrary input. Clean pattern.
  • Docs consistency — all references to dynamic pricing, price override, and bid param removed across 7 doc files. Thorough.

Copy link
Contributor

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Round 3

Prior rounds addressed host/scheme normalization, dead code removal (get_cpm, DEFAULT_CPM, MAX_AREA_BONUS), unused JS variable, and fragile test assertions. The branch is CI-clean (fmt, clippy, tests, WASM targets all pass).

One blocking item remains from round 2 that was partially addressed — the SIZE_MAP values were documented as legacy but not actually zeroed out.

CI Status

  • fmt: PASS
  • clippy: PASS
  • tests: PASS (all workspace targets)
  • cargo check --features "fastly cloudflare": PASS
  • cargo check --target wasm32-unknown-unknown (Cloudflare): PASS

… with fixed pricing

- Cross-check ext.trusted_server host/scheme against server-observed values
- Enforce timestamp freshness window (±5 min) to prevent replay attacks
- Add field-order warning comment on SigningPayload struct
- Fix ts unit mismatch: align tests with docs (milliseconds)
- Add positive-path Ed25519 round-trip test with deterministic keypair
- Remove debug log leftover in fetch_jwks
- Simplify APS size selection to area-based ranking (replaces CPM)
- Remove CPM from /sizes endpoint response
- Rename test to test_ext_bid_override_is_ignored
- Docs sweep: remove bid override references across all doc pages
Align remaining docs with fixed /bin/zsh.01 bidding and update Playwright /_/sizes assertions to match the cpm-free response, so review cleanups and CI checks reflect the current API behavior.
- Normalize X-Forwarded-Proto: split on comma, trim, lowercase, fall back
  to request URI scheme before defaulting to https
- Add canonicalize_host() to strip default ports (:443/:80) and lowercase
  both sides of host/scheme cross-checks in verification
- Log deprecation warning when imp.ext.mocktioneer.bid is present but ignored
- Remove dead pricing code: get_cpm(), DEFAULT_CPM, MAX_AREA_BONUS
- Update CLAUDE.md key constants to reflect FIXED_BID_CPM
- Remove unused JS variable 'b' in creative template
- Strengthen timestamp test assertions with matches! on error variant
…ot bidder host

The host cross-check was comparing ext.trusted_server.request_host (the
publisher's domain) against the bidder's own ForwardedHost, which will
never match in header bidding. Changed to compare against site.domain
from the OpenRTB request instead.

Also removed the redundant scheme cross-check since request_scheme is
already verified cryptographically as part of the signed payload.
Remove the phf dependency entirely — SIZE_MAP values were zeroed out
and only used for key membership checks. Replace with a sorted const
array of (i64, i64) tuples. O(n) scan over 13 entries is trivially
fast and eliminates the build-time codegen dependency.
Update all references from $0.01 to $0.20 across docs, CLAUDE.md,
and inline code comments to match the actual constant value.
@ChristianPavilonis ChristianPavilonis force-pushed the fix/request-verification-issue branch from 00c23d8 to 4766574 Compare March 23, 2026 22:04
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.

Document fixed CPM behavior for auction responses Align signature verification with Trusted Server v1.1

2 participants