Skip to content

Ingest /auction EIDs from the request body instead of the trimmed ts-eids cookie - #1188

Open
dhruv8sh wants to merge 9 commits into
mainfrom
fix/auction-eid-kv-truncation
Open

dhruv8sh wants to merge 9 commits into
mainfrom
fix/auction-eid-kv-truncation

Conversation

@dhruv8sh

Copy link
Copy Markdown
Collaborator

Summary

  • /auction's KV identity-graph writes only read the size-capped ts-eids cookie, silently dropping partners once the browser's 3072-char trim kicked in — even though the request body already carried the full EID set.
  • Thread /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.
  • Also close a related gap found during review: the KV write path only checked TCF Purpose 1 (device storage) consent, not Purpose 4 (personalized ads) — now gated the same way the outbound bid request already is.

Changes

File Change
crates/trusted-server-core/src/ec/mod.rs New client_eids field + accessors on EcContext
crates/trusted-server-core/src/auction/endpoints.rs handle_auction hands its parsed body EIDs to ec_context
crates/trusted-server-core/src/ec/prebid_eids.rs collect_eid_cookie_updates renamed to collect_eid_updates; merges body EIDs alongside cookie EIDs
crates/trusted-server-core/src/ec/finalize.rs New collect_consent_gated_eid_updates gates the merged cookie + body update list on TCF Purpose 4 before any KV write
crates/trusted-server-js/lib/src/integrations/prebid/index.ts fitAuctionEidsToCookie now warns which sources it drops when the cookie still needs trimming
crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts Test for deterministic trimming + warning content

Closes

Closes #1184

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve — not run; fastly CLI isn't installed in this environment
  • Other: cargo clippy-cloudflare, cargo check-cloudflare (wasm32-unknown-unknown), cargo test-cloudflare

Checklist

  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses log macros (not println!) — note: template says "tracing", this repo uses log, not tracing
  • New code has tests
  • No secrets or credentials committed

…eids cookie

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh
dhruv8sh requested a review from jevansnyc September 21, 2026 11:41

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_response or KvIdentityGraph in 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(), every expect() uses a "should ..." message, json! throughout, Arrange-Act-Assert with descriptive assertion messages, vi.spyOn used directly so the vi.hoisted() rule doesn't apply. The real vendor domains in the test data (id5-sync.com, sharedid.org, ...) match existing precedent on main for 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_denied proves it.
  • The ec_id.is_some() gate on client_eids correctly covers the US/GPC opt-out case that gate_eids_by_consent alone would miss, and the comment explaining why is genuinely good.
  • The premise checks out end-to-end: buildRequests sends the untrimmed collectAuctionEids() in the body independent of fitAuctionEidsToCookie, 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.

Comment thread crates/trusted-server-core/src/ec/finalize.rs Outdated
Comment thread crates/trusted-server-core/src/ec/prebid_eids.rs Outdated
Comment thread crates/trusted-server-core/src/ec/mod.rs
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_eids field doc omits the cookie fallback (suggestion): see inline at crates/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 ConsentContext fixture: see inline at crates/trusted-server-core/src/ec/finalize.rs:1033
  • 📌 Pull sync does not apply the new Purpose 4 rule: see Cross-cutting below
  • 📌 /_ts/admin/eids preview 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: /auction body 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:517 and the sequence diagram at :539 say "Ingest ts-eids cookie for future requests". docs/guide/integration-guide.md:359 says 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. :251 says only "a later ts-eids cookie" 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 /auction ingests request-body EIDs, with precedence ts-eids cookie, then body, then sharedId cookie. 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 on ec_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/eids preview 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 /auction body EIDs. A follow-up could run the preview through the same consent gate, or at least report consent_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

Comment thread crates/trusted-server-core/src/ec/finalize.rs Outdated
Comment thread crates/trusted-server-core/src/ec/finalize.rs Outdated
Comment thread crates/trusted-server-core/src/ec/finalize.rs Outdated
Comment thread crates/trusted-server-core/src/ec/mod.rs Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts Outdated
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>
@dhruv8sh

Copy link
Copy Markdown
Collaborator Author

@aram356 Pushed 28addcb (code) and 6338bcf (docs). Inline threads are answered individually.

Docs (blocking finding), 6338bcf

  • docs/guide/integrations/prebid.md: Identity Forwarding step 5 and the sequence diagram now describe response-time ingestion of the /auction body EIDs plus the ts-eids/sharedId cookies, the precedence, the cookie's role on body-less routes, and the Purpose 1 + 4 requirement.
  • docs/guide/integration-guide.md: the hybrid EID forwarding bullet says the same.
  • docs/guide/edge-cookies.md: the GDPR consent bullet notes that EID writes also need Purpose 4. The Partner Sync Channels diagram and the renamed "Prebid EID Flow" section document the source order (ts-eids cookie, then the /auction body, then the sharedId cookie; a later source wins, matching collect_eid_updates + dedupe_partner_updates). They also cover the cookie's role for GET /_ts/page-bids and navigations, and the consent rule, including GDPR-without-TCF withholding. The seeding diagram gains the post-response /auction ingestion step, and the closing paragraph now lists the body alongside the cookies.

Verification: cargo fmt --check, cargo clippy-fastly, cargo clippy-axum, cargo test-axum, and cargo test-fastly (core: 2699 passed) all pass. npx vitest run: 962 passed. JS and docs npm run format are clean.

Out of scope, left for follow-ups

  • Pull sync (ec/pull_sync.rs) still writes partner UIDs without the Purpose 4 gate that response-time EID ingestion now applies.
  • The /_ts/admin/eids preview does not reflect the body-EID ingestion or the Purpose 4 gating, so it can diverge from what finalization actually writes.

@dhruv8sh
dhruv8sh requested a review from aram356 September 24, 2026 12:24

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 on 28addcbe or 6338bcf1: 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. mergeStateStatus is DIRTY, and GitHub skips pull_request workflows while a PR can't be merged. Run Tests, Run Format, Integration Tests and CodeQL Advanced ran on every head up to 2c2e1c870, but not on 28addcbe or 6338bcf1; only the dynamic "Code Quality" CodeQL job reported. Merging main (4 commits ahead of the base) conflicts in two files:

    • crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts: main (#1054) added preserves an opaque LiveRamp envelope in the ts-eids cookie where this PR adds its three trim tests. Keep all four tests. A union-style resolution does not work: git lines up the shared mockRequestBids / installPrebidNpm setup lines and splits the blocks, which leaves a file that won't parse. Take main's file and insert this PR's three it(...) blocks whole, just before clears 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 take main's version. The same flow is documented in docs/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 every trusted-server-core test on wasm32-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 bounded ts-eids cookie" and "A later request can ingest it". With this PR, /auction ingests 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_context checks only ec_allowed()), and the /_ts/admin/eids preview 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 ungated pub ingest_* 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 is CONFLICTING); full gate passes locally, listed above

Comment thread docs/guide/edge-cookies.md Outdated
Co-authored-by: AG <132480+aram356@users.noreply.github.com>
@dhruv8sh
dhruv8sh requested a review from aram356 September 28, 2026 07:36
@dhruv8sh
dhruv8sh changed the base branch from main to rc/202609 September 29, 2026 11:37
@dhruv8sh
dhruv8sh changed the base branch from rc/202609 to main October 1, 2026 10:53

@jevansnyc jevansnyc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/trusted-server-core/src/ec/finalize.rs
Comment thread crates/trusted-server-core/src/consent/mod.rs
Comment thread crates/trusted-server-core/src/ec/prebid_eids.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 main is 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; main no longer does that: see inline at docs/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 main is now non-trivial, and two pitfalls are invisible to git. Merging main conflicts in six files. The resolution I verified:

    1. ec/prebid_eids.rs: take main's side. #1157 deleted the public ingest_eid_cookies / ingest_prebid_eids / ingest_sharedid_cookie API, which also makes #1189 closable. Pitfall 1: main added four new tests that call the old three-argument collect_eid_cookie_updates(cookie, sharedid, registry). They merge without conflict but do not compile against this PR's rename; change each to collect_eid_updates(cookie, sharedid, None, registry).

    2. ec/mod.rs: keep both sides. client_eids sits next to main's pull_sync_marker and eid_sync_source in the struct, the constructors, and the accessor block.

    3. ec/finalize.rs: main collects returning-user EIDs only when ec_context.eid_sync_source() is set, and writes through sync_eid_cookie_updates / sync_eid_cookie_updates_from_snapshot. That function still returns early on empty updates without refreshing the snapshot (kv.rs:571), and main sets EidSyncSource::Navigation on 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 by sync_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;, and main's pull_sync_marker import. main dropped TrustedServerError and Report from this file.

    4. Pitfall 2, tests: on main the Fastly adapter sets the sync source (app.rs:665, app.rs:840), not handle_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_absent fails at its KV assertion (line 891 of endpoints.rs in the rebased tree). Add ec_context.set_eid_sync_source(crate::ec::EidSyncSource::Auction);, as main's sibling snapshot test already does.
      • finalize_withholds_eid_kv_writes_when_purpose_four_is_denied passes without exercising the gate: "nothing was written" holds trivially when nothing is collected. Add set_eid_sync_source(EidSyncSource::Auction).
      • finalize_persists_every_configured_partner_from_client_eids_over_a_trimmed_cookie: add set_eid_sync_source(EidSyncSource::Auction).
      • finalize_recovers_orphaned_ec_when_purpose_four_denial_empties_updates: add set_eid_sync_source(EidSyncSource::Navigation).
    5. index.test.ts: take main's file and insert this PR's three trim tests whole, just before clears ts-eids cookie after bidsBackHandler when no current EIDs remain. Line-level union breaks the syntax.

    6. docs/guide/integration-guide.md: take main's version; #1049 removed the edited section.

    7. docs/guide/edge-cookies.md: take main'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:304 on main), and build_pull_sync_context gates only on ec_allowed() (Purpose 1) and entry.consent.ok, which KvEntry::new sets to true for 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 partner pull.example.com, its UID present in the ts-eids cookie, 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=true
    

    On main without 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 37 ec::pull_sync / ec::pull_sync_marker tests 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." from edge-cookies.md:122 once 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 (/auction now 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 in docs/guide/configuration.md under managed User IDs.

  • 📌 Admin preview follow-up is still unfiled. jevansnyc raised it as well. main reworded the comment at ec/admin.rs:602, but it still says the preview reports "exactly what an eligible request would store". After this PR it ignores both the /auction body channel and the Purpose 4 gate. Either wire it through collect_eid_updates plus allows_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, 6338bcf1f or 28addcbe (PR is CONFLICTING); last ran green on 2c2e1c870

Comment thread docs/guide/edge-cookies.md Outdated
Comment thread crates/trusted-server-core/src/consent/mod.rs
Comment thread docs/guide/edge-cookies.md Outdated
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>
@dhruv8sh

dhruv8sh commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

Round 3 addressed in f2ce88d (merge with main) and 3b92170:

  • Merge with main: merged rather than rebased, so no force-push. Followed the conflict recipe, including both pitfalls: main's new tests now call collect_eid_updates(.., None, ..), and the four affected tests set an EidSyncSource (Auction or Navigation), so the Purpose 4 test actually exercises the gate.
  • Pull sync and Purpose 4: build_pull_sync_context now also requires allows_eid_persistence, covered by build_pull_sync_context_skips_when_eid_persistence_is_denied. Removed "Pull sync does not apply the Purpose 4 check yet" from edge-cookies.md.
  • RampID docs: prebid.md steps 4-5 now describe same-request body ingestion. The consent paragraph separates browser-side storage from identity-graph persistence, which needs Purpose 1 + 4. configuration.md managed User IDs lists the consent requirement.
  • Follow-ups: the admin preview is filed as Align /_ts/admin/eids preview with EID ingestion (consent gate and /auction body) #1232. Reduce EID KV write conflicts during page loads #1157 removed the public ingest_* API, so Ungated public ingest_* EID KV write path bypasses TCF Purpose 4 gating #1189 looks closable.

Ran locally on 3b92170: cargo fmt, all eight clippy-* aliases, test-fastly, test-axum, test-cloudflare, test-spin, the parity suite, vitest (1188, on Node 24.12.0), JS format, node build-all.mjs, docs and markdown format: all pass.

@dhruv8sh
dhruv8sh requested review from aram356 and jevansnyc October 4, 2026 18:29

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

KV Ingestion of IDs reads cookie at response instead of body EID array: truncation issue

5 participants