fix(security): RBAC M4 follow-up — role_cache reload + Telegram DM owner-escalation#3241
Merged
Conversation
…ner-escalation Two follow-up fixes for MERGED #3202 (RBAC M4 channel-native role mapping): 1. AuthManager::reload() now clears role_cache alongside users and channel_index. Previously, an operator editing channel bindings or [channel_role_mapping] and triggering a hot reload would still see the OLD resolved role for any sender already cached this session — the new policy applied only to fresh senders. Cached entries kept stale (often elevated) privileges until daemon restart. 2. Telegram DM `creator` no longer auto-promotes to Owner. The Bot API returns status="creator" for getChatMember(chat_id=user_id, user_id=user_id) because the user "owns" their own DM with the bot. Combined with creator_role = "owner", any user who DMed the bot was silently elevated to Owner. New translate_platform_role_for_sender() detects the self-DM signature (chat_id == user_id, !is_group) and drops the `creator` token so resolution falls through to default- deny Viewer. Group `creator` (the legitimate chat owner) and `administrator` / `member` tokens are untouched. Tests added in channel_role_tests: - telegram_dm_creator_does_not_auto_promote_to_owner - telegram_group_creator_still_maps_to_owner - telegram_dm_administrator_unaffected_by_dm_guard - reload_clears_role_cache_so_mapping_edits_take_effect
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two follow-up security fixes for merged PR #3202 (RBAC M4 — channel-native role mapping). Both touch
crates/librefang-kernel/src/auth.rsand ship together because they share the channel-role resolution path.Fix 1 —
AuthManager::reload()leftrole_cachepopulatedreload()clearedusersandchannel_indexbut neverrole_cache. After an operator edited[[users]]channel bindings or[channel_role_mapping]and triggered a hot reload, any sender whose role was already resolved that session kept the old (potentially elevated) role from the cache until the daemon restarted. The new policy only applied to senders the cache hadn't seen yet.reload()now callsself.role_cache.clear()alongside the existing clears.DashMap::cleartakes per-shard locks internally, so no extra coordination is needed even if aresolve_role_for_sendercall races the reload — concurrent calls observe either pre- or post-clear state, never a torn one.Fix 2 — Telegram DM
creatorauto-promoted every DMer to OwnergetChatMember(chat_id=user_id, user_id=user_id)— what the resolver issues for a Telegram DM — returnsstatus = "creator"because the user "owns" their own private conversation with the bot. With the documented mappingcreator_role = "owner", any user who DMs the bot was silently promoted to Owner of the LibreFang instance.The fix wraps
translate_platform_rolewith a sender-aware variant that detects the self-DM signature (chat_id == sender.user_idand!is_group) for Telegram and drops thecreatortoken in that case, falling through to default-denyViewer. Group/supergroupcreator(the legitimate chat owner) is unaffected, andadministrator/membertokens are not gated since they don't appear for the self-DM query and aren't a privilege risk.Repro for Fix 2 (pre-fix)
config.toml:[[users]]binding required) open a private chat with the bot and send a message.getChatMember(chat_id=<user_id>, user_id=<user_id>), gets backcreator, maps it toownervia the configuredcreator_role, and the user now passes every authorization check up to and includingUserRole::Owner.Post-fix the same flow resolves to
Viewer(default-deny). Group chats where someone is genuinely the owner still resolve toOwneras designed.Tests
Added to
auth::channel_role_testsincrates/librefang-kernel/src/auth.rs:telegram_dm_creator_does_not_auto_promote_to_owner—chat_id == user_id+creatormapping must yieldViewer, notOwner.telegram_group_creator_still_maps_to_owner— guards that the legitimate group-owner case is unchanged (is_group: true,chat_id != user_id).telegram_dm_administrator_unaffected_by_dm_guard— confirms the DM gate iscreator-specific and doesn't accidentally suppress other tokens.reload_clears_role_cache_so_mapping_edits_take_effect— populates the cache via one resolve, callsreload()with a different mapping, asserts the next resolve reflects the new mapping (not the cached old role).All 46 tests in
auth::*pass;cargo test -p librefang-kernel -p librefang-channels --libis green (1451 tests).Test plan
cargo check -p librefang-kernel -p librefang-channels --lib --tests— cleancargo test -p librefang-kernel -p librefang-channels --lib— 627 + 824 passingcargo clippy -p librefang-kernel -p librefang-channels --lib --tests -- -D warnings— zero warningscargo fmt --allViewernotOwnerafter the fix.