fix(notifications): stop notifying users who may not read mail - #235
fix(notifications): stop notifying users who may not read mail#235Waiel5 wants to merge 3 commits into
Conversation
Inbound fanout picked recipients from `inbox_permissions` plus admins and applied no further check. Two kinds of user therefore kept receiving notifications after being cut off from the API: - A banned user. `/mcp` refuses them outright as "account suspended", but their subscriptions were still delivered to. - A user with no registered passkey. `requirePasskey` 403s every `/api/*` request they make, so they cannot open the message — yet the notification still arrived. This matters because the payload is not a bare "you have mail" ping. It carries the sender's name, the subject, and a 140-character body preview, so delivering it to a user the API refuses hands them mail content through a side channel and makes requirePasskey's own stated purpose — "passkey registration is actually required to access data" — false for anyone holding a push subscription. It is the same reasoning the MCP handler already gives for re-checking passkeys on bearer requests instead of exempting itself. `filterNotifiableUsers` applies both conditions between target computation and delivery, so `computeFanoutTargets` stays pure and separately testable. The passkey condition is skipped in development, matching requirePasskey and the MCP handler, so local dev and e2e still receive notifications. The tests pass the env explicitly rather than relying on the ambient test bindings, which set DISABLE_PASSKEY_GATE="true" — under those the passkey branch never executes and a test written against them would assert nothing.
|
Note on the red check: it is the Exactly one check-run exists on this commit. I can't clear it myself — Locally: 56 passed across |
|
There isn't a path to banning a user at this moment, but I think there's an opportunity of making it so that when a user is removed, all the subscriptions should be removed as well? |
|
Agreed, and I think that is the better shape — removing a user should take their subscriptions with them. Worth noting The case this PR is about is the one where the user is still present but should not be receiving mail content. Two of those exist right now:
So I would frame it as: cascade-on-delete handles the deleted user, and this handles the present-but-refused one. Happy to drop the ban half if you would rather not carry a check for a state nothing sets yet — the passkey half is the one I actually hit. |
…r cap The eligibility filter added earlier on this branch passes the whole candidate list to `inArray` in one statement. `MAX_ADMIN_FANOUT` caps the admin half of that list at 50, but the `inbox_permissions` half is uncapped — every member of the recipient inbox lands in the same `IN (?, ?, …)`. D1 allows a maximum of 100 bound parameters per query, so a shared inbox with enough members makes the query throw before it returns a single row. That failure was invisible. `filterNotifiableUsers` runs inside the `ctx.waitUntil` block in `email-handler.ts`, whose catch logged a warning and moved on, so the entire fanout was skipped and nobody — not the 200 members, not the admins — received a notification for that message. Inbound mail still succeeded, which is why nothing surfaced. The security fix therefore broke delivery on exactly the deployments it was written for: collaborative inboxes with many members. `main` has no such query, so this is a regression introduced by this branch, not a pre-existing one. Both `inArray` sites are now issued in chunks of 90 ids. The cap is 100 and these queries bind nothing but ids today, so 90 leaves room for a future condition on the same statement without silently reintroducing the ceiling. Chunks run concurrently and their rows are concatenated; the ban and passkey predicates are applied to the union afterwards exactly as before, so the same candidates in produce the same eligible users out. Below 90 candidates the behaviour is byte-for-byte the previous single query. The catch is also part of the bug. A thrown fanout and a fanout with no eligible recipients both left the same quiet trace, so an operator reading logs could not tell "nobody needed notifying" from "everybody was missed". It now logs at error level and names the inbox and email id, while still swallowing the error — a notification that fails to send must never fail the inbound message. Verification, rather than trusting the documented limit: `select … where id in (…)` against the test D1 succeeds at 100 bound parameters and throws at 101, matching Cloudflare's published "maximum bound parameters per query: 100". The new test asserts that ceiling directly — 250 ids rejected in one statement — before asserting that `filterNotifiableUsers` handles the same 250, so the chosen count is proven to fail without the chunking rather than assumed to. It fails on the parent commit with the D1 error and passes here. No CHANGELOG entry: the regression exists only on this branch and was never released, so there is nothing for a reader of the released history to learn.
|
Pushed a fix to this branch — I introduced a bug in it and would rather flag it than let it through.
That is worst on exactly the deployments this PR is for: a collaborative inbox with a real team. Both The ceiling is verified rather than assumed: Cloudflare documents 100, and probing the workerd D1 the suite runs against gives n=100 OK / n=101 throws. Two tests, one of which asserts that the chosen size genuinely exceeds the ceiling, so if that limit ever rises the test fails loudly instead of quietly covering nothing. Red-before-green checked by reverting the chunking and confirming the failure is the D1 error itself. I also bumped that catch from |
2 participants
Summary
Inbound fanout picks recipients from
inbox_permissionsplus admins and applies no further check, so two kinds of user keep receiving notifications after being cut off from the API:/mcprefuses them outright as "account suspended", but their subscriptions are still delivered to.requirePasskey403s every/api/*request they make, so they cannot open the message — yet the notification still arrives.This matters because the payload is not a bare "you have mail" ping. It carries the sender's name, the subject, and a 140-character body preview (
do/notifications.ts), so delivering it to a user the API refuses hands them mail content through a side channel.Changes
filterNotifiableUsersapplies both conditions between target computation and delivery, socomputeFanoutTargetsstays pure and separately testable.banned && (!banExpires || banExpires > now)), so an expired ban still notifies.isDevEnvironment, matchingrequirePasskeyand the MCP handler, so local dev and e2e still receive notifications.Release impact
Patch — bug fix or internal change, no new behaviour
Bug Fix
Test plan
yarn teston notification-fanout, do-notifications, notifications-router, inbound-forward — 56 passedprettier --checkcleanNotes for reviewers
The passkey half is the debatable one, so I want to be explicit about the reasoning rather than slip it in. My first instinct was that a passkey-less user is merely mid-onboarding and should still be told mail arrived. What changed my mind is
require-passkey.ts's own stated purpose — "so passkey registration is actually required to access data" — and a subject line plus body preview is data. It is the same argumentmcp/http.tsmakes for re-checking passkeys on bearer requests instead of exempting itself. If you read that differently I am happy to gate only onbannedand drop the passkey condition; it is a two-line change.One test detail worth noting: the tests pass
envexplicitly rather than relying on the ambient bindings, which setDISABLE_PASSKEY_GATE="true". Under those the passkey branch never executes, so a test written against them would silently assert nothing.Checklist
yarn db:generate) if the schema changed — n/aCHANGELOG.mdunder## [Unreleased]