Conversation
…eids cookie Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Review: approve-with-nits
Reviewed at 94b8d18e8a6d3a2b17e831e8c8ba5d5784c25a52 in a scratch worktree. No CRITICAL or HIGH findings — the core change is sound and well-bounded, and the Purpose 4 gate is a real privacy fix beyond the stated scope of #1184.
Security question I went looking for: can a caller poison the identity graph? No.
The pre-existing KV write source was the ts-eids cookie, which TSJS writes from page script and is not HttpOnly — any same-origin script could already set it to arbitrary values. The /auction body is reachable by exactly the same actor, so this is not a new trust boundary.
A cross-site attacker cannot escalate: ts-ec is Secure; SameSite=Lax; HttpOnly (ec/cookies.rs:93), so a cross-origin fetch(..., {credentials:'include'}) POST carries no EC, ec_allowed() is false, ec_id is None, and client_eids is never set (auction/endpoints.rs:293-306). Poisoning stays confined to the caller's own EC row.
Write amplification: bounded at every layer
256 KiB body cap enforced twice (endpoints.rs:127-160); MAX_CLIENT_EID_SOURCES = 64 plus per-source UID and byte caps (endpoints.rs:491-537); the registry filter drops unconfigured sources so the update set is capped at registry.len(); dedupe_partner_updates collapses to one entry per partner; and apply_partner_id_updates returns false when every UID already matches, so the steady state costs zero KV writes.
Merge precedence matches the new doc comment
cookie -> body -> sharedId, then BTreeMap dedup = last-wins. Body beats a stale cookie, sharedId still wins its own source, and because finalize receives the raw cookie separately the union never loses a cookie-only partner. Correct shape.
One question before merge (see inline on finalize.rs)
Purpose 4 denial now empties updates, and an empty update list makes upsert_partner_ids_from_snapshot early-return without the load_snapshot refresh that orphan-EC recovery gates on. I believe that leaves a narrow segment (Purpose 1 granted / Purpose 4 denied, orphaned EC, non-GET publisher navigation) permanently unable to recover. Details inline — I'd like to know whether that coupling was intended.
Notes, not findings
- Adapter parity: this whole path is Fastly-only — zero references to
ec_finalize_responseorKvIdentityGraphin the Cloudflare/Spin/Axum adapters. Pre-existing, and the same for the cookie ingestion this extends, so the parity suite can't regress on it. Flagging only so it's on the record. - Test quality vs AGENTS.md: passes. No
unwrap(), everyexpect()uses a"should ..."message,json!throughout, Arrange-Act-Assert with descriptive assertion messages,vi.spyOnused directly so thevi.hoisted()rule doesn't apply. The real vendor domains in the test data (id5-sync.com,sharedid.org, ...) match existing precedent onmainfor EID source domains, and no real operator config values are introduced.
Praise
- The Purpose 4 gate (
finalize.rs:125-150) catches something the stated scope didn't ask for: the bid request was already being stripped, but the identity graph kept persisting EIDs the user had opted out of.finalize_withholds_eid_kv_writes_when_purpose_four_is_deniedproves it. - The
ec_id.is_some()gate onclient_eidscorrectly covers the US/GPC opt-out case thatgate_eids_by_consentalone would miss, and the comment explaining why is genuinely good. - The premise checks out end-to-end:
buildRequestssends the untrimmedcollectAuctionEids()in the body independent offitAuctionEidsToCookie, so the fix recovers real data rather than theoretical data.
Verification I ran locally (scratch worktree, not the PR branch)
| Command | Result |
|---|---|
cargo fmt --all -- --check |
PASS |
cargo test -p trusted-server-core (native) |
PASS — 2703 passed, 0 failed; all 4 new tests green by name |
cargo clippy-fastly (-D warnings --all-targets --all-features) |
PASS, no warnings |
cargo check-axum / cargo clippy-cloudflare / cargo check-spin |
PASS |
npx vitest run |
PASS — 960 tests, no type errors |
Not run locally: cargo test-fastly under Viceroy, test-cloudflare, test-spin, the parity suite, and the format jobs — all green on CI for this head. GitHub CI is 20/20.
Filing the ungated public ingest_* API as a separate follow-up issue rather than PR work.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed 94b8d18e8a6d3a2b17e831e8c8ba5d5784c25a52 against 2ca5d39ca7a5f600285f585435e169a5dad4dabe. The request-body EID ingestion and Purpose 4 write gate are supported by focused tests, and I found no additional actionable issues. Approving on the requested assumption that any blocking findings from other reviews are addressed before merge.
…arrow client EID visibility, report partial UID trims Purpose 4 denial was emptying the EID update list without refreshing an unread KV snapshot, silently disabling orphan-EC recovery on non-GET publisher navigations. Force a snapshot read in that narrow case only, leaving the Failed/Present/Missing/no-data invariants untouched. Also narrows set_client_eids/client_eids to pub(crate) (no external callers), and makes the ts-eids cookie trim warning report sources that lost some UIDs but were retained, not just sources dropped outright. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
aram356
left a comment
There was a problem hiding this comment.
Summary
Ingesting /auction body EIDs through EcContext fixes #1184 cleanly, and the Fastly adapter carries the same ec state from handle_auction into finalize, so the body set does reach the KV write in production. Adding the Purpose 4 gate to KV writes is the right call. What blocks merge is the docs: they still describe cookie-only ingestion and a Purpose-1-only rule. The remaining findings are non-blocking polish plus two follow-ups where the new Purpose 4 rule is not yet applied.
3 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The other comments describe the fix in prose because it spans several hunks or files, or would also need test changes.
Blocking
🔧 wrench
- Docs still describe cookie-only KV ingestion and a Purpose-1-only consent rule: see Cross-cutting below
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick / 📌 out of scope
- 🤔 The trim warning fires on every auction at the default log level: see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:2010 - ⛏ A source trimmed and then dropped is reported under both labels (suggestion): see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1987 - ♻️ Orphan-recovery condition parses the cookie and body a second time (suggestion): see inline at
crates/trusted-server-core/src/ec/finalize.rs:74-109 - ⛏
client_eidsfield doc omits the cookie fallback (suggestion): see inline atcrates/trusted-server-core/src/ec/mod.rs:237-240 - ⛏ Purpose 4 stripping on the KV path adds per-request info logs: see inline at
crates/trusted-server-core/src/ec/finalize.rs:173 - ♻️ Duplicated Purpose-1-only
ConsentContextfixture: see inline atcrates/trusted-server-core/src/ec/finalize.rs:1033 - 📌 Pull sync does not apply the new Purpose 4 rule: see Cross-cutting below
- 📌
/_ts/admin/eidspreview no longer matches what ingestion stores: see Cross-cutting below
Cross-cutting / body-level findings
-
🔧 Docs still describe cookie-only KV ingestion and a Purpose-1-only consent rule. This PR changes two operator-visible behaviours:
/auctionbody EIDs now reach the identity graph, and identity-graph EID writes now need TCF Purpose 1 and Purpose 4. The docs describe neither:docs/guide/integrations/prebid.md:517and the sequence diagram at:539say "Ingest ts-eids cookie for future requests".docs/guide/integration-guide.md:359says the same.docs/guide/edge-cookies.md:131-176(Partner Sync Channels and Prebid EID Cookie Flow) shows the cookie as the only browser channel into KV.:251says only "a laterts-eidscookie" can replace a stored UID.- The consent model in
edge-cookies.md(:122) lists only Purpose 1 for GDPR. Nothing says the identity graph withholds EID writes without Purpose 4. Operators and DPOs will look for that consent change in these pages.
Suggested fix: document that
/auctioningests request-body EIDs, with precedencets-eidscookie, then body, thensharedIdcookie. State that the cookie now matters for body-less routes (GET /_ts/page-bids, navigations). Add the Purpose 4 gate on identity-graph EID writes to the consent section. Those files are outside the diff, so this can't be a one-click suggestion. -
📌 Pull sync does not apply the new Purpose 4 rule.
build_pull_sync_context(crates/trusted-server-core/src/ec/pull_sync.rs:62) gates only onec_allowed(), which is Purpose 1. For a Purpose-1-granted, Purpose-4-denied user, pull sync still sends the raw EC ID to partners and writes the returned UIDs into KV. This PR's reasoning ("EIDs require Purpose 4 before they may be transmitted") therefore covers one of the two browser-request-driven write channels, and the uncovered one is more sensitive because it discloses the EC ID to third parties. Worth a follow-up issue that decides whether pull sync should use the same gate. -
📌
/_ts/admin/eidspreview no longer matches what ingestion stores.handle_admin_eids_lookup(crates/trusted-server-core/src/ec/admin.rs:600-602) says the preview "reports exactly what a navigation would store". It ignores the new Purpose 4 gate, so a Purpose-4-denied request shows matches that are never written. It also can't show/auctionbody EIDs. A follow-up could run the preview through the same consent gate, or at least reportconsent_gated: true.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- CLAUDE.md symlink guard: PASS
- prepare integration artifacts: PASS
- Analyze (actions): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- vitest: PASS
- Analyze (javascript-typescript): PASS
Add consent::allows_eid_persistence, matching gate_eids_by_consent's decision (TCF Purpose 1 + 4, fail closed when GDPR applies without TCF) without its per-request info logs. EC finalization collects EID updates once, withholds them with a debug log when the predicate denies, and uses the pre-gating emptiness for the orphan-recovery snapshot read instead of re-collecting. Extract the Purpose-1-only test consent into a helper, update the client_eids doc to cover the ts-eids cookie fallback, and in TSJS report a trimmed-then-dropped EID source only as dropped and log cookie trimming at debug level. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Update the Prebid, integration, and Edge Cookie guides: /auction writes request-body EIDs to the identity graph (applied after ts-eids and before sharedId), the ts-eids cookie remains the source for body-less routes such as GET /_ts/page-bids and navigations, and identity-graph EID writes require TCF Purpose 1 and Purpose 4 under GDPR. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
@aram356 Pushed 28addcb (code) and 6338bcf (docs). Inline threads are answered individually. Docs (blocking finding), 6338bcf
Verification: Out of scope, left for follow-ups
|
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 2, reviewed at 6338bcf1f. 28addcbe addresses every code finding from the last round. The allows_eid_persistence predicate, with its equivalence test against gate_eids_by_consent, the collect-once finalize path, the purpose_one_only_consent() helper, the client_eids doc, and the JS trimmed-then-dropped fix and debug-level log all match what was discussed, and I found no new code problems. 6338bcf1 fixes the stale docs.
Two things still block merge. The branch conflicts with main, so the pull_request CI workflows never ran on the last two commits. And one line in the new docs claims more privacy protection than the code provides.
With no remote CI to rely on, I ran the full gate locally on 6338bcf1f. All of these pass: cargo fmt, all eight clippy-* aliases, test-fastly (2917), test-axum (41), test-cloudflare (44), test-spin (86), the parity suite, vitest (962), JS format, node build-all.mjs, and docs format.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it as a commit on the PR branch.
Blocking
🔧 wrench
- Branch conflicts with
main; Run Tests / Run Format / Integration Tests / CodeQL Advanced never ran on28addcbeor6338bcf1: see Cross-cutting below - Consent-model bullet says every partner-EID write needs Purpose 4, but pull sync does not check it (suggestion): see inline at
docs/guide/edge-cookies.md:122
Non-blocking
🏕 camp site / 📌 out of scope
- 🏕 During the rebase, bring
main's new LiveRamp RampID docs in line with this PR: see Cross-cutting below - 📌 The two deferred follow-ups still have no issues: see Cross-cutting below
Cross-cutting / body-level findings
-
🔧 Branch conflicts with
main, so CI has not run on the last two commits.mergeStateStatusisDIRTY, and GitHub skipspull_requestworkflows while a PR can't be merged. Run Tests, Run Format, Integration Tests and CodeQL Advanced ran on every head up to2c2e1c870, but not on28addcbeor6338bcf1; only the dynamic "Code Quality" CodeQL job reported. Mergingmain(4 commits ahead of the base) conflicts in two files:crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts:main(#1054) addedpreserves an opaque LiveRamp envelope in the ts-eids cookiewhere this PR adds its three trim tests. Keep all four tests. A union-style resolution does not work: git lines up the sharedmockRequestBids/installPrebidNpmsetup lines and splits the blocks, which leaves a file that won't parse. Takemain's file and insert this PR's threeit(...)blocks whole, just beforeclears ts-eids cookie after bidsBackHandler when no current EIDs remain.docs/guide/integration-guide.md:main's docs refresh (#1049) rewrote the page and removed the "Hybrid EID forwarding" section this PR edits, so takemain's version. The same flow is documented indocs/guide/integrations/prebid.md, which auto-merges with this PR's changes.
I checked that resolution locally in a scratch merge, then discarded it. It passed vitest (1133, including all four tests above), JS format,
node build-all.mjs,cargo fmt,clippy-fastly, and everytrusted-server-coretest onwasm32-wasip1(2761). So the rebase should be mechanical, but the CI gates need to run on the rebased head before merge. -
🏕 During the rebase, bring
main's new LiveRamp RampID docs in line with this PR. They landed after this branch forked and describe the old ingestion model:docs/guide/integrations/prebid.md, "Resolution timing and data flow", steps 4-5: "The browser persists the same opaque value in the boundedts-eidscookie" and "A later request can ingest it". With this PR,/auctioningests the body entry after the response on the same request, and the cookie matters only for body-less routes.- Same section, consent paragraph: "Purpose 4 ... denying it alone does not block IdentityLink resolution or storage." That is true for browser-side storage. Identity-graph persistence of the RampID now requires TCF Purpose 1 + 4, which is worth one sentence so operators don't read the paragraph as covering KV.
docs/guide/configuration.md(managed User IDs, "Persisting a resolved ID into the Edge Cookie identity graph additionally requires a matching[[ec.partners]]entry"): add the Purpose 1 + 4 consent requirement next to the partner-entry requirement.
-
📌 The two deferred follow-ups still have no issues. Pull sync not applying the Purpose 4 gate (
crates/trusted-server-core/src/ec/pull_sync.rs:62,build_pull_sync_contextchecks onlyec_allowed()), and the/_ts/admin/eidspreview diverging from what ingestion writes (crates/trusted-server-core/src/ec/admin.rs:600-602). Neither has a tracking issue yet; #1189 covers only the ungatedpubingest_*functions. Please file them before merge so the gap stays tracked.
CI Status
- Analyze (javascript-typescript) (CodeQL - Code Quality): PASS
- Run Tests, Run Format, Integration Tests, CodeQL Advanced: not run on
6338bcf1f(PR isCONFLICTING); full gate passes locally, listed above
Co-authored-by: AG <132480+aram356@users.noreply.github.com>
jevansnyc
left a comment
There was a problem hiding this comment.
Traced the core fix and it lands: handle_auction takes &mut ec.ec_context and into_finalize_state moves that same instance into EcFinalizeState, so the body EIDs really do reach ec_finalize_response in production, not just in the test. collect_eid_updates ordering plus last-wins dedupe gives the claimed body-over-cookie precedence, body EIDs are bounded by MAX_CLIENT_EID_SOURCES, and the JS slice to pop rewrite is behavior-preserving.
/_ts/page-bids still sees only the trimmed cookie, but once /auction has written the full set to KV, resolve_auction_eids picks it up from the graph, so that path self-heals after one auction. Not worth changing here.
The three comments below are all on the Purpose 4 consent gate added as a related gap, not on the truncation fix itself.
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 3, reviewed at ad518fb08. The only new commit applies the round-2 suggestion for edge-cookies.md:122. The branch still conflicts with main, which is now 14 commits ahead, so the pull_request CI workflows have not run since 2c2e1c870. More importantly, main reworked the EID write path this PR modifies (#1157, #900, #901, #903). The rebase now touches ec/finalize.rs, ec/mod.rs and ec/prebid_eids.rs, and two of its pitfalls compile or pass silently.
To make the recipe below concrete, I rebased onto main in a scratch worktree, resolved all 17 conflict hunks, applied the test and pull-sync fixes described here, and ran the full gate on the result. All of these pass: cargo fmt, all eight clippy-* aliases, test-fastly (3105), test-axum (43), test-cloudflare (53), test-spin (87), the parity suite, vitest (1188), JS format, node build-all.mjs, and docs format. I then discarded the merge; nothing was pushed.
Separately, the Purpose 4 write gate interacts with pull sync in a way that discloses the EC ID to partners for exactly the users who denied Purpose 4. A scratch probe confirms it, and a three-line fix closes it.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it as a commit on the PR branch.
Blocking
🔧 wrench
- Rebase onto
mainis now non-trivial, and two pitfalls are invisible to git: see Cross-cutting below - After the rebase, docs claim browser EIDs overwrite stored partner UIDs;
mainno longer does that: see inline atdocs/guide/edge-cookies.md:260 - The Purpose 4 KV gate makes pull sync disclose the EC ID for Purpose-4-denied users: see Cross-cutting below
Non-blocking
🤔 thinking / ⛏ nitpick / 🏕 camp site / 📌 out of scope
- 🤔 Proxy consent mode withholds every EID write when a TC cookie is present: see inline at
crates/trusted-server-core/src/consent/mod.rs:532 - ⛏ Line 159 still states the consent rule for all identity-graph EID writes (suggestion): see inline at
docs/guide/edge-cookies.md:159 - 🏕 RampID docs alignment from round 2 is still pending: see Cross-cutting below
- 📌 Admin preview follow-up is still unfiled: see Cross-cutting below
Cross-cutting / body-level findings
-
🔧 Rebase onto
mainis now non-trivial, and two pitfalls are invisible to git. Mergingmainconflicts in six files. The resolution I verified:-
ec/prebid_eids.rs: takemain's side. #1157 deleted the publicingest_eid_cookies/ingest_prebid_eids/ingest_sharedid_cookieAPI, which also makes #1189 closable. Pitfall 1:mainadded four new tests that call the old three-argumentcollect_eid_cookie_updates(cookie, sharedid, registry). They merge without conflict but do not compile against this PR's rename; change each tocollect_eid_updates(cookie, sharedid, None, registry). -
ec/mod.rs: keep both sides.client_eidssits next tomain'spull_sync_markerandeid_sync_sourcein the struct, the constructors, and the accessor block. -
ec/finalize.rs:maincollects returning-user EIDs only whenec_context.eid_sync_source()is set, and writes throughsync_eid_cookie_updates/sync_eid_cookie_updates_from_snapshot. That function still returns early on emptyupdateswithout refreshing the snapshot (kv.rs:571), andmainsetsEidSyncSource::Navigationon non-GET navigations too, so this PR's orphan-recovery read is still needed. The returning-user block I verified:let source = ec_context.eid_sync_source(); let collected = source .map(|_| { collect_eid_updates( eids_cookie, sharedid_cookie, ec_context.client_eids(), registry, ) }) .unwrap_or_default(); let had_eid_updates = !collected.is_empty(); let updates = gate_eid_updates_by_consent(collected, ec_context.consent()); if updates.is_empty() && had_eid_updates && ec_context.recovery_eligible() && matches!(ec_context.kv_snapshot(), EcKvSnapshot::NotRead) { let snapshot = graph.load_snapshot(&ec_id); ec_context.set_kv_snapshot(snapshot); } else if let Some(source) = source { sync_eid_cookie_updates(graph, ec_context, &ec_id, &updates, source); }
The new-EC branch becomes
gate_eid_updates_by_consent(collect_eid_updates(.., ec_context.client_eids(), ..), ec_context.consent())followed bysync_eid_cookie_updates(graph, ec_context, &ec_id, &updates, EidSyncSource::NewEc). Imports:use crate::consent::{ConsentContext, allows_eid_persistence};,use super::prebid_eids::collect_eid_updates;, andmain'spull_sync_markerimport.maindroppedTrustedServerErrorandReportfrom this file. -
Pitfall 2, tests: on
mainthe Fastly adapter sets the sync source (app.rs:665,app.rs:840), nothandle_auction. Without one, this PR's tests either fail or stop testing anything:auction_body_eids_reach_kv_even_when_the_ts_eids_cookie_is_absentfails at its KV assertion (line 891 ofendpoints.rsin the rebased tree). Addec_context.set_eid_sync_source(crate::ec::EidSyncSource::Auction);, asmain's sibling snapshot test already does.finalize_withholds_eid_kv_writes_when_purpose_four_is_deniedpasses without exercising the gate: "nothing was written" holds trivially when nothing is collected. Addset_eid_sync_source(EidSyncSource::Auction).finalize_persists_every_configured_partner_from_client_eids_over_a_trimmed_cookie: addset_eid_sync_source(EidSyncSource::Auction).finalize_recovers_orphaned_ec_when_purpose_four_denial_empties_updates: addset_eid_sync_source(EidSyncSource::Navigation).
-
index.test.ts: takemain's file and insert this PR's three trim tests whole, just beforeclears ts-eids cookie after bidsBackHandler when no current EIDs remain. Line-level union breaks the syntax. -
docs/guide/integration-guide.md: takemain's version; #1049 removed the edited section. -
docs/guide/edge-cookies.md: takemain's two sequence-diagram blocks and add the consent note to the KV step, e.g.TS->>KV: Add missing partner IDs in one conditional write<br/>skip when UIDs already match<br/>(requires TCF Purpose 1 + 4 under GDPR). Then fix line 260 per the inline comment.
CI must run green on the rebased head before merge.
-
-
🔧 The Purpose 4 KV gate makes pull sync disclose the EC ID for Purpose-4-denied users. This adds evidence and a verified fix to jevansnyc's thread on
finalize.rs:165. Pull-sync eligibility is "partner key absent" (is_partner_pull_eligible,ec/pull_sync.rs:304onmain), andbuild_pull_sync_contextgates only onec_allowed()(Purpose 1) andentry.consent.ok, whichKvEntry::newsets totruefor every live row (ec/kv_types.rs:288). When this PR withholds a browser-supplied UID, the slot stays empty and a pull-enabled partner becomes eligible.Scratch probe on the rebased tree, using
main's pull-sync code. Setup: pull-enabled partnerpull.example.com, its UID present in thets-eidscookie, a live row without it,EidSyncSource::Navigation.PROBE pgrant: ec_allowed=true stored_partner=Some("BROWSER_UID") pull_sync_context=false PROBE pdeny4: ec_allowed=true stored_partner=None pull_sync_context=trueOn
mainwithout this PR, the Purpose-4-denied user follows the first path: the browser UID is written and no partner request is made. With this PR, the edge sends the raw EC ID to the partner server-to-server after the response and writes whatever UID comes back, through a path with no Purpose 4 check. The gate meant to honor the opt-out therefore causes a third-party disclosure for exactly those users.Verified fix (
ec/pull_sync.rs,build_pull_sync_context):use crate::consent::allows_eid_persistence; // ... if registry.pull_enabled_partners().is_empty() || !ec_context.ec_allowed() || !allows_eid_persistence(ec_context.consent()) { return None; }
With it, the probe's denied case returns
pull_sync_context=false, and all 37ec::pull_sync/ec::pull_sync_markertests pass; it is included in the full-gate run above. Please add a regression test, e.g.build_pull_sync_context_skips_when_eid_persistence_is_denied, and remove "Pull sync does not apply the Purpose 4 check yet." fromedge-cookies.md:122once it lands. Gating at context construction stops both the outbound request and the write-back. It also covers every route, because all post-send pull sync goes through this builder. -
🏕 RampID docs alignment from round 2 is still pending. During the rebase, update
docs/guide/integrations/prebid.md"Resolution timing and data flow" steps 4-5 (/auctionnow ingests the body entry on the same request) and its consent paragraph (identity-graph persistence needs TCF Purpose 1 + 4, unlike the browser-side storage it describes). Also add the consent requirement next to the[[ec.partners]]requirement indocs/guide/configuration.mdunder managed User IDs. -
📌 Admin preview follow-up is still unfiled. jevansnyc raised it as well.
mainreworded the comment atec/admin.rs:602, but it still says the preview reports "exactly what an eligible request would store". After this PR it ignores both the/auctionbody channel and the Purpose 4 gate. Either wire it throughcollect_eid_updatesplusallows_eid_persistence, or say the preview is cookie-only and ungated, and file the issue if it stays out of scope. If the pull-sync fix above lands here, the pull-sync follow-up from earlier rounds is resolved.
CI Status
- Analyze (javascript-typescript) (CodeQL - Code Quality): PASS
- Run Tests, Run Format, Integration Tests, CodeQL Advanced: not run on
ad518fb08,6338bcf1for28addcbe(PR isCONFLICTING); last ran green on2c2e1c870
Resolve conflicts with the add-only EID sync rework: thread request-body EIDs and the Purpose 4 gate through sync_eid_cookie_updates, set an EID sync source in the affected tests, and align edge-cookies docs with add-only browser EID sync. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Without TCF Purpose 1 + 4, finalization withholds browser-supplied partner UIDs, which left partner slots empty and made pull sync disclose the EC ID to partners for users who opted out. Also document proxy-mode fail-closed behavior, mark the admin EIDs preview as cookie-only, and align RampID docs with body ingestion and the consent gate. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
Round 3 addressed in f2ce88d (merge with
Ran locally on 3b92170: |
Summary
/auction's KV identity-graph writes only read the size-cappedts-eidscookie, silently dropping partners once the browser's 3072-char trim kicked in — even though the request body already carried the full EID set./auction's parsed request-body EIDs through to KV ingestion so every registry-configured partner in the request lands in the identity graph, not just what fit in the cookie.Changes
crates/trusted-server-core/src/ec/mod.rsclient_eidsfield + accessors onEcContextcrates/trusted-server-core/src/auction/endpoints.rshandle_auctionhands its parsed body EIDs toec_contextcrates/trusted-server-core/src/ec/prebid_eids.rscollect_eid_cookie_updatesrenamed tocollect_eid_updates; merges body EIDs alongside cookie EIDscrates/trusted-server-core/src/ec/finalize.rscollect_consent_gated_eid_updatesgates the merged cookie + body update list on TCF Purpose 4 before any KV writecrates/trusted-server-js/lib/src/integrations/prebid/index.tsfitAuctionEidsToCookienow warns which sources it drops when the cookie still needs trimmingcrates/trusted-server-js/lib/test/integrations/prebid/index.test.tsCloses
Closes #1184
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— not run;fastlyCLI isn't installed in this environmentcargo clippy-cloudflare,cargo check-cloudflare(wasm32-unknown-unknown),cargo test-cloudflareChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!) — note: template says "tracing", this repo useslog, nottracing