Repository navigation
fix(eventing): decide group-member suppression on the store, not ce_groupid - #903
Conversation
…roupid Group-member push suppression read `ce_groupid` off the event, so a forged frame that simply OMITTED the attribute was not a group member as far as the check was concerned: it was pushed, stored in the member's transcript and shown on the group page. A forged terminal frame was pushed at priority 5 even with NTFY_GROUP_NOTIFY_ERRORS off, and did not count the member as failed -- so the batch's own arithmetic disagreed with the notification the operator had just received. Reachable even with signing fully enforced, because this is a routing decision made on an attacker-supplied attribute, not a signature check. DESIGN_PHASE2.md §8.5.4 states the rule: if the decision reads an attribute off the event, the forger controls it, including by leaving it out. Membership now comes from `group_of(correlationid)` on the event's own routed store -- an existing method over `group_members`, backed by the `group_members_by_corr` index, so no new query or migration. Redeliveries are unaffected: both submission paths call `add_group_member` before publishing the request, and `on_member_event` records membership on arrival ahead of the ntfy fan-out, so a genuine member is always recorded by the time suppression runs. `test_a_redelivered_member_event_after_completion_still_does_not_notify` asserted that an unrecorded correlation id claiming a group stays silent. That is the forgery this fixes, so the assertion is split: the redelivery still stays silent, and the unrecorded claimant now notifies. Fixes #891 Assisted-By: Claude Code Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
aslom
left a comment
There was a problem hiding this comment.
Summary
The fix is right and the reasoning in the description is the kind worth copying: it
names the invariant (DESIGN_PHASE2.md §8.5.4), shows the omission case rather than
just the forged-value case, and proves the new tests fail against the pre-change code.
group_members_by_corr already exists for precisely this query — store.py's own
schema comment calls it "what lets the responses consumer answer does this event
belong to a group? with a primary-key hit instead of a scan, on the hot path" — so the
new lookup lands where the schema anticipated it. Splitting
test_a_redelivered_member_event_after_completion_still_does_not_notify instead of
quietly deleting the assertion that now inverts is the correct call, and the
redelivery-ordering argument checks out against the source: group_service.submit_members
and handlers both add_group_member before publish_request, and kafka_in.run()
calls _on_member_event before _on_event.
One substantive gap, raised as a suggestion rather than a blocker because it is
pre-existing and not a regression: in multi-tenant mode the store is still selected
from ce_userkey, which is omissible in exactly the same way ce_groupid was, so the
forgery this PR closes remains reachable there. Details inline. OwnerIndex.owner_of()
is the existing non-attacker-supplied answer, and owners is already constructed in
__main__.py and passed to handlers the same way.
One correction to the description, not the code: "there is no new query" holds for the
method and the index, but not for the call count. Previously an event with no
ce_groupid returned before touching the store at all; now every event whose phase is
in NTFY_PHASES does one indexed group_of SELECT, and a grouped event does two store
reads instead of one. That is cheap and clearly intended by the index, so it is a wording
nit — but a future reader debugging consumer latency should not be told there is no new
query on the path.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Python (eventing/eventbridge/ntfy.py), Tests (eventing/tests/test_groups.py)
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes (DCO passing)
CI status: passing — all 12 checks green (eventing-test, lint, codeql, trivy-scan, dependency-review, hadolint, test, test-startup)
Verified against: head 13285619, source read at that SHA (ntfy.py, store.py, store_registry.py, group_service.py, handlers.py, kafka_in.py, owner_index.py, shared/signing.py, shared/ce.py, __main__.py)
Review: #903 (approved), two suggestions — docstring scoping now, the ownership-index fix as a follow-up. - The `_suppressed_member` docstring now states its own scope: the omission case is closed for single-tenant only, because in multi-tenant mode the store the `group_members` row is looked up in is itself chosen by `ce_userkey` — an attribute the forger controls exactly as they controlled `ce_groupid`. A frame that keeps the groupid but omits the userkey routes to shared/, where the row is not, and escapes suppression. Points at #904, filed from the review. - `RecordingNtfy` gains a `stores=` passthrough, so the fixture can exercise multi-store routing at all: the verdict is now "is there a row in the store this event ROUTED to", and a single-store test can never distinguish the right store from the only store. - `member_event` gains a `userkey` kwarg. - The multi-store case the review sketched is in as `xfail(strict=True)` citing #904 — both the service and the publisher go through one registry, so the membership row IS in the per-user store the intact frame routes to (asserted), and the only thing standing between the stripped frame and suppression is the routing itself. The test flips to a pass the moment #904's fix lands, and turns loud (XPASS strict) if anyone 'fixes' routing in a way that does not close the forgery. eventing suite: 900 passed, 7 skipped, 1 xfailed. ruff clean. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
What
Group-member notification suppression now resolves membership from the store instead of
trusting the
ce_groupidattribute on the event.Fixes #891.
Why
The decision was made on an attribute the forger supplies, so a frame that omits it
was not a group member as far as the check was concerned:
shown on the group page;
NTFY_GROUP_NOTIFY_ERRORSoff, and did not count the member as failed — so thebatch's own arithmetic disagreed with the notification just received.
Reachable with signing fully enforced: this is a routing decision, not a signature
check.
DESIGN_PHASE2.md§8.5.4 already states the rule — prefer deciding on state theattacker does not supply.
How
Membership comes from
group_of(correlationid)on the event's own routed store. Themethod already exists over
group_membersand is backed by thegroup_members_by_corrindex, so there is no new query and no migration. Phase 3's per-user
_store_for(event)routing is preserved — the group row is in its owner's store, same as the body lookup.
Redeliveries are unaffected, which is the property worth checking in review, since
the ~47-notifications-per-batch incident is what the current shape was written for. A
genuine member is always recorded before suppression runs:
group_service.submit_members,handlers) calladd_group_memberbefore publishing the request, explicitly so a fast agent'sterminal event can be attributed;
on_member_eventrecords membership again on arrival, andkafka_in.run()invokes itbefore the ntfy fan-out (
_on_member_event, then_on_event).Behaviour change, called out
test_a_redelivered_member_event_after_completion_still_does_not_notifyalso assertedthat
late-agent-0009— a correlation id that is not a recorded member but carriesthe group's id — stays silent. Under this fix it notifies, because that is exactly the
forgery being closed: an event must not be able to silence itself by asserting a group
it does not belong to. The test is split accordingly — the redelivery still stays
silent, and the unrecorded claimant now notifies, with a dedicated test naming why.
Verification
tests/test_groups.py— 72 passed, including all five pre-existing suppression tests(the regression surface for a membership-source change)
eventing/suite on this base — 900 passed, 7 skipped (3 new tests)ruff checkcleanreproduces this issue's claim verbatim:
priority: 5with errors offcarrying
ce_groupidis suppressed, the same frame with the attribute deleted issuppressed, and a genuinely ungrouped event still notifies