Filter notification recipients by current board access; reconcile stale assignments

[jeremy]

Problem

Two board-move follow-ups spun off from #3070 (H1 #3512076), both in the card access/notification path.

P1 — notification leak via unfiltered card.watchers (security)

card.watchers is watch-scoped, not access-scoped:

has_many :watchers, -> { active.merge(Watch.watching) }, through: :watches, source: :user

After a card moves to a board an assignee/watcher can't access, handle_board_change only schedules async cleanup (clean_inaccessible_data_later, remove_inaccessible_notifications_later). In that window the watch row still exists, so Notifier::CommentEventNotifier#recipients (and CardEventNotifier's comment branch, and card_published's force-included card.assignees) select recipients who no longer hold Access to the card's board. A new comment then pushes a private comment excerpt to them. Pre-existing; independent of #3070 (which turns the permanent over-grant into a transient window, not the origin).

P2 — stale Assignment after access loss (correctness)

Neither Card#clean_inaccessible_data nor Board#clean_inaccessible_data_for reconciles Assignment rows. So a user can remain assigned to a card on a board they can't access. Cards::AssignmentsController#create resolves toggles via @board.users.active.find(...), so unassigning that user raises ActiveRecord::RecordNotFound; the stale rows also count toward Assignment::LIMIT. Reachable today via admin access-revocation (Access#destroy → Board#clean_inaccessible_data_for), and via the move path once #3070's grant-skip lands.

Fix

P1 — one access gate at the notify point (Notifier#notify), covering every branch (comment, publish, assignment, mention) and any future notifier, rather than patching each card.watchers/assignee call site:

def accessible_recipients
  accessible_user_ids = source.card.board.accesses.pluck(:user_id).to_set
  recipients.select { |recipient| !recipient.active? || accessible_user_ids.include?(recipient.id) }
end

The !recipient.active? clause preserves existing behavior for deactivated users: User#deactivate destroys their accesses, and notifications are still created for them (never pushed — they have no session). The leak class is active users who lost access; card.watchers is already active-scoped, so this matches the actual exposure. On all_access boards every active user holds an Access row, so nothing legitimate is filtered.

Defense at the read/notify point, independent of the async cleanup — a pure per-send read filter, so no lock/marker/persisted intermediate state and no new TOCTOU or regenerate-race surface. Fails closed (no access row ⇒ active user not notified).

P2 — reconcile stale assignments in both cleanup paths, alongside the existing pins/watches reconciliation:

• Card#clean_inaccessible_data (move path): assignments.where.not(assignee_id: accessible_user_ids).in_batches.delete_all
• Board#clean_inaccessible_data_for(user) (access-revoke path): assignments_for(user).delete_all

delete_all avoids emitting spurious card_unassigned events for a cleanup.

Tests

• Notifier::EventNotifierTest — an active watcher who loses board access on a move is not notified when a new comment is posted (stale watch asserted present in the window). Red before the notifier change, green after.
• CardTest — clean_inaccessible_data removes an assignment for a user without board access. Red before, green after.
• AccessTest — assignments are destroyed when a user's board access is revoked (mirrors the existing watches/pins/mentions coverage). Red before, green after.

Deactivated-user notification behavior is unchanged (existing NotificationDeliveryTest passes).

Verification

• Full non-system suite green: 1590 tests, 6015 assertions, 0 failures (1 pre-existing skip). Rubocop clean, Brakeman 0 warnings. System tests deferred to CI.
• All three new tests confirmed red on the unpatched code, green after — no vacuous assertions.
• Scanned the diff and this description for credentials/PII/customer data — none.

Out of scope / notes

• The notify-time gate also stops mentioning an active user who lacks board access from delivering an excerpt — a strict improvement, no legitimate flow relies on it.
• Fizzy is not HackerOne-bounty-eligible; H1 #3512076 resolves as kudos.

Relates to H1 #3512076; follow-up to #3070.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-29T06:28:55.899557Z	72501b3	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Copilot]

Pull request overview

Filters notification recipients by current board access and removes inaccessible card assignments.

Changes:

• Adds centralized notification access filtering.
• Cleans stale assignments during card moves and access revocation.
• Adds regression coverage for both behaviors.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
app/models/notifier.rb	Filters recipients by current board access.
app/models/card/accessible.rb	Removes inaccessible assignments after moves.
app/models/board/accessible.rb	Removes assignments after access revocation.
test/models/notifier/event_notifier_test.rb	Tests stale-watcher filtering.
test/models/card_test.rb	Tests card assignment cleanup.
test/models/access_test.rb	Tests cleanup following access revocation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 72501b3d23

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Make the access check atomic with notification creation

If an Access row is revoked after this pluck snapshot but before Notification.create_or_find_by runs, the recipient remains selected and the notification and its push job are still created after access was lost. This leaves a TOCTOU path for delivering private card/comment content during concurrent revocation; coordinate notification creation with access revocation (for example through locking) or revalidate authorization at delivery rather than relying on this earlier snapshot. Board authorization is explicitly represented by per-user Access records.

AGENTS.md reference: AGENTS.md:L31-L33

Useful? React with 👍 / 👎.

[jeremy]

🤖 Assessed — not acting, by design. This is the async-notification window the operator explicitly accepted on the parent card ("the async window is accepted throughout Basecamp; the operation is complete when the eventual jobs have been performed, not at HTTP response"). Two things bound it: (1) this filter already collapses the pre-existing leak from the seconds-long post-board-move cleanup window down to the microseconds between the pluck and create_or_find_by in a single job; (2) access revocation fires Board::CleanInaccessibleDataJob → clean_inaccessible_data_for, which async-destroys any notification created for the now-inaccessible user. A recipient notified in that residual window was authorized when the comment was created; the revocation is concurrent and self-heals. Coordinating notify with revocation via a lock is the over-serialization AGENTS.md warns against for an accepted async window. Flagged to the operator in case he wants the stronger real-time guarantee.

[chatgpt-codex-connector]

Recheck access before deleting assignments

Because this cleanup runs asynchronously, a user can regain board access after the check on line 44 but before this deletion; they can even receive a new valid assignment during that interval, and this unrestricted query then destroys it. The method already skips cleanup when access was restored before the job began, so deleting assignments when restoration happens moments later is an inconsistent data-loss race; recheck access immediately before deletion or serialize cleanup with access grants.

AGENTS.md reference: AGENTS.md:L6-L9

Useful? React with 👍 / 👎.

[jeremy]

🤖 Assessed — not acting. This deletion follows the exact idiom already used by the four sibling cleanups in this same method (mentions_for_user/notifications_for_user/watches_for/pins_for): guard on accessible_to?(user), then bulk-destroy that user's board data. The regain-access-then-reassign-within-the-window race you describe applies identically to all of them, so it is a property of the method's accepted async design, not something this change introduces. Special-casing a per-record access recheck for assignments alone would be inconsistent with the siblings and would not close the same window on them; if we want that guarantee it belongs as a uniform serialize-cleanup-with-grants change — a separate design decision neither this card nor #3070 raises. Flagged to the operator.