Implement Edge Cookie identity system with KV graph and integration tests

[ChristianPavilonis]

Summary

• Implement the Edge Cookie (EC) identity system — a publisher-owned, first-party cookie-based identity mechanism that replaces the previous Synthetic ID concept. EC enables consent-gated identity generation, a KV-backed identity graph with CAS concurrency, partner sync (pixel, batch, pull), an identify endpoint, and auction bidstream decoration with partner EIDs.
• Add 7 integration test scenarios covering the full EC lifecycle: generation, consent withdrawal, identify edge cases, concurrent partner syncs, and batch sync flows.
• Rename "Synthetic ID" → "Edge Cookie" across the entire codebase, configuration, and documentation.

Changes

File / Area	Change
crates/trusted-server-core/src/ec/ (new module)	Full EC implementation: mod.rs (lifecycle orchestration), generation.rs (HMAC-SHA256), cookies.rs (cookie read/write), consent.rs (consent gating), device.rs (device signal derivation, bot gate), kv.rs + kv_types.rs (KV identity graph with CAS), partner.rs (partner registry + admin), sync_pixel.rs (pixel sync), batch_sync.rs (S2S batch sync), pull_sync.rs (background pull sync), identify.rs (identity lookup with CORS), eids.rs (EID encoding), finalize.rs (response finalization middleware), admin.rs (admin endpoints)
crates/integration-tests/	7 new EC lifecycle test scenarios in tests/common/ec.rs and tests/frameworks/scenarios.rs; updated integration.rs test runner and Viceroy config fixtures
crates/trusted-server-adapter-fastly/src/main.rs	New EC endpoint routes namespaced under /_ts/api/v1/ and /_ts/admin/; send_to_client() pattern with background pull sync dispatch
crates/trusted-server-core/src/auction/	endpoints.rs and formats.rs updated to decorate bid requests with partner EIDs from the KV identity graph (user.id, user.eids, user.consent)
crates/trusted-server-core/src/integrations/prebid.rs	Bidder deduplication fix; blank auction EC header handling
crates/js/lib/src/integrations/prebid/index.ts	JS-side Prebid integration updates for EC headers
crates/trusted-server-core/src/synthetic.rs	Deleted — replaced by the EC module
crates/trusted-server-core/src/cookies.rs	Slimmed down; EC-specific cookie logic moved to ec/cookies.rs
crates/trusted-server-core/src/settings.rs + settings_data.rs	EC configuration fields, partner config, consent config migration
crates/trusted-server-core/src/consent/	Updated consent module for EC-specific consent checks; KV consent storage
crates/trusted-server-core/src/http_util.rs	New HTTP helpers for EC endpoints (CORS, request parsing)
crates/trusted-server-core/src/proxy.rs	EC header propagation in proxy layer
crates/trusted-server-core/src/publisher.rs	Publisher config updates for EC
crates/trusted-server-core/src/constants.rs	New EC-related constants
docs/guide/ec-setup-guide.md (new)	End-to-end EC setup documentation
docs/guide/edge-cookies.md (new)	EC concept overview
docs/guide/api-reference.md	API reference updated with all EC endpoints
docs/guide/configuration.md	Configuration docs updated for EC settings
docs/guide/synthetic-ids.md	Deleted — replaced by EC docs
docs/ (various)	Documentation updates reflecting Synthetic ID → Edge Cookie rename
docs/internal/superpowers/specs/2026-03-24-ssc-technical-spec-design.md	EC technical spec updated with schema extensions, device signals, bot gate
fastly.toml	New KV store bindings for EC
trusted-server.toml	EC configuration section added

Closes

Closes #532
Closes #533
Closes #534
Closes #535
Closes #536
Closes #537
Closes #538
Closes #539
Closes #540
Closes #541
Closes #542
Closes #543
Closes #544
Closes #611
Closes #612

Follow-up

Follow-up: The pull sync mechanism (ec/pull_sync.rs) currently passes ec_id and client_ip as query parameters to partner endpoints. While connections are HTTPS-only, these values can appear in partner server access logs and CDN logs. A future improvement should migrate to POST body or hash/encrypt identifiers before transmission. Tracked separately.

Test plan

• cargo test --workspace
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• Other: 7 EC lifecycle integration test scenarios (full lifecycle, consent withdrawal, identify edge cases, concurrent partner syncs, batch sync)

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

Comprehensive EC identity subsystem: HMAC-based ID generation, KV identity graph with CAS concurrency, partner sync (pixel, batch, pull), identify endpoint, and auction bidstream decoration. The architecture is well-designed — clean separation of concerns, strong input validation, and solid concurrency model. A few cleartext logging issues and a docs inconsistency need addressing before merge.

Blocking

🔧 wrench

• Cleartext EC ID logging: Full EC IDs are logged at warn!/error!/trace! levels in 5 locations across ec/mod.rs (lines 128, 211, 296), sync_pixel.rs (line 91), and pull_sync.rs (line 91). The log_id() truncation helper was introduced in this PR for exactly this purpose but is not used consistently. See inline comments for fixes.
• Stale env var in docs: TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY still appears in configuration.md (lines 40, 956) and error-reference.md (line 154). The PR renamed the TOML section from [edge_cookie] to [ec] and updated some env var references but missed these. Users following the docs will set a variable that is silently ignored, resulting in a startup failure from the placeholder passphrase. Should be TRUSTED_SERVER__EC__PASSPHRASE.

❓ question

• HttpOnly omitted from EC cookie (ec/cookies.rs:11): Intentionally omitted for a hypothetical future JS use case. Is there a concrete plan? The identify endpoint already exposes the EC ID. If not, HttpOnly would be cheap XSS defense-in-depth.

Non-blocking

🤔 thinking

• Uppercase EC ID rejection in batch sync (batch_sync.rs:209): is_valid_ec_id rejects uppercase hex, but batch sync validates before normalizing (line 225). Partners submitting uppercase EC IDs get invalid_ec_id instead of normalization.

♻️ refactor

• _pull_enabled index lacks CAS (partner.rs:564): Read-modify-write without generation markers. Concurrent partner registrations can overwrite each other's index updates. Self-healing via fallback, but inconsistent with the CAS discipline used elsewhere.

🌱 seedling

• Integration tests don't verify identify response shape: FullLifecycle and ConcurrentPartnerSyncs only assert uids.<partner>. The ec, consent, degraded, eids, and cluster_size fields from the API reference are never checked.
• Pixel sync failure paths untested end-to-end: ts_reason=no_ec, no_consent, domain mismatch redirects are unit-tested but not covered in integration tests.

📝 note

• trusted-server.toml ships banned placeholder: passphrase = "trusted-server" is rejected by reject_placeholder_secrets. Works in CI via env override, but new contributors will hit a confusing startup error. A TOML comment would help.

⛏ nitpick

• Eid/Uid missing Deserialize: openrtb.rs types derive Serialize only. If the auction ever needs to parse EIDs from provider responses, Deserialize will be needed.

CI Status

• cargo fmt: PASS
• clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: FAIL
• CodeQL: FAIL (likely related to cleartext logging findings above)

[prk-Jr]

Summary

Comprehensive EC identity subsystem: HMAC-based ID generation, KV identity graph with CAS concurrency, partner sync (pixel, batch, pull), identify endpoint, and auction bidstream decoration. The architecture is well-designed with clean separation of concerns, strong concurrency discipline, and solid security choices throughout. Four new blocking findings join the existing ones — three are input-validation gaps and one is a PII leak in the pull sync URL builder that should be resolved before merge.

Blocking

🔧 wrench

• PII leak: client IP forwarded to partners — pull_sync.rs:273: build_pull_request_url appends the raw user IP as the ip query parameter on every outbound pull-sync request, exposing it in partner access logs. Contradicts the privacy-preserving design intent and is likely a GDPR Article 5 concern. Remove the ip parameter or gate it behind an explicit per-partner allow_ip_forwarding config flag.
• Pull sync response body unbounded — pull_sync.rs:325: take_body_bytes() with no size cap before JSON parsing. A malicious partner can exhaust WASM memory. Batch sync and admin endpoints both have MAX_BODY_SIZE guards; this path needs one too (64 KiB is generous for a {"uid":"..."} response).
• Whitespace-only UIDs bypass validation — batch_sync.rs:217 and sync_pixel.rs:41: is_empty() passes " " and "\t", which get stored as garbage EID values in the KV graph. Replace with trim().is_empty() at both sites.
• rand::thread_rng() WASM compatibility unverified — generation.rs:52: requires getrandom with the wasi feature on wasm32-wasip1. Native cargo test passing does not prove the WASM build is safe; integration tests are already failing so this may not have been caught. Switch to OsRng or add getrandom = { version = "0.2", features = ["wasi"] } explicitly.

Non-blocking

🤔 thinking

• process_mappings UpsertResult branches untested — batch_sync.rs:232: NotFound, ConsentWithdrawn, and Stale have zero unit test coverage. Stale is documented as "counted as accepted" with no regression test.
• Shared error messages make pull sync validation tests non-isolating — partner.rs:152,165: both missing-URL and missing-token conditions return the identical error string, so the tests asserting on substrings pass even if the wrong branch fires. Use distinct messages per condition.

♻️ refactor

• ec_consent_granted has no tests — consent.rs:20: the entry-point gate for all EC creation has no #[cfg(test)] section. Add smoke tests for granted and denied paths.
• KV tombstone path never exercised in finalize tests — finalize.rs:149: all finalize tests pass kv: None, so the tombstone write and the cookie-EC ≠ active-EC case in withdrawal_ec_ids are untested.
• Missing HMAC prefix stability test — generation.rs:228: no test asserts that the same IP + passphrase always produces the same 64-char hash prefix, which is the core deduplication guarantee for the KV graph.
• normalize_ec_id duplicated in integration tests — integration-tests/tests/common/ec.rs:374: reimplements normalize_ec_id_for_kv from core. Re-export and use the canonical implementation.

⛏ nitpick

• Use saturating_sub for consistency — kv.rs:605: the subtraction is safe due to the guard above but inconsistent with saturating_sub used throughout the rest of the module.
• log_id should encapsulate the … suffix — mod.rs:51: every call site manually appends "…" in the format string; move it inside the function.

Praises 👍

• CAS-based optimistic concurrency (kv.rs): bounded retries, graceful ItemPreconditionFailed handling, and MAX_CAS_RETRIES preventing infinite loops — textbook correct for a single-writer KV model.
• Constant-time API key comparison (partner.rs): subtle::ConstantTimeEq for timing attack prevention on key lookups. Many implementations miss this entirely.
• KvMetadata fast-path consent check (kv_types.rs): mirroring ok/country/cluster_size in metadata to avoid streaming the full KV body is an excellent performance optimisation with the right constraint test.
• evaluate_known_browser with OnceLock-cached hash table (device.rs): pre-hashing fingerprints once and caching via OnceLock is the right WASM-compatible lazy-init pattern.
• HMAC stability explicitly documented (generation.rs:30-33): noting that the output format is a "stable contract" that would invalidate all existing identities if changed is exactly the kind of correctness annotation that prevents future breakage.

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: FAIL
• CodeQL: FAIL (cleartext logging — covered in prior review)

[aram356]

Summary

This PR lands a large, well-partitioned Edge Cookie identity subsystem, and the prior blocking feedback (PII logging via log_id(), whitespace UID checks, pull-sync response size cap, dropping raw client IP from pull-sync query strings, constant-time auth comparisons) has been materially addressed. Blocking issues remaining are: a CI-red integration test (deterministic, root cause in the test client, not the server), an unverified rand::thread_rng() on wasm32-wasip1 that CI cannot catch because no wasm build runs, two new CodeQL alerts on ec/kv.rs riding the pre-existing settings-taint false-positive flow, one untested backend-state machine in batch_sync, and two questions on the auction / partner surfaces.

Blocking

🔧 wrench

• CI red — test_ec_lifecycle_fastly deterministic failure: the test client omits Sec-Fetch-Dest: document and Accept: text/html, so is_navigation_request() correctly returns false and EC never generates (crates/integration-tests/tests/common/ec.rs:93).
• rand::thread_rng() on wasm32-wasip1 unverified; CI builds no wasm target (crates/trusted-server-core/src/ec/generation.rs:52). Add a wasm build to CI and/or switch to an explicit getrandom with the wasi feature.
• Two new CodeQL alerts on ec/kv.rs:637 and :789 ride the same settings.reject_placeholder_secrets() taint flow that already produces noise on main. Both are false positives but block the CodeQL gate. Fix with a scoped suppression or a repo-level CodeQL config filter on rust/cleartext-logging for settings-derived values.
• UpsertResult::{NotFound, ConsentWithdrawn, Stale} untested in batch_sync::process_mappings (crates/trusted-server-core/src/ec/batch_sync.rs:231-244). Prior reviewer flagged this; still unresolved.

❓ question

• Auction user.id = "" on the no-consent path is emitted into the OpenRTB bid request JSON (crates/trusted-server-core/src/auction/endpoints.rs:60-77, formats.rs:186). Has this been validated against the live Prebid Server target? If not, change UserInfo.id to Option<String> with skip_serializing_if.
• update_pull_enabled_index race self-heal path untested (crates/trusted-server-core/src/ec/partner.rs:552-604). The documented fallback at partner.rs:350 is the only thing keeping the index correct under concurrent upserts — please add a concurrency test or file a tracked follow-up.

Non-blocking

🤔 thinking

• MAX_CAS_RETRIES = 3, no backoff — likely to starve under contention on hot prefixes; consider exponential backoff + jitter (crates/trusted-server-core/src/ec/kv.rs:300-339).
• HashMap<String, KvPartnerId> non-deterministic serialization — breaks future hash-based dedup and byte-diffing of stored values; use BTreeMap or IndexMap (crates/trusted-server-core/src/ec/kv_types.rs:59).
• seen_domains drop-newest eviction freezes long-lived ECs on their first 50 domains, defeating the last timestamp; switch to LRU or document (crates/trusted-server-core/src/ec/kv.rs:627-642).

♻️ refactor

• EID gating failures log at debug! — bump to warn! so ad-stack anomalies surface in production (crates/trusted-server-core/src/auction/endpoints.rs:80).
• ec_consent_granted() is a 1-line pass-through — inline or document the sealing-point intent (crates/trusted-server-core/src/ec/consent.rs:20-22).
• Unvalidated domain strings in EC graph writes — add a 255-byte length cap and hostname-shape check at the write boundary (crates/trusted-server-core/src/ec/kv.rs update_last_seen, kv_types.rs).

🌱 seedling

• No integration test for the GPC / consent-denied identify path. Once the Sec-Fetch-Dest fix lands, add ec_full_lifecycle_with_gpc sending Sec-GPC: 1 and asserting /_ts/api/v1/identify returns 403 / no EID payload (crates/integration-tests/tests/frameworks/scenarios.rs:501-565).
• No wasm-target build in CI — root cause of how the rand::thread_rng() concern reached this point unnoticed. Adding cargo build -p trusted-server-adapter-fastly --target wasm32-wasip1 would catch an entire class of deps-pin regressions.

📌 out of scope

• Pull-sync EC ID still travels as a URL query parameter — acknowledged in the PR description as deferred. Please file the follow-up issue and reference it from the commit that ships this PR so it is not lost (crates/trusted-server-core/src/ec/pull_sync.rs:260-263).

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test (native): PASS
• vitest: PASS
• integration tests: FAIL — test_ec_lifecycle_fastly (see blocking #1)
• CodeQL: FAIL — 15 high alerts; 13 are pre-existing settings.reject_placeholder_secrets() taint-flow false positives already present on main, 2 are new on ec/kv.rs riding the same flow (see blocking #3)

[aram356]

Summary

Large PR (98 files, +11,974 / −3,099). Architecture and consent plumbing are solid; this review focuses on concrete defects and hardening gaps verified against 109c929c. Three CI checks currently fail (integration tests, format-typescript, CodeQL); two of those are trivially fixable and inline-commented below. The CodeQL failure appears to be a false positive that needs a team decision.

Blocking

🔧 wrench

• Integration tests fail to compile: crates/integration-tests/Cargo.toml missing trusted-server-core dev-dependency while tests/common/ec.rs:19 and tests/frameworks/scenarios.rs:5 import it. (inline)
• format-typescript CI failure: crates/js/lib/src/integrations/prebid/index.ts:413 fails prettier — one-line fix via npm run format. (inline)
• Unbounded cookie payloads: ec/prebid_eids.rs:43 (ts-eids) and ec/prebid_eids.rs:111 (sharedId) decode/clone attacker-controlled cookie values with no size ceiling. (inline)

❓ question

• Consent-withdrawal split-brain at ec/finalize.rs:56: consent-store delete failures are logged-and-continued while KV tombstones still write. Intentional (KV = source of truth) or should withdrawal fail-closed? (inline)
• CodeQL cross-cutting: 15 high-severity rust/cleartext-logging alerts all name settings.reject_placeholder_secrets() as the tainted source, pinned to log-call lines that do not actually reference that method (e.g. auction/orchestrator.rs:125 is a timeout warning). This is taint-analyzer propagation through a boolean validator that uses .expose() internally; the logged bytes are field names, not secret material. How would the team like to resolve: (a) #[allow] on the validator with a comment explaining the false positive, (b) rename reject_placeholder_secrets → validate_secrets_not_placeholders to break the taint-name heuristic, or (c) configure a CodeQL suppression?

Non-blocking

🤔 thinking

• Bot gate is presence-only (ec/device.rs:76) — attackers with spoofed JA4 pass. Document threat model. (inline)
• user.id = None when EC not allowed (auction/endpoints.rs:60) — correct for consent, but may depress bidder dedup/yield. Consider a session-scoped ephemeral ID. (inline)
• Pull-sync only on organic routes (trusted-server-adapter-fastly/src/main.rs:373) — auction-heavy SPA publishers never refresh KV. Document or extend. (inline)

♻️ refactor

• Duplicated bearer parser in ec/identify.rs:34 and ec/batch_sync.rs:101 — extract to ec/auth.rs. (inline)
• Rate-limiter hourly→minute conversion is lossy by up to ~2× (ec/rate_limiter.rs:59) — document and test the overshoot bound. (inline)

🌱 seedling

• Non-deterministic EID order on timestamp ties (ec/eids.rs:64) — add partner_id as secondary sort key. (inline)
• No post-deserialize bounds on KvEntry (ec/kv.rs:158) — legacy/corrupt oversized entries will round-trip through. (inline)
• platform/test_support.rs defines stubs (NoopConfigStore, NoopSecretStore, noop_services) that no unit test exercises, so trait drift will silently break them. Either consume in ≥1 unit test or remove.
• Auction ext metadata unsrialized: auction/formats.rs computes strategy/provider-count/timing into result.metadata but never emits it into ext.trustedServer on the OpenRTB response — a useful debug surface.

⛏ nitpick

• has_cookie_mismatch naming (ec/mod.rs:398) — ambiguous; prefer header_overrides_cookie. (inline)

📌 out of scope

• ec_id/client_ip as query parameters in pull sync — PR description acknowledges as follow-up.

CI Status

• cargo fmt: PASS
• clippy: PASS
• cargo test (workspace): PASS
• vitest: PASS
• format-typescript: FAIL (see inline)
• integration tests: FAIL — compile error (see inline)
• CodeQL: FAIL — 15 high (likely false positives) (see question above)
• browser integration tests: PASS
• format-docs: PASS

[ChristianPavilonis]

Addressed the remaining merge blockers in 4d2ef30:

• added the missing trusted-server-core dev-dependency for crates/integration-tests
• formatted crates/js/lib/src/integrations/prebid/index.ts to clear the Prettier failure
• added raw size guards for ts-eids and sharedId cookie ingestion, with focused regression tests
• removed the remaining EC-ID-bearing log fields from the two CodeQL false-positive sites to unblock the cleartext-logging findings

Validation run locally:

• cargo test --workspace
• cargo fmt --all -- --check
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cd crates/js/lib && npx vitest run
• cd crates/js/lib && npx prettier --check src/integrations/prebid/index.ts
• cargo test --manifest-path crates/integration-tests/Cargo.toml --target "$(rustc -vV | sed -n 's/^host: //p')" --no-run

[ChristianPavilonis]

Follow-up update in 6016c48 to address the remaining open review threads:

• extracted shared Bearer auth to crates/trusted-server-core/src/ec/auth.rs and removed duplication from identify.rs / batch_sync.rs
• made EID ordering deterministic on synced ties with a stable partner_id secondary key
• documented + tested the rate-limiter hourly→per-minute rounding behavior
• added post-deserialize validation for loaded KvEntry values
• renamed has_cookie_mismatch to cookie_differs_from_active_ec
• documented the withdrawal authority model (KV tombstone authoritative, consent-store cleanup best-effort)
• clarified the browser-heuristic threat model, the no-consent auction identity choice, and the current pull-sync trigger contract

Validation re-run locally:

• cargo test --workspace
• cargo fmt --all -- --check
• cargo clippy --workspace --all-targets --all-features -- -D warnings

I also replied to and resolved the remaining open review threads tied to these changes.

[prk-Jr]

Summary

This PR lands the Edge Cookie identity system end to end, but I found two merge blockers in the request lifecycle.

Blocking

🔧 wrench

• Identify GET still missing browser-direct CORS support: OPTIONS /_ts/api/v1/identify reflects CORS headers, but GET /_ts/api/v1/identify never applies them, so the documented browser-direct flow still fails after a successful preflight. classify_origin() also accepts matching hosts regardless of scheme, while the spec only allows https origins.

Fix: classify origin in the GET path too, return 403 for denied origins, reflect CORS headers on allowed GET responses, and require origin_url.scheme() == "https" in classify_origin(). (crates/trusted-server-core/src/ec/identify.rs:31, crates/trusted-server-core/src/ec/identify.rs:171)

CI Status

• cargo fmt: PASS
• cargo test: PASS
• vitest: PASS
• browser integration tests: PASS
• integration tests: FAIL (test_ec_lifecycle_fastly: organic GET / should set ts-ec cookie)
• CodeQL: FAIL (12 open alerts on the merge ref)

[aram356]

Summary

Edge Cookie identity subsystem is materially complete and the prior review's blockers have been addressed: navigation-headers fix unblocks test_ec_lifecycle_fastly, UpsertResult variants are tested, user.id is Option<String> with skip_serializing_if, the partner-registry race is gone (architecture is now config-derived), MAX_CAS_RETRIES is 5 with backoff+jitter, raw client IP is no longer forwarded to pull-sync partners, and the WASM build now runs in CI.

Remaining blocking issues: a plaintext partner pull-token sitting on a Debug-deriving struct, an unintentional-looking read/write asymmetry in the bot gate, and a still-failing CodeQL gate riding the same Settings Debug taint-flow false positive that was flagged on the prior review.

Blocking

🔧 wrench

• Plaintext ts_pull_token on PartnerConfig (crates/trusted-server-core/src/ec/registry.rs:44, :208)

❓ question

• Bot-gate read/write asymmetry intentional? (crates/trusted-server-adapter-fastly/src/main.rs:208-212 vs. :272-276)
• CodeQL CI gate is failing on 12 new rust/cleartext-logging alerts, all riding the settings.reject_placeholder_secrets() → log::debug!("Settings {settings:?}") taint flow that was flagged on the prior review. Settings Debug is redacted via Redacted<T>, so the alerts are benign — but they block the gate. Sites: crates/trusted-server-adapter-fastly/src/main.rs:71, crates/trusted-server-core/src/publisher.rs:178/:341/:382, crates/trusted-server-core/src/html_processor.rs:70, crates/trusted-server-core/src/consent/mod.rs:173, crates/trusted-server-core/src/auction/orchestrator.rs:125/:278/:338/:408, crates/trusted-server-core/src/integrations/nextjs/html_post_process.rs:123/:456. Plan to suppress at PR level or add a repo-wide CodeQL filter on Redacted-derived flows before merge?

Non-blocking

🤔 thinking

• std::thread::sleep in CAS retry loop on wasm32-wasip1 (crates/trusted-server-core/src/ec/kv.rs:343, :426, :471, :558)
• seen_domains still uses HashMap while ids was converted to BTreeMap (crates/trusted-server-core/src/ec/kv_types.rs:133)
• MIN_PASSPHRASE_LENGTH = 8 is weak entropy for the HMAC-SHA256 anchor (crates/trusted-server-core/src/settings.rs:353)
• seen_domains drop-newest eviction freezes long-lived ECs at the 50-domain cap (crates/trusted-server-core/src/ec/kv.rs:667-683) — repeat from prior review

♻️ refactor

• PartnerRegistry::all() exposes HashMap iteration order to callers (crates/trusted-server-core/src/ec/registry.rs:174-176)
• RateLimiter::exceeded_per_minute round-trips through hourly math (crates/trusted-server-core/src/ec/rate_limiter.rs:42-46, :49-52)

🌱 seedling

• No end-to-end integration tests for batch sync 429 / 400 / 413 paths (crates/integration-tests/tests/frameworks/scenarios.rs:761-779)
• Bot-gate observability hook (crates/trusted-server-core/src/ec/device.rs:84-86) — operators have no metric for what fraction of traffic is excluded

📌 out of scope

• Repo-level CodeQL filter for the Redacted-derived rust/cleartext-logging taint flow so this stops gating PRs. Second consecutive PR review where the same false-positive flow blocks CI.

⛏ nitpick

• derive(Debug) on PartnerRegistry / PartnerConfig (crates/trusted-server-core/src/ec/registry.rs:17, :53) — combined with the plaintext token wrench, this is the loaded gun.

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test (workspace): PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• WASM release build: PASS
• CodeQL: FAIL — 12 new alerts, all on the same Settings Debug taint-flow false-positive flow (see ❓ above)

[ChristianPavilonis]

Clearing an old pending local review so I can reply to the current threads from the branch update.

[aram356]

Summary

Pass-2 review of the EC subsystem after the rebase onto current main. The pure-EC code (KV graph, consent gating, finalize, identify, batch sync, pull sync, device signals) is in good shape: clippy/test/fmt clean locally, no unwrap() on hot paths, withdrawal semantics carefully handled, and the unit-test surface is thorough (1178 core tests). Most pass-1 findings are now resolved upstream. Three blocking items remain at the edges of the PR.

Proposed fixes for the implementable findings below are in a stacked PR: #705. Merge it into this branch to absorb them. Two findings (the two ❓ questions) need product/privacy sign-off rather than code; one 🔧 (/ad-proxy/) was intentionally left for you to decide whether to drop or to land in a follow-up PR with proper routing.

Blocking

🔧 wrench

• /auction body parsed without size cap — crates/trusted-server-core/src/auction/endpoints.rs:53. handle_auction does serde_json::from_slice(&req.take_body_bytes()) with no limit; an authenticated client can buffer arbitrary bytes into the Wasm worker before any validation. Sister endpoint /batch-sync got the right pattern in the rebase; /auction did not. Proposed fix in #705 (256 KiB cap, Content-Length precheck + post-read check, returns 413).
• /ad-proxy/ URLs minted but not routed — crates/trusted-server-core/src/integrations/prebid.rs:422-502, 1138. transform_prebid_response rewrites every winning bid's adm, nurl, and burl to https://<request_host>/ad-proxy/... URLs. No /ad-proxy/ route exists (not in adapter-fastly/main.rs, not in PrebidIntegration::routes()). Live impact: every winning bid from the five hardcoded CDN patterns serves a creative whose resources 404; nurl/burl billing pixels also 404 — bidders may stop bidding. Regression vs. main (the rewriter did not exist there). Recommend either (a) gating it behind a /ad-proxy/ route handler in a separate PR or (b) dropping the rewriter from this PR (scope creep). Not implemented in #705 by request.

❓ question

• Jurisdiction::Unknown fail-closed → silent EC blackhole when geo is missing — crates/trusted-server-core/src/consent/mod.rs:506-508, crates/trusted-server-adapter-fastly/src/main.rs (geo wiring). allows_ec_creation returns false for Unknown. GeoInfo::from_request may return None if geo isn't enabled on the deployed Fastly service. Result: zero EC issuance everywhere (auction EID decoration, /identify, organic generation) with no error and no startup warning. Is the operator-mistake blast radius intentional? At minimum, consider a startup log::warn! if a bootstrap geo probe returns None, or surface on /health.
• TCF storage-consent overrides US-Privacy opt_out_sale=Yes in US states — crates/trusted-server-core/src/consent/mod.rs:486-498, test ec_allowed_us_state_tcf_takes_priority_over_us_privacy. For UsState, if a CMP wrote a TCF storage-consent string and the user also opted out via US-Privacy, allows_ec_creation returns true. The precedence ("TCF beats CCPA opt-out") is codified in the test. Was this signed off by privacy/legal? It's a real-world failure mode (stale TCF from a prior EU visit + fresh US opt-out → consent bypass).

Non-blocking

♻️ refactor

• IntegrationRegistry::handle_proxy >7 args, papered over with #[allow(clippy::too_many_arguments)] — crates/trusted-server-core/src/integrations/registry.rs:651-660. CLAUDE.md disallows >7 arguments. Proposed fix in #705 (ProxyDispatchInput struct, allow-attr removed).
• content_length_exceeds_limit duplicated — crates/trusted-server-core/src/ec/batch_sync.rs:174-178, and (with the /auction fix in #705) auction/endpoints.rs. The natural home is http_util, but http_util has migrated to http::Request<EdgeBody> while batch_sync/auction still use fastly::Request. Defer the lift until those callers migrate, so the helper can land in the right type system.

🤔 thinking

• /auction lacks per-route rate limiting — adjacent to the body-size finding. /batch-sync and pull sync have rate limiters; /auction is reachable from any first-party client with no per-IP/per-EC limit. Worth a follow-up issue (not blocking).
• Bot gate is presence-only and gates pull-sync too — crates/trusted-server-core/src/ec/device.rs:84-86, adapter-fastly/main.rs:170-179, 457-465. A client without ja4_class (rare — Fastly Compute over HTTP/1.1 or non-TLS test traffic) silently loses pull-sync forever. Documented as a known limitation; flagged for product visibility.

📝 note

• reject_placeholder_secrets doesn't cover partner api_tokens — crates/trusted-server-core/src/settings.rs. Shipped trusted-server.toml includes three partners (sharedid, inttest, inttest2) whose api_tokens are placeholder strings that pass the 32-byte length check. If an operator deploys without overriding them, /_ts/api/v1/batch-sync write access and /_ts/api/v1/identify read access are granted to those tokens by default. inttest* are at least labeled test-only in comments; sharedid is not. Worth adding to the placeholder list, or shipping enabled=false by default.
• Cluster size capped at one list(prefix, limit=100) page — crates/trusted-server-core/src/ec/kv.rs:606-631. For high-traffic publishers the prefix space could exceed 100 in legitimate traffic; the count is capped silently with no pagination. Worth instrumenting later to confirm rarity.

⛏ nitpick

• Leftover synthetic naming in test_support.rs, stale /_ts/api/v1/sync doc-comments in ec/mod.rs::consent_mut and integration-tests/.../scenarios.rs, /identify example missing cluster_size in docs/guide/api-reference.md. All four addressed in #705.

🌱 seedling

• No end-to-end CAS-conflict or consent-withdrawal-mid-flight scenario — crates/integration-tests/tests/frameworks/scenarios.rs. The seven EcScenarios are happy-path coverage. KV unit tests cover the code path; no integration coverage of the concurrent paths that the CAS retry/tombstone logic exists to handle. Follow-up.

👍 praise

• EC test surface is genuinely comprehensive at the unit level — 1178 trusted-server-core tests, including invalid-legacy-entries / unsupported-schema-version / refusing-to-serialize-invalid / cluster cache hit. Solid.
• finalize.rs cleanly centralizes the cookie/header/tombstone lifecycle — every route now goes through one helper; consent-withdrawal path well-tested; partner-scoped x-ts-<id> headers stripped on withdrawal.
• Withdrawal targets both header-EC and cookie-EC (finalize.rs::withdrawal_ec_ids) — tombstones both when they differ, deduplicates when they match. Good edge-case handling.
• Bearer-auth helper extracted to ec/auth.rs — single source of truth for /identify and /batch-sync, crisp tests rejecting malformed schemes / multi-token headers.
• Redacted<String> discipline on every secret field (api_token, ts_pull_token, passphrase, proxy_secret) with a regression test that Debug output does not leak.
• CodeQL sensitive-data fixes (log_id truncation, redacted Settings Debug) are surgical and well-targeted.

CI status

• cargo fmt --all -- --check — PASS (local)
• cargo clippy --workspace --all-targets --all-features -- -D warnings — PASS (local)
• cargo test --workspace — PASS, 0 failures (1178 core + 21 openrtb + 20 adapter-fastly + 2 doc; local)
• cd crates/js/lib && npx vitest run — PASS (318 tests)
• WASM release build with integration-test env — PASS (locally, with the upstream-fixed passphrase of length ≥32)

Pass-1 findings resolved upstream since the previous review

• ✅ Branch is now rebased (4 main commits behind, fine).
• ✅ /identify responses set CORS headers on every status (200/204/401/403), not just preflight.
• ✅ /batch-sync has a Content-Length precheck before buffering.
• ✅ partners/register and /_ts/api/v1/sync docs removed from api-reference.md.
• ✅ MIN_PASSPHRASE_LENGTH raised 8 → 32.
• ✅ CAS loops no longer thread::sleep.
• ✅ Prebid EID id length now checked against MAX_UID_LENGTH.
• ✅ CI build-env passphrase length now satisfies MIN_PASSPHRASE_LENGTH.

[aram356]

Summary

Pass-3 review of the Edge Cookie (EC) identity system after the round-8 CHANGES_REQUESTED. Reviewed against current main in an isolated worktree at PR head 61c3a14; all 126 changed files were read in full across parallel subsystem passes. The EC subsystem is mature and well-tested, and most round-8 findings are genuinely resolved (see Resolved since round 8 below). Four items remain — two new, two carried over.

Blocking

Full detail is in the inline comments; summary:

🔧 wrench

• Set-Cookie data loss in rebuild_response_with_body — crates/trusted-server-core/src/proxy.rs:150. The header-copy loop changed append_header → set_header and the main regression test was deleted; multiple Set-Cookie headers on rewritten proxy responses collapse to the last one. Regression vs main.
• GPP-only US opt-out not treated as a withdrawal — crates/trusted-server-core/src/consent/mod.rs:546. has_explicit_ec_withdrawal checks gpc/us_privacy but not gpp.us_sale_opt_out, while allows_ec_creation does — a GPP-string opt-out blocks new EC but leaves the existing cookie + KV row alive.
• Partner api_token placeholders shipped + embedded — trusted-server.toml:38. Three committed partner tokens pass the 32-byte check; reject_placeholder_secrets() does not cover them; build.rs bakes them into the binary — default Bearer credentials for batch-sync/identify in a public repo.

❓ question

• TCF overrides a fresh US opt-out — crates/trusted-server-core/src/consent/mod.rs:501. A stale TCF storage-consent string silently beats a fresh US opt_out_sale=Yes; no freshness guard. Carried over from round 8 — needs privacy/legal sign-off.

Non-blocking

♻️ refactor

• Dead KvEntry::minimal + stale upsert_partner_id doc — ec/kv.rs:524, ec/kv_types.rs:299. The upsert_partner_id doc describes a "missing-root recovery path" the code does not implement (it rejects missing keys, correctly). KvEntry::minimal has no production caller and builds a consent.ok = true entry with country: "ZZ" and no consent strings — delete it so it cannot be reintroduced as a consent-gate bypass, and fix the doc.
• exceeded_per_minute double-converts — ec/rate_limiter.rs:42. It multiplies a per-minute limit by 60 and routes back through hourly_limit_to_per_minute_limit, lossless only because the product is a multiple of 60. Implement exceeded_per_minute directly against the 60s counter.
• Inconsistent UID whitespace handling — ec/batch_sync.rs:198, ec/pull_sync.rs, ec/prebid_eids.rs. batch_sync accepts and stores untrimmed partner_uid (" abc " passes trim().is_empty() but the raw value is written to KV); prebid_eids trims. The same partner identity can fragment into whitespace-variant duplicates. Trim once and store uniformly.
• Dead _request_info parameter — integrations/prebid.rs:878. to_openrtb takes _request_info: RequestInfo (never used; recomputed internally). Drop it and the make_request_info call sites.
• Inline use inside functions — ec/pull_sync.rs:376, ec/batch_sync.rs:37. use super::kv_types::MAX_UID_LENGTH; inside a function / after a const violates the CLAUDE.md "no local imports" rule. Hoist to the top import block.

🤔 thinking

• /auction has no per-route rate limiting — auction/endpoints.rs, adapter-fastly/src/main.rs:368. It is an unauthenticated POST that does KV reads and HTTP fan-out to every bid provider; batch-sync and pull-sync both gate with FastlyRateLimiter. The 256 KiB body cap bounds per-request memory but not request volume. Recommend a per-EC-ID (or per-IP) limiter before production traffic.
• Bot gate permanently denies EC to UA-unrecognized browsers — ec/device.rs:84. looks_like_browser() requires both ja4_class and platform_class, and platform_class is a substring match against a fixed UA allowlist. A privacy browser with a frozen/stripped UA, or a future OS whose UA token is not listed, fails the gate on every request and permanently loses KV-backed EC generation + pull-sync. Consider gating on ja4_class alone, or treating platform_class == None as "unknown → allow".
• cluster_size saturates silently at 100 — ec/kv.rs:765. count_hash_prefix_keys caps at one 100-key list() page by design, but the value then caches at exactly 100 forever and consumers cannot tell 100 from 100,000. Add an info/warn log when the count hits CLUSTER_LIST_LIMIT.
• /auction never refreshes pull-sync — adapter-fastly/src/main.rs:518. Pull-sync is dispatched only on organic routes; a user whose only repeated traffic is /auction (SPA / ad-refresh pages) never gets partner UIDs filled. Documented as an intentional follow-up — flagging so it is a conscious product decision.
• fitAuctionEidsToCookie silently drops identity data — js/lib/src/integrations/prebid/index.ts:507. It progressively drops uids then whole EID sources to fit the cookie budget, from the end of the array, with no test coverage. Add a >3072-byte test and a console.warn on truncation.
• buildRequests unguarded document access — js/lib/src/integrations/prebid/index.ts:319. clearPrebidEidsCookie() touches document.cookie unconditionally, unlike sendAuction/mirrorSourcepointConsent which guard their globals. Add a typeof document === 'undefined' early-return, consistent with the rest of the file.
• Sourcepoint looksLikeGpp heuristic is fragile — js/lib/src/integrations/sourcepoint/index.ts:104. It accepts any consent string containing a ~ as GPP and mirrors it into __gpp. A GPP-header prefix check would be safer than matching a stray separator.

🌱 seedling

• EC integration scenarios are happy-path only — integration-tests/tests/frameworks/scenarios.rs. ec_consent_withdrawal asserts the ts-ec cookie is expired but never confirms the server wrote a KV tombstone — it would still pass if withdrawal only cleared the cookie. Cheap single-threaded-safe additions: seed a consent.ok = false row and assert batch-sync to it is rejected; assert batch-sync to a non-existent EC returns rejected. (The CAS-conflict path genuinely cannot be exercised single-threaded under Viceroy.)
• No test for the build-all.mjs user-ID code generator — js/lib/build-all.mjs. generatePrebidUserIdModules does registry validation, dev-overrides, and manifest hashing at build time only; a node test asserting the generated imports match user_id_modules.json would catch drift in _user_ids.generated.ts.

📝 note

• Bearer auth lacks an explaining comment — ec/auth.rs. The presented token is SHA-256-hashed and looked up by 64-hex digest in a HashMap; this is sound (the lookup key is itself a hash of the secret, so there is no token-revealing timing channel) and is why no constant-time compare is needed. Three review passes independently re-flagged it — a one-line comment would close it permanently.
• Stale doc comments — ec/mod.rs:142 (cookie_ec_value doc claims a request header "may take precedence", but parse_ec_from_request reads only the ts-ec cookie and ignores x-ts-ec on input); auction/formats.rs:76 (convert_tsjs_to_auction_request doc still says it generates an EC ID and lists "Fresh EC ID generation fails" under # Errors, but it is read-only now).
• Dead Viceroy secret-store entry — integration-tests/fixtures/configs/viceroy-template.toml. [[local_server.secret_stores.api-keys]] api_key = "test-api-key" is unused; EC tests authenticate with the inttest* partner tokens from trusted-server.toml. Remove it or comment why the store must exist.
• Pull-sync has no per-request timeout — ec/pull_sync.rs:261. PendingRequest::wait() has no timeout (the SDK exposes none); a hung partner ties up the compute instance until the platform cap. Best-effort by design and acknowledged — follow-up when timeout APIs land.
• MISSING_GEO_WARNING_LOGGED is process-global — consent/mod.rs. On Fastly's per-request instance model the warn-once may fire on nearly every request, or suppress a recurring outage after the first hit. Acceptable noise-reduction; worth a comment noting it is per-instance.

⛏ nitpick

• is_valid_ec_id vs is_valid_ec_hash case asymmetry — ec/generation.rs:155. is_valid_ec_id rejects uppercase hex while is_valid_ec_hash accepts it; an uppercase-hex ts-ec cookie would be silently excluded from the withdrawal tombstone path. Not an active bug (TS-minted cookies are lowercase) — normalize before validating, or cross-reference the deliberate asymmetry in a comment.
• viceroy-template.toml is loaded statically — it has no substitution step despite the "template" name; rename to viceroy.toml.
• parse_client_auction_uid double-fetches ext — auction/endpoints.rs:291. Use uid.get("ext").filter(|v| v.is_object()).cloned().
• scripts/batch-sync.sh — passes the JSON body via -d "$BODY" on the command line (visible in ps//proc) and treats any 2xx as success; if the endpoint ever returns 200 with rejected > 0, mappings are silently dropped. Pipe the body via stdin and parse rejected.
• count_hash_prefix_keys byte-slices hash_prefix — ec/kv.rs:783. Safe today (ASCII hex); hash_prefix.chars().take(8).collect::<String>() is panic-proof.

Resolved since round 8

• ✅ /auction body size cap — Content-Length precheck + post-read check, both 413.
• ✅ Dead /ad-proxy/ URL rewriting removed; all EC endpoints routed.
• ✅ IntegrationRegistry::handle_proxy >7 args → ProxyDispatchInput struct.
• ✅ Geo-unavailable log::warn! once added.
• ✅ Rate-limiter rounding, Prebid EID length cap, batch-sync Content-Length precheck — present and tested.

CI Status

• fmt: PASS (GitHub)
• clippy: PASS — cargo clippy --workspace --exclude trusted-server-cli --all-targets --all-features -- -D warnings (local)
• rust tests: PASS — cargo test --workspace --exclude trusted-server-cli (local; Analyze (rust) / cargo test still pending on GitHub)
• js tests: PASS — vitest (GitHub)

[aram356]

🔧 wrench — rebuild_response_with_body drops all but the last Set-Cookie (regression vs main).

This header-copy loop was changed from append_header → set_header, and the main regression test rebuild_response_with_body_preserves_multiple_set_cookie_headers was deleted in this PR. Response::get_headers() yields one entry per header value, so iterating multiple Set-Cookie values with set_header (replace semantics) means each call overwrites the previous — only the last Set-Cookie survives. This is reached on the text/html/text/css creative-rewrite proxy paths via finalize_proxied_response, so any proxied origin that sets more than one cookie silently loses all but one. Verified against main with a two-dot diff.

Fix: special-case Set-Cookie (and other inherently multi-valued headers) to use append_header, or restore append_header for all as main had it:

for (name, value) in headers {
    if name == header::CONTENT_LENGTH || name == header::CONTENT_TYPE {
        continue;
    }
    if name == header::CONTENT_ENCODING && !preserve_encoding {
        continue;
    }
    resp.append_header(name, value);
}

Restore the deleted rebuild_response_with_body_preserves_multiple_set_cookie_headers test.

[aram356]

🔧 wrench — GPP-only US opt-out is not treated as an explicit EC withdrawal.

allows_ec_creation's UsState arm checks ctx.gpp.us_sale_opt_out (lines 505-509) to block new EC, but this has_explicit_ec_withdrawal arm checks only gpc and us_privacy — never gpp.us_sale_opt_out. A returning user who opts out only via a GPP US string (no us_privacy cookie, no GPC header) gets new EC blocked, but their existing ts-ec cookie is not expired and the KV row is not tombstoned (finalize.rs gates the withdrawal path on consent_withdrawn). The identity persists in KV and on the client after a legally valid opt-out.

Fix — mirror the allows_ec_creation precedence (gpc → tcf → gpp → us_privacy):

jurisdiction::Jurisdiction::UsState(_) => {
    if ctx.gpc {
        return true;
    }
    if let Some(tcf) = effective_tcf(ctx) {
        return !tcf.has_storage_consent();
    }
    if ctx.gpp.as_ref().and_then(|g| g.us_sale_opt_out) == Some(true) {
        return true;
    }
    ctx.us_privacy
        .as_ref()
        .is_some_and(|usp| usp.opt_out_sale == PrivacyFlag::Yes)
}

Add a withdrawal test driven purely by a GPP string.

[aram356]

❓ question — TCF storage-consent unconditionally overrides a fresh US opt-out (carried over from the round-8 review).

effective_tcf(ctx) returns here before the GPP us_sale_opt_out check (lines 505-509) and the US-Privacy opt_out_sale check (lines 511-513) are ever reached. So a TCF storage-consent string — including a stale one from a prior EU visit, or one carried via a GPP section-2 — silently overrides a fresh US opt_out_sale=Yes. There is no timestamp comparison: apply_expiration_check only clears TCF once it exceeds max_consent_age_days (default 395), so a ~13-month-old TCF grant still wins over a brand-new US opt-out. has_explicit_ec_withdrawal mirrors the same precedence, and both behaviors are now codified by ec_allowed_us_state_tcf_takes_priority_over_us_privacy / ec_us_state_tcf_takes_priority_over_gpp_us.

The inline comment cites "the Sourcepoint GPP design documents this precedence decision," but the rationale isn't reproduced here and this is a real consent-bypass path in US states. Has privacy/legal signed off that an opt-in TCF signal beats a fresh CCPA-style opt-out? If yes, please document the reasoning inline and consider a freshness guard (the US opt-out wins when it is newer than the TCF signal). If no, US opt-out signals (us_privacy, gpp.us_sale_opt_out) should be evaluated before TCF in this arm.

[aram356]

🔧 wrench — Partner api_token placeholders ship committed and pass all validation.

This token — and inttest-api-key-1-32-bytes-minimum / inttest2-api-key-2-32-bytes-minimum below — are all ≥32 bytes, so they pass validate_api_token's minimum-length check. Settings::reject_placeholder_secrets() only inspects ec.passphrase and publisher.proxy_secret — never partner api_tokens. crates/trusted-server-core/build.rs reads trusted-server.toml and bakes the merged config into the build. In a public repo, any deployment that doesn't override the [[ec.partners]] block grants /_ts/api/v1/batch-sync write access and /_ts/api/v1/identify read access to anyone who reads this file. The 32-byte length check gives false assurance, and sharedid.org is a real partner shipped enabled by default (not labeled test-only).

Fix: extend reject_placeholder_secrets() to reject known partner-token placeholders (e.g. an EcPartner::API_TOKEN_PLACEHOLDERS const, or a shape check for the shipped -32-bytes / -minimum style values). Alternatively, ship the sharedid.org partner as opt-in and move the inttest* partners into a test-only config that is never embedded in release builds.