feat: add origin to room name to prevent cross-server conflicts #37034
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 |
|
WalkthroughUpdates federation Matrix invite handling to compute DM room names from the other participant’s username, derive non-DM roomName and fname from Matrix room name and origin when no internal mapping exists (including stripping leading '!' and replacing ':' with '_'), and set extraData.fname only for non-DM rooms. Changes
Sequence Diagram(s)sequenceDiagram
participant Matrix as Matrix Homeserver
participant Invite as Invite Handler
participant Room as Room.create
Matrix->>Invite: inviteEvent (roomId, matrixRoom.name, origin, participants)
alt Direct Message (DM)
Note over Invite: roomName = other participant's username\n(matrixRoom.name === sender ? invitee : sender)
Invite->>Room: create({ name: roomName, extraData: { federated: true } })
else Non-DM (no internalMappedRoomId)
Note over Invite: roomId <- inviteEvent.roomId (strip leading '!' and replace ':' with '_')\nroomFname <- `${matrixRoom.name}:${matrixRoom.origin}`\nroomName <- transformed roomId
Invite->>Room: create({ name: roomName, extraData: { federated: true, fname: roomFname } })
end
Note over Invite: removed previous fallback try/catch name derivation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
268-270: DM naming logic needs clarification.The current implementation determines the DM room name by checking if
matrixRoom.name === senderUser.username, but this approach seems fragile. The TODO comment indicates this needs reconsideration.Consider implementing a more robust DM naming strategy that doesn't rely on comparing room names with usernames, which could break if naming conventions change.
packages/rest-typings/src/v1/rooms.ts (1)
818-818: Endpoint signature inconsistency with type definition.The endpoint signature shows
federationEnabled: booleanas required, but the type definitionGETRoomsNameExistsshows it as optional (federationEnabled?: boolean). This creates a mismatch between the API contract and implementation.- GET: (params: { roomName: string; federationEnabled: boolean }) => { + GET: (params: { roomName: string; federationEnabled?: boolean }) => {
📜 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 (5)
apps/meteor/app/api/server/v1/rooms.ts(1 hunks)apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)packages/rest-typings/src/v1/rooms.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-64)packages/models/src/index.ts (3)
MatrixBridgedRoom(216-216)Rooms(200-200)Subscriptions(206-206)
🔇 Additional comments (7)
apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx (1)
141-141: LGTM! Federation context now properly passed to name validation.The addition of
federationEnabled: federatedto the name existence check correctly provides federation context to the backend, enabling server-side federation-aware name validation.ee/packages/federation-matrix/src/FederationMatrix.ts (2)
202-204: Updated federation room naming strategy implemented correctly.The new naming approach creates distinct local display names:
roomName:${room.name}_${this.serverName}for unique identificationroomFname:${room.name} @ ${this.serverName}for user-friendly displaymatrixRoomName: Used for the actual Matrix room creationThis prevents cross-server name conflicts while maintaining user experience.
217-218: Local room state properly synchronized with federation naming.The code correctly updates both the local room record and user subscription with the new federation-aware names, ensuring consistent display across the application.
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
285-287: Federation-aware room naming properly implemented.The implementation correctly creates federation-aware room names:
roomName:${matrixRoom.name}_${matrixRoom.origin}for unique identificationroomFname:${matrixRoom.name} @ ${matrixRoom.origin}for displayThis aligns with the broader federation naming strategy to prevent cross-server conflicts.
packages/rest-typings/src/v1/rooms.ts (2)
447-447: API typing correctly extended for federation support.The addition of the optional
federationEnabledparameter properly extends the API contract while maintaining backward compatibility.
456-458: Schema validation properly updated for federation parameter.The schema correctly defines
federationEnabledas an optional boolean, maintaining API contract consistency.apps/meteor/app/api/server/v1/rooms.ts (1)
111-121: Verify federation domain extraction — avoid split('//')[1] assumptionsplit('//')[1] assumes Federation_Service_Domain includes a protocol; repo search shows only this use at apps/meteor/app/api/server/v1/rooms.ts:114. Confirm the setting format. Recommend normalizing the value before use (strip leading https?:// and any path, e.g. value?.replace(/^https?:///, '').split('/')[0]) so room name construction is robust.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37034 +/- ##
===========================================================
- Coverage 67.34% 67.32% -0.03%
===========================================================
Files 3342 3342
Lines 113379 113376 -3
Branches 20657 20650 -7
===========================================================
- Hits 76358 76328 -30
- Misses 34421 34444 +23
- Partials 2600 2604 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
fix/federation-room-name-conflict
branch
from
c67512d to
63ffa50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
268-270: Validate DM room name before deriving the other participantmatrixRoom.name can be arbitrary or empty — explicitly check whether it equals senderUser.username or inviteeUser.username and pick the other participant only in those cases; if neither matches, fall back to a deterministic name (e.g.
${senderUser.username}_${inviteeUser.username}) or another safe fallback. File: ee/packages/federation-matrix/src/api/_matrix/invite.ts lines 268–270.
📜 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/api/_matrix/invite.ts(3 hunks)
⏰ 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)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (3)
268-270: LGTM! DM room naming logic is clear and functional.The DM-specific room naming logic correctly determines the room name by selecting the non-sender participant. The implementation handles the case where
matrixRoom.namemight be either the sender or invitee username.
285-286: LGTM! Origin-based naming prevents cross-server conflicts effectively.The implementation correctly addresses the PR objective by incorporating the origin into both the internal room name (
roomName) and display name (roomFname). This approach should effectively prevent naming conflicts between rooms from different federated servers.
roomName: Uses underscore separator for internal consistencyroomFname: Uses @ symbol for human-readable display
297-297: Verify Room.create API supports extraData.fname parameter.The code adds
fnameto theextraDataobject for non-DM rooms, but I want to ensure this parameter is properly supported by the Room.create API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/lib/rooms/roomTypes/direct.ts (1)
56-77: Guard against federated name collisions and avoid bypassing admin preference.Always preferring
fnamecan (a) ignore theUI_Use_Real_Nameadmin setting and (b) hide the origin suffix used to disambiguate federated DMs, leading to identical-looking entries when two remote users share the same display name. Recommend falling back toname(with origin) when a collision is detected in federated rooms, and consider retaining the admin preference for non‑federated rooms.Apply this minimal disambiguation in-place:
- return subscription.fname || subscription.name; + const displayName = subscription.fname || subscription.name; + + // For federated DMs, if another DM shares the same display name, show the unique username (with origin). + if (isRoomFederated(roomData as IRoom) && displayName) { + const collision = Subscriptions.state.find( + (s) => + s.t === 'd' && + s.rid !== roomData._id && + ((s.fname || s.name) === displayName), + ); + if (collision) { + return subscription.name || displayName; + } + } + + return displayName;Optional (if product wants to keep honoring admin preference for non‑federated): use
settingsto preferfnameonly whenUI_Use_Real_Nameis true.Please confirm intended behavior: should DMs ignore
UI_Use_Real_Name, and should federated origins remain visible when needed for disambiguation?
📜 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/client/lib/rooms/roomTypes/direct.ts(1 hunks)
⏰ 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
ricardogarim
force-pushed
the
fix/federation-room-name-conflict
branch
3 times, most recently
from
648fdbc to
878b1db
Compare
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/api/_matrix/invite.ts (1)
131-144: Backoff timing mixes seconds and milliseconds; retry schedule is broken.
delayis treated as seconds in math/logs but passed tosetTimeoutas milliseconds, while the next call receivesdelay * 1000(ms) as “seconds”, exploding the schedule.Apply this diff to fix units and cap growth at 625s:
async function runWithBackoff(fn: () => Promise<void>, delaySec = 5) { try { await fn(); } catch (e) { - const delay = delaySec === 625 ? 625 : delaySec ** 2; - console.log(`error occurred, retrying in ${delay}ms`, e); - setTimeout(() => { - runWithBackoff(fn, delay * 1000); - }, delay); + const nextDelaySec = Math.min(delaySec === 625 ? 625 : delaySec ** 2, 625); + const delayMs = delaySec * 1000; + console.log(`error occurred, retrying in ${delaySec}s`, e); + setTimeout(() => { + void runWithBackoff(fn, nextDelaySec); + }, delayMs); } }
📜 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/api/_matrix/invite.ts(3 hunks)
⏰ 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)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
298-298: Good call addingextraData.fname.This preserves the human-friendly title while keeping the unique, origin-qualified
name.
289-301: Mapping persisted — no action required.invite.ts calls MatrixBridgedRoom.createOrUpdateByLocalRoomId(...) after Room.create, so the non‑DM path writes the federated mapping (ee/packages/federation-matrix/src/api/_matrix/invite.ts:316).
ricardogarim
force-pushed
the
fix/federation-room-name-conflict
branch
from
878b1db to
c9cd34c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/api/_matrix/invite.ts (1)
134-143: Fix backoff: seconds vs milliseconds; next delay passed incorrectly.
delayis treated as seconds but logged/scheduled as ms, and the squared value is passed to the next call as ms. This explodes the delay and mislabels logs.Apply:
async function runWithBackoff(fn: () => Promise<void>, delaySec = 5) { try { await fn(); } catch (e) { - const delay = delaySec === 625 ? 625 : delaySec ** 2; - console.log(`error occurred, retrying in ${delay}ms`, e); - setTimeout(() => { - runWithBackoff(fn, delay * 1000); - }, delay); + const nextDelaySec = delaySec === 625 ? 625 : Math.min(delaySec ** 2, 625); + console.log(`error occurred, retrying in ${nextDelaySec}s`, e); + setTimeout(() => { + void runWithBackoff(fn, nextDelaySec); + }, nextDelaySec * 1000); } }
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
285-288: Prefer existing getValidRoomName; sanitize origin; anchored '!' removal; add fname fallback.
- Replace the three lines with safer parsing + fname fallback and use the repo's canonical room-name helper (it enforces slug/validation/max-length/UTF8 settings): apps/meteor/app/utils/server/lib/getValidRoomName.ts (also exposed via apps/meteor/server/services/room/service.ts).
- Minimal replacement (conceptual):
- const [rawLocal] = inviteEvent.roomId.split(':');
- const roomId = rawLocal.replace(/^!/, '');
- const roomFname = matrixRoom.name ?
${matrixRoom.name}:${matrixRoom.origin}:${roomId}:${matrixRoom.origin};- const roomName = await getValidRoomName(
${roomId}_${String(matrixRoom.origin)});- Rationale: getValidRoomName already handles name sanitization/slug rules (limax/UTF8 settings) and prevents reimplementing validation/length rules; anchored '!' removal avoids accidental removal of other characters.
File: ee/packages/federation-matrix/src/api/_matrix/invite.ts (around lines 285–288).
📜 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/api/_matrix/invite.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/invite.ts
⏰ 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)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
268-283: DM roomName picks “self”; name DMs after the other participant (inviter).Current ternary returns our own username half the time. For DMs, just use the inviter’s (sender) username; also set a friendly fname with origin to avoid cross‑server ambiguity.
-// TODO: Rethink room name on DMs -// get the other user than ourself -const roomName = matrixRoom.name === senderUser.username ? inviteeUser.username : senderUser.username; +// TODO: Rethink room name on DMs +// Name DM after the other participant (inviter) +const roomName = senderUser.username; @@ extraData: { federated: true, + fname: `${senderUser.username}@${matrixRoom.origin}`, },
298-298: Setting extraData.fname looks good.Populating
fnamealigns with the PR goal; with the fallback/sanitization above, this is solid.Please confirm UI escapes
fname(no HTML injection) and whether there’s a display length cap to avoid truncation.
ricardogarim
force-pushed
the
fix/federation-room-name-conflict
branch
from
c9cd34c to
abb1fa1
Compare
ggazzo
changed the title
Summary by CodeRabbit