Add Edge Cookie (EC) sync support for trusted-server integration#98
Add Edge Cookie (EC) sync support for trusted-server integration#98ChristianPavilonis wants to merge 7 commits intomainfrom
Conversation
Implement mocktioneer as a full EC sync partner with three new endpoints: - GET /sync/start: pixel sync redirect chain (sets mtkid, redirects to TS /sync) - GET /sync/done: callback endpoint completing the redirect chain - GET /resolve: S2S pull sync resolution (deterministic UID from ec_hash+IP) Also adds OpenRTB 2.6 user.eids support and EC identity metadata in creatives, enabling end-to-end demo of the trusted-server EC identity pipeline. Security hardening from review: - Constant-time token comparison via subtle::ConstantTimeEq (SHA-256 digest) - Hostname validation on ts_domain (rejects path/auth/port injection) - Domain allowlist via MOCKTIONEER_TS_DOMAINS env var - Hex-only ec_hash validation - Log sanitization for user-supplied values - Deterministic mtkid generation (SHA-256 of host, no randomness)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… for configurable bind address
Prebid Server places eids under user.ext.eids (OpenRTB 2.5) rather than the top-level user.eids (OpenRTB 2.6). extract_ec_info() now checks both locations, with top-level taking priority when both are present.
…ntegration Document the three new EC endpoints (/sync/start, /sync/done, /resolve), the trusted-server integration guide with partner registration walkthrough, and update existing pages (API overview, tracking, creatives, configuration) to reflect deterministic mtkid generation and new route/env var additions.
…d creative metadata
The /resolve endpoint now accepts ec_id in {64-hex}.{6-alnum} format
instead of the bare 64-hex ec_hash prefix. The hash is extracted
internally via extract_ec_hash(). EdgeCookieInfo collapses ec_value
and ec_hash into a single ec_id field. Docs updated to match.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid EC sync implementation with good security practices (constant-time auth, open redirect protection, log sanitization, deterministic UIDs). Two items need resolution before merge.
Findings
🔧 Needs Change
cargo fmtfailure: import ordering inroutes.rs:21—use crate::render::extract_ec_hashis out of alphabetical order. Runcargo fmt.- edgezero deps on feature branch: all five
edgezero-*deps inCargo.toml:24-28point atbranch = "feature/configureable-axum-host"instead ofmain. Merging this puts mocktioneer on an unmerged upstream branch.
❓ Questions
ts_syncedunvalidated:SyncDoneParamsderivesValidatebutts_syncedhas no constraint — any non-"1"value silently means failure. Intentional? (routes.rs:599)
🤔 Thoughts
- Auth fails open on WASM:
std::env::varreturnsErron Cloudflare Workers, silently disabling pull-token auth and domain allowlists. Well-documented in code comments, but for production use, config injection (rather than env vars at request time) would be safer. Not blocking.
🌱 Seeds
ts_reasonunbounded: no max length validation on the query param (routes.rs:602). Low risk since HTTP servers cap query strings, but#[validate(length(max = ...))]would be defensive.
👍 Praise
- Constant-time token comparison (
routes.rs:854-858): SHA-256 both sides beforect_eq— correct approach, no length leak. - Open redirect protection (
routes.rs:641-663): two-layer defense (hostname char validation + optional allowlist) with good injection test coverage. - EC hash extraction (
render.rs:60-70): strict{64-hex}.{6-alnum}format validation with single parse+validate function. - Deterministic UID generation (
routes.rs:794-801):SHA-256(ec_hash|ip)consistent with no-randomness principle. - Log sanitization (
routes.rs:876-881): control char stripping + truncation, used consistently for user-supplied values.
CI Status
- fmt: FAIL (import ordering)
- clippy: PASS
- tests: PASS (100 passed, 1 ignored)
| edgezero-adapter-fastly = { git = "https://github.com/stackpop/edgezero.git", branch = "main", package = "edgezero-adapter-fastly", default-features = false } | ||
| edgezero-cli = { git = "https://github.com/stackpop/edgezero.git", branch = "main", package = "edgezero-cli" } | ||
| edgezero-core = { git = "https://github.com/stackpop/edgezero.git", branch = "main", package = "edgezero-core" } | ||
| edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero.git", branch = "feature/configureable-axum-host", package = "edgezero-adapter-axum", default-features = false } |
There was a problem hiding this comment.
🔧 edgezero deps pinned to unmerged feature branch
All five edgezero-* dependencies point at branch = "feature/configureable-axum-host" instead of main. Merging this PR puts mocktioneer on an unmerged edgezero feature branch — if that branch is rebased, force-pushed, or deleted, CI breaks.
Suggested fix: either (a) merge the edgezero feature branch to main first and repoint these deps, or (b) split the dep bump into its own PR. If the feature branch is genuinely needed for this PR's functionality, document why in the PR description.
There was a problem hiding this comment.
Acknowledged. The edgezero feature/configureable-axum-host branch (PR stackpop/edgezero#231) needs to be merged to main first. Once that lands, we'll repoint all five deps to main. Leaving this unresolved until that happens.
- Add #[validate(length(min=1, max=1))] to ts_synced for early rejection - Add #[validate(length(max=256))] to ts_reason as defensive bound - Fix import ordering for cargo fmt compliance - Add mkt field extraction from user.eids[].uids[].ext.mkt - Log EC/EID info during OpenRTB auction for observability - Configure Fastly service_id and debug logging
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR adds EC sync support with 3 new endpoints, OpenRTB 2.6 EID types, and creative EC metadata. The security posture is strong (constant-time auth, log sanitization, cookie flags), but there are CI blockers and a few design questions that need addressing before merge.
Findings
🔧 Needs Change
cargo fmtfailure: Import ordering issue inroutes.rs— two separateuse crate::render::imports, out of alphabetical order. CI will reject. (routes.rs:21)- edgezero deps pinned to feature branch: All 5 edgezero workspace deps point to
branch = "feature/configureable-axum-host"instead ofmain. This PR can't merge cleanly unless that edgezero branch is merged first. The branch name also has a typo ("configureable" → "configurable"). If the branch is deleted or force-pushed, CI breaks for everyone. (Cargo.toml:24-28) std::env::remove_varunsound in tests: Multiple non-ignored tests callremove_varwhich is deprecated-unsafe since Rust 1.83 (project uses 1.91.1).cargo testruns in parallel — this is a data race. (routes.rs:1483-1695)
❓ Questions
- Deterministic
mtkid= same ID for ALL users on same host:mtkidis derived fromSHA-256("mtkid:" || host). Every visitor to the same host gets the exact same cookie value. This satisfies determinism but defeats the purpose of a buyer UID — trusted-server will see all users as the same buyer. Is this intentional for testing (predictable assertions)? If so, it should be documented prominently. (routes.rs:345-376)
🤔 Thoughts
ts_domainvalidation gaps: Character-class-based, not RFC-compliant. Accepts double-dot TLDs, dash-leading labels, and overlong labels. Acceptable for a mock bidder, but worth noting since the PR emphasizes "open-redirect protection." (routes.rs:575)- Auth silently disabled on Cloudflare Workers:
std::env::varreturnsErron CF Workers, so/resolveauth and domain allowlist are both silently disabled.register_partner.shsetsMOCKTIONEER_PULL_TOKENwhich creates a false sense of security for CF deployments. Consider a startup log warning on wasm32 targets. (routes.rs:764-767)
🌱 Seeds
- GET with query params for S2S
/resolve:ec_idandipappear in access/proxy logs. For S2S this is acceptable, but POST would be cleaner for a production sync API.
📝 Notes
ts_syncedaccepts any string: Only"1"means success; all other values silently treated as failure. (routes.rs:598)ResolveResponse.uidalwaysSome:Option<String>butNoneis never returned. Could simplify toString. (routes.rs:616)ipparam has no format validation: Any 1-45 char string accepted. No security risk (SHA-256 input only), but could confuse callers. (routes.rs:611)
⛏️ Nitpicks
sed -Ein example script: Not POSIX-portable. Minor. (register_partner.sh:25)
CI Status
- fmt: FAIL
- clippy: PASS
- tests: PASS (100 passed, 1 ignored)
| use validator::{Validate, ValidationError}; | ||
|
|
||
| use crate::aps::ApsBidRequest; | ||
| use crate::render::extract_ec_hash; |
There was a problem hiding this comment.
🔧 cargo fmt failure — import ordering
use crate::render::extract_ec_hash is out of alphabetical order relative to the other crate:: imports, and there are two separate use crate::render:: imports (lines 21 and 26). CI will reject this.
Fix: Consolidate into one import block and let cargo fmt sort:
use crate::render::{creative_html, extract_ec_hash, info_html, render_svg, render_template_str, SignatureStatus};| #[test] | ||
| fn handle_resolve_returns_deterministic_uid() { | ||
| // Ensure no auth token is set (tests may run concurrently) | ||
| std::env::remove_var(PULL_TOKEN_ENV); |
There was a problem hiding this comment.
🔧 std::env::remove_var is unsound in concurrent tests on Rust 1.91.1
Multiple non-ignored tests call std::env::remove_var(PULL_TOKEN_ENV) (lines 1483, 1511, 1539, 1588, 1695). Since Rust 1.83, set_var/remove_var are deprecated as unsafe — they are not thread-safe and cargo test runs tests in parallel. This is a data race.
The #[ignore] test at line 1559 correctly notes this, but the non-ignored tests have the same problem.
Fix options:
- Remove
remove_varcalls from non-ignored tests — ensure the env var is never set during normal runs and test the "no auth" path implicitly - Use
#[serial]from theserial_testcrate for all tests that touch env vars - Use a test-local config struct instead of env vars
| const TS_ALLOWED_DOMAINS_ENV: &str = "MOCKTIONEER_TS_DOMAINS"; | ||
|
|
||
| /// Returns true if `s` looks like a valid hostname (no path, auth, port, or fragment). | ||
| fn is_valid_hostname(s: &str) -> bool { |
There was a problem hiding this comment.
🤔 Hostname validation is character-class-based, not RFC-compliant
This rejects path/auth/port/fragment characters but accepts:
evil.com..(double dots)-starts-with-dash- Labels > 63 chars
- Total length > 253 chars
For a mock bidder this is acceptable, but since the PR description emphasizes "open-redirect protection," worth tightening — or at minimum adding a comment noting the intentional trade-off.
|
|
||
| /// Constant-time token comparison using `subtle::ConstantTimeEq`. | ||
| /// Compares SHA-256 digests to avoid leaking length information. | ||
| fn constant_time_token_eq(provided: &str, expected: &str) -> bool { |
There was a problem hiding this comment.
🤔 Correctly implemented constant-time comparison
SHA-256 double-hash before ConstantTimeEq is the right pattern — normalizes length to avoid leaking it via timing. The sha2 crate is well-justified with 3 uses (mtkid, UID derivation, and here).
|
|
||
| #[derive(Deserialize, Validate)] | ||
| struct SyncDoneParams { | ||
| /// Whether the sync succeeded ("1") or failed ("0"). |
There was a problem hiding this comment.
📝 ts_synced accepts any string value
The handler only checks == "1" for success. Values like "yes", "true", "" are silently treated as failure. Consider validating to "0" | "1" with a custom validator, or at minimum documenting the expected values.
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| struct ResolveResponse { |
There was a problem hiding this comment.
📝 uid is Option<String> but always Some
handle_resolve always returns Some(uid) — the None variant is never used. Simplify to String, or add a code path that returns None (e.g., when EC lookup fails).
| #[validate(custom(function = "validate_ec_id"))] | ||
| ec_id: String, | ||
| /// Client IP address. | ||
| #[validate(length(min = 1, max = 45))] |
There was a problem hiding this comment.
📝 ip has no format validation
Only length-validated (1-45 chars). Any string is accepted, including "not-an-ip". No security risk since it's only fed to SHA-256, but could confuse callers expecting validation.
| MOCKTIONEER_PULL_TOKEN="${MOCKTIONEER_PULL_TOKEN:-mtk-pull-token-change-me}" | ||
|
|
||
| # Extract hostname from mocktioneer URL for allowed_return_domains | ||
| MOCKTIONEER_HOST=$(echo "${MOCKTIONEER_BASE_URL}" | sed -E 's|https?://||' | sed -E 's|/.*||') |
There was a problem hiding this comment.
⛏️ sed -E is not POSIX
sed -E is a GNU/BSD extension. Minor since this is an example script, but sed 's|https\{0,1\}://||' would be more portable.
3 participants
Summary
/sync/start,/sync/done,/resolve) that enable mocktioneer to act as a full EC sync partner with trusted-server — supporting pixel sync redirect chains, callback handling, and S2S pull sync resolutionuser.eidssupport and embed EC identity metadata (EdgeCookieInfo) in creative HTML comments, enabling end-to-end demo of the trusted-server EC identity pipelineexamples/register_partner.shfor one-step partner registration with trusted-serverNew Endpoints
/sync/start?ts_domain=...mtkidcookie, redirects browser to TS/sync/sync/done?ts_synced=.../resolve?ec_hash=...&ip=...mtk-{sha256(ec_hash|ip)[0:12]}Security
subtle::ConstantTimeEqon SHA-256 digests (no length leak)ts_domainvalidated as clean hostname (no/,@,:,?,#) + optional allowlist viaMOCKTIONEER_TS_DOMAINSenv varec_hashrequires exactly 64 hex characters; log output sanitized against control charsmtkidderived fromSHA-256("mtkid:" || host)— no randomness per CLAUDE.mdTesting
--ignored)cargo fmt,cargo clippycleanWASM Note
MOCKTIONEER_PULL_TOKENandMOCKTIONEER_TS_DOMAINSusestd::env::varwhich returnsErron Cloudflare Workers. Auth and domain allowlist are silently disabled on that platform. Documented in code comments.