security(dci): scope notification delivery per subscriber (consent + filter)#258
security(dci): scope notification delivery per subscriber (consent + filter)#258gonzalesedwin1123 wants to merge 3 commits into
Conversation
…filter) The DCI notification pipeline built full DCI person/group records with no sender context and fanned the identical payload out to every subscription matched only on event/registry/active/expiry. Stored filter_expression was never applied, consent was never checked, and auto_approve removed the human gate -- so any active/auto-approved subscriber to a broad event received full PII (names, birthdate, address, phone, email, household, programmes) for every changed registrant, outside its requested filter, consent, or legal basis. Make delivery per-subscription and sender-scoped: - spp_dci_server: capture filter_type at subscribe time; add eligibility helpers on spp.dci.subscription -- _consent_allows_partner (correct primitive: has_legal_basis_bypass / can_access_registrant; NOT the dead status=="active" domain), _partner_matches_filter (base fail-closed), _eligible_partner_ids, _build_notification_records (hook), _delete_records_for. notify_event now takes (event_type, partner_ids, reg_type, delete_payloads) and, per matching subscription, delivers only consent-allowed + filter-matching records built with that subscription's sender context. - spp_dci_server_social: override _partner_matches_filter (compile the stored filter via the search service; drop on parse failure) and _build_notification_records (build with sender_id). Snapshot per-subscription eligibility at unlink so delete notifications go only to eligible subscribers; _execute_dci_notification hands off partner_ids / scoped delete payloads instead of a single global payload. Decisions: legal-basis-bypass senders pass consent (filter still applies), documented; delete scoped via unlink-time eligibility snapshot. Deferred to follow-ups: field minimization (response_fields / layer C), and the parallel dead consent filter on the search path (build_consented_domain status=="active"). Tests: spp_dci_server 320/320, spp_dci_server_social 82/82.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the DCI subscription notification system to scope delivery per subscriber, ensuring that notifications are only sent to authorized senders and match their subscription filters. It introduces registry-specific hooks for consent checks, filter evaluation, and payload building, and implements these hooks for the social registry. Additionally, delete notifications now precompute eligibility before records are unlinked. The review feedback highlights a performance bottleneck where evaluating filters individually for each partner ID leads to an O(N * M) database query pattern. To resolve this, the reviewer suggests implementing a batched filter evaluation helper (_filter_matching_partners) in both the base subscription model and the social registry subclass to process all partner IDs in a single query.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #258 +/- ##
==========================================
+ Coverage 74.75% 75.23% +0.48%
==========================================
Files 1090 1093 +3
Lines 64813 64954 +141
==========================================
+ Hits 48453 48871 +418
+ Misses 16360 16083 -277
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Address review on PR #258: - Gemini (perf): replace per-partner filter evaluation (O(partners x subscriptions) queries) with a batched `_filter_matching_partners(partner_ids)` that evaluates the filter in a single query per subscription. The social override now intersects partner_ids with the compiled filter domain in one search; `_partner_matches_filter` is a thin wrapper; `_eligible_partner_ids` applies consent per record then filters in one batch. - Codecov: add generic unit tests for the new subscription helpers (_filter_matching_partners base policy, _partner_matches_filter wrapper, _consent_allows_partner bypass, _eligible_partner_ids, _delete_records_for, notify_event delete scoping) that the generic spp_dci_server test DB exercises. Tests: spp_dci_server 326/326, spp_dci_server_social 82/82.
Staff review of the implemented branch caught a HIGH over-delivery regression:
the social filter evaluator defaulted query_type to "expression" when
filter_type was unset. The DCI client never sends filter_type, so a stored
idtype-value filter (a specific person) was parsed as an expression with no
seq/or keys -> empty domain -> matched EVERY registrant in the batch. For
legal-basis-bypass senders (consent does not backstop) this re-introduced the
full unscoped PII fan-out the PR set out to fix.
Fix: in _filter_matching_partners, treat the SPDCI {"type":"*","value":"*"}
wildcard as match-all (the client's "subscribe to all" default; consent still
gates), but for any other filter require a known filter_type
(idtype-value/expression/predicate) and fail closed (match nothing) when the
discriminator is missing/unknown rather than guessing.
Tests: add regressions for the two cases that would have caught this -
real idtype-value filter without filter_type -> matches nothing; wildcard
filter without filter_type -> matches all. spp_dci_server_social 84/84.
|
Staff/adversarial review of the implemented branch (post-Gemini refactor) found one HIGH issue, now fixed in
Fix: treat the SPDCI The rest of the implementation (consent-then-filter ordering, per-subscriber scoped build, fail-closed delete snapshot, base hooks denying for non-overridden registries) was confirmed sound by the review. |
1 participant
Summary
The DCI Social Registry notification pipeline leaked PII to overbroad subscribers.
res.partnercreate/update/delete built full DCI person/group records viaDCISocialSearchServicewith no sender context andnotify_eventfanned the identical payload out to every subscription matched only on event/registry/active/expiry. The sender's storedfilter_expressionwas never applied, consent/legal-basis was never checked, andsender.auto_approveremoved the human gate. Net: any active/auto-approved subscriber to a broad event (REGISTRATION/UPDATE) received the full PII record (names, birthdate, gender, phone, email, address, household, programme enrollments) for every changed registrant — outside its requested filter, allowed fields, consent, or legal basis.Fix — per-subscription, sender-scoped delivery
Build and queue the payload per subscription, scoped to that subscription's sender, instead of once globally.
spp_dci_serverfilter_typefield onspp.dci.subscription(the discriminator the subscribe handler previously dropped), now captured at subscribe time._consent_allows_partner(uses the correct primitive —has_legal_basis_bypass/can_access_registrant→check_api_consent; notbuild_consented_domain, which filters the non-existentstatus == "active"and matches nothing),_partner_matches_filter(base = fail-closed for any unparseable filter),_eligible_partner_ids,_build_notification_records(registry hook),_delete_records_for.notify_event(event_type, partner_ids, reg_type, delete_payloads=None)— per matching subscription, delivers only consent-allowed + filter-matching records, built with that subscription's sender context.spp_dci_server_social_partner_matches_filter(compiles the stored filter via the search service and tests the partner; drops on parse failure) and_build_notification_records(builds withsender_idcontext)._dci_delete_payloadssnapshots per-subscription eligibility at unlink (record still exists) so delete notifications reach only eligible subscribers;_execute_dci_notificationhands offpartner_ids/ scoped delete payloads instead of a single global payload.Decisions (with maintainer)
filter_expressionstill applies. Documented that auto_approve + bypass = full firehose by design.Deferred / tracked separately (not this PR)
response_fields(also dropped at subscribe time) so eligible senders receive only authorised fields. Records currently carry the full field set for eligible senders.build_consented_domain/_apply_consent_filter) uses the same deadstatus == "active"predicate — it returns nothing for consent-required senders and would flip open if naively "fixed." Worth its own ticket.Tests
Updated the 4 tests affected by the
notify_eventsignature/behavior change (delete payloads moved to a kwarg; legacy delete without an eligibility snapshot now fail-closed). Added a scoping suite: consent blocks without basis; legal-basis bypass allows; filter match / non-match / unparseable-fail-closed / no-filter; record building with sender context; delete eligibility include/exclude.All green:
spp_dci_server320/320 ·spp_dci_server_social82/82. Lint-clean (ruff/ruff-format).