Repository navigation
Conversation
ChristianPavilonis
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Clarifying the two cookie decisions, and recording the implemented follow-up. Existing
|
jevansnyc
left a comment
jevansnyc
left a comment
There was a problem hiding this comment.
Review of the implementation commits (through 1eaf3c5)
Thanks for carrying this through to working code. The foundation pieces are things I want to keep: expiry-aware records, source ownership, the bounded response inspection that preserves the original bytes, and send-before-write on Fastly. The two follow-up commits also already close three of the things I had flagged:
ids.id5id.eidnesting inid5_token, plus the test now assertingIssuedinstead oflen() == 1.- Legacy USP (section 6) inside a GPP string now decodes, with the malformed-section cases covered.
- Writer-keyed provenance reconstruction for Lockr-written ID5 records. Keying on the persisted writer rather than
identity_owneris cleaner than the static table in my spec, and it answers the parity requirement.
What is left falls into two groups: decisions from the original spec I want to keep open rather than accept by default, and a handful of code points. Inline comments carry the file-level detail.
Spec decisions I want to reopen
Same-response cookie and next-auction availability (original requirements 2 and 3). The spec had the captured ID written to a server-owned cookie on the same response and to KV after send. The implementation drops the cookie half and makes "no additional EID cookie" an invariant. On the affected route the Lockr page-view call and the first /auction both fire right after the consent gate, a few hundred milliseconds apart, so KV-only delivery loses the ID for the whole first page view, and there is no delivery path when the row is NotRead, failed, or absent. I am open to not adding ts-eids-s if we have a measured answer for how often the first auction beats the post-send write on that route. If we cannot measure it, I would like the cookie back.
Retiring ts-eids. The spec said the JS collector does not change and ts-eids keeps covering User ID module providers. The commit removes the writer, the server readers, the /auction fallback, the admin diagnostic and the x-ts-eids headers. That is a decision for every publisher running User ID modules, not a Lockr decision, and it has side effects on its own: late IDs that arrive after the last auction on a page are never persisted, and Axum, Cloudflare and Spin go from forwarding consented client EIDs to forwarding none, since they have no KV graph. The operator docs (docs/guide/ec-setup-guide.md, docs/guide/edge-cookies.md) still describe both. Body-EID persistence at finalization is a good addition and can land additively with the cookie as fallback. If you want to retire the cookie, let's give it its own spec and PR with a migration plan.
Code points still open
restrictive_identity_signals_allowstill denies a GPP string with no US, USP or TCF section, which is theDBAAshape CMPs emit when nothing applies. Inline atconsent/mod.rs.- Post-send capture writes have no
is_real_browsergate, unlike pull sync. Inline atmain.rs. EidSyncSource::Auctionis now set in both core and the Fastly adapter. Inline atendpoints.rs.- Six
change_contextcalls inlockr.rsbecamemap_err(|_| Report::new(..)), dropping the source error. Inline. - Small: hard-coded
512inid5_token,PartnerRegistry::from_configrebuilt per captured response with a silent skip on error, andhandle_page_bidsno longer rejects a non-UTF-8 Cookie header now thathandle_request_cookiesis gone from that route.
Rollout notes worth stating in the Lockr guide
- The
id5-sync.compartner entry needsidentity_owner = "lockr"orstage_capturedrops every outcome without a log line. - An owned source rejects legacy batch writes and cannot have pull sync enabled, so an existing deployment with ID5 on pull sync has to migrate first.
- Rolling back to a UID-only binary after the first managed write strips
expires_at,writerandrevision. You call this out already; it needs a named procedure before activation on a production service. - With
capture_identitydefaulting to true, I would rather it default to false until the live acceptance steps have run once.
Requested changes
- Split into separate PRs so each can be reviewed and rolled back on its own: the foundation; the Lockr capture consumer; and a separate proposal for body-EID persistence and any
ts-eidschange. - Keep #1260 as the spec PR. Your foundation spec and plan can go in their own docs PR, and my spec gets a short decisions section recording what we agree to change and why.
- Keep
ts-eids, its readers, the admin diagnostic and thex-ts-eidsheaders in this round. - Fix the
DBAAcase and the browser gate before anything is activated. - Drop the CI image commit from this branch.
Questions
- What is your preferred answer for the first auction on a first visit without a cookie, and how would we measure how often it happens on the affected route?
- Which GPP strings did you test the capture gate against? I want to confirm the
DBAAcase rather than assume it. - Why retire
ts-eidsnow rather than after body persistence has run in production for a while?
| if gpp.us_sale_opt_out == Some(true) | ||
| || gpp.eu_tcf.as_ref().is_some_and(|tcf| !tcf_allowed(tcf)) | ||
| || (gpp.section_ids.contains(&GPP_SECTION_ID_TCF_EU_V2) && gpp.eu_tcf.is_none()) | ||
| || (gpp.us_sale_opt_out.is_none() && gpp.eu_tcf.is_none()) |
There was a problem hiding this comment.
This still returns false for a decodable GPP string that has no US section, no USP section and no TCF section. That is the header-only DBAA shape a CMP emits when no section applies to the visitor, and also any string carrying only a non-US, non-EU section. The existing EC consent path treats that string as permissive, so capture ends up stricter than ID creation for visitors who have signalled nothing restrictive.
Suggest: a GPP string with no applicable section passes this check; only a decoded opt-out, a failing TCF section, or a declared-but-undecodable section (the new line above) should deny.
| } | ||
|
|
||
| /// Keeps capture effects through cookie finalization, then consumes them after send. | ||
| fn send_identity_response( |
There was a problem hiding this comment.
run_edgezero_pull_sync_after_send refuses to run when !ec_state.is_real_browser (line 637), but this path applies the staged capture effects regardless. A script replaying a copied ts-ec cookie can POST to the Lockr proxy and, if upstream issues a token, enrich the row; with capture_global_withdrawal on, it can tombstone it. Can we apply the same gate here before send_then_apply_identity?
| && consent_allows_server_side_auction(&consent_context) | ||
| { | ||
| ec_context.stage_browser_eid_updates(collect_prebid_eid_updates_from_eids(eids, registry)); | ||
| ec_context.set_eid_sync_source(crate::ec::EidSyncSource::Auction); |
There was a problem hiding this comment.
The Fastly adapter already sets EidSyncSource::Auction before dispatch (app.rs, NamedRouteHandler::Auction), and the doc on set_eid_sync_source describes it as adapter-assigned after request filters pass. With both writers an adapter that chooses not to classify a request gets persistence re-enabled here. Could we keep this in one place, probably the adapter?
|
|
||
| let backend_name = Self::backend_name_for_url(services, &self.config.api_endpoint) | ||
| .change_context(Self::error("Failed to determine backend for API proxy"))?; | ||
| .map_err(|_| Report::new(Self::error("Failed to determine backend for API proxy")))?; |
There was a problem hiding this comment.
Minor: this and the five sibling sites moved from change_context to map_err(|_| Report::new(..)), which drops the backend lookup, request build and HTTP client errors from the report. The redaction goal is met by taking the URL out of the message; the chain can stay. AGENTS.md asks for change_context for this.
| .get("universal_uid")? | ||
| .as_str()? | ||
| .to_string(); | ||
| if uid.is_empty() || uid.len() > 512 { |
There was a problem hiding this comment.
Nit: crate::ec::kv_types::MAX_UID_LENGTH is the cap stage_capture and KvEntry::validate enforce. Using it here keeps the parser and the shared boundary in step if it changes.
|
|
||
| Invariants: | ||
|
|
||
| 1. No additional EID cookie. `ts-ec` remains the pointer to an existing EC row. |
There was a problem hiding this comment.
This is the point I most want to talk through. The original spec had the captured ID delivered two ways: a server-owned HttpOnly cookie on the same response, for the auction(s) on the current page, and KV after send for later navigations. With this invariant the first auction on a first visit never sees the ID, and a visitor whose row is missing or unreadable gets nothing until a navigation creates one. One ID5 token is 184 bytes on the wire.
I am open to not adding ts-eids-s if we can measure how often the first auction beats the post-send write on the affected route. Without that measurement I would like the cookie back as designed.
|
|
||
| Delayed availability is accepted: a later auction sees an ID only after the | ||
| deferred write completes and its KV read can see the record. The first auction | ||
| may miss it. There is no cookie fallback or durable capture retry queue. |
There was a problem hiding this comment.
This sentence is where the original requirement 3 (next auction on the same page sees the ID, cookie as fallback) is dropped. See my comment on the foundation spec invariant; I would rather this stay a requirement than a stated non-goal, unless we have numbers showing the loss is rare on the affected route.
| log.debug(`[tsjs-prebid] synced ${payload.length} EID sources to cookie`); | ||
| } catch (err) { | ||
| log.warn('[tsjs-prebid] failed to sync EIDs cookie', err); | ||
| function expireRetiredPrebidEidsCookie(): void { |
There was a problem hiding this comment.
I did not want ts-eids retired as part of this work. The spec listed the JS collector as out of scope and said ts-eids keeps covering User ID module providers. Retiring it affects every publisher running those modules, drops IDs that arrive after the last auction on a page, and on Axum, Cloudflare and Spin (no KV graph) means no client EIDs reach the auction at all.
Body-EID persistence at finalization is a good addition and can land with the cookie still in place. If you want to propose retiring the cookie, let's do that as its own spec and PR with a migration plan, and the x-ts-eids header question can go with it.
| /// graph. Only the Fastly adapter has a KV identity store, so Axum, Cloudflare, | ||
| /// and Spin pass `None`; client cookie EIDs are still resolved and consent-gated | ||
| /// on every adapter, so no adapter forwards unconsented EIDs. | ||
| /// and Spin pass `None` and cannot resolve server-side EIDs. Retired EID cookies |
There was a problem hiding this comment.
This doc comment is the regression stated plainly: before this change these adapters forwarded consented client cookie EIDs on page-bids auctions, and after it they forward none. That is another reason to keep the cookie path in this round.
| # sdk_url = "https://identity.example.com/trusted-server.js" | ||
| # cache_ttl_seconds = 3600 | ||
| # rewrite_sdk = true | ||
| # capture_identity = false # require provider approval and old-binary drain |
There was a problem hiding this comment.
Given how many gates still have to pass before this is safe to run, could capture_identity default to false in code as well, not only in the example? A deploy that enables the integration and sets identity_owner would otherwise start capturing before the live acceptance steps have run.
Lockr-issued IDs reach client-side bidders through ortb2.user.ext.eids but never the server-side auction, the ts-eids cookie, or the EC identity graph (#1246). This spec captures the IDs in the first-party proxy response from lockr's API, sets a server-owned HttpOnly cookie on that response, and defers the KV upsert to the post-send slot so no KV write sits on the hot path.
Read the associated ID5 EID from ids.id5id.eid and assert the issued identity rather than only counting capture outcomes. Normalize legacy USP sections in shared GPP decoding while preserving opt-out precedence and failing acquisition closed on malformed signals. Add parser, mixed-signal and capture persistence regressions.
Use the persisted acquisition writer and source to restore Lockr's fixed EID provenance without changing the KV schema or capture activation. Keep other writers unattributed and suppress reconstructed provenance when browser input adds mixed-origin UIDs for the same source. Update the specs, guide and verification plan, with regressions for writer eligibility, browser precedence and the KV-to-bidder path.
Restore bounded absent-source availability and post-send retry through a default-off publisher/full-EC-bound ts-eids-s handoff. Use dedicated rotating signing keys, fixed deadlines and fill-absent-only CAS. Preserve pending expiry and writer metadata without granting cookies authority over stored records. Keep browser-written ts-eids retired and document the existing KV snapshot consistency and live rollout gates.
cbc7b42 to
16b6d09
Compare
Brings #1260 at 16b6d09 (draft). Integration decisions: - Fastly delivery uses #1260's send_identity_response, extended with the RC's SendContext and post-send steps: identity effects apply after send, pull sync runs only when capture storage is available, and access telemetry always reports the delivered response. - require_identity_graph stays untimed; its doc now names identity capture as a second post-send caller. - The /auction response keeps #1107's trace cache privacy and drops the x-ts-eids headers, as #1260 does. - The integration proxy route reads #1260's integration id from the RC's three-field route tuple. - #1260 removes server-side ts-eids cookie parsing. #1107's trace cookie health now checks the ts-eids format with a local validator that accepts the same shapes the removed parser did. - ec_finalize_kv_lands_before_freeze drives its KV round trip with a SharedID cookie instead of the removed ts-eids ingestion.
This branch has not been deployed
5 participants
Summary
Implements the server-side identity foundation and ID5-only Lockr proxy-response capture, with the reviewed specs and implementation plan.
/auctionbody EIDs conservatively during existing finalization. Retirets-eidsingestion/writer and EID diagnostic emission; preserve inbound strips and authenticated local admin retirement on all adapters.ts-ec; no replacement EID cookie.Verification
cargo test-fastly: 219 adapter + 2,924 core tests passed (six core ignored), shared/doc tests passed.cargo test-fastly-reuse, all three native adapter suites,./scripts/test-cli.shincluding required ignored browser fixtures, and native build-digest tests passed.Evidence and reproducible commands:
docs/superpowers/plans/2026-10-09-server-side-identity-foundation-and-lockr.md.Activation gates / deliberately deferred
This is a draft implementation, not permission to activate capture. The rollout example explicitly sets
capture_identity = falseandcapture_global_withdrawal = false. Capture defaults to true for compatible configuration; operators must explicitly disable it until the gates pass.Not verified: provider-approved sanitized fixtures/mappings, consent/revoke scope and application-success semantics, overlap ordering, live acceptance, metadata-compatible deployment propagation and old-invocation drain, external diagnostic consumers, and remote CI. Old writers can erase lifecycle metadata and old readers ignore expiry; do not activate managed writes during mixed-binary deployment.
Native LiveRamp resolution, identifier intake, refresh/invalidation scheduling, durable retries and cross-browser linkage remain deferred. Other adapters do not claim Fastly-style post-send KV persistence.
Refs #1246, #1245.