Rename Synthetic ID to Edge Cookie (EC) and simplify generation#479
Rename Synthetic ID to Edge Cookie (EC) and simplify generation#479ChristianPavilonis wants to merge 6 commits intomainfrom
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Comprehensive rename of Synthetic ID → Server Side Cookie (SSC) with simplified HMAC generation (client IP only, dropping Handlebars templates). Cookie, header, query parameter, config section, and error variants are all renamed. The handlebars dependency is removed.
Blocking
🔧 wrench
- Missing new SSC headers in
INTERNAL_HEADERS:x-ts-ssc-freshandx-ts-ssc-trusted-serverare defined but not added to theINTERNAL_HEADERSblocklist, allowing potential leak of user-identity headers to third-party origins viacopy_custom_headers(crates/common/src/constants.rs:49)
Non-blocking
🤔 thinking
- Cookie name break:
synthetic_id→ts-sscsilently invalidates existing user cookies (crates/common/src/constants.rs:3) - Query parameter break:
synthetic_id→ts-sscin proxy forwarding may break downstream consumers (crates/common/src/proxy.rs:438) - Entropy reduction: HMAC now uses only client IP; users behind same NAT share the HMAC prefix (
crates/common/src/ssc.rs:63)
📝 note
- Silent template field removal: Old configs with
template = "..."are silently ignored (nodeny_unknown_fields). Operators won't know the field is unused.
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test: PASS
- vitest: PASS
- format-docs: PASS
- format-typescript: PASS
- CodeQL: PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This PR completes the Synthetic ID → Server Side Cookie rename and simplifies ID generation, but I found a couple of migration/privacy regressions plus a docs/behavior mismatch that should be fixed before merge.
Blocking
🔧 wrench
- Docs and implementation diverge on consent + proxy behavior: the new SSC docs state that ID generation/forwarding are consent-gated and that
/first-party/proxygenerates a new SSC ID when missing, but the current implementation still generates IDs unconditionally incrates/common/src/publisher.rs:239,crates/common/src/integrations/registry.rs:659, andcrates/common/src/auction/formats.rs:81, while proxy forwarding only reuses an existing ID incrates/common/src/proxy.rs:427. Either implement the documented rules or update the docs so they match the code.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
692fcca to
6e51272
Compare
Addressed: added x-ts-ssc-fresh to INTERNAL_HEADERS, removed x-ts-ssc-trusted-server. Non-blocking items are intentional design choices.
28391cd to
11e39dd
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Thorough rename of Synthetic ID → Edge Cookie (EC) across 71 files with a clean simplification of the HMAC generation (removing handlebars templating and multi-field inputs in favor of IP-only). All CI checks pass. No blocking issues found.
Non-blocking
🤔 thinking
- Client IP logged at
debuginstead oftrace: the old code usedlog::trace!for the HMAC input string; the new code useslog::debug!for the client IP, which is more likely to be enabled in production (edge_cookie.rs:79) X-Synthetic-Trusted-Serverheader silently removed: the old auction response set three headers; the new code sets two — the third (which duplicated the first) is dropped. Worth noting in the "Breaking changes" section if any downstream consumer depends on it (formats.rs:312-313)
👍 praise
- Clean dependency removal: removing
handlebarssimplifies the pipeline, reduces the dep tree, and eliminates theTemplateerror variant. The entropy trade-off is well-documented in thegenerate_ec_iddoc comment - Thorough rename: only one intentional reference to the old name remains (the
openrtb.rsbreaking-change doc comment). Zero stale references across all crates, configs, docs, and CI scripts - DRY improvement in
proxy.rs: the duplicated query-param upsert logic is consolidated intoupsert_ec_query_param(proxy.rs:446)
⛏ nitpick
- PR checklist mentions
tracing: the checklist says "Usestracingmacros" but the project convention and actual code uselogmacros - Indentation on HMAC error context: the
.change_context(TrustedServerError::Ec {block reads oddly —cargo fmtaccepts it but a manual tweak would improve readability (edge_cookie.rs:82)
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test: PASS
- vitest: PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Large rename from "Synthetic ID" → "Edge Cookie (EC)" across ~71 files. Removes synthetic.rs and replaces it with a simplified HMAC-based edge_cookie.rs. Also renames wire-format fields (OpenRTB, HTTP headers, query params) and the [synthetic] → [edge_cookie] config section. All CI checks pass.
Blocking
🔧 wrench
-
Wire-format breaking changes need migration strategy: This PR introduces several wire-format breaking changes that will affect downstream consumers:
- OpenRTB
user.ext.synthetic_fresh→user.ext.ec_fresh(crates/common/src/openrtb.rs:56) — Any PBS module or analytics pipeline readingsynthetic_freshwill silently lose this signal. - Query param
synthetic_id→ts-ec(crates/common/src/proxy.rs:453) — Any downstream endpoint parsingsynthetic_idfrom the URL will stop receiving the ID. - Response headers
X-Synthetic-ID/X-Synthetic-Fresh/X-Synthetic-Trusted-Server→X-TS-EC/X-TS-EC-Fresh(crates/common/src/auction/formats.rs:309) — Also removesX-Synthetic-Trusted-Serverentirely. - Config section
[synthetic]→[edge_cookie]— Existing deployments withTRUSTED_SERVER__SYNTHETIC__*env vars will silently lose their overrides.
Recommendations:
- Consider
#[serde(alias = "synthetic_fresh")]onec_freshfor backward-compatible deserialization - For query params, consider accepting both
synthetic_idandts-ecduring a transition period - Document the env var migration:
TRUSTED_SERVER__SYNTHETIC__*→TRUSTED_SERVER__EDGE_COOKIE__* - Confirm whether any live PBS modules or analytics consumers depend on these field names
- OpenRTB
❓ question
templatefield removed from config (crates/common/src/settings.rs): The oldSyntheticstruct had a validatedtemplate: Stringfield. The newEdgeCookieremoves it and hardcodes HMAC-based generation. Was the template used in any production deployment? Is there a migration path for publishers who customized their ID template?
Non-blocking
🤔 thinking
- EC ID logged at debug level (
crates/common/src/edge_cookie.rs:92): Full EC ID is logged. In Fastly production logs this could surface user-trackable identifiers. See inline comment.
⛏ nitpick
- Indentation of
.change_context(crates/common/src/edge_cookie.rs:82-84): Struct literal closing brace misaligned. See inline comment.
📝 note
counter_storeandopid_storefields remain inEdgeCookie(crates/common/src/settings.rs): These fields exist in the config struct but are not used byedge_cookie.rs(which only readssecret_key). If they are still used elsewhere (e.g.,crates/fastly) this is fine — if not, they could be cleaned up.
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS
- vitest: PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This PR does the EC rename thoroughly and CI is currently passing, but I found a few behavior and upgrade issues that should be addressed before merge. The most important ones are consent bypass on integration routes, increased logging of sensitive identifier inputs/outputs, and the hard config/env break with no compatibility shim.
Blocking
🔧 wrench
- Config/env compatibility break: The runtime now only deserializes
edge_cookieconfig andTRUSTED_SERVER__EDGE_COOKIE__*env vars, so existing deployments using[synthetic]/TRUSTED_SERVER__SYNTHETIC__*will silently stop populating this section unless they are migrated in lockstep. Consider keeping a one-release alias for the old names or emitting a targeted migration error so upgrades fail clearly instead of implicitly. (settings.rs, settings.rs)
Non-blocking
♻️ refactor
- Remove or justify dead EC config surface:
counter_storeandopid_storestill appear required in runtime config and docs, but after the IP-only simplification I couldn't find any runtime reads of those fields. If they are no longer needed, removing them will simplify operator setup and prevent confusion. If they are planned for follow-up work, the docs should say that explicitly. (settings.rs, configuration.md)
CI
GitHub checks are currently passing (cargo test, cargo fmt, vitest, integration tests, browser integration tests, CodeQL, and analysis jobs).
- Rename all external identifiers: x-synthetic-id → x-ts-ec, synthetic_id cookie → ts-ec, synthetic_fresh → ec_fresh - Simplify hash generation to use only client IP with HMAC-SHA256, removing User-Agent, Accept-Language, Accept-Encoding, and template rendering - Rename config section [synthetic] → [ec] with backward-compat alias - Rename ec.rs to edge_cookie.rs for clarity - Remove handlebars dependency (and transitive deps) - Add x-ts-ec-fresh to internal headers blocklist - Update all docs with new Edge Cookie (EC) terminology - Fix review findings: remove redundant serde rename, stale optimization entry, leftover 'synthetic' references in agent configs and docs Closes #462
- Rename allows_ssc_creation → allows_ec_creation and update all doc comments, test names, and assertion messages to use Edge Cookie (EC) - Fix intra-doc link [`ec`] → [`edge_cookie`] in lib.rs - Downgrade test log from info to debug in edge_cookie.rs for consistency - Add fallback comment and wire-protocol breaking-change doc in openrtb.rs - Run prettier --write on 3 doc files to fix format-docs CI - Update integration-tests Cargo.lock to sync derive_more 2.1.1
…ences - Rename TRUSTED_SERVER__SYNTHETIC__SECRET_KEY to TRUSTED_SERVER__EC__SECRET_KEY in CI action and local integration test scripts (root cause of CI failure: Viceroy could not start without the EC secret key) - Update stale doc reference synthetic.secret_key → ec.secret_key - Update stale comments in consent_config.rs and consent/types.rs
- Rename struct Ec → EdgeCookie, field settings.ec → settings.edge_cookie - Add serde alias "ec" for backward compatibility with existing configs - Update all TOML configs, env vars, CI actions, scripts, and docs - TRUSTED_SERVER__EC__* env vars → TRUSTED_SERVER__EDGE_COOKIE__* - Validation messages now reference edge_cookie.secret_key
- Downgrade EC ID/IP log statements from debug to trace to prevent sensitive data appearing in production logs (edge_cookie.rs) - Fix .change_context indentation in HMAC error handling - Remove unused counter_store and opid_store fields from EdgeCookie config struct, all test fixtures, TOML configs, and documentation - Add serialization test asserting ec_fresh wire field name - Fix edge-cookies.md config section reference and consent language - Update error-reference.md to reflect HMAC-based generation - Update configuration.md to remove dead KV store field docs
d3b92a5 to
b82e839
Compare
Addressing review feedback@aram356 @prk-Jr — pushed a commit addressing the actionable items from your reviews: Fixed
Acknowledged / deferred
All CI checks pass (fmt, clippy, 707 Rust tests, 282 JS tests). |
Summary
tracelevel since client IP and EC IDs are sensitive datacounter_storeandopid_storeconfig fields (vestigial from template-based generation)Changes
crates/common/src/edge_cookie.rs(new)synthetic.rs; simplified hash to IP-only HMAC-SHA256; removed template renderingcrates/common/src/synthetic.rs(deleted)edge_cookie.rscrates/common/src/constants.rsSYNTHETIC_ID_HEADER→HEADER_X_TS_EC, cookie/query names updatedcrates/common/src/cookies.rssynthetic_id→ts-ec, function names to ECcrates/common/src/error.rsSyntheticId→Ecerror variant; removedTemplatevariantcrates/common/src/settings.rs[synthetic]→[edge_cookie]; removedtemplate,counter_store,opid_storefieldscrates/common/src/settings_data.rscrates/common/src/lib.rssynthetic→edge_cookiecrates/common/src/http_util.rscrates/common/src/openrtb.rssynthetic_fresh→ec_fresh(breaking wire-protocol change)crates/common/src/proxy.rssynthetic_id→ts-eccrates/common/src/publisher.rscrates/common/src/consent/mod.rsallows_ssc_creation→allows_ec_creationwith updated docs and testscrates/common/src/consent/kv.rscrates/common/src/auction/formats.rsX-Synthetic-ID→X-TS-EC,X-Synthetic-Fresh→X-TS-EC-Freshcrates/common/src/integrations/*.rscrates/common/src/test_support.rscrates/common/Cargo.tomlhandlebarsdependencycrates/js/lib/src/integrations/gpt/index.tsCargo.lockcrates/integration-tests/Cargo.locktrusted-server.toml[synthetic]→[edge_cookie]; removedtemplate,counter_store,opid_storefieldsCLAUDE.mddocs/guide/edge-cookies.md(new)synthetic-ids.mddocs/guide/synthetic-ids.md(deleted)edge-cookies.mddocs/**/*.md(30+ files)Breaking changes
This is a clean break with no backward-compatible aliases. All downstream consumers must update in lockstep.
user.ext.synthetic_freshuser.ext.ec_freshX-Synthetic-IDX-TS-ECX-Synthetic-FreshX-TS-EC-FreshX-Synthetic-Trusted-ServerX-Synthetic-ID; no replacementsynthetic_idts-ecsynthetic_idts-ec[synthetic][edge_cookie]TRUSTED_SERVER__SYNTHETIC__*TRUSTED_SERVER__EDGE_COOKIE__*counter_store,opid_storetemplateCloses
Closes #462
Test plan
cargo test --workspace— all tests passingcargo clippy --all-targets --all-features -- -D warnings— zero warningscargo fmt --all -- --check— cleancd crates/js/lib && npx vitest run— all tests passingcd docs && npx prettier --check .— cleanChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)