Add Edge Cookie (EC) sync support for trusted-server integration

[ChristianPavilonis]

Summary

• Add three new endpoints (/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 resolution
• Add OpenRTB 2.6 user.eids support and embed EC identity metadata (EdgeCookieInfo) in creative HTML comments, enabling end-to-end demo of the trusted-server EC identity pipeline
• Add examples/register_partner.sh for one-step partner registration with trusted-server

New Endpoints

Route	Method	Purpose
/sync/start?ts_domain=...	GET	Sets mtkid cookie, redirects browser to TS /sync
/sync/done?ts_synced=...	GET	Callback from TS — returns 1x1 pixel
/resolve?ec_hash=...&ip=...	GET	Pull sync — returns deterministic mtk-{sha256(ec_hash|ip)[0:12]}

Security

• Constant-time auth: Token comparison uses subtle::ConstantTimeEq on SHA-256 digests (no length leak)
• Open redirect protection: ts_domain validated as clean hostname (no /, @, :, ?, #) + optional allowlist via MOCKTIONEER_TS_DOMAINS env var
• Input validation: ec_hash requires exactly 64 hex characters; log output sanitized against control chars
• Deterministic IDs: mtkid derived from SHA-256("mtkid:" || host) — no randomness per CLAUDE.md

Testing

• 97 unit tests + 1 ignored (env-var auth, passes separately with --ignored)
• 12 APS integration tests, 7 endpoint integration tests
• cargo fmt, cargo clippy clean

WASM Note

MOCKTIONEER_PULL_TOKEN and MOCKTIONEER_TS_DOMAINS use std::env::var which returns Err on Cloudflare Workers. Auth and domain allowlist are silently disabled on that platform. Documented in code comments.

[aram356]

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 fmt failure: import ordering in routes.rs:21 — use crate::render::extract_ec_hash is out of alphabetical order. Run cargo fmt.
• edgezero deps on feature branch: all five edgezero-* deps in Cargo.toml:24-28 point at branch = "feature/configureable-axum-host" instead of main. Merging this puts mocktioneer on an unmerged upstream branch.

❓ Questions

• ts_synced unvalidated: SyncDoneParams derives Validate but ts_synced has no constraint — any non-"1" value silently means failure. Intentional? (routes.rs:599)

🤔 Thoughts

• Auth fails open on WASM: std::env::var returns Err on 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_reason unbounded: 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 before ct_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)

[aram356]

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 fmt failure: Import ordering issue in routes.rs — two separate use 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 of main. 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_var unsound in tests: Multiple non-ignored tests call remove_var which is deprecated-unsafe since Rust 1.83 (project uses 1.91.1). cargo test runs in parallel — this is a data race. (routes.rs:1483-1695)

❓ Questions

• Deterministic mtkid = same ID for ALL users on same host: mtkid is derived from SHA-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_domain validation 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::var returns Err on CF Workers, so /resolve auth and domain allowlist are both silently disabled. register_partner.sh sets MOCKTIONEER_PULL_TOKEN which 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_id and ip appear in access/proxy logs. For S2S this is acceptable, but POST would be cleaner for a production sync API.

📝 Notes

• ts_synced accepts any string: Only "1" means success; all other values silently treated as failure. (routes.rs:598)
• ResolveResponse.uid always Some: Option<String> but None is never returned. Could simplify to String. (routes.rs:616)
• ip param 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 -E in example script: Not POSIX-portable. Minor. (register_partner.sh:25)

CI Status

• fmt: FAIL
• clippy: PASS
• tests: PASS (100 passed, 1 ignored)

[aram356]

PR Review

Summary

Solid, well-tested addition of the Edge Cookie (EC) sync surface area — three new endpoints, OpenRTB 2.6 user.eids support, and a partner registration script. Security hardening (constant-time auth, hostname validation, log sanitization) is thoughtfully done and the test coverage is adversarial and thorough. However, a handful of issues need to be resolved before merge, most notably an unmerged upstream dep pin, an empty-env-var auth bypass, a silent behavioral change on an already-shipped endpoint, and a well-known default token baked into register_partner.sh.

Findings

🔧 Needs Change

• Workspace deps pin to unmerged upstream branch (Cargo.toml:24-28) — see inline.
• Empty MOCKTIONEER_PULL_TOKEN silently bypasses auth (routes.rs:774-787) — see inline.
• /pixel mtkid semantically changed from per-visitor UUID → per-host deterministic hash (routes.rs:378-410) — see inline.
• Default MOCKTIONEER_PULL_TOKEN baked into register_partner.sh (examples/register_partner.sh:22) — see inline.
• /sync/done Cache-Control doc/code mismatch (docs/api/sync.md:170) — see inline.

❓ Questions

• WASM silent auth/allowlist bypass on Cloudflare — on CF Workers, std::env::var returns Err, so both MOCKTIONEER_PULL_TOKEN and MOCKTIONEER_TS_DOMAINS are silently ignored. Code comments acknowledge this but there's no runtime signal — operators who configure these via wrangler.toml will think they're enforced. Should we log a warning at startup when running on wasm32-unknown-unknown / wasm32-wasip1? Or route secret reads through an adapter-level config hook? At minimum, docs/integrations/trusted-server.md should call this out more prominently than the code comment does.
• Open-redirect-by-default when allowlist is unset (routes.rs:651-663) — see inline.

🤔 Thoughts

• Log injection via attacker-controlled cookie (routes.rs:684-688) — inline.
• Log injection via ip param (routes.rs:803-808) — inline.
• .expect() on post-validation unwrap (routes.rs:792) — inline.
• ts_synced accepts arbitrary strings (routes.rs:723) — inline.
• ip is length-only validated (routes.rs:611) — inline.

⛏ Nitpicks

• Stale empty impl SizeDimensions {} (crates/mocktioneer-core/src/routes.rs:116) — pre-existing, but easy campground cleanup while the file is open.
• extract_ec_hash invoked twice per resolve (validation then extraction) — micro, acceptable.
• sed-based host extraction is fragile (examples/register_partner.sh:25-29) — inline.
• JSON heredoc interpolates env vars unquoted (examples/register_partner.sh:40-58) — inline.
• Docs repeat the well-known default token (docs/api/resolve.md:26) — inline.
• hex_encode uses lowercase while urlencoding uses uppercase HEX_CHARS; consistent internally but the two tables duplicate.

🌱 Seeds

• PARTNER_ID hardcoded (routes.rs:558) — inline.
• Consider a startup log summarizing active EC-sync security posture (which env vars are set/unset). Would help diagnose the empty-token and WASM bypass issues above.
• is_local_host doesn't cover 0.0.0.0 or private ranges — fine for the current test use, note for later.

📌 Out of Scope

• The upstream branch name feature/configureable-axum-host has a typo ("configureable"). That lives in stackpop/edgezero, not here — worth fixing there.

👍 Praise

• Constant-time auth via SHA-256 digest + subtle::ConstantTimeEq (routes.rs:853-858) — textbook-correct. Hashing both sides before ct_eq avoids any length-leak concern.
• HTML-comment -- escape (render.rs:164-165) — small but important detail for embedding attacker-adjacent JSON in a comment.
• Top-level user.eids vs user.ext.eids fallback with top-level priority (render.rs:84-97) — pragmatic handling of the OpenRTB 2.5/2.6 split.
• is_local_host handles bracketed IPv6 with port (routes.rs:861-872) — a detail most implementations miss.
• Adversarial tests included — path injection, auth injection, non-hex ec_id, missing params, deterministic UID verification, cookie reuse. Good coverage.

CI Status

• fmt: PASS
• clippy: PASS (-D warnings, --all-targets --all-features)
• tests: PASS (109 passed, 1 intentionally #[ignore]'d for env-var race)

[prk-Jr]

Summary

This PR adds Edge Cookie (EC) sync infrastructure: two new endpoints (/sync/start, /sync/done), a /resolve endpoint, deterministic mtkid cookie generation via SHA-256, pull-token authorization, EC-ID validation, and accompanying documentation. The overall approach is sound and well-structured. The new tests cover the happy path and several validation edge cases thoroughly.

Three issues block merge as written.

---

Findings

Blocking (must fix before merge)

1. is_valid_hostname accepts bare IP addresses
is_valid_hostname at routes.rs:578 passes 127.0.0.1 as valid (confirmed by the test at line 1715). Any IP literal is therefore accepted as ts_domain, making a redirect to an arbitrary IP address possible. validate_jwks_host in verification.rs explicitly rejects all IP addresses for this exact reason. Add an IpAddr parse guard inside is_valid_hostname (see inline comment on the function).

2. max = 256 should be max = 253 on ts_domain
RFC 1035 caps a valid FQDN at 253 characters. The #[validate(length(min = 1, max = 256))] attribute on SyncStartParams::ts_domain admits three extra bytes that are structurally invalid. The length guard inside is_valid_hostname already uses 253 correctly — the struct attribute should match (see inline comment on the attribute).

3. New EC endpoints absent from tests/endpoints.rs
The integration-level endpoint inventory does not include /sync/start, /sync/done, or /resolve. All three should appear alongside existing entries so that routing regressions are caught at the integration layer and not only in unit tests.

---

Should fix (important but not blocking)

4. Domain allowlist branch is untested
The MOCKTIONEER_TS_DOMAINS code path in handle_sync_start — both the accept-when-in-list and reject-when-not-in-list branches — has zero test coverage. std::env::var is process-scoped and can be set from unit tests; add at least two tests covering the allowed and blocked cases (see inline comment in the test section).

---

Suggestions (non-blocking)

5. is_valid_hostname vs validate_jwks_host asymmetry
is_valid_hostname (routes.rs) and validate_jwks_host (verification.rs:307) perform overlapping but divergent hostname validation: validate_jwks_host trims whitespace, rejects single-label names, and rejects all IPs; is_valid_hostname does none of these. Without a shared helper these two validators will continue to drift. Consider extracting a shared utility (see inline comment on the function).

6. HEX_CHARS uppercase / hex_encode lowercase asymmetry
HEX_CHARS is b"0123456789ABCDEF" (uppercase), but hex_encode produces lowercase output and its own test asserts lowercase. The constant value and name imply uppercase output. Either change the value to b"0123456789abcdef" and rename to HEX_CHARS_LOWER, or add a comment explaining the lowercase override (see inline comment on the constant).

7. Redundant .clone() in render.rs:110
mocktioneer_eid_uid.clone() creates an unnecessary allocation: the value is immediately consumed by .or(...). Because the only post-.or use is .is_some() on line 112, the clone can be removed (see inline comment).

8. OPTIONS routes not covered in integration tests
The new EC sync routes registered in edgezero.toml include implicit OPTIONS handling for CORS preflight. None of the new tests exercise OPTIONS requests on /sync/start, /sync/done, or /resolve. Add at least one OPTIONS test to confirm CORS headers are present.

9. toml = "1.0" unexplained in crates/mocktioneer-core/Cargo.toml
toml was already a workspace dependency before this PR and no new code in this diff reads TOML at runtime. A brief comment or usage reference would clarify its purpose and prevent accidental removal.

[ChristianPavilonis]

Addressed the latest review feedback in d2d3f47.

Summary:

• Rejected IP literals in is_valid_hostname while preserving local/demo hostname support.
• Aligned ts_domain max length with the 253-byte DNS limit.
• Added env-safe allowlist coverage via a pure helper rather than mutating process env in parallel tests.
• Added integration coverage for /sync/start, /sync/done, /resolve, plus OPTIONS/CORS coverage for the new EC routes.
• Clarified the uppercase percent-encoding table name and removed the redundant Option<String> clone in render.rs.
• Removed Mocktioneer’s unused direct toml dependency; EdgeZero still brings its own TOML parser transitively.

Validation passed:

• cargo test -p mocktioneer-core is_valid_hostname
• cargo test -p mocktioneer-core handle_sync_start
• cargo test -p mocktioneer-core --test endpoints
• cargo test --workspace --all-targets
• cargo fmt --all -- --check
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo check --workspace --all-targets --features "fastly cloudflare"

All inline threads from the latest review have been replied to and resolved, and review has been requested again.

[aram356]

👍 Looks good

[prk-Jr]

👍 Looks good