Support Sourcepoint GPP consent for EC generation#642
Support Sourcepoint GPP consent for EC generation#642ChristianPavilonis wants to merge 27 commits intofeature/edge-cookies-finalfrom
Conversation
7009838 to
43df212
Compare
0e5c07c to
a8b5b1c
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Sourcepoint GPP consent is a small, well-motivated slice — the GPP-US decoder, the allows_ec_creation gating branch, and their tests are solid. But as shipped, the feature is non-functional (the JS module is never served to browsers), the PR fails CI, and it bundles several large unrelated changes that should be separate PRs.
Blocking
🔧 wrench
-
Sourcepoint JS module is built but never served.
crates/trusted-server-core/src/integrations/registry.rs:790-792hardcodesJS_ALWAYS = &["creative"]. The design spec (docs/superpowers/specs/2026-04-15-sourcepoint-gpp-consent-design.md:38) says the integration follows the same "JS-only, no Rust registration" pattern ascreative, but that pattern requiresJS_ALWAYSto list the module.integrations/mod.rshas nosourcepoint::registerandJS_ALWAYShas no"sourcepoint"entry, soIntegrationRegistry::js_module_ids()will never include it —/static/tsjs=…will never concatenatetsjs-sourcepoint.jsinto a served bundle. End-to-end, no browser will ever executemirrorSourcepointConsent(). Fix: add"sourcepoint"toJS_ALWAYSand a test asserting it ships injs_module_ids_immediate(). -
Orphaned dead-code file
crates/trusted-server-core/src/ec/admin.rs. 380 lines implementingPOST /_ts/admin/partners/register, butec/mod.rscontains nopub mod admin;declaration (grep -rn "mod admin\|ec::admin::" crates/returns zero hits). The base branch (feature/edge-cookies-final) replaced the KV-backed partner registry with config-based partners in commit049a501e; that change droppedadmin.rsbut the rebase on this branch reinstated it unreferenced. Fix: deletecrates/trusted-server-core/src/ec/admin.rs. -
Clippy
-D dead_codefailures block CI. Three dead symbols introduced by the same rebase and carrying no callers:crates/trusted-server-core/src/platform/test_support.rs:174—recorded_request_headersmethodcrates/trusted-server-core/src/platform/test_support.rs:336—build_services_with_http_clientfunctioncrates/integration-tests/tests/common/runtime.rs:56—PartnerRegistrationFailedvariant (this is the actual failing CI log message)
Confirmed locally:
cargo clippy --workspace --all-targets --all-features -- -D warningsfails in the PR's worktree. Fix: delete them all — nothing references any of the three. -
Undisclosed scope: creative iframe sandbox lockdown.
crates/js/lib/src/core/render.ts:7-18removesallow-scripts,allow-same-origin, andallow-formsfrom the creative iframesandbox=attribute, andcrates/trusted-server-core/src/creative.rs:361-367strips<iframe>elements entirely during HTML sanitization. Together, any creative relying on JavaScript (viewability pixels, click tracking, rich media, VPAID) or on third-party ad-tag iframes will stop rendering. This is a major ad-tech behavioral change wholly unrelated to Sourcepoint GPP consent, and it is not mentioned in the PR body. It needs its own PR with a threat model, product sign-off, and a rollout plan. Fix: extract these two changes to a dedicated PR.
❓ question
- Why is Prebid User ID Module support shipping inside this PR? The title and body are "Support Sourcepoint GPP consent for EC generation," but the diff also contains ~132 lines of
build-all.mjsUser-ID-submodule generation, a new_user_ids.generated.ts, ~290 lines of new Prebid test coverage, a 212-line design spec (specs/2026-04-16-prebid-user-id-module-design.md), and a 599-line implementation plan. Two independent features in one review is hard to evaluate. Can Prebid User ID Module move to its own PR?
Non-blocking
📌 out of scope
- No re-mirror after mid-session CMP interaction.
mirrorSourcepointConsent()runs once when the module is parsed. If a user opens the Sourcepoint CMP and updates consent mid-session,__gpp/__gpp_sidwon't reflect the new state until the next page load — meaning the same session can continue to block EC creation even after consent is granted. Follow-up: subscribe to__gppapievents (or astoragelistener) and re-run on change.
|
Addressed the requested review feedback in 3bbdb1d2. Summary:
Validation run locally:
I also replied to and resolved the inline threads above. |
3bbdb1d to
8a6df3a
Compare
9261993 to
d8c943d
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Follow-up review focused on areas not covered in the prior pass. Sourcepoint flow (mirror + GPP US decoding + EC gating) is functional and well-tested, but two doc/file-stale issues from the scoping commit (8a6df3af) need correction before merge, and the always-shipped Sourcepoint module has cross-CMP failure modes worth designing around.
PR effective scope note: vs main the diff is 80 commits / 111 files, because the intended base (feature/edge-cookies-final) has merged. The Sourcepoint-specific changes are a small subset (~14 files); reviewers landing on this PR via GitHub should read the description with that in mind.
Blocking
🔧 wrench
- Stale Prebid User ID docs reference removed
TSJS_PREBID_USER_IDSenv var (docs/guide/integrations/prebid.md:226-261) _user_ids.generated.tsclaims to be auto-generated but is now hand-edited (crates/js/lib/src/integrations/prebid/_user_ids.generated.ts:1)
Non-blocking
🤔 thinking
- Sourcepoint hardcoded as always-shipped will clobber
__gppset by other CMPs (registry.rs:792+sourcepoint/index.ts:60-72) - First page load race: mirror runs before Sourcepoint CMP populates localStorage (
sourcepoint/index.ts:91-93) - TCF presence silently overrides explicit GPP US
sale_opt_out=Yesin US states (consent/mod.rs:506-515) - Session-scoped cookie + run-once mirror leaves stale
__gppafter mid-session retraction (sourcepoint/index.ts:39)
♻️ refactor
- Stale comment displaced by
us_sale_opt_outinsertion (consent/gpp.rs:74-75) - Make clearing logic CMP-safe by tracking write source (
sourcepoint/index.ts:42-46)
⛏ nitpick
- Unused/inconsistent default export (
sourcepoint/index.ts:95)
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS (1001 lib + 21 misc)
- js tests: PASS (294)
8643550 to
24d4515
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Follow-up review focused on what's new since the last pass and on items I think were missed. The marker-cookie response in a2c6d831 substantively addresses most prior feedback (clearing is now CMP-safe, retries cover the first-load race, the stale comment is fixed, the default export is gone). The remaining issues are: the symmetric write-path clobber that the marker doesn't cover, a real CI failure inherited from the merge-base, and continued scope creep beyond the Sourcepoint feature.
Local verification on the PR HEAD passes: cargo check, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo test --workspace, and npx vitest run (301 tests). CI fails because the prepare integration artifacts job hits a build-time validation panic.
Blocking
🔧 wrench
-
CI: integration test passphrase below 32 chars.
prepare integration artifactspanics inbuild.rs:42viaEc::validate_passphrase—MIN_PASSPHRASE_LENGTHwas bumped from 8 to 32 in commit8e08e642(which is on the merge-base, hence inherited), but the integration env still setsTRUSTED_SERVER__EC__PASSPHRASE=integration-test-ec-secret(26 chars) in:.github/actions/setup-integration-test-env/action.yml:83.github/workflows/test.yml:58scripts/integration-tests.sh:56scripts/integration-tests-browser.sh:35
These files are not in this PR's diff, so the fix is either land it here or in a small precursor PR — but the PR cannot merge with red CI. Suggested value:
integration-test-ec-secret-padded-32(or longer). -
Sourcepoint mirror clobbers other CMPs'
__gppon the write path — see inline atsourcepoint/index.ts:115.
Non-blocking
🤔 thinking
-
Scope: PR still bundles unrelated commits after the partial scope-down.
8e3ff806removed the Prebid User ID feature, but the diff vsfeature/edge-cookies-finalstill containsfedcc347("Wire request signing… and Move logging initialization") which re-applies already-merged PRs #609/#610 — once the base branch is rebased onto currentmain, this commit is duplicate work and a guaranteed merge-conflict source.24d45156("Remove generated Prebid user ID shim") and theclearPrebidEidsCookiechange atprebid/index.ts:454are also outside the Sourcepoint feature. Suggestion: rebasefeature/edge-cookies-finalonto currentmainfirst to dropfedcc347, and split the Prebid User ID shim removal into its own PR. -
__gpp_sidmarker-check on the write path is dead code — see inline. -
scheduleInitialRetryleaks a realsetTimeoutacross Vitest tests — see inline. -
DOMContentLoaded+setTimeout(500)can double-firemirrorSourcepointConsent— see inline. -
Prebid
clearPrebidEidsCookieon missinggetUserIdsAsEidsis a behavior change — see inline.
♻️ refactor
decode_us_sale_opt_outaccumulator can dropsaw_not_opted_out— see inline.
🌱 seedling
- Cookie size limits not enforced client-side. Server caps at
MAX_GPP_STRING_LEN: 8192inconsent/gpp.rs:33, but browsers cap individual cookies near ~4096 bytes. A long Sourcepoint GPP payload (compound multi-section) can be silently truncated by the browser, leaving the server unable to decode. Worth a follow-up: cap and log client-side instead of writing a doomed cookie.
📌 out of scope
- TCF-vs-GPP-US precedence is documented but not configurable. The spec section "TCF-before-GPP precedence is intentional" makes a permanent policy choice some publishers may want to override. Tracking issue would prevent this from getting buried.
- Sourcepoint integration runs on every Trusted Server page (
registry.rs:803,sourcepoint/index.ts:151). With the marker cookie this is correctness-safe, but every visitor on every site pays ~3 KB of payload + a localStorage scan + cookie writes for a feature that only applies to Sourcepoint customers. Worth a follow-up: opt-in via publisher config, or only register listeners after a first valid mirror.
⛏ nitpick
- Spec lists test file at the wrong path — see inline.
CI Status
- fmt: PASS (local)
- clippy: PASS (local,
--all-targets --all-features -- -D warnings) - rust tests: PASS (local, 1073 + 21 + 19)
- js tests: PASS (local, 301 tests / 24 files)
- integration tests: FAIL (
prepare integration artifactspanics; see Blocking above)
f9d95fc to
0c542e3
Compare
Prebid's liveIntentIdSystem.js uses a dynamic require() inside a build-flag-guarded branch that their gulp pipeline dead-codes via constant folding. esbuild leaves the require() in the output, causing ReferenceError: require is not defined at browser runtime. Remove from the bundle until we add an esbuild resolver plugin (or switch to Prebid's own build pipeline) — tracked as a follow-up in the design spec.
Introduces TSJS_PREBID_USER_IDS env var (mirroring TSJS_PREBID_ADAPTERS) to control which Prebid User ID submodules are bundled. The hardcoded imports in index.ts are replaced with a generated file written by build-all.mjs at build time, defaulting to the same 13-submodule set. - build-all.mjs: generatePrebidUserIds() validates names, denylists liveIntentIdSystem, and writes _user_ids.generated.ts. Existence check also probes dist/src/public/ to handle modules shipped as .ts in sources (sharedIdSystem). - index.ts: replaces 13 hardcoded submodule imports with import './_user_ids.generated' - _user_ids.generated.ts: committed default with all 13 submodules - Tests: updated mocks and regression guard; added 9 syncPrebidEidsCookie behavior tests - Docs: new "User ID Modules" section in prebid.md with TSJS_PREBID_USER_IDS usage; spec follow-up #1 marked complete
__gpp and __gpp_sid are read by the Rust server over HTTPS; they must be Secure. Also sets Max-Age=86400 (matching ts-eids) so stale consent state doesn't outlast the session, and replaces the legacy expires= deletion pattern with Max-Age=0.
e7b561a to
d0dd47b
Compare
Summary
_sp_user_consent_*from localStorage and mirrors GPP consent into__gpp/__gpp_sidcookiessale_opt_outfrom US GPP sections (IDs 7–23)allows_ec_creation()between existing TCF andus_privacychecksCloses #640
Test plan
cargo test --workspace— 992 tests including 8 new)npx vitest run— 288 tests including 6 new)🤖 Generated with Claude Code