Fix EC review findings: error handling, docs, dead code, and UID limit 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

Author: ChristianPavilonis

crates/integration-tests/tests/common/ec.rs

@@ -32,6 +32,10 @@ pub struct EcTestClient {
     client: Client,
     pub base_url: String,
     /// The active `ts-ec` cookie value, updated after each response.
+    ///
+    /// Uses `RefCell` for interior mutability because the test runner is
+    /// single-threaded. This would need to become `Arc<Mutex<..>>` if tests
+    /// are ever parallelized.
     ec_cookie: std::cell::RefCell<Option<String>>,
 }
 

crates/integration-tests/tests/frameworks/scenarios.rs

@@ -569,6 +569,12 @@ fn ec_consent_withdrawal(base_url: &str) -> TestResult<()> {
     // 2. Second request with GPC=1 should revoke consent and expire the EC
     // cookie. This endpoint was selected because step #1 proved EC was
     // allowed for this client in the active runtime config.
+    //
+    // Implementation note: this works because GPC + no geo headers →
+    // `Jurisdiction::Unknown` → fail-closed → consent denied. If the
+    // consent engine ever becomes more permissive for Unknown jurisdictions,
+    // this test should be updated to use an explicit GDPR consent denial
+    // (e.g. a TCF string with denied purposes).
     let resp = client.get_with_headers("/", &[("sec-gpc", "1")])?;
 
     if !is_ec_cookie_expired(&resp) {

crates/trusted-server-adapter-fastly/src/main.rs

@@ -188,13 +188,7 @@ async fn route_request(
     match enforce_basic_auth(settings, &req) {
         Ok(Some(mut response)) => {
             if is_real_browser {
-                ec_finalize_response(
-                    settings,
-                    geo_info.as_ref(),
-                    &ec_context,
-                    kv_graph.as_ref(),
-                    &mut response,
-                );
+                ec_finalize_response(settings, &ec_context, kv_graph.as_ref(), &mut response);
             }
             finalize_response(settings, geo_info.as_ref(), &mut response);
             return Ok(RouteOutcome {
@@ -337,13 +331,7 @@ async fn route_request(
 
     // Bot gate: skip EC cookie writes and pull sync for unrecognized clients.
     if is_real_browser {
-        ec_finalize_response(
-            settings,
-            geo_info.as_ref(),
-            &ec_context,
-            kv_graph.as_ref(),
-            &mut response,
-        );
+        ec_finalize_response(settings, &ec_context, kv_graph.as_ref(), &mut response);
     }
 
     finalize_response(settings, geo_info.as_ref(), &mut response);

crates/trusted-server-core/src/ec/batch_sync.rs

@@ -260,6 +260,12 @@ fn process_mappings(
     (accepted, errors)
 }
 
+/// Normalizes an EC ID for use as a KV key by lowercasing the hash prefix.
+///
+/// `hex::encode` (used in `generate_ec_id`) always produces lowercase hex,
+/// so internal EC IDs are already lowercase. This normalization is a
+/// defense-in-depth measure for EC IDs submitted by external partners
+/// (via batch sync) that may use uppercase hex.
 fn normalize_ec_id_for_kv(ec_id: &str) -> String {
     let mut parts = ec_id.splitn(2, '.');
     let hash = parts.next().unwrap_or_default();

crates/trusted-server-core/src/ec/device.rs

@@ -175,15 +175,17 @@ const KNOWN_BROWSERS: &[(&str, &str, bool)] = &[
     ("t13d1717h2", "1:65536;2:0;4:131072;5:16384", true),
 ];
 
-/// Computes H2 fingerprint hashes for the known browser allowlist.
+/// Returns H2 fingerprint hashes for the known browser allowlist.
 ///
-/// Recomputed on each call — the list is tiny (3 entries) so the cost
-/// is negligible compared to any KV I/O.
-fn known_browser_h2_hashes() -> Vec<(&'static str, String, bool)> {
-    KNOWN_BROWSERS
-        .iter()
-        .map(|(ja4, h2_raw, known)| (*ja4, compute_h2_fp_hash(h2_raw), *known))
-        .collect()
+/// Computed once on first call and cached via `OnceLock`.
+fn known_browser_h2_hashes() -> &'static Vec<(&'static str, String, bool)> {
+    static CACHE: std::sync::OnceLock<Vec<(&str, String, bool)>> = std::sync::OnceLock::new();
+    CACHE.get_or_init(|| {
+        KNOWN_BROWSERS
+            .iter()
+            .map(|(ja4, h2_raw, known)| (*ja4, compute_h2_fp_hash(h2_raw), *known))
+            .collect()
+    })
 }
 
 /// Evaluates whether a request comes from a known browser.
@@ -199,8 +201,7 @@ pub fn evaluate_known_browser(ja4_class: Option<&str>, h2_fp_hash: Option<&str>)
     let ja4 = ja4_class?;
     let h2_hash = h2_fp_hash?;
 
-    let hashes = known_browser_h2_hashes();
-    for (known_ja4, known_h2_hash, is_browser) in &hashes {
+    for (known_ja4, known_h2_hash, is_browser) in known_browser_h2_hashes() {
         if ja4 == *known_ja4 && h2_hash == *known_h2_hash {
             return Some(*is_browser);
         }

crates/trusted-server-core/src/ec/finalize.rs

@@ -10,7 +10,6 @@ use fastly::Response;
 use crate::consent::allows_ec_creation;
 use crate::consent::kv::delete_consent_from_kv;
 use crate::constants::HEADER_X_TS_EC;
-use crate::geo::GeoInfo;
 use crate::settings::Settings;
 
 use super::cookies::{expire_ec_cookie, set_ec_cookie};
@@ -33,7 +32,6 @@ const EC_RESPONSE_HEADERS: &[&str] = &[
 /// and cookie writes for new EC generation.
 pub fn ec_finalize_response(
     settings: &Settings,
-    _geo_info: Option<&GeoInfo>, // reserved for future route-specific finalize behavior
     ec_context: &EcContext,
     kv: Option<&KvIdentityGraph>,
     response: &mut Response,
@@ -233,7 +231,7 @@ mod tests {
         response.set_header("x-ts-ec", "stale");
         response.set_header("x-ts-eids", "[]");
 
-        ec_finalize_response(&settings, None, &ec_context, None, &mut response);
+        ec_finalize_response(&settings, &ec_context, None, &mut response);
 
         assert!(
             response.get_header("x-ts-ec").is_none(),
@@ -268,7 +266,7 @@ mod tests {
         );
         let mut response = Response::new();
 
-        ec_finalize_response(&settings, None, &ec_context, None, &mut response);
+        ec_finalize_response(&settings, &ec_context, None, &mut response);
 
         let header = response
             .get_header("x-ts-ec")
@@ -301,7 +299,7 @@ mod tests {
         );
         let mut response = Response::new();
 
-        ec_finalize_response(&settings, None, &ec_context, None, &mut response);
+        ec_finalize_response(&settings, &ec_context, None, &mut response);
 
         let header = response
             .get_header("x-ts-ec")
@@ -334,7 +332,7 @@ mod tests {
         );
         let mut response = Response::new();
 
-        ec_finalize_response(&settings, None, &ec_context, None, &mut response);
+        ec_finalize_response(&settings, &ec_context, None, &mut response);
 
         let header = response
             .get_header("x-ts-ec")
@@ -355,7 +353,7 @@ mod tests {
         let ec_context = make_context(None, None, false, false, Jurisdiction::Unknown);
         let mut response = Response::new();
 
-        ec_finalize_response(&settings, None, &ec_context, None, &mut response);
+        ec_finalize_response(&settings, &ec_context, None, &mut response);
 
         assert!(
             response.get_header("x-ts-ec").is_none(),

crates/trusted-server-core/src/ec/identify.rs

@@ -186,7 +186,7 @@ fn json_response<T: serde::Serialize>(
     status: StatusCode,
     body: &T,
 ) -> Result<Response, Report<TrustedServerError>> {
-    let body = serde_json::to_string(body).change_context(TrustedServerError::Configuration {
+    let body = serde_json::to_string(body).change_context(TrustedServerError::EdgeCookie {
         message: "Failed to serialize identify response".to_owned(),
     })?;
 

crates/trusted-server-core/src/ec/mod.rs

@@ -460,7 +460,7 @@ pub(crate) fn current_timestamp() -> u64 {
         .duration_since(std::time::UNIX_EPOCH)
         .map(|d| d.as_secs())
         .unwrap_or_else(|err| {
-            log::warn!("SystemTime::now() failed, falling back to epoch 0: {err}");
+            log::error!("SystemTime::now() failed, falling back to epoch 0: {err}");
             0
         })
 }

crates/trusted-server-core/src/ec/partner.rs

@@ -532,6 +532,13 @@ impl PartnerStore {
     /// Reads the current index, adds or removes the partner ID, and writes
     /// it back. All errors are logged and swallowed — the primary record
     /// write has already succeeded.
+    ///
+    /// **Race condition:** This performs a read-modify-write without CAS.
+    /// Concurrent partner registrations can overwrite each other's index
+    /// updates (last write wins). The index is self-healing: any lost entry
+    /// will be re-added on the next `upsert()` for that partner, and
+    /// `pull_enabled_partners()` falls back to `list_registered()` when the
+    /// index is missing or stale.
     fn update_pull_enabled_index(
         &self,
         store: &KVStore,

crates/trusted-server-core/src/ec/pull_sync.rs

@@ -334,7 +334,9 @@ fn extract_pull_uid(mut response: fastly::Response, partner_id: &str) -> Option<
         }
     };
 
-    const MAX_UID_LENGTH: usize = 256;
+    // Must match MAX_UID_LENGTH in sync_pixel.rs / batch_sync.rs so that
+    // partners see a consistent limit regardless of sync mechanism.
+    const MAX_UID_LENGTH: usize = 512;
 
     let uid = payload.uid.filter(|value| !value.is_empty());
     match uid {
@@ -526,12 +528,12 @@ mod tests {
 
     #[test]
     fn extract_pull_uid_rejects_oversized_uid() {
-        let long_uid = "x".repeat(257);
+        let long_uid = "x".repeat(513);
         let body = format!("{{\"uid\":\"{long_uid}\"}}");
         let response = fastly::Response::from_status(StatusCode::OK).with_body(body);
 
         let uid = extract_pull_uid(response, "ssp_x");
-        assert!(uid.is_none(), "should reject uid exceeding 256 bytes");
+        assert!(uid.is_none(), "should reject uid exceeding 512 bytes");
     }
 
     #[test]