Align signature verification with Trusted Server v1.1#80
Align signature verification with Trusted Server v1.1#80ChristianPavilonis wants to merge 4 commits intomainfrom
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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.
| request_scheme, | ||
| timestamp, | ||
| version, | ||
| )?; |
There was a problem hiding this comment.
🔧 request_host, request_scheme, and ts are all read from ext.trusted_server (untrusted client input) and passed straight into the signed payload with no cross-check against server-observed values.
This means the signature proves "someone with the private key signed some payload" — but not that the payload matches the actual request context. A compromised key for domain A could generate valid signatures claiming any host/scheme/timestamp.
handle_openrtb_auction already has ForwardedHost(host) available but doesn't pass it into verification. Suggested fix:
- Pass the server-observed host and scheme into
verify_request_id_signature - After extracting
request_host/request_schemefromext, compare them against the trusted values - Enforce a
tsfreshness window (e.g. ±5 minutes) to prevent replay
Verification is informational today, but this should be fixed before it's ever promoted to enforcement.
| #[derive(Serialize)] | ||
| struct SigningPayload<'a> { | ||
| version: &'a str, | ||
| kid: &'a str, | ||
| host: &'a str, | ||
| scheme: &'a str, | ||
| id: &'a str, | ||
| ts: u64, | ||
| } |
There was a problem hiding this comment.
🌱 Field order in this struct is load-bearing — serde_json::to_string serializes struct fields in declaration order, and the output is used as the canonical signing payload. If anyone reorders these fields, all existing signatures silently break.
Consider adding a comment like:
// IMPORTANT: Field order defines the canonical signing payload.
// Reordering fields will break signature verification.| fn build_signing_payload_uses_v11_shape() { | ||
| let payload = build_signing_payload( | ||
| "req-123", | ||
| "kid-abc", | ||
| "publisher.example", | ||
| "https", | ||
| 1706900000, | ||
| "1.1", | ||
| ) | ||
| .expect("payload"); | ||
|
|
||
| assert_eq!( | ||
| payload, | ||
| "{\"version\":\"1.1\",\"kid\":\"kid-abc\",\"host\":\"publisher.example\",\"scheme\":\"https\",\"id\":\"req-123\",\"ts\":1706900000}" |
There was a problem hiding this comment.
🔧 Two issues with this test:
1. ts unit mismatch. The test uses 1706900000 (seconds) but the docs describe ts as "Unix timestamp (milliseconds)" and use 1706900000000 in examples. Since SigningPayload just passes through the u64, the canonical payload will differ depending on which unit the Trusted Server actually sends. Align on one — if millis, update the tests; if seconds, update the docs.
2. No positive-path E2E test. This test validates the shape of the canonical payload, but there's no test that actually verifies a valid Ed25519 signature against a known keypair. Adding a deterministic test vector (generate a keypair, sign a canonical payload, verify it round-trips) would catch regressions in both the payload construction and the signature verification path.
| .max_by(|a, b| a.2.partial_cmp(&b.2).unwrap_or(std::cmp::Ordering::Equal)); | ||
|
|
||
| let Some((w, h, price)) = best_size else { | ||
| let Some((w, h, _)) = best_size else { |
There was a problem hiding this comment.
♻️ This code still calls get_cpm() to rank sizes by CPM, picks the highest-CPM size, then throws away the price and uses FIXED_BID_CPM. The size selection is now effectively arbitrary (just happens to match the old CPM table ordering).
Since all bids are the same price, simplify to pick by largest area (or just first standard size). This also removes the last dependency on the old SIZE_MAP CPM values from the bid path.
docs/api/openrtb-auction.md
Outdated
| | `ext.trusted_server.kid` | string | No | Key ID used for signature | | ||
| | `ext.trusted_server.request_host` | string | No | Host included in signed payload | | ||
| | `ext.trusted_server.request_scheme` | string | No | Scheme included in signed payload | | ||
| | `ext.trusted_server.ts` | integer | No | Unix timestamp (milliseconds) | |
There was a problem hiding this comment.
🔧 Docs say ts is "Unix timestamp (milliseconds)" here and the example at line 65 uses 1706900000000, but the verification tests all use 1706900000 (seconds). One of them is wrong — see corresponding comment on verification.rs.
| Without a price override, Mocktioneer uses fixed CPM prices based on ad size. Prices range from $1.70 (300x50) to $4.20 (970x250). Non-standard sizes use an area-based fallback formula. | ||
|
|
||
| See the [complete pricing table](/api/#supported-sizes) for all supported sizes and their CPM values. | ||
| If `imp[].ext.mocktioneer.bid` is present, it is ignored. |
There was a problem hiding this comment.
🔧 This section was updated but several other docs pages still describe the old pricing/override behavior:
openrtb-auction.md:182— "With Price Override" curl example still shows"ext": {"mocktioneer": {"bid": 10.00}}prebidjs.md:76-93— documentsbidas a supported parameter with override explanationprebid-server.md:65,121,163,184,201,212,223,291,313— multiple examples usingmocktioneer.bidaps-bid.md:119— decoded price example shows2.50instead of0.01docs/index.md:24— landing page advertises "price overrides"
A repo-wide docs sweep should accompany this pricing change.
| var validSig = { | ||
| verified: true, | ||
| failed: true, | ||
| not_present: true | ||
| }; | ||
| var sig = validSig[sigParam] ? sigParam : "not_present"; |
There was a problem hiding this comment.
💯 Good call adding the allowlist sanitization for sig. The old code did badges[sig] with raw user input from query params — this bounds it to known-safe values and defaults to not_present. Clean pattern.
… 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
Review feedback addressed in 3bbbbc7🔧 Trust model for signed fields (verification.rs)
🌱 Field order comment on
|
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.
2 participants
Summary
ext.trusted_serverfields (version,kid,request_host,request_scheme,ts) to prevent partial or ambiguous verification inputs.$0.01) for generated OpenRTB and APS responses, and remove request-level bid override behavior from generated ad markup.Changes
crates/mocktioneer-core/src/verification.rsversion,kid,host,scheme,id,ts), required v1.1 ext fields, enforced signing version1.1, and added payload/version tests.crates/mocktioneer-core/src/render.rscrates/mocktioneer-core/static/templates/creative.html.hbsverified/failed/not_present).crates/mocktioneer-core/src/auction.rs$0.01), removed request-supplied bid overrides from response generation, and updated related tests.docs/api/openrtb-auction.mdCloses
Closes #79
Closes #81
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare"cargo run -p mocktioneer-adapter-axumcd tests/playwright && npm test)cargo fmt --all -- --check;cargo test -p mocktioneer-coreChecklist
mocktioneer-core, not in adapter cratesroutes.rsandedgezero.tomlrender.rs, not inlined in handlers#[cfg(test)]or intests/)