regression(federation): Old federation users #37102
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces selective user projections with full-user fetches; adds federated-username validators and helpers (createOrUpdateFederatedUser, getUsernameServername); refactors federation DM/invite/join flows to use username-based lookups and helpers; updates event handlers and typings for native-federation fields; removes multi-invite federation callback. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant App as App Server
participant Fed as FederationMatrix
participant Users as Users DB
participant Matrix as Matrix HS
Client->>App: Create DM (members: IUser[])
App->>Fed: createDirectMessageRoom(room, members, creatorId)
Fed->>Fed: validateFederatedUsername(member.username)
alt member is federated
Fed->>Users: createOrUpdateFederatedUser({ username, origin, name? })
Users-->>Fed: userId
Fed->>Matrix: create room / invite federated user
else local member
Fed->>Users: find local user by username
end
Matrix-->>Fed: room created / invite accepted
Fed-->>App: DM room ready
App-->>Client: Success
sequenceDiagram
autonumber
participant Matrix as Matrix HS
participant InviteAPI as /_matrix/invite
participant Fed as FederationMatrix
participant Users as Users DB
Matrix->>InviteAPI: acceptInvite(inviteEvent, username)
InviteAPI->>Users: findOneByUsername(sender)
alt sender not found
InviteAPI->>Fed: createOrUpdateFederatedUser({ username: sender, origin })
Fed-->>InviteAPI: senderUserId
end
InviteAPI->>InviteAPI: startJoiningRoom (with backoff)
InviteAPI-->>Matrix: joinRoom request
Matrix-->>InviteAPI: join result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)ee/packages/federation-matrix/src/events/index.ts (2)
⏰ 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). (2)
🔇 Additional comments (4)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37102 +/- ##
==================================================
- Coverage 67.43% 65.96% -1.48%
==================================================
Files 3329 3052 -277
Lines 113387 109163 -4224
Branches 20576 19595 -981
==================================================
- Hits 76464 72005 -4459
- Misses 34319 34861 +542
+ Partials 2604 2297 -307
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the federation system to handle old federation users by changing how federated users are identified and stored. The changes transition from using the federation.mui field to using the username field directly for user lookups, and introduces better type safety for federated user identifiers.
- Updates type definitions to enforce Matrix User Identifier format for federated usernames
- Replaces database queries from
federation.muifield to direct username lookups - Introduces helper functions for federated user creation and validation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core-typings/src/IUser.ts | Updates type definitions to enforce Matrix ID format for federated users |
| ee/packages/federation-matrix/src/events/*.ts | Changes user lookups from federation.mui to username-based queries |
| ee/packages/federation-matrix/src/api/_matrix/invite.ts | Refactors user creation to use new helper function and imports |
| ee/packages/federation-matrix/src/FederationMatrix.ts | Adds validation and creation helpers, simplifies DM room creation logic |
| apps/meteor/app/lib/server/functions/createDirectRoom.ts | Removes projection constraint from user query |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/federation-matrix/src/events/member.ts (1)
61-77: Replace inline user creation with the newcreateOrUpdateFederatedUserhelper.The manual user insertion duplicates logic that's now centralized in the
createOrUpdateFederatedUserhelper introduced inFederationMatrix.ts(lines 62-97). Using the helper ensures consistency across the codebase and handles the upsert pattern for users that might exist with incomplete federation data.Apply this diff to use the helper:
- const { insertedId } = await Users.insertOne({ - username: internalUsername, - type: 'user', - status: UserStatus.OFFLINE, - active: true, - roles: ['user'], - name: data.content.displayname || internalUsername, - requirePasswordChange: false, - createdAt: new Date(), - _updatedAt: new Date(), - federated: true, - federation: { - version: 1, - mui: data.sender, - origin: serverName, - }, - }); + const { createOrUpdateFederatedUser } = await import('../FederationMatrix'); + const { insertedId } = await createOrUpdateFederatedUser({ + username: internalUsername, + name: data.content.displayname || internalUsername, + status: UserStatus.OFFLINE, + origin: serverName, + });ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
193-201: Handle the case wherecreateOrUpdateFederatedUserupdates an existing user.The
createOrUpdateFederatedUserhelper usesUsers.updateOnewithupsert: true, which returns{ upsertedId }. When an existing user is updated (not inserted),upsertedIdwill benullorundefined, but the code at line 200 assigns it directly tosenderUserId.Apply this diff to handle both insert and update cases:
if (!senderUser) { const { createOrUpdateFederatedUser } = await import('../../FederationMatrix'); const createdUser = await createOrUpdateFederatedUser({ username: inviteEvent.sender, status: UserStatus.ONLINE, origin: matrixRoom.origin, }); - senderUserId = createdUser.insertedId; + senderUserId = createdUser.insertedId || (await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } }))?._id; }
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
74-76: Reinstate targeted projection for user lookup
Re-add a projection toUsers.findUsersByUsernames(...)to fetch only the fields actually used in this function (_id,name,username). No federation-specific fields are accessed here, so fetching full documents is unnecessary.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
314-316: Skip check might be too broad.The condition
if (existingUser && isUserNativeFederated(existingUser))skips users that exist and are natively federated. However, this doesn't verify that the user's federation data (origin, mui) matches the current username. A user might exist with outdated federation metadata.Consider removing the skip or verifying federation metadata matches:
const existingUser = await Users.findOneByUsername(username); if (existingUser && isUserNativeFederated(existingUser)) { + // Verify federation metadata is correct + const expectedOrigin = username.split(':')[1]; + if (existingUser.federation.origin === expectedOrigin && existingUser.federation.mui === username) { - continue; + continue; + } }
📜 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 (8)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(2 hunks)ee/packages/federation-matrix/src/events/edu.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(1 hunks)ee/packages/federation-matrix/src/events/room.ts(3 hunks)packages/core-typings/src/IUser.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/lib/server/functions/createDirectRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/lib/server/functions/createDirectRoom.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
createOrUpdateFederatedUser(62-97)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)
⏰ 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 (13)
ee/packages/federation-matrix/src/events/room.ts (2)
31-34: Verify upstream username validation.Same concern as the room.name handler: ensure
userIdis validated as a federated username before this handler processes it.
47-55: Verify upstream username validation for both userId and senderId.Both
userIdandsenderIdare now treated as usernames. Ensure upstream validation confirms these are valid federated usernames.ee/packages/federation-matrix/src/events/message.ts (2)
126-130: LGTM: Defensive error handling maintained.The comment indicates the user should exist, but the code appropriately throws an error if the lookup fails. This defensive approach is good practice for federation event handling.
289-294: LGTM: Consistent username-based lookup in redaction handler.The redaction handler correctly uses the same username-based lookup pattern as the main message handler.
ee/packages/federation-matrix/src/events/edu.ts (2)
23-27: LGTM: Appropriate defensive checks.The handler correctly validates both the user existence and username field after the lookup, with clear debug logging.
45-54: LGTM: Federated flag validation ensures correct user type.The presence handler appropriately checks the
federatedflag (line 51-53) to ensure only federated users receive presence updates from Matrix, preventing incorrect status updates for local users.packages/core-typings/src/IUser.ts (1)
268-271: Approve code changes ThecreateOrUpdateFederatedUserhelper inpackages/federation-matrix/src/FederationMatrix.tsupserts existing federated users with the newly requiredusernamefield, covering the migration path.ee/packages/federation-matrix/src/events/member.ts (1)
31-31: Verify the username format for sender lookups.Similar to the
state_keylookup,data.senderis now used directly withUsers.findOneByUsername. Confirm thatdata.senderalways contains a valid federated username format.ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
187-187: Verify sender username format for federation lookups.The change to
Users.findOneByUsername(inviteEvent.sender)assumesinviteEvent.senderis a valid username. Ensure that the sender field from the invite event is consistently formatted as a federated username (@username:domain).ee/packages/federation-matrix/src/FederationMatrix.ts (4)
311-322: Good use of the new helpers!The refactored
ensureFederatedUsersExistLocallynow leveragesvalidateFederatedUsernameandcreateOrUpdateFederatedUser, which improves consistency and maintainability.
363-377: Simplified member handling for group DMs.The refactored loop now skips non-federated members instead of throwing errors, which is a more graceful approach for group DMs with mixed membership.
348-354: Verify local-to-local DM behaviorWe found no tests covering non-federated (local-to-local) DM creation, and the new throw at FederationMatrix.ts:352–354 will cause those flows to fail. Confirm whether local users should still be able to open 1-on-1 DMs; if so, swap the error for an early return:
- if (!isUserNativeFederated(otherMember)) { - throw new Error('Other member is not federated'); - } + if (!isUserNativeFederated(otherMember)) { + this.logger.debug('Other member is not federated, skipping Matrix DM creation'); + return; + }
330-387: No changes needed: all callers updated to pass IUser[]
Verified thatcreateDirectMessageRoomis only invoked withparams.members, which is typed asIUser[]in the afterCreateDirectRoom hook—matching the updated signature.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
315-336: Fix origin extraction to handle domains with ports.
Replace the use ofusername.split(':')[1](line 329) with logic that captures everything after the first colon—e.g.const origin = username.slice(username.indexOf(':') + 1);so that domains containing ports (e.g.
example.com:8448) are preserved.
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
51-54: Regex still doesn't match Matrix spec for user IDs.The current regex allows uppercase letters (
A-Z), but the Matrix specification only permits lowercase letters in the localpart. Additionally, the regex is missing/and+characters which are valid in Matrix user IDs according to the spec.Apply this diff to enforce the Matrix spec correctly:
export function validateFederatedUsername(username: string): username is `@${string}:${string}` { - return /^@[a-zA-Z0-9._=-]+:[a-zA-Z0-9.:_-]+$/.test(username); + return /^@[a-z0-9._=\-/+]+:[a-z0-9.:_-]+$/.test(username); }Note: This enforces lowercase-only localpart and includes all valid punctuation characters (
.,_,=,-,/,+).Based on past review comments.
📜 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)
ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
360-362: Federation-only DM path is guarded; local DMs remain unaffected
ThecreateDirectMessageRoomcall inFederationMatrixis only invoked inside theshouldPerformFederationActionguard (and after handlingparams.mrid), so local 1-on-1 DMs with non-federated users continue using the standard code path. Disregard the concern about blocking non-federated DMs here.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
74-95: Do not overwritecreatedAton every upsert
createdAtis still inside$set, so every legacy user “update” will reset their creation timestamp. This was flagged earlier and remains unresolved—please movecreatedAtunder$setOnInsertso historical metadata survives the migration.{ $set: { username, name, type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, mui: username, origin, }, - createdAt: new Date(), _updatedAt: new Date(), }, + $setOnInsert: { + createdAt: new Date(), + }, },
📜 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 (2)
ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
createOrUpdateFederatedUser(62-105)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)
⏰ 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
ggazzo
force-pushed
the
fix/old-fed
branch
2 times, most recently
from
c179fcd to
3d3e7fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
51-82: Critical: Localpart regex still rejects valid Matrix user IDs.The current regex at line 61 allows
[a-z0-9._\-]and hex-encoded characters (=HH), but the Matrix specification permits additional characters in the localpart:/and+. This means valid Matrix IDs like@user/test:example.orgor@user+tag:example.orgwill be rejected, breaking federation with compliant homeservers.Apply this diff to include the missing characters:
- const localpartRegex = /^(?:[a-z0-9._\-]|=[0-9a-fA-F]{2}){1,255}$/; + const localpartRegex = /^(?:[a-z0-9._\-/+]|=[0-9a-fA-F]{2}){1,255}$/;Based on previous review feedback and Matrix specification requirements.
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
408-422: Consider logging skipped non-federated members.Lines 413-415 silently skip non-federated members in group DMs. While functionally correct, adding a debug log would improve troubleshooting when users expect all members to be invited but some are excluded.
Apply this diff to add logging:
if (!isUserNativeFederated(member)) { + this.logger.debug('Skipping non-federated member in group DM', { username: member.username, roomId: room._id }); continue; }Based on previous review feedback.
📜 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 (2)
ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
createOrUpdateFederatedUser(97-142)
⏰ 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: 🚀 Notify external services - draft
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
2-2: LGTM on imports.The new imports for
IUserandcreateOrUpdateFederatedUserare properly used in the refactored user creation flow below.Also applies to: 15-15
188-195: LGTM on refactored sender user resolution.The change from federation.mui-based lookup to username-based lookup with centralized user creation via
createOrUpdateFederatedUseris a solid improvement. The fallback pattern correctly handles both existing and new federated users, and error handling at line 197 ensuressenderUserIdis present before proceeding.ee/packages/federation-matrix/src/FederationMatrix.ts (5)
83-89: LGTM on domain extraction logic.This correctly preserves ports and IPv6 literals by using
indexOfandsubstringinstead ofsplit, addressing the past review concern. For example,@user:example.org:8448will correctly extractexample.org:8448as the origin.
97-142: LGTM on federated user upsert logic.The implementation correctly uses
$setOnInsertforcreatedAt(line 126), addressing the past review concern. The fallback pattern foruserIdretrieval (line 134) properly handles both insert and update cases. TheUserStatus.OFFLINEdefault is appropriate for newly discovered federated users, as their actual presence will be updated through separate EDU events.
352-372: Verify federated user filtering doesn't skip valid usernames.The logic correctly uses
validateFederatedUsernameto filter federated usernames andextractDomainFromMatrixUserIdto preserve ports. However, ifvalidateFederatedUsernamerejects valid Matrix IDs (as flagged above), some federated users won't be created here.Ensure the
validateFederatedUsernamefix is applied so this function doesn't skip valid federated usernames containing/or+.
375-375: LGTM on simplified function signature.Requiring
IUser[]instead of(IUser | string)[]is a cleaner API that ensures all members are resolved before room creation, aligning with the federated user validation pattern.
392-401: LGTM on 1-on-1 DM federated user validation.The logic correctly validates that the other member is natively federated before creating a direct message room, preventing invalid federation scenarios. The error at line 398 provides clear feedback when attempting to create a federated DM with a non-federated user.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
ee/packages/federation-matrix/src/events/room.ts (1)
15-18: Normalize federated usernames before callingfindOneByUsername.
Line 15: the handler now feeds the raw Matrix identifier (e.g.,@alice:remote.example) straight intoUsers.findOneByUsername. SincecreateOrUpdateFederatedUserpersists our local federated users with the Rocket.Chat username shape (alice@remote.example), this query will always miss remote members and throw “mapped user not found” for any rename/topic/role event emitted by an external homeserver. Please run the incoming ID throughvalidateFederatedUsernameandgetUsernameServername(and do the same forsenderId) before the lookup, or fall back to the formerfederation.muimatcher.Also applies to: 31-34, 47-55
ee/packages/federation-matrix/src/events/member.ts (2)
18-23: Validatedata.state_keyformat before user lookup.The code uses
data.state_keydirectly infindOneByUsernamewithout validating its format. This could fail if the state_key is not in the expected federated username format.Consider using
validateFederatedUsernamefrom the imported helpers:+ if (!validateFederatedUsername(data.state_key)) { + logger.error(`Invalid federated username in state_key: ${data.state_key}`); + return; + } const affectedUser = await Users.findOneByUsername(data.state_key);Based on learnings from past reviews.
68-72: Validatedata.state_keybefore using as federated username.Line 69 casts
data.state_keyto a federated username type without validation. If the state_key is malformed, this could create invalid user records.Apply this diff:
+ if (!validateFederatedUsername(data.state_key)) { + logger.error(`Invalid federated username in state_key: ${data.state_key}`); + return; + } + const insertedId = await createOrUpdateFederatedUser({ username: data.state_key as `@${string}:${string}`, origin: serverName, name: data.content.displayname || (data.state_key as `@${string}:${string}`), });ee/packages/federation-matrix/src/FederationMatrix.ts (1)
51-82: Regex still rejects spec-compliant Matrix localparts containing/or+.The localpart regex on line 61 excludes
/and+characters, which are valid per the Matrix specification. This will reject legitimate federated user IDs like@watch/for/slashes:example.orgor@user+tag:domain.com.Apply this diff to include the missing characters:
- const localpartRegex = /^(?:[a-z0-9._\-]|=[0-9a-fA-F]{2}){1,255}$/; + const localpartRegex = /^(?:[a-z0-9._\-/+]|=[0-9a-fA-F]{2}){1,255}$/;Based on learnings from past reviews and Matrix specification requirements.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
74-74: Inconsistent user fetching patterns.Line 74 fetches full user documents without projection, while lines 153-156 fetch users with an explicit projection (
{ 'username': 1, 'settings.preferences': 1 }). Both queries serve similar purposes within the same function—preparing member data for subscription creation.Apply consistent projection patterns. If full documents are required for federation (verify with the script above), update line 153-156:
- const membersWithPreferences: IUser[] = await Users.find( - { _id: { $in: memberIds } }, - { projection: { 'username': 1, 'settings.preferences': 1 } }, - ).toArray(); + const membersWithPreferences: IUser[] = await Users.find( + { _id: { $in: memberIds } } + ).toArray();Otherwise, restore the projection at line 74 to avoid fetching unnecessary fields:
- const roomMembers = await Users.findUsersByUsernames(membersUsernames).toArray(); + const roomMembers = await Users.findUsersByUsernames(membersUsernames, { + projection: { _id: 1, name: 1, username: 1, customFields: 1 } + }).toArray();Also applies to: 153-156
📜 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 (16)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(1 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(0 hunks)apps/meteor/ee/server/hooks/federation/index.ts(0 hunks)apps/meteor/lib/callbacks.ts(0 hunks)apps/meteor/server/methods/addRoomModerator.ts(1 hunks)apps/meteor/server/methods/addRoomOwner.ts(1 hunks)apps/meteor/server/methods/removeRoomModerator.ts(1 hunks)apps/meteor/server/methods/removeRoomOwner.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(6 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)ee/packages/federation-matrix/src/events/edu.ts(2 hunks)ee/packages/federation-matrix/src/events/index.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(4 hunks)ee/packages/federation-matrix/src/events/message.ts(1 hunks)ee/packages/federation-matrix/src/events/room.ts(3 hunks)packages/core-typings/src/IUser.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/meteor/ee/server/hooks/federation/index.ts
- apps/meteor/lib/callbacks.ts
- apps/meteor/app/lib/server/methods/addUsersToRoom.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/events/edu.ts
- packages/core-typings/src/IUser.ts
- ee/packages/federation-matrix/src/events/message.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/lib/server/functions/createDirectRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/lib/server/functions/createDirectRoom.ts
🧬 Code graph analysis (4)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
createOrUpdateFederatedUser(115-160)getUsernameServername(96-108)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-23)packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-120)ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
acceptInvite(287-326)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)ee/packages/federation-matrix/src/FederationMatrix.ts (2)
getUsernameServername(96-108)createOrUpdateFederatedUser(115-160)
ee/packages/federation-matrix/src/events/index.ts (1)
ee/packages/federation-matrix/src/events/member.ts (1)
member(83-99)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (16)
ee/packages/federation-matrix/src/events/index.ts (1)
11-23: LGTM! Services parameter threaded correctly.The addition of the
servicesparameter toregisterEventsand its forwarding to thememberevent handler is clean and consistent with the broader refactoring to support federated user management.ee/packages/federation-matrix/src/api/_matrix/invite.ts (4)
2-2: LGTM! Imports aligned with new functionality.The added imports support the username-based lookup, federated user creation, and native federation checks introduced in this refactor.
Also applies to: 12-12, 15-15
169-282: LGTM! Username-based user resolution correctly implemented.The refactored
joinRoomfunction properly usesfindOneByUsernameand falls back tocreateOrUpdateFederatedUserwhen needed. The DM and room creation flows handle federated users appropriately.
286-326: LGTM! Local invite validation correctly enforced.The
acceptInvitefunction properly validates that both inviter and invitee are local (non-native-federated) users before processing the join operation, preventing external federation users from using the internal invite flow.
284-284: Export binding correctly wraps joinRoom with backoff.The
startJoiningRoomexport provides a cancellable, backoff-wrapped version ofjoinRoomfor use in the invite processing flow.ee/packages/federation-matrix/src/events/member.ts (3)
3-3: LGTM! Imports support federated user management.The new imports provide the necessary utilities and services for username-based user resolution and federated user creation.
Also applies to: 5-5, 7-7
50-62: LGTM! Local user handling correctly checks for existing subscriptions.The use of
getUsernameServernameto identify local users and the subscription check (lines 56-59) prevents duplicate room memberships.
83-99: LGTM! Services parameter correctly forwarded.The updated
memberfunction signature accepts and forwards theservicesparameter tomembershipJoinAction, enabling federated user management.ee/packages/federation-matrix/src/FederationMatrix.ts (8)
83-89: LGTM! Domain extraction correctly preserves ports.The function correctly extracts the domain portion including any port numbers by slicing from the first colon, addressing previous concerns about port preservation.
96-108: LGTM! Username/server resolution handles local vs. remote users correctly.The function appropriately strips the domain for local users while preserving the full Matrix ID for remote federated users.
115-160: LGTM! Upsert correctly uses$setOnInsertforcreatedAt.The function properly separates mutable fields (
$set) from insert-only fields ($setOnInsert), preserving the original creation timestamp on updates. The fallback lookup for the user ID correctly handles both insert and update cases.
313-320: LGTM! Services correctly provided to event registration.The
createdmethod properly passeshomeserverServicestoregisterEvents, enabling federated user management in event handlers.
375-396: LGTM! Federated user provisioning correctly uses validation and domain extraction.The function properly validates federated usernames, skips existing native federated users, and uses
extractDomainFromMatrixUserIdto preserve port information when creating users.
398-455: LGTM! Direct message creation correctly validates federation requirements.The refactored signature (
members: IUser[]) and the validation that 1-on-1 DMs require a native federated user (lines 420-422) properly enforce federation constraints.Note: A past review comment flagged lines 436-438 for silently skipping non-federated members without logging. Consider adding debug logging if this becomes confusing during troubleshooting.
695-717: LGTM! Invite flow correctly handles both federated and local users.The function appropriately validates federated usernames and routes local users through the
acceptInviteflow, aligning with the new internal invite handling.
29-29: LGTM! Export ofacceptInviteenables internal invite handling.Adding
acceptInviteto the exports allows theinviteUsersToRoommethod to process local user invites correctly.
| const roomMembers: IUser[] = await Users.findUsersByUsernames(membersUsernames, { | ||
| projection: { _id: 1, name: 1, username: 1, settings: 1, customFields: 1 }, | ||
| }).toArray(); | ||
| const roomMembers = await Users.findUsersByUsernames(membersUsernames).toArray(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify necessity of fetching full user documents.
The selective projection was removed, causing all user fields to be fetched instead of only _id, name, username, settings, and customFields. While the PR context mentions federation fixes, the downstream code (lines 76-79, 135-174) only uses _id, name, username, and customFields.
Run the following script to check if federation-related fields (e.g., federated, isRemote, federatedId) are accessed elsewhere in the call chain:
Additionally, please search the web to confirm whether this aligns with Matrix Federation specification requirements for user data:
🏁 Script executed:
#!/bin/bash
# Description: Search for federation field usage in callbacks and related files
# Check afterCreateDirectRoom callback implementations for federation field usage
rg -n -C3 'afterCreateDirectRoom' --type=ts
# Check for federation-related field access patterns in room/subscription creation
rg -nP --type=ts '\b(federated|isRemote|federatedId|federation)\b' -g '*createDirectRoom*' -g '*callbacks*'Length of output: 2124
🌐 Web query:
Matrix Federation user data requirements for direct messages
💡 Result:
Short answer — what gets shared over federation for direct/1:1 Matrix messages:
-
If it's a room-based 1:1 DM: room PDUs are sent to participant homeservers. That includes the event envelope (room_id, event type, sender user_id, event_id, origin/origin_server_ts, prev_events/auth_events, content). If E2EE is used the content is encrypted, but metadata (room membership, sender ID, room_id, timestamps, and any unencrypted state) is still shared. [1][2]
-
If it's a true send-to-device message (e.g., Olm/Device key/one-to-one keying): servers exchange m.direct_to_device EDUs containing sender user_id, event type, message_id and the encrypted payload — delivered to the target device(s) on the remote homeserver. These are not PDUs in a room graph but are federated EDUs. [1][2]
-
Other federated data related to DMs: profile/profile queries (displayname, avatar_url) are available via federation queries; presence or presence-invite EDUs may be exchanged; media referenced by mxc:// URIs requires homeserver-to-homeserver fetches. Note that redactions remove content but usernames/IDs and replicated copies on remote homeservers can remain. [1][2][3]
Notes: end-to-end encryption protects message content from remote servers only if the client used E2EE; servers still see routing/room/participant metadata. [1][2][3]
Sources: Matrix federation spec (send-to-device / PDUs) and Matrix privacy notice. [1][2][3]
Restore selective projection on Users.findUsersByUsernames
Reinstate { projection: { _id: 1, name: 1, username: 1, settings: 1, customFields: 1 } } on line 74 to avoid fetching full user documents when only those fields are used (callbacks and subscription creation here don’t require any additional properties). This also aligns with the explicit projection applied later at lines 153–156.
🤖 Prompt for AI Agents
In apps/meteor/app/lib/server/functions/createDirectRoom.ts around line 74, the
Users.findUsersByUsernames call currently fetches full user documents; restore
the selective projection by passing { projection: { _id: 1, name: 1, username:
1, settings: 1, customFields: 1 } } as the second argument so only those fields
are returned (matching the explicit projection used later at lines 153–156) to
avoid fetching unnecessary data.
| function runWithBackoff<T extends (...args: any[]) => Promise<void>>(fn: T, delaySec = 5): T & { stop: () => void } { | ||
| let timeoutId: NodeJS.Timeout | null = null; | ||
| let currentDelay = delaySec; | ||
|
|
||
| const execute = async (...args: Parameters<T>) => { | ||
| try { | ||
| await fn(...args); | ||
| currentDelay = delaySec; // Reset delay on success | ||
| } catch (e) { | ||
| const delay = currentDelay === 625 ? 625 : currentDelay ** 2; | ||
| console.log(`error occurred, retrying in ${delay}ms`, e); | ||
| currentDelay = delay; | ||
| timeoutId = setTimeout(() => execute(...args), delay); | ||
| } | ||
| }; | ||
|
|
||
| return Object.assign( | ||
| (...args: Parameters<T>) => { | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| execute(...args); | ||
| }, | ||
| { stop: () => clearTimeout(timeoutId ?? 0) }, | ||
| ) as T & { stop: () => void }; | ||
| } |
There was a problem hiding this comment.
Replace console.log with proper logger.
Line 152 uses console.log for error logging. This should use the existing logger instance for consistency with the rest of the codebase and proper log level management.
Apply this diff:
function runWithBackoff<T extends (...args: any[]) => Promise<void>>(fn: T, delaySec = 5): T & { stop: () => void } {
let timeoutId: NodeJS.Timeout | null = null;
let currentDelay = delaySec;
const execute = async (...args: Parameters<T>) => {
try {
await fn(...args);
currentDelay = delaySec; // Reset delay on success
} catch (e) {
const delay = currentDelay === 625 ? 625 : currentDelay ** 2;
- console.log(`error occurred, retrying in ${delay}ms`, e);
+ logger.error(`Error occurred, retrying in ${delay}s`, e);
currentDelay = delay;
timeoutId = setTimeout(() => execute(...args), delay);
}
};Note: You'll need to import or have access to a logger instance in this file's scope.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/invite.ts around lines 142 to
167, replace the console.log in the catch block with the project's logger (e.g.
logger.error) so errors follow the codebase logging conventions; ensure a logger
instance is imported or available in this module's scope, call logger.error with
a clear message and the error object (and/or error.stack) to preserve details,
and keep the existing retry/backoff logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
ee/packages/federation-matrix/src/events/room.ts (1)
15-15: Validate federated usernames before DB lookup.All four
findOneByUsernamecalls on lines 15, 31, 47, and 52 useuserIdorsenderIdwithout validating the format. Import and callvalidateFederatedUsernamebefore each lookup to prevent malformed usernames from reaching the database.Apply this diff:
+import { validateFederatedUsername } from '../FederationMatrix'; + export function room(emitter: Emitter<HomeserverEventSignatures>) { emitter.on('homeserver.matrix.room.name', async (data) => { const { room_id: roomId, name, user_id: userId } = data; const localRoomId = await Rooms.findOne({ 'federation.mrid': roomId }, { projection: { _id: 1 } }); if (!localRoomId) { throw new Error('mapped room not found'); } + if (!validateFederatedUsername(userId)) { + throw new Error(`Invalid federated username format: ${userId}`); + } const localUserId = await Users.findOneByUsername(userId, { projection: { _id: 1 } });Repeat this pattern for
userIdon line 31 and for bothuserId(line 47) andsenderId(line 52) in the role handler.Also applies to: 31-31, 47-47, 52-52
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
152-152: Replaceconsole.logwith logger.Use the existing logger instance for consistency with the rest of the codebase.
Apply this diff:
} catch (e) { const delay = currentDelay === 625 ? 625 : currentDelay ** 2; - console.log(`error occurred, retrying in ${delay}ms`, e); + logger.error(`Error occurred, retrying in ${delay}s`, { error: e, delay }); currentDelay = delay;Note: You'll need to ensure a logger instance is available in this module's scope.
ee/packages/federation-matrix/src/events/member.ts (2)
19-19: Validate federated usernames before lookup in leave handler.Lines 19 and 32 call
findOneByUsernamewithdata.state_keyanddata.senderwithout validating the format. Import and callvalidateFederatedUsernamebefore each lookup to prevent malformed usernames from reaching the database.Apply this diff:
+import { createOrUpdateFederatedUser, getUsernameServername, validateFederatedUsername } from '../FederationMatrix'; async function membershipLeaveAction(data: HomeserverEventSignatures['homeserver.matrix.membership']) { const room = await Rooms.findOne({ 'federation.mrid': data.room_id }, { projection: { _id: 1 } }); if (!room) { logger.warn(`No bridged room found for Matrix room_id: ${data.room_id}`); return; } + if (!validateFederatedUsername(data.state_key)) { + logger.error(`Invalid federated username: ${data.state_key}`); + return; + } const affectedUser = await Users.findOneByUsername(data.state_key);Repeat for
data.senderon line 32.Also applies to: 32-32
68-72: Validatedata.state_keybefore callingcreateOrUpdateFederatedUser.Line 69 casts
data.state_keywithout validating the format. Import and callvalidateFederatedUsernameto ensure the username is well-formed before creating/updating the user.Apply this diff:
+ if (!validateFederatedUsername(data.state_key)) { + logger.error(`Invalid federated username: ${data.state_key}`); + return; + } const insertedId = await createOrUpdateFederatedUser({ username: data.state_key as `@${string}:${string}`,ee/packages/federation-matrix/src/FederationMatrix.ts (1)
28-59: BroadenlocalpartRegexto include Matrix-compliant characters.Line 38 rejects spec-compliant Matrix IDs containing
/or+(e.g.,@user/name:example.org). Per the Matrix spec, localparts may include lowercase a-z, digits 0-9, and punctuation. _ = - / +. Update the regex to allow these characters.Apply this diff:
- const localpartRegex = /^(?:[a-z0-9._\-]|=[0-9a-fA-F]{2}){1,255}$/; + const localpartRegex = /^(?:[a-z0-9._\-/+]|=[0-9a-fA-F]{2}){1,255}$/;
📜 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 (16)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(1 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(0 hunks)apps/meteor/ee/server/hooks/federation/index.ts(0 hunks)apps/meteor/lib/callbacks.ts(0 hunks)apps/meteor/server/methods/addRoomModerator.ts(1 hunks)apps/meteor/server/methods/addRoomOwner.ts(1 hunks)apps/meteor/server/methods/removeRoomModerator.ts(1 hunks)apps/meteor/server/methods/removeRoomOwner.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)ee/packages/federation-matrix/src/events/edu.ts(1 hunks)ee/packages/federation-matrix/src/events/index.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(4 hunks)ee/packages/federation-matrix/src/events/message.ts(1 hunks)ee/packages/federation-matrix/src/events/room.ts(3 hunks)packages/core-typings/src/IUser.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/meteor/lib/callbacks.ts
- apps/meteor/ee/server/hooks/federation/index.ts
- apps/meteor/app/lib/server/methods/addUsersToRoom.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/app/lib/server/functions/createDirectRoom.ts
- apps/meteor/server/methods/addRoomModerator.ts
- packages/core-typings/src/IUser.ts
- ee/packages/federation-matrix/src/events/edu.ts
- apps/meteor/server/methods/removeRoomModerator.ts
- ee/packages/federation-matrix/src/events/message.ts
- apps/meteor/server/methods/removeRoomOwner.ts
- ee/packages/federation-matrix/src/events/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
createOrUpdateFederatedUser(92-137)getUsernameServername(73-85)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-120)ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
acceptInvite(287-326)
ee/packages/federation-matrix/src/events/member.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
getUsernameServername(73-85)createOrUpdateFederatedUser(92-137)
⏰ 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 (5)
apps/meteor/server/methods/addRoomOwner.ts (1)
27-27: Projection change is correct and requiredThe
federationfield is consumed by EE federation hooks (e.g. FederationMatrix in apps/meteor/ee/server/hooks/federation/index.ts) to accessroom.federation.mrid; including it ensuresbeforeChangeRoomRolecallbacks work correctly.ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
286-326:acceptInvitelogic looks correct.The function correctly validates that both inviter and invitee are local (non-native-federated) users and uses
getUsernameServernameto extract the appropriate username for lookup. The guard clauses ensure only valid local-to-local invites are processed.ee/packages/federation-matrix/src/FederationMatrix.ts (3)
269-285:ensureFederatedUsersExistLocallyimplementation looks correct.The function filters usernames with
validateFederatedUsernameand callscreateOrUpdateFederatedUseronly for valid federated usernames. Error handling and logging are appropriate.
288-345:createDirectMessageRoomcorrectly enforces federation requirements.The function validates that DM participants are native federated users and throws errors for non-federated members in 1-on-1 DMs. For group DMs, it silently skips non-federated members (line 326-328), which is acceptable given the context.
585-607:inviteUsersToRoomcorrectly uses validation and acceptInvite.The function validates federated usernames before inviting and calls
acceptInvitefor local users. The logic correctly handles both federated and local invite flows.
| const senderUser = await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } }); | ||
|
|
||
| senderUserId = createdUser.insertedId; | ||
| } | ||
| const senderUserId = | ||
| senderUser?._id || | ||
| (await createOrUpdateFederatedUser({ | ||
| username: inviteEvent.sender as `@${string}:${string}`, | ||
| origin: matrixRoom.origin, | ||
| })); |
There was a problem hiding this comment.
Validate inviteEvent.sender before calling createOrUpdateFederatedUser.
The sender username is cast and passed to createOrUpdateFederatedUser without validation. Import and call validateFederatedUsername to ensure the format is correct before creating/updating the user.
Apply this diff:
+ if (!validateFederatedUsername(inviteEvent.sender)) {
+ throw new Error(`Invalid federated username format: ${inviteEvent.sender}`);
+ }
const senderUserId =
senderUser?._id ||
(await createOrUpdateFederatedUser({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const senderUser = await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } }); | |
| senderUserId = createdUser.insertedId; | |
| } | |
| const senderUserId = | |
| senderUser?._id || | |
| (await createOrUpdateFederatedUser({ | |
| username: inviteEvent.sender as `@${string}:${string}`, | |
| origin: matrixRoom.origin, | |
| })); | |
| const senderUser = await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } }); | |
| if (!validateFederatedUsername(inviteEvent.sender)) { | |
| throw new Error(`Invalid federated username format: ${inviteEvent.sender}`); | |
| } | |
| const senderUserId = | |
| senderUser?._id || | |
| (await createOrUpdateFederatedUser({ | |
| username: inviteEvent.sender as `@${string}:${string}`, | |
| origin: matrixRoom.origin, | |
| })); |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/invite.ts around lines 203 to
210, validate inviteEvent.sender before calling createOrUpdateFederatedUser:
import validateFederatedUsername at the top of the file, check that
inviteEvent.sender is a string and passes validateFederatedUsername (handle
failure by logging/throwing or returning early), and only then call
createOrUpdateFederatedUser with the casted username; otherwise avoid calling
createOrUpdateFederatedUser and ensure senderUserId remains undefined/null or
handle the error path consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
152-153: Use the module logger instead ofconsole.log.This was already raised earlier and is still unresolved—please wire this through the existing logger so federation retries are logged consistently with the rest of the service. Suggest importing
Loggerand logging vialogger.error.
203-210: ValidateinviteEvent.senderbefore upserting.We’re still calling
createOrUpdateFederatedUserwith unchecked input. Bring invalidateFederatedUsernameand fail fast when the sender MXID is malformed to avoid persisting garbage usernames.-import { createOrUpdateFederatedUser, getUsernameServername } from '../../FederationMatrix'; +import { createOrUpdateFederatedUser, getUsernameServername, validateFederatedUsername } from '../../FederationMatrix'; … - const senderUser = await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } }); + if (!validateFederatedUsername(inviteEvent.sender)) { + throw new Error(`Invalid federated username format: ${inviteEvent.sender}`); + } + const senderUser = await Users.findOneByUsername(inviteEvent.sender, { projection: { _id: 1 } });ee/packages/federation-matrix/src/FederationMatrix.ts (1)
38-40: Allow/and+in Matrix localparts.The checker still rejects spec-compliant MXIDs like
@bot/relay:example.organd@room+service:example.org. Please widen the character class accordingly as previously requested.- const localpartRegex = /^(?:[a-z0-9._\-]|=[0-9a-fA-F]{2}){1,255}$/; + const localpartRegex = /^(?:[a-z0-9._\/+\-]|=[0-9a-fA-F]{2}){1,255}$/;ee/packages/federation-matrix/src/events/member.ts (1)
43-71: Guard membership usernames withvalidateFederatedUsername.We continue to trust
data.sender/data.state_keywithout validation, even though earlier feedback asked to gate them. ImportvalidateFederatedUsernameand bail out (or log) when either MXID is malformed before we hitgetUsernameServername,Users.findOneByUsername, orcreateOrUpdateFederatedUser.-import { createOrUpdateFederatedUser, getUsernameServername } from '../FederationMatrix'; +import { createOrUpdateFederatedUser, getUsernameServername, validateFederatedUsername } from '../FederationMatrix'; … - const [username, serverName, isLocal] = getUsernameServername(data.sender, services.config.serverName); + if (!validateFederatedUsername(data.sender) || !validateFederatedUsername(data.state_key)) { + logger.error(`Invalid federated username in membership event`, { sender: data.sender, stateKey: data.state_key }); + return; + } + const [username, serverName, isLocal] = getUsernameServername(data.sender, services.config.serverName);
📜 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 (16)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(1 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(0 hunks)apps/meteor/ee/server/hooks/federation/index.ts(0 hunks)apps/meteor/lib/callbacks.ts(0 hunks)apps/meteor/server/methods/addRoomModerator.ts(1 hunks)apps/meteor/server/methods/addRoomOwner.ts(1 hunks)apps/meteor/server/methods/removeRoomModerator.ts(1 hunks)apps/meteor/server/methods/removeRoomOwner.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(5 hunks)ee/packages/federation-matrix/src/events/edu.ts(1 hunks)ee/packages/federation-matrix/src/events/index.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(4 hunks)ee/packages/federation-matrix/src/events/message.ts(1 hunks)ee/packages/federation-matrix/src/events/room.ts(3 hunks)packages/core-typings/src/IUser.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/meteor/lib/callbacks.ts
- apps/meteor/ee/server/hooks/federation/index.ts
- apps/meteor/app/lib/server/methods/addUsersToRoom.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core-typings/src/IUser.ts
- apps/meteor/server/methods/addRoomModerator.ts
- apps/meteor/app/lib/server/functions/createDirectRoom.ts
- ee/packages/federation-matrix/src/events/index.ts
- ee/packages/federation-matrix/src/events/room.ts
- ee/packages/federation-matrix/src/events/edu.ts
- ee/packages/federation-matrix/src/events/message.ts
🧰 Additional context used
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(276-277)IUser(186-255)packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-120)ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
acceptInvite(287-326)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
createOrUpdateFederatedUser(92-137)getUsernameServername(73-85)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/events/member.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
getUsernameServername(73-85)createOrUpdateFederatedUser(92-137)
⏰ 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 (2)
apps/meteor/server/methods/addRoomOwner.ts (1)
27-27: No action needed—federationis projected consistently across all add/remove room owner/moderator methods.apps/meteor/server/methods/removeRoomOwner.ts (1)
25-25: Retainfederationin the projection.The added
federation: 1is required by downstream federation logic—EE’sbeforeChangeRoomRolehooks (apps/meteor/ee/server/hooks/federation/index.ts) and theisRoomNativeFederatedtype guard both accessroom.federation. No changes needed.
| const inviter = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>( | ||
| getUsernameServername(inviteEvent.sender, services.config.serverName)[0], | ||
| { | ||
| projection: { _id: 1, username: 1 }, | ||
| }, | ||
| ); | ||
|
|
||
| if (!inviter) { | ||
| throw new Error('Sender user ID not found'); | ||
| } | ||
| if (isUserNativeFederated(inviter)) { | ||
| throw new Error('Sender user is native federated'); | ||
| } | ||
|
|
||
| const user = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>(username, { projection: { _id: 1 } }); | ||
|
|
||
| // we cannot accept invites from users that are external | ||
| if (!user) { | ||
| throw new Error('User not found'); | ||
| } | ||
| if (isUserNativeFederated(user)) { | ||
| throw new Error('User is native federated'); | ||
| } |
There was a problem hiding this comment.
Include federation fields when checking for native federated users.
Both inviter and user queries strip federated/federation from the projection, so isUserNativeFederated(...) always returns false and the guard never trips. Either pull those fields or drop the restrictive generic so the full document comes back.
- const inviter = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>(
+ const inviter = await Users.findOneByUsername(
getUsernameServername(inviteEvent.sender, services.config.serverName)[0],
{
- projection: { _id: 1, username: 1 },
+ projection: { _id: 1, username: 1, federated: 1, federation: 1 },
},
);
…
- const user = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>(username, { projection: { _id: 1 } });
+ const user = await Users.findOneByUsername(username, {
+ projection: { _id: 1, username: 1, federated: 1, federation: 1 },
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const inviter = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>( | |
| getUsernameServername(inviteEvent.sender, services.config.serverName)[0], | |
| { | |
| projection: { _id: 1, username: 1 }, | |
| }, | |
| ); | |
| if (!inviter) { | |
| throw new Error('Sender user ID not found'); | |
| } | |
| if (isUserNativeFederated(inviter)) { | |
| throw new Error('Sender user is native federated'); | |
| } | |
| const user = await Users.findOneByUsername<Pick<IUser, '_id' | 'username'>>(username, { projection: { _id: 1 } }); | |
| // we cannot accept invites from users that are external | |
| if (!user) { | |
| throw new Error('User not found'); | |
| } | |
| if (isUserNativeFederated(user)) { | |
| throw new Error('User is native federated'); | |
| } | |
| const inviter = await Users.findOneByUsername( | |
| getUsernameServername(inviteEvent.sender, services.config.serverName)[0], | |
| { | |
| projection: { _id: 1, username: 1, federated: 1, federation: 1 }, | |
| }, | |
| ); | |
| if (!inviter) { | |
| throw new Error('Sender user ID not found'); | |
| } | |
| if (isUserNativeFederated(inviter)) { | |
| throw new Error('Sender user is native federated'); | |
| } | |
| const user = await Users.findOneByUsername(username, { | |
| projection: { _id: 1, username: 1, federated: 1, federation: 1 }, | |
| }); | |
| // we cannot accept invites from users that are external | |
| if (!user) { | |
| throw new Error('User not found'); | |
| } | |
| if (isUserNativeFederated(user)) { | |
| throw new Error('User is native federated'); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/invite.ts around lines 301 to
323, the inviter and user queries explicitly project only _id and username so
the federated/federation fields are missing and isUserNativeFederated(...) can
never detect native federated users; update the queries to include the
federation-related fields (e.g., federated and/or federation) in the projection
or remove the restrictive generic/explicit projection so the full user document
is returned, then re-run the isUserNativeFederated checks.
| const [domain, port] = domainAndPort.split(':'); | ||
|
|
||
| const hostnameRegex = /^(?=.{1,253}$)([a-z0-9](?:[a-z0-9\-]{0,61}[a-z0-9])?)(?:\.[a-z0-9](?:[a-z0-9\-]{0,61}[a-z0-9])?)*$/i; | ||
| const ipv4Regex = /^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/; | ||
| const ipv6Regex = /^\[([0-9a-f:.]+)\]$/i; | ||
|
|
||
| if (!(hostnameRegex.test(domain) || ipv4Regex.test(domain) || ipv6Regex.test(domain))) { | ||
| return false; | ||
| } | ||
|
|
||
| if (port !== undefined) { | ||
| const portNum = Number(port); | ||
| if (!/^[0-9]+$/.test(port) || portNum < 1 || portNum > 65535) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle IPv6 homeservers correctly.
domainAndPort.split(':') shatters IPv6 literals like @user:[2001:db8::1], so domain becomes '[2001' and the regex test fails even though the MXID is valid. Parse the host/port without blindly splitting on every colon—for example, detect the bracketed form first, peel off the optional :port, then validate the host.
- const [domain, port] = domainAndPort.split(':');
+ let domain = domainAndPort;
+ let port: string | undefined;
+
+ if (domainAndPort.startsWith('[')) {
+ const closingBracket = domainAndPort.indexOf(']');
+ if (closingBracket === -1) {
+ return false;
+ }
+ domain = domainAndPort.slice(0, closingBracket + 1);
+ const remainder = domainAndPort.slice(closingBracket + 1);
+ if (remainder) {
+ if (!remainder.startsWith(':')) {
+ return false;
+ }
+ port = remainder.slice(1);
+ }
+ } else {
+ const colonIndex = domainAndPort.indexOf(':');
+ if (colonIndex !== -1) {
+ domain = domainAndPort.slice(0, colonIndex);
+ port = domainAndPort.slice(colonIndex + 1);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [domain, port] = domainAndPort.split(':'); | |
| const hostnameRegex = /^(?=.{1,253}$)([a-z0-9](?:[a-z0-9\-]{0,61}[a-z0-9])?)(?:\.[a-z0-9](?:[a-z0-9\-]{0,61}[a-z0-9])?)*$/i; | |
| const ipv4Regex = /^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/; | |
| const ipv6Regex = /^\[([0-9a-f:.]+)\]$/i; | |
| if (!(hostnameRegex.test(domain) || ipv4Regex.test(domain) || ipv6Regex.test(domain))) { | |
| return false; | |
| } | |
| if (port !== undefined) { | |
| const portNum = Number(port); | |
| if (!/^[0-9]+$/.test(port) || portNum < 1 || portNum > 65535) { | |
| return false; | |
| } | |
| } | |
| let domain = domainAndPort; | |
| let port: string | undefined; | |
| if (domainAndPort.startsWith('[')) { | |
| const closingBracket = domainAndPort.indexOf(']'); | |
| if (closingBracket === -1) { | |
| return false; | |
| } | |
| domain = domainAndPort.slice(0, closingBracket + 1); | |
| const remainder = domainAndPort.slice(closingBracket + 1); | |
| if (remainder) { | |
| if (!remainder.startsWith(':')) { | |
| return false; | |
| } | |
| port = remainder.slice(1); | |
| } | |
| } else { | |
| const colonIndex = domainAndPort.indexOf(':'); | |
| if (colonIndex !== -1) { | |
| domain = domainAndPort.slice(0, colonIndex); | |
| port = domainAndPort.slice(colonIndex + 1); | |
| } | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 41 to 56,
the current domain/port parsing uses domainAndPort.split(':') which breaks IPv6
bracketed literals; update parsing to first check for a leading '[' (bracketed
IPv6) and if present find the matching ']' to extract the full IPv6 host and
then optionally strip a single leading ':' plus port after the closing ']',
otherwise for non-bracketed input split only on the last ':' to separate an
optional port from the host; after extracting host and optional port, run the
existing hostnameRegex / ipv4Regex / ipv6Regex validation on the host and
validate the port is all digits and between 1 and 65535, returning false on any
validation failure.
Co-authored-by: Diego Sampaio <chinello@gmail.com>
Co-authored-by: Diego Sampaio <chinello@gmail.com>
Co-authored-by: Diego Sampaio <chinello@gmail.com>
FDR-193
Co-authored-by: Diego Sampaio chinello@gmail.com
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Other