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
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.
| let updates = collect_consent_gated_eid_updates( | ||
| eids_cookie, | ||
| sharedid_cookie, | ||
| ec_context, | ||
| registry, | ||
| ); |
There was a problem hiding this comment.
MEDIUM — did you intend Purpose 4 denial to also disable orphan-EC recovery?
upsert_partner_ids_from_snapshot early-returns the incoming snapshot untouched when updates is empty (ec/kv.rs:557-559), and it is the only thing on this path that performs the load_snapshot refresh (ec/kv.rs:582-591). Orphan recovery immediately below gates on that refreshed snapshot being Missing:
ec_context.set_kv_snapshot(snapshot);
if matches!(ec_context.kv_snapshot(), EcKvSnapshot::Missing { .. })
&& ec_context.recovery_eligible()
{
confirm_then_recover_orphaned_ec(...);
}Failure scenario. A GDPR user with TCF Purpose 1 granted and Purpose 4 denied, with an orphaned EC, on a non-GET publisher navigation — e.g. a form POST. recovery_eligible comes from is_publisher_navigation = ec.is_real_browser && is_navigation_request(&req) (adapter-fastly/src/app.rs:858,905), which does not require GET, whereas should_preload_ec_snapshot(is_navigation, is_get, ...) (publisher.rs:2959-2966) does.
Before this PR their ts-eids cookie produced non-empty updates -> snapshot refreshed -> Missing -> recovery ran. After this PR the Purpose 4 gate empties updates -> snapshot stays NotRead -> recovery silently never fires, and their EC stays orphaned indefinitely.
Narrow — GET navigations preload the snapshot at publisher.rs:4419-4431 and are unaffected — but it's a real coupling: consent-gating the payload also disabled an unrelated liveness mechanism. If unintended, the fix is to decouple the refresh from the update list (resolve the snapshot before the gate, or call load_snapshot when updates.is_empty() && recovery_eligible()). No suggestion block since it touches kv.rs too.
| registry: &PartnerRegistry, | ||
| ) { | ||
| let updates = collect_eid_cookie_updates(eids_cookie, sharedid_cookie, registry); | ||
| let updates = collect_eid_updates(eids_cookie, sharedid_cookie, None, registry); |
There was a problem hiding this comment.
MEDIUM — this public API still bypasses the new Purpose 4 gate
The ingest_eid_cookies / ingest_prebid_eids / ingest_sharedid_cookie family is pub, funnels through here, and writes to KV with no consent gating at all — while the path right next to it just gained a Purpose 4 gate.
Not exploitable today: these three have zero in-repo callers (only a comment reference in ec/admin.rs:602). But it leaves a public, ungated write path adjacent to a newly-gated one, which is the kind of divergence that gets picked up by accident later.
Either delete the dead pub surface or route it through collect_consent_gated_eid_updates. Not suggestion-eligible — it needs a ConsentContext threaded through three public signatures, or a cross-file deletion. Happy to file this as a follow-up issue instead of holding up the PR.
| /// Records the current request's own EIDs (e.g. an `/auction` body) for | ||
| /// use by response finalization's KV ingestion. | ||
| pub fn set_client_eids(&mut self, eids: Vec<Eid>) { | ||
| self.client_eids = Some(eids); | ||
| } | ||
|
|
||
| /// Returns the current request's own EIDs, if the route captured any. | ||
| #[must_use] | ||
| pub fn client_eids(&self) -> Option<&[Eid]> { | ||
| self.client_eids.as_deref() | ||
| } |
There was a problem hiding this comment.
LOW — pub wider than needed
AGENTS.md: "Consider visibility carefully — avoid unnecessary pub." Both accessors are used only from auction/endpoints.rs:305 and ec/finalize.rs (plus tests), and replace_with_generated directly below is already pub(crate).
Scratch-verified as a pair: applied in a worktree, then cargo clippy-fastly (0 warnings), cargo check-axum, cargo clippy-cloudflare and cargo check-spin all clean.
| /// Records the current request's own EIDs (e.g. an `/auction` body) for | |
| /// use by response finalization's KV ingestion. | |
| pub fn set_client_eids(&mut self, eids: Vec<Eid>) { | |
| self.client_eids = Some(eids); | |
| } | |
| /// Returns the current request's own EIDs, if the route captured any. | |
| #[must_use] | |
| pub fn client_eids(&self) -> Option<&[Eid]> { | |
| self.client_eids.as_deref() | |
| } | |
| /// Records the current request's own EIDs (e.g. an `/auction` body) for | |
| /// use by response finalization's KV ingestion. | |
| pub(crate) fn set_client_eids(&mut self, eids: Vec<Eid>) { | |
| self.client_eids = Some(eids); | |
| } | |
| /// Returns the current request's own EIDs, if the route captured any. | |
| #[must_use] | |
| pub(crate) fn client_eids(&self) -> Option<&[Eid]> { | |
| self.client_eids.as_deref() | |
| } |
| const dropped = payload.pop(); | ||
| if (dropped) { | ||
| droppedSources.add(dropped.source); | ||
| } |
There was a problem hiding this comment.
LOW — the warning under-reports partial EID loss
droppedSources only records sources removed wholesale by payload.pop(). The branch just above it — last.uids = last.uids.slice(0, last.uids.length - 1) — silently discards UIDs from a source that is retained, and that loss never reaches the warning. A page that trims 4 of a source's 5 UIDs logs nothing at all.
Minor, since the auction body carries the untrimmed set anyway. Raising it because the warning's whole purpose is diagnosing what the cookie dropped, and right now it's half-blind to that.
ChristianPavilonis
left a comment
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.
3 participants
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