Review fixes for #621#687
Closed
aram356 wants to merge 55 commits into
Closed
Conversation
- 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
Subresource requests (fonts, images, CSS) may omit the Sec-GPC header, causing the server to incorrectly generate ts-ec cookies when the user has opted out via Global Privacy Control. Gate generate_if_needed() on the request Accept header containing text/html so only navigations trigger EC identity creation.
Move admin route matching and basic-auth coverage to /_ts/admin for a hard cutover, and align tests and docs so operational guidance matches runtime behavior.
Addresses issue #612 - spec now correctly documents that the full EC ID ({64-hex}.{6-alnum}) is used as the KV store key, not just the 64-char hash prefix. Changes: - Updated §4.1: ec_hash() now documented as for logging/debugging only - Updated §7.2: KV key description changed from '64-character hex hash' to 'Full EC ID in {64-char hex}.{6-char alphanumeric} format' - Updated §7.3: All KvIdentityGraph method parameters renamed from ec_hash to ec_id with proper documentation - Updated §9.3: Batch sync request field renamed from ec_hash to ec_id - Updated §9.4: Validation and error reasons updated (invalid_ec_hash → invalid_ec_id, ec_hash_not_found → ec_id_not_found) - Updated §10.4: Pull sync URL parameter changed from ec_hash to ec_id - Updated consent pipeline integration throughout to use full EC ID - Updated all rate limiting descriptions (per EC ID, not per hash) Rationale: The random suffix provides uniqueness for users behind the same NAT/proxy infrastructure who would otherwise share identical IP-derived hash prefixes.
Extends EC KV schema for cross-property identity resolution: - Add asn field to GeoInfo (from Fastly geo.as_number()) - Add asn and dma fields to KvGeo for network identification - Add KvDomainVisit and KvPubProperties for consortium-level domain tracking - Add pub_properties field to KvEntry with 50-domain cap - Track publisher domain visits in KvEntry::new() and update_last_seen() - Respect existing 300s debounce for organic requests only All new fields use Option types or serde(default) for backward compatibility. Existing v1 entries continue to deserialize without error.
Implements cluster size evaluation to distinguish individual users from shared networks (VPNs, corporate offices): - Add KvNetwork struct with cluster_size and last_evaluated timestamp - Add network field to KvEntry with TTL-gated cluster rechecks - Add cluster_size to KvMetadata and IdentifyResponse - Implement count_hash_prefix_keys() to list keys with common prefix - Implement evaluate_cluster() on KvIdentityGraph (one-page, 100-key limit) - Call cluster evaluation in handle_identify endpoint - Return cluster_size in JSON body and x-ts-cluster-size header - Add cluster_trust_threshold (default 10) and cluster_recheck_secs (default 3600) config Cluster evaluation uses best-effort semantics: size unknown if list exceeds 100 keys. Cache hit avoids re-evaluation within recheck interval.
Derives coarse browser signals from TLS/H2/UA on every request to gate EC identity operations. Unrecognized clients (known_browser != true) are proxied normally but leave no trace in the identity graph. - Add KvDevice struct (is_mobile, ja4_class, platform_class, h2_fp_hash, known_browser) and device field on KvEntry, written once on creation - Add ec/device.rs with DeviceSignals::derive(), UA parsing, JA4 Section 1 extraction, H2 fingerprint hashing, known browser allowlist (Chrome/ Safari/Firefox) - Add is_mobile and known_browser to KvMetadata for fast propagation checks - Wire DeviceSignals through EcContext to KvEntry creation path - Add bot gate in Fastly adapter: suppress KV graph, ec_finalize_response, and pull sync when known_browser != Some(true)
…bot gate Document all KV schema additions implemented in the preceding commits: geo extensions (asn/dma), publisher domain tracking, network cluster evaluation, device signal derivation, and the bot gate architecture. - Add §7A Device Signals and Bot Gate (signal derivation, allowlist, bot gate behavior matrix, KvDevice write policy, privacy rationale) - Update §7.2 with full KvEntry schema including KvGeo, KvPubProperties, KvDomainVisit, KvDevice, KvNetwork, and extended KvMetadata - Update §2 architecture diagram with Phase 0 bot gate step - Update §4.3 EcContext with device_signals field - Update §5.4 lifecycle with Phase 0 and ec_finalize gating - Update §11 /identify with cluster_size in JSON and x-ts-cluster-size header - Update §14 config with cluster_trust_threshold and cluster_recheck_secs - Update §17.1 main.rs pseudocode with full bot gate wiring
The known_browser fingerprint allowlist (3 entries) was too narrow and blocked legitimate browsers whose JA4/H2 combinations were not listed. Replace the gate with DeviceSignals::looks_like_browser() which checks for signal presence: ja4_class.is_some() && platform_class.is_some(). Real browsers always produce both; raw HTTP clients typically lack one or both. known_browser is still computed and stored on KvDevice for analytics but no longer gates identity operations.
Entries created before pub_properties was added have the field as None. The previous code only updated pub_properties when it already existed (if let Some), so returning users on pre-existing entries never got domain tracking. Now when pub_properties is None, update_last_seen initializes it with the current domain as origin_domain and first seen_domains entry. This is a one-time backfill per entry — subsequent visits take the existing update path.
…t consistency Address PR review findings across 11 files: - Elevate current_timestamp() fallback log from warn to error - Cache known_browser_h2_hashes() with OnceLock instead of recomputing per request - Unify MAX_UID_LENGTH to 512 across sync_pixel, batch_sync, and pull_sync - Fix identify json_response to use EdgeCookie error variant instead of Configuration - Remove unused _geo_info param from ec_finalize_response - Remove dead uppercase X- prefix check in copy_custom_headers - Add navigation-request Accept-header fallback debug logging - Document RefCell intent, pull-enabled index race condition, normalize_ec_id_for_kv defense-in-depth, and consent withdrawal test jurisdiction dependency
…odule Resolve compilation errors from rebasing onto main's PlatformKvStore abstraction layer: - Add asn field to platform GeoInfo and remove duplicate struct from geo.rs - Merge geo_from_fastly helper to include ASN extraction - Pass kv_store: None in EcContext consent pipeline (EC manages own KV) - Update finalize.rs consent deletion to use Fastly KVStore directly - Fix TrustedServerError::Ec -> EdgeCookie in error test - Allow deprecated GeoInfo::from_request in EC and adapter entry points - Update route_tests for RouteOutcome struct and EC architecture changes - Restore runtime_services construction in adapter entry point
Add log_id() helper that returns only the first 8 chars of an EC ID for log messages. Addresses CodeQL 'cleartext logging of sensitive information' findings on kv.rs:611 and kv.rs:748, and applies the same treatment to all ec_id log statements in the file for consistency.
…and code quality Address review findings across the EC identity system: - Extract shared log_id() and truncate EC IDs/client IPs in all 26 log sites to satisfy CodeQL cleartext-logging rules (P0) - Enforce HTTPS on sync pixel return URLs, restrict EC ID validation to lowercase hex, add cookie debug_assert and API key hash collision guard (P1) - Add 2MB body size limit on batch sync, minimum passphrase length, whitespace-only pull token rejection (P1/P2) - Fix withdrawal_ec_ids double computation, pull_sync double ec_value() call, and downgrade CAS conflict on update_last_seen to debug (P1/P2) - Deduplicate MAX_UID_LENGTH and normalize_ec_id_for_kv to shared locations, unify ec::consent wrapper usage (P1/P2) - Fix test extract_ec_cookie short-circuit bug, MinimalOrigin HTTP response whitespace, pixel sync Location header assertion (P0/P1) - Switch ad-proxy base64 to URL_SAFE_NO_PAD for path-safe encoding (P1) - Cleanup: remove stale #[allow(unused)], fix typos, add Debug derives, add mobile signal named constants, redact api_key in Debug output
Security fixes: - Fix cleartext EC ID logging at 7 call sites by using log_id() - Encapsulate trailing … inside log_id() (returns String now) and remove manual … from all ~40 format strings across the codebase - Remove raw client IP forwarding to pull sync partners (PII leak) - Add HttpOnly to EC cookie for XSS defense-in-depth - Add 64 KiB body size guard on pull sync partner responses Input validation fixes: - Reject whitespace-only UIDs in batch sync, pixel sync, and pull sync - Normalize EC IDs before validation in batch sync so uppercase hex from partners is accepted after lowercasing Code quality fixes: - Use saturating_sub for timestamp arithmetic consistency in kv.rs - Use distinct error messages for pull_sync_url vs ts_pull_token validation in partner.rs Documentation fixes: - Replace stale TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY env var references with TRUSTED_SERVER__EC__PASSPHRASE in docs
…bid EID cookie sync Remove the pixel sync endpoint and admin partner registration endpoint. Partners are now defined in [[ec.partners]] TOML config and loaded into an in-memory PartnerRegistry at startup with O(1) lookups by ID, API key hash, and source domain. The identify endpoint now requires Bearer token authentication and returns only the requesting partner's UID (scoped response). Client-side ID collection uses Prebid's getUserIdsAsEids() API — the TSJS Prebid integration writes a ts-eids cookie after each auction, and the backend ingests matched partner UIDs into the KV identity graph during ec_finalize_response. Batch sync and pull sync are retained, updated to use PartnerRegistry.
… 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
… 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>
… sensitive information' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Addresses review finding on #621: cors_preflight_identify set CORS headers on the OPTIONS preflight, but handle_identify only set X-TS-EC on the 200 and nothing on 204/401/403. A credentialed cross-origin fetch() from a publisher subdomain passed preflight then rejected the response. Classify the request origin once and apply the CORS headers to every returned response when the origin is allowed.
Addresses review finding on #621: batch-sync read the entire request body into memory via take_body_bytes() and only then compared length to the limit, so an oversized body had already been allocated; the auction endpoint had no limit at all. Add http_util::read_body_bounded, which reads at most max_bytes + 1 bytes so an oversized or hostile body cannot cause an unbounded allocation on the wasm32-wasip1 instance, and use it in both handlers (returning 413 when exceeded).
Addresses review finding on #621: api-reference.md documented POST /_ts/admin/v1/partners/register and GET /_ts/api/v1/sync, neither of which is routed (partners are configured statically in trusted-server.toml). Remove those sections and the matching /_ts/api/v1/sync references in ec/mod.rs and the integration-test scenario doc comment.
Addresses review finding on #621: ingest_prebid_eids checked that eid.id was non-empty but, unlike pull_sync and batch_sync, did not reject ids longer than MAX_UID_LENGTH before writing them to the KV identity graph. Add the symmetric length check.
Addresses review finding on #621: the Synthetic ID -> Edge Cookie rename left VALID_SYNTHETIC_ID and its doc comment behind in test_support. Rename to VALID_EC_ID.
Collaborator
Author
|
Closing — stacked on a stale head of #621; the PR has since been rebased and most of these findings are already addressed upstream. Re-running the review against the current head. |
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
2 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
Implements the actionable review findings for #621 so the branch is closer to a clean merge candidate. Stacked on top of
feature/edge-cookies-final— merge this into that branch to absorb the fixes.Findings addressed
/identifyresponses —ec/identify.rs: preflight set CORS headers buthandle_identifydidn't set them on the actual 200/204/401/403 responses, breaking the documented credentialed cross-origin browser use case. Now classifies the origin once and applies CORS headers to every returned response when the origin is allowed.ec/batch_sync.rs,auction/endpoints.rs,http_util.rs: both endpoints read the full request body into memory before (or without) checking the size limit. Addedhttp_util::read_body_bounded, which reads at mostmax_bytes + 1so an oversized/hostile body can't OOM the wasm instance; both handlers now return413when exceeded.docs/guide/api-reference.md,ec/mod.rs,integration-tests/.../scenarios.rs: dropped thePOST /_ts/admin/v1/partners/registerandGET /_ts/api/v1/syncsections and the matching doc-comment references (partners are configured statically intrusted-server.toml).ec/prebid_eids.rs: added theMAX_UID_LENGTHcheck oneid.idfor symmetry withpull_sync/batch_sync.synthetictest constant —test_support.rs:VALID_SYNTHETIC_ID→VALID_EC_ID.Not addressed here (need author/legal decisions or a rebase): the stale-base/rebase requirement, the
Jurisdiction::Unknownfail-closed blast radius, and the TCF-vs-US-Privacy precedence question — see the review on #621.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace(1020 tests, 0 failures)cd crates/js/lib && npx vitest run(282 tests, 0 failures)prettier --checkon the modified doc