psa-systems/lets-chat

Watch

Fork 0

Scope the remaining room-visibility queries to the viewer's enclaves (LC-606) #562

Merged

longjacksonle merged 1 commit from fix/lc606-scope-remaining-room-queries into main

2026-07-19 04:27:18 +02:00

longjacksonle commented

2026-07-19 04:20:38 +02:00

Owner

Closes LC-606.

LC-604 fixed the two unread queries that leaked message bodies. This finishes the class: four more consumers carried the same drifted predicate (room_type = 'public' OR room_members, no enclave condition), and the audit this ticket asked for found two dashboard card queries with no visibility filter at all. All now use accessible_rooms_sql, the shared fragment mirroring is_room_accessible.

What was leaking

list_rooms fed the forward-message picker (forward.rs:42) and the rooms API (api.rs:98), so both offered channels from enclaves the caller is not in and would be refused on open. Not a write vector - the forward POST already re-validates the destination. The separate admin query is gone: the admin arm of the fragment reduces to the same room_type != 'dm' it ran.

The activity feed (three queries) could surface an item from a room the viewer cannot open: someone in another enclave @-mentions you, or you keep receiving replies and reactions for messages you posted before being removed from an enclave. feed_for_user now takes is_admin, threaded from the route.

count_unread_mentions_per_room had no room filter whatsoever - it counted mentions in any room, so a mention written in an enclave the viewer is not in produced a count and, on the dashboard's Mentions card, revealed that the room exists.

followed_threads_with_unread selected the room name and the parent message body with no visibility filter, so a viewer removed from an enclave kept receiving both for threads they still follow.

room_ids_with_drafts was audited and deliberately left alone: it reads the viewer's own drafts and returns no room content.

That closes the "unverified either way" item I left on LC-604 - two of those three cards were in fact leaking, which is why I would not call them clean on the strength of an empty card.

Tests

Five leak tests in inbox_enclave_scoping.rs, each verified to fail against the old predicate and pass with the new one (I reverted the predicate in place, keeping arity, to prove it):

list_rooms_hides_channels_from_enclaves_the_viewer_is_not_in ... FAILED   (before)
activity_feed_hides_mentions_from_enclaves_the_viewer_is_not_in ... FAILED   (before)
followed_threads_hide_rooms_the_viewer_can_no_longer_access ... FAILED   (before)

Two fixtures had to be corrected, and this is the interesting part

db_mentions and db_private_rooms created rooms with enclave_id = None - which no production channel has (migration 0009 backfills every non-DM room) and which is_room_accessible treats as unreachable for a non-admin.

Left alone, they would have gone green for the wrong reason:

the mention counts came back empty, so delete_mentions_returns_unread_users would have passed because everything was filtered out, not because the rows were deleted;
test_list_rooms_excludes_private_for_non_member would have held trivially, since nothing was visible to anyone.

Both now use enclave-backed fixtures, and the private-room test puts the viewer in the enclave but not the room, so it tests room membership rather than holding vacuously. test_list_rooms_includes_private_for_member is what caught this - it failed loudly, which is what prompted checking its siblings.

Verified live

An enclave member's forward picker still offers a legitimate destination in her enclave, and no longer offers the rooms from an enclave she is not in (previously it listed enclave 1's general and random, which she cannot open).
A non-member is still refused the source message with 403.

just check clean, full suite green, and the lc77 fixtures stay clean after a local run now that LC-605 has landed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

Closes LC-606. LC-604 fixed the two unread queries that leaked message bodies. This finishes the class: four more consumers carried the same drifted predicate (`room_type = 'public' OR room_members`, no enclave condition), and the audit this ticket asked for found **two dashboard card queries with no visibility filter at all**. All now use `accessible_rooms_sql`, the shared fragment mirroring `is_room_accessible`. ## What was leaking **`list_rooms`** fed the forward-message picker (`forward.rs:42`) and the rooms API (`api.rs:98`), so both offered channels from enclaves the caller is not in and would be refused on open. Not a write vector - the forward POST already re-validates the destination. The separate admin query is gone: the admin arm of the fragment reduces to the same `room_type != 'dm'` it ran. **The activity feed** (three queries) could surface an item from a room the viewer cannot open: someone in another enclave `@`-mentions you, or you keep receiving replies and reactions for messages you posted before being removed from an enclave. `feed_for_user` now takes `is_admin`, threaded from the route. **`count_unread_mentions_per_room`** had *no room filter whatsoever* - it counted mentions in any room, so a mention written in an enclave the viewer is not in produced a count and, on the dashboard's Mentions card, revealed that the room exists. **`followed_threads_with_unread`** selected the room name **and the parent message body** with no visibility filter, so a viewer removed from an enclave kept receiving both for threads they still follow. **`room_ids_with_drafts`** was audited and deliberately left alone: it reads the viewer's own drafts and returns no room content. That closes the "unverified either way" item I left on LC-604 - two of those three cards were in fact leaking, which is why I would not call them clean on the strength of an empty card. ## Tests Five leak tests in `inbox_enclave_scoping.rs`, each **verified to fail against the old predicate and pass with the new one** (I reverted the predicate in place, keeping arity, to prove it): ``` list_rooms_hides_channels_from_enclaves_the_viewer_is_not_in ... FAILED (before) activity_feed_hides_mentions_from_enclaves_the_viewer_is_not_in ... FAILED (before) followed_threads_hide_rooms_the_viewer_can_no_longer_access ... FAILED (before) ``` ## Two fixtures had to be corrected, and this is the interesting part `db_mentions` and `db_private_rooms` created rooms with `enclave_id = None` - which no production channel has (migration 0009 backfills every non-DM room) and which `is_room_accessible` treats as unreachable for a non-admin. Left alone, they would have gone green for the wrong reason: - the mention counts came back empty, so `delete_mentions_returns_unread_users` would have passed because everything was **filtered out**, not because the rows were deleted; - `test_list_rooms_excludes_private_for_non_member` would have held trivially, since nothing was visible to anyone. Both now use enclave-backed fixtures, and the private-room test puts the viewer *in the enclave but not the room*, so it tests room membership rather than holding vacuously. `test_list_rooms_includes_private_for_member` is what caught this - it failed loudly, which is what prompted checking its siblings. ## Verified live - An enclave member's forward picker still offers a legitimate destination in her enclave, and **no longer offers the rooms from an enclave she is not in** (previously it listed enclave 1's `general` and `random`, which she cannot open). - A non-member is still refused the source message with 403. `just check` clean, full suite green, and the `lc77` fixtures stay clean after a local run now that LC-605 has landed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

longjacksonle added 1 commit

2026-07-19 04:20:39 +02:00

fix(db): scope the remaining room-visibility queries to the viewer's enclaves

check-secrets / Nosey parker (push) Successful in 4s

Details

check-secrets / Kingfisher (push) Successful in 5s

Details

check-secrets / TruffleHog (push) Successful in 6s

Details

check-secrets / Nosey parker (pull_request) Successful in 4s

Details

check-secrets / Kingfisher (pull_request) Successful in 6s

Details

Check / clippy + fmt + tests (pull_request) Successful in 6m35s

Details

check-secrets / TruffleHog (pull_request) Successful in 7s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

cc851be164

LC-604 fixed the two unread queries that leaked message bodies. Four more consumers carried the same drifted predicate - `room_type = 'public' OR room_members`, with no enclave condition - and two dashboard card queries had no visibility filter at all. All now use `accessible_rooms_sql`, the shared fragment that mirrors `is_room_accessible`.

list_rooms fed the forward-message destination picker and the rooms API, so both offered channels from enclaves the caller is not in and would be refused on open. Not a write vector: the forward POST already re-validates the destination. The separate admin query is gone, since the admin arm of the fragment reduces to the same `room_type != 'dm'` it ran.

The activity feed's three queries (mentions, replies to you, reactions to you) could surface an item from a room the viewer cannot open - someone in another enclave @mentions you, or you keep receiving replies for messages you posted before being removed from an enclave. feed_for_user now takes is_admin, threaded from the route.

count_unread_mentions_per_room had no room filter whatsoever: it counted mentions in any room, so a mention written in an enclave the viewer is not in produced a count and, on the dashboard's Mentions card, revealed the room exists. It is now joined to rooms and filtered.

followed_threads_with_unread selected the room name and the parent message body with no visibility filter, so a viewer removed from an enclave kept receiving both for threads they still follow. Following a thread is not a lasting grant of access.

room_ids_with_drafts was audited and left alone: it reads the viewer's own drafts and returns no room content.

Tests: five leak tests in inbox_enclave_scoping.rs, each verified to fail against the old predicate and pass with the new one, covering list_rooms, the activity feed, the mention counts and followed threads alongside the LC-604 pair.

Two fixtures had to be corrected to keep their assertions meaningful. db_mentions and db_private_rooms created rooms with `enclave_id = None`, which no production channel has (migration 0009 backfills every non-DM room) and which `is_room_accessible` treats as unreachable for a non-admin. Left as they were, the mention counts came back empty and `delete_mentions_returns_unread_users` would have passed because everything was filtered out rather than because the rows were deleted. The private-room test that asserts a non-member sees nothing now puts the viewer in the enclave but not the room, so it tests room membership instead of holding trivially.

Verified live: an enclave member's forward picker still offers a legitimate destination in her enclave and no longer offers the rooms from an enclave she is not in; a non-member is still refused the source message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

longjacksonle scheduled this pull request to auto merge when all checks succeed

2026-07-19 04:21:33 +02:00