fix: prioritizes federation not allowed error message over user not f… #37364
Conversation
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
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds runtime validation and input sanitization to prevent federated users from being added or invited to non‑federated rooms, integrates federation checks into the invite slash command, and re-exports a federated-username validator from the federation package. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InviteCmd as Invite Slash Command
participant FM as FederationMatrix
participant AddUsers as addUsersToRoom
participant RoomSvc as Room Service
User->>InviteCmd: /invite <usernames>
InviteCmd->>InviteCmd: sanitize & split usernames
InviteCmd->>InviteCmd: identify federated usernames
alt federated usernames present
alt room allows federation
InviteCmd->>FM: ensure federated users exist locally
FM-->>InviteCmd: confirmed/created locally
else federation not allowed
InviteCmd-->>User: emit ephemeral error(s) for federated users
InviteCmd->>InviteCmd: remove federated usernames
InviteCmd-->>User: abort if no usernames remain
end
end
InviteCmd->>AddUsers: call addUsersToRoom with remaining usernames
AddUsers->>AddUsers: sanitize & validate each username (validateFederatedUsername)
alt federated username in non-federated room
AddUsers-->>User: throw error (error-federated-users-in-non-federated-rooms)
else
AddUsers->>RoomSvc: add users to room
RoomSvc-->>User: additions complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/app/lib/server/functions/createRoom.ts(2 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(2 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.ts
🧬 Code graph analysis (2)
apps/meteor/app/slashcommands-invite/server/server.ts (2)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
sanitizeUsername(22-29)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
validateFederatedUsername(29-59)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
validateFederatedUsername(29-59)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37364 +/- ##
===========================================
- Coverage 68.13% 67.06% -1.07%
===========================================
Files 3364 3418 +54
Lines 115753 117874 +2121
Branches 20914 21535 +621
===========================================
+ Hits 78866 79056 +190
- Misses 34192 36121 +1929
- Partials 2695 2697 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
54-56: Early return properly prevents spurious error message.This addresses the concern from the previous review about avoiding a misleading "User doesn't exist" error when all usernames are filtered out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/slashcommands-invite/server/server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
🧬 Code graph analysis (1)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
sanitizeUsername(22-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/app/slashcommands-invite/server/server.ts (3)
1-8: LGTM: Imports support the new federation checks.The new imports are necessary for the federation validation logic and are correctly placed.
20-26: LGTM: Username parsing adjusted for reassignment.Changing
usernamesfromconsttoletis necessary to support filtering out federated usernames later (line 51).
28-35: LGTM: Room validation prevents downstream errors.The early validation ensures the room exists before attempting federation checks and user invitations, providing clear feedback to users.
ggazzo
previously approved these changes
Nov 10, 2025
ggazzo
reviewed
Nov 10, 2025
ggazzo
approved these changes
Nov 10, 2025
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
This was referenced Dec 15, 2025
This was referenced Dec 18, 2025
This was referenced Dec 26, 2025
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.
https://rocketchat.atlassian.net/browse/FDR-257
Proposed changes (including videos or screenshots)
When inviting unknown federated users to non-federated rooms we are returning "user not found" errors instead of the expected "federated user in non-federated room" error. These changes prioritize the latter for both modal invites and slash command invites.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features