Gate DM Delete on the effective room role, not the org role (LC-599) #563

Merged

longjacksonle merged 1 commit from fix/lc599-dm-delete-uses-effective-role into main

2026-07-20 03:46:31 +02:00

longjacksonle commented

2026-07-20 03:40:45 +02:00

Owner

Closes LC-599. Found while auditing the message menu for LC-598, not introduced by it.

The divergence

dm.rs computed the view flag from the global org role: m.user_id == user.id || user.role == "admin" || user.role == "moderator". The delete endpoint at room.rs (which serves DM messages too) instead calls db::room_rbac::is_room_moderator, which resolves effective_role = max_role(org_role, per_room_override). A user whose moderator status comes from an override passed the endpoint check and failed the template check.

The template was more restrictive than the endpoint, so this is a missing affordance, not a privilege escalation. Worth stating plainly, since a can_delete mismatch would otherwise read as a security issue.

The fix

dm.rs now resolves the effective role once before the render loop, exactly as get_room already does, and both timelines read the same predicate. The change is strictly widening: is_room_moderator takes the max of the org role and the override, so every user who saw Delete before still sees it. The same flag also gates Require acknowledgement, whose endpoint enforces require_author_or_mod, so that affordance realigns with its endpoint as a consequence.

Reachability, which the ticket left open

Confirmed reachable. post_grant guards with require_can_manage, a role whitelist, and an enclave-membership check on the target. DM rooms carry no enclave, so the membership check is skipped and require_can_manage falls through to room_can_manage_overrides(None, site_role), which a site admin satisfies. There is no DM exclusion anywhere on that path, so an admin can grant a moderator override on a DM room today.

Verification

Two integration tests in routes_room_rbac.rs, alongside the existing LC-84 override coverage. dm_offers_delete_to_a_moderator_by_room_override grants the override through the real HTTP endpoint, asserts the Delete control renders, and then asserts the endpoint actually accepts the delete, pinning the two together rather than testing the template in isolation. It fails against the old predicate (verified by reverting the line in place). dm_hides_delete_from_a_plain_member proves the affordance is still withheld without an override, so the positive test is not vacuous, and it asserts the message body renders so a blank or errored page cannot pass it.

just check and the full just test suite are green.

Noted, not done

MessageView has no is_owner, can_pin, or is_mod field, so can_delete doubles as the moderator proxy in the template. That is what let this predicate drift in two places. A named field would prevent a third, but it touches every message-rendering path and belongs in its own change rather than riding along with a one-line correctness fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BE9k4nWNUhPrjte9BcvASg

Closes LC-599. Found while auditing the message menu for LC-598, not introduced by it. ## The divergence `dm.rs` computed the view flag from the global org role: `m.user_id == user.id || user.role == "admin" || user.role == "moderator"`. The delete endpoint at `room.rs` (which serves DM messages too) instead calls `db::room_rbac::is_room_moderator`, which resolves `effective_role` = `max_role(org_role, per_room_override)`. A user whose moderator status comes from an override passed the endpoint check and failed the template check. The template was *more* restrictive than the endpoint, so this is a missing affordance, not a privilege escalation. Worth stating plainly, since a `can_delete` mismatch would otherwise read as a security issue. ## The fix `dm.rs` now resolves the effective role once before the render loop, exactly as `get_room` already does, and both timelines read the same predicate. The change is strictly widening: `is_room_moderator` takes the max of the org role and the override, so every user who saw Delete before still sees it. The same flag also gates Require acknowledgement, whose endpoint enforces `require_author_or_mod`, so that affordance realigns with its endpoint as a consequence. ## Reachability, which the ticket left open Confirmed reachable. `post_grant` guards with `require_can_manage`, a role whitelist, and an enclave-membership check on the target. DM rooms carry no enclave, so the membership check is skipped and `require_can_manage` falls through to `room_can_manage_overrides(None, site_role)`, which a site admin satisfies. There is no DM exclusion anywhere on that path, so an admin can grant a moderator override on a DM room today. ## Verification Two integration tests in `routes_room_rbac.rs`, alongside the existing LC-84 override coverage. `dm_offers_delete_to_a_moderator_by_room_override` grants the override through the real HTTP endpoint, asserts the Delete control renders, and then asserts the endpoint actually accepts the delete, pinning the two together rather than testing the template in isolation. It fails against the old predicate (verified by reverting the line in place). `dm_hides_delete_from_a_plain_member` proves the affordance is still withheld without an override, so the positive test is not vacuous, and it asserts the message body renders so a blank or errored page cannot pass it. `just check` and the full `just test` suite are green. ## Noted, not done `MessageView` has no `is_owner`, `can_pin`, or `is_mod` field, so `can_delete` doubles as the moderator proxy in the template. That is what let this predicate drift in two places. A named field would prevent a third, but it touches every message-rendering path and belongs in its own change rather than riding along with a one-line correctness fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BE9k4nWNUhPrjte9BcvASg

longjacksonle added 1 commit

2026-07-20 03:40:45 +02:00

fix(dm): gate Delete on the effective room role, not the org role

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

Details

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

Details

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

Details

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

Details

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

Details

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

Details

Check / clippy + fmt + tests (pull_request) Successful in 5m45s

Details

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

Details

fb812cff1f

The DM timeline computed `can_delete` from `user.role` alone, while the delete endpoint (`room.rs`, which serves DM messages too) resolves it through `db::room_rbac::is_room_moderator`, which takes the higher of the org role and any per-room override. A user whose moderator status came from an override therefore passed the endpoint check but got no Delete item in the DM overflow menu. The template was stricter than the endpoint, so this was a missing affordance rather than a privilege escalation.

`dm.rs` now resolves the effective role once per render, the way `get_room` already does, and both paths read the same predicate. The same flag also gates Require acknowledgement, whose endpoint enforces `require_author_or_mod`, so that affordance realigns with its endpoint too.

Reachability was open in the ticket and is now confirmed: DM rooms carry no enclave, so `require_can_manage` falls through to the site role and an org admin can grant an override on a DM room. No DM exclusion exists on that path.

Two integration tests in `routes_room_rbac.rs`. The positive one fails against the old predicate; the negative one proves a plain member is still refused, so the positive is not vacuous, and it asserts the message body renders so a blank page cannot pass it.

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

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

2026-07-20 03:43:19 +02:00