Implement Edge Cookie identity system with KV graph and integration tests#621
Open
ChristianPavilonis wants to merge 52 commits intomainfrom
Open
Implement Edge Cookie identity system with KV graph and integration tests#621ChristianPavilonis wants to merge 52 commits intomainfrom
ChristianPavilonis wants to merge 52 commits intomainfrom
Conversation
ChristianPavilonis
force-pushed
the
feature/edge-cookies-final
branch
from
de0525d to
0940c34
Compare
ChristianPavilonis
requested review from
aram356 and
prk-Jr
and removed request for
aram356
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aram356
requested changes
Apr 10, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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 acrossec/mod.rs(lines 128, 211, 296),sync_pixel.rs(line 91), andpull_sync.rs(line 91). Thelog_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_KEYstill appears inconfiguration.md(lines 40, 956) anderror-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 beTRUSTED_SERVER__EC__PASSPHRASE.
❓ question
HttpOnlyomitted 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,HttpOnlywould be cheap XSS defense-in-depth.
Non-blocking
🤔 thinking
- Uppercase EC ID rejection in batch sync (
batch_sync.rs:209):is_valid_ec_idrejects uppercase hex, but batch sync validates before normalizing (line 225). Partners submitting uppercase EC IDs getinvalid_ec_idinstead of normalization.
♻️ refactor
_pull_enabledindex 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:
FullLifecycleandConcurrentPartnerSyncsonly assertuids.<partner>. Theec,consent,degraded,eids, andcluster_sizefields 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.tomlships banned placeholder:passphrase = "trusted-server"is rejected byreject_placeholder_secrets. Works in CI via env override, but new contributors will hit a confusing startup error. A TOML comment would help.
⛏ nitpick
Eid/UidmissingDeserialize:openrtb.rstypes deriveSerializeonly. If the auction ever needs to parse EIDs from provider responses,Deserializewill 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
requested changes
Apr 10, 2026
Collaborator
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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_urlappends the raw user IP as theipquery 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 theipparameter or gate it behind an explicit per-partnerallow_ip_forwardingconfig 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 haveMAX_BODY_SIZEguards; this path needs one too (64 KiB is generous for a{"uid":"..."}response). - Whitespace-only UIDs bypass validation —
batch_sync.rs:217andsync_pixel.rs:41:is_empty()passes" "and"\t", which get stored as garbage EID values in the KV graph. Replace withtrim().is_empty()at both sites. rand::thread_rng()WASM compatibility unverified —generation.rs:52: requiresgetrandomwith thewasifeature onwasm32-wasip1. Nativecargo testpassing does not prove the WASM build is safe; integration tests are already failing so this may not have been caught. Switch toOsRngor addgetrandom = { version = "0.2", features = ["wasi"] }explicitly.
Non-blocking
🤔 thinking
process_mappingsUpsertResult branches untested —batch_sync.rs:232:NotFound,ConsentWithdrawn, andStalehave zero unit test coverage.Staleis 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_grantedhas 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 passkv: None, so the tombstone write and the cookie-EC ≠ active-EC case inwithdrawal_ec_idsare 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_idduplicated in integration tests —integration-tests/tests/common/ec.rs:374: reimplementsnormalize_ec_id_for_kvfrom core. Re-export and use the canonical implementation.
⛏ nitpick
- Use
saturating_subfor consistency —kv.rs:605: the subtraction is safe due to the guard above but inconsistent withsaturating_subused throughout the rest of the module. log_idshould 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, gracefulItemPreconditionFailedhandling, andMAX_CAS_RETRIESpreventing infinite loops — textbook correct for a single-writer KV model. - Constant-time API key comparison (
partner.rs):subtle::ConstantTimeEqfor timing attack prevention on key lookups. Many implementations miss this entirely. KvMetadatafast-path consent check (kv_types.rs): mirroringok/country/cluster_sizein metadata to avoid streaming the full KV body is an excellent performance optimisation with the right constraint test.evaluate_known_browserwithOnceLock-cached hash table (device.rs): pre-hashing fingerprints once and caching viaOnceLockis 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
requested changes
Apr 14, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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_fastlydeterministic failure: the test client omitsSec-Fetch-Dest: documentandAccept: text/html, sois_navigation_request()correctly returnsfalseand EC never generates (crates/integration-tests/tests/common/ec.rs:93). rand::thread_rng()onwasm32-wasip1unverified; 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 explicitgetrandomwith thewasifeature.- Two new CodeQL alerts on
ec/kv.rs:637and:789ride the samesettings.reject_placeholder_secrets()taint flow that already produces noise onmain. Both are false positives but block the CodeQL gate. Fix with a scoped suppression or a repo-level CodeQL config filter onrust/cleartext-loggingfor settings-derived values. UpsertResult::{NotFound, ConsentWithdrawn, Stale}untested inbatch_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, changeUserInfo.idtoOption<String>withskip_serializing_if. update_pull_enabled_indexrace self-heal path untested (crates/trusted-server-core/src/ec/partner.rs:552-604). The documented fallback atpartner.rs:350is 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; useBTreeMaporIndexMap(crates/trusted-server-core/src/ec/kv_types.rs:59).seen_domainsdrop-newest eviction freezes long-lived ECs on their first 50 domains, defeating thelasttimestamp; switch to LRU or document (crates/trusted-server-core/src/ec/kv.rs:627-642).
♻️ refactor
- EID gating failures log at
debug!— bump towarn!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-Destfix lands, addec_full_lifecycle_with_gpcsendingSec-GPC: 1and asserting/_ts/api/v1/identifyreturns 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. Addingcargo build -p trusted-server-adapter-fastly --target wasm32-wasip1would 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 onmain, 2 are new onec/kv.rsriding the same flow (see blocking #3)
- Rename 'Synthetic ID' to 'Edge Cookie (EC)' across all external-facing identifiers, config, internal Rust code, and documentation - Simplify EC hash generation to use only client IP (IPv4 or /64-masked IPv6) with HMAC-SHA256, removing User-Agent, Accept-Language, Accept-Encoding, random_uuid inputs and Handlebars template rendering - Downgrade EC ID generation logs to trace level since client IP and EC IDs are sensitive data - Remove unused counter_store and opid_store config fields and KV store declarations (vestigial from template-based generation) - Remove handlebars dependency Breaking changes: wire field synthetic_fresh → ec_fresh, response headers X-Synthetic-ID → X-TS-EC, cookie synthetic_id → ts-ec, query param synthetic_id → ts-ec, config section [synthetic] → [edge_cookie]. Closes #462
…igration - Add ec/ module with EcContext lifecycle, generation, cookies, and consent - Compute cookie domain from publisher.domain, move EC cookie helpers - Fix auction consent gating, restore cookie_domain for non-EC cookies - Add integration proxy revocation, refactor EC parsing, clean up ec_hash - Remove fresh_id and ec_fresh per EC spec §12.1 - Migrate [edge_cookie] config to [ec] per spec §14
Implement Story 3 (#536): KV-backed identity graph with compare-and-swap concurrency, partner ID upserts, tombstone writes for consent withdrawal, and revive semantics. Includes schema types, metadata, 300s last-seen debounce, and comprehensive unit tests. Also incorporates earlier foundation work: EC module restructure, config migration from [edge_cookie] to [ec], cookie domain computation, consent gating fixes, and integration proxy revocation support.
Implement Story 4 (#537): partner KV store with API key hashing, POST /admin/partners/register with basic-auth protection, strict field validation (ID format, URL allowlists, domain normalization), and pull-sync config validation. Includes index-based API key lookup and comprehensive unit tests.
Implement Story 5 (#538): centralize EC cookie set/delete and KV tombstone writes in finalize_response(), replacing inline mutation scattered across publisher and proxy handlers. Adds consent-withdrawal cleanup, EC header propagation on proxy requests, and docs formatting.
Implement Story 8 (#541): POST /api/v1/sync with Bearer API key auth, per-partner rate limiting, batch size cap, per-mapping validation and rejection reasons, 200/207 response semantics, tolerant Bearer parsing, and KV-abort on store unavailability.
Implement Story 9 (#542): server-to-server pull sync that runs after send_to_client() on organic traffic only. Refactors the Fastly adapter entrypoint from #[fastly::main] to explicit Request::from_client() + send_to_client() to enable post-send background work. Pull sync enumerates pull-enabled partners, checks staleness against pull_sync_ttl_sec, validates URL hosts against the partner allowlist, enforces hourly rate limits, and dispatches concurrent outbound GETs with Bearer auth. Responses with uid:null or 404 are no-ops; valid UIDs are upserted into the identity graph. Includes EC ID format validation to prevent dispatch on spoofed values, partner list_registered() for KV store enumeration, and configurable pull_sync_concurrency (default 3).
Implement Story 11 (#544): Viceroy-driven E2E tests covering full EC lifecycle (generation, pixel sync, identify, batch sync, consent withdrawal, auth rejection). Adds EC test helpers with manual cookie tracking, minimal origin server with graceful shutdown, and required KV store fixtures. Fixes integration build env vars.
Consolidate is_valid_ec_hash and current_timestamp into single canonical definitions to eliminate copy-paste drift across the ec/ module tree. Fix serialization error variants in admin and batch_sync to use Ec instead of Configuration. Add scaling and design-decision documentation for partner store enumeration, rate limiter burstiness, and plaintext pull token storage. Use test constructors consistently in identify and finalize tests.
- Rename ssc_hash → ec_hash in batch sync wire format (§9.3) - Strip x-ts-* prefix headers in copy_custom_headers (§15) - Strip dynamic x-ts-<partner_id> headers in clear_ec_on_response (§5.2) - Add PartnerNotFound and PartnerAuthFailed error variants (§16) - Rename Ec error variant → EdgeCookie (§16) - Validate EC IDs at read time, discard malformed values (§4.2) - Add rotating hourly offset for pull sync partner dispatch (§10.3) - Add _pull_enabled secondary index for O(1+N) pull sync reads (§13.1)
…nd cleanup - Add body size limit (64 KiB) to partner registration - Validate partner UID length (max 512 bytes) in batch sync and sync pixel - Replace linear scan with binary search in encode_eids_header - Use constant-time comparison inline in partner lookup, remove unused verify_api_key - Remove unused PartnerAuthFailed error variant, fix PartnerNotFound → 404 - Add Access-Control-Max-Age CORS header to identify endpoint - Tighten consent-denied integration test to expect only 403 - Add stability doc-comment to normalize_ip - Log warning instead of silent fallback on SystemTime failure
…ror variants Resolve integration issues from rebasing onto feature/ssc-update: - Restore prepare_runtime() and validate_cookie_domain() lost in conflict resolution - Add InsecureDefault error variant and wire reject_placeholder_secrets() into get_settings() - Add sha2/subtle imports for constant-time auth comparison - Fix error match arms (Ec → EdgeCookie, remove nonexistent PartnerAuthFailed) - Fix orchestrator error handling to use send_to_client() pattern - Remove dead cookie helpers superseded by ec/cookies module
… sync coverage
Fix integration test deterministic failure by adding Sec-Fetch-Dest: document
and Accept: text/html headers to EcTestClient.get() so is_navigation_request()
recognizes organic requests. Update all EC scenarios for the new architecture:
config-based partners, Bearer-authenticated identify with scoped responses,
batch sync instead of pixel sync.
Change UserInfo.id from String to Option<String> with skip_serializing_if to
avoid emitting empty user.id in OpenRTB bid requests on the no-consent path.
Add batch_sync unit tests for UpsertResult::{NotFound, ConsentWithdrawn, Stale}
rejection paths (previously untested).
Add safety comments on ec/kv.rs log sites where CodeQL flags false positives
(log_id() already truncates the EC ID).
Add integration test partners (inttest, inttest2) to trusted-server.toml.
Read the sharedId cookie (set by Prebid's SharedID User ID Module) on organic requests and store the value as a partner UID in the EC identity graph. Uses the same debounce and best-effort semantics as the ts-eids cookie ingestion path. Add sharedid partner config (source_domain: sharedid.org, atype: 1) to trusted-server.toml. The backend matches the cookie value to this partner via PartnerRegistry.find_by_source_domain() during ec_finalize_response.
… module Origin/main introduced RuntimeServices platform abstraction (PR2-PR6) after our branch diverged. The rebase left inconsistencies between the trait definitions from our branch and the implementations from origin/main. Restore source files to their ORIG_HEAD state to reconcile: - Remove services: &RuntimeServices from IntegrationProxy::handle implementations (datadome, didomi, gpt, lockr, permutive, testlight, gtm) - Remove services from run_auction, proxy_request, AuctionContext - Restore publisher, proxy, orchestrator, types to branch-correct state - Restore streaming/html/rsc_flight modules to branch state
ChristianPavilonis
force-pushed
the
feature/edge-cookies-final
branch
from
7009838 to
43df212
Compare
aram356
requested changes
Apr 20, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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.tomlmissingtrusted-server-coredev-dependency whiletests/common/ec.rs:19andtests/frameworks/scenarios.rs:5import it. (inline) - format-typescript CI failure:
crates/js/lib/src/integrations/prebid/index.ts:413fails prettier — one-line fix vianpm run format. (inline) - Unbounded cookie payloads:
ec/prebid_eids.rs:43(ts-eids) andec/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-loggingalerts all namesettings.reject_placeholder_secrets()as the tainted source, pinned to log-call lines that do not actually reference that method (e.g.auction/orchestrator.rs:125is 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) renamereject_placeholder_secrets→validate_secrets_not_placeholdersto 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 = Nonewhen 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:34andec/batch_sync.rs:101— extract toec/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) — addpartner_idas 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.rsdefines 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.rscomputes strategy/provider-count/timing intoresult.metadatabut never emits it intoext.trustedServeron the OpenRTB response — a useful debug surface.
⛏ nitpick
has_cookie_mismatchnaming (ec/mod.rs:398) — ambiguous; preferheader_overrides_cookie. (inline)
📌 out of scope
ec_id/client_ipas 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
… sensitive information' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Collaborator
Author
|
Addressed the remaining merge blockers in 4d2ef30:
Validation run locally:
|
… sensitive information' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
… sensitive information' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Collaborator
Author
|
Follow-up update in 6016c48 to address the remaining open review threads:
Validation re-run locally:
I also replied to and resolved the remaining open review threads tied to these changes. |
prk-Jr
requested changes
Apr 22, 2026
Collaborator
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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/identifyreflects CORS headers, butGET /_ts/api/v1/identifynever 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 allowshttpsorigins.
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: organicGET /should setts-eccookie) - CodeQL: FAIL (12 open alerts on the merge ref)
prk-Jr
reviewed
Apr 22, 2026
aram356
requested changes
Apr 23, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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_tokenonPartnerConfig(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-212vs.:272-276) - CodeQL CI gate is failing on 12 new
rust/cleartext-loggingalerts, all riding thesettings.reject_placeholder_secrets()→log::debug!("Settings {settings:?}")taint flow that was flagged on the prior review.SettingsDebug is redacted viaRedacted<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 onRedacted-derived flows before merge?
Non-blocking
🤔 thinking
std::thread::sleepin CAS retry loop on wasm32-wasip1 (crates/trusted-server-core/src/ec/kv.rs:343, :426, :471, :558)seen_domainsstill usesHashMapwhileidswas converted toBTreeMap(crates/trusted-server-core/src/ec/kv_types.rs:133)MIN_PASSPHRASE_LENGTH = 8is weak entropy for the HMAC-SHA256 anchor (crates/trusted-server-core/src/settings.rs:353)seen_domainsdrop-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()exposesHashMapiteration order to callers (crates/trusted-server-core/src/ec/registry.rs:174-176)RateLimiter::exceeded_per_minuteround-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-derivedrust/cleartext-loggingtaint flow so this stops gating PRs. Second consecutive PR review where the same false-positive flow blocks CI.
⛏ nitpick
derive(Debug)onPartnerRegistry/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
SettingsDebug taint-flow false-positive flow (see ❓ above)
Collaborator
Author
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Clearing an old pending local review so I can reply to the current threads from the branch update.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
crates/trusted-server-core/src/ec/(new module)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/tests/common/ec.rsandtests/frameworks/scenarios.rs; updatedintegration.rstest runner and Viceroy config fixturescrates/trusted-server-adapter-fastly/src/main.rs/_ts/api/v1/and/_ts/admin/;send_to_client()pattern with background pull sync dispatchcrates/trusted-server-core/src/auction/endpoints.rsandformats.rsupdated 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.rscrates/js/lib/src/integrations/prebid/index.tscrates/trusted-server-core/src/synthetic.rscrates/trusted-server-core/src/cookies.rsec/cookies.rscrates/trusted-server-core/src/settings.rs+settings_data.rscrates/trusted-server-core/src/consent/crates/trusted-server-core/src/http_util.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/constants.rsdocs/guide/ec-setup-guide.md(new)docs/guide/edge-cookies.md(new)docs/guide/api-reference.mddocs/guide/configuration.mddocs/guide/synthetic-ids.mddocs/(various)docs/internal/superpowers/specs/2026-03-24-ssc-technical-spec-design.mdfastly.tomltrusted-server.tomlCloses
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
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)