feat(federation): add improved errors messaging #37581
Open
+180
−24
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 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 |
|
Contributor
WalkthroughAdds pre-federation domain allowlist and federated-user validation to room creation and federation hooks; exposes domain-check helpers and a FederationMatrix.validateFederatedUsers API; introduces FederationValidationError, configuration for validation timeouts, and bumps federation-sdk dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CreateRoom as createRoom Handler
participant DomainAPI as isFederationDomainAllowedForUsernames
participant FedMatrix as FederationMatrix.validateFederatedUsers
participant SDK as federation-sdk
Client->>CreateRoom: createRoom(members: [usernames])
CreateRoom->>DomainAPI: isFederationDomainAllowedForUsernames(usernames)
alt Domain Allowed
DomainAPI-->>CreateRoom: true
CreateRoom->>FedMatrix: validateFederatedUsers(federated_usernames)
FedMatrix->>SDK: validateOutboundUser(matrixId) for each federated username
alt Validation Success
SDK-->>FedMatrix: verified
FedMatrix-->>CreateRoom: success
CreateRoom-->>Client: room created
else Validation Failure
SDK-->>FedMatrix: error (connection/user/policy)
FedMatrix-->>CreateRoom: throw FederationValidationError
CreateRoom-->>Client: Meteor.Error (mapped)
end
else Domain Blocked
DomainAPI-->>CreateRoom: false
CreateRoom-->>Client: Meteor.Error (federation-policy-denied)
end
sequenceDiagram
participant Client
participant Hook as beforeAddUserToRoom
participant Perm as Permission Service
participant DomainAPI as isFederationDomainAllowedForUsernames
participant Invite as invite flow
Client->>Hook: add user to room (username)
Hook->>Perm: check access-federation
alt Permitted
Perm-->>Hook: allowed
Hook->>DomainAPI: isFederationDomainAllowedForUsernames([username])
alt Domain Allowed
DomainAPI-->>Hook: true
Hook->>Invite: proceed (may call FederationMatrix)
Invite-->>Client: success
else Domain Denied
DomainAPI-->>Hook: false
Hook-->>Client: Meteor.Error (federation-policy-denied)
end
else Not Permitted
Perm-->>Hook: denied
Hook-->>Client: permission error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
956a5cd to
1ef7c23
Compare
![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: 1
🧹 Nitpick comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
3-3: Federation hooks correctly integrate allow‑list and outbound user validationThe new checks in
beforeAddUserToRoomandbeforeCreateDirectRoomare properly gated on native federation, reuse the shared allow‑list helper, and translateFederationValidationErrorinto Meteor errors as per the permission/error‑handling patterns used in this module.You may later want to centralize the repeated
federation-policy-deniedmessage into a constant to avoid divergence across call sites.
Based on learnings, this matches the intended federation permission behavior.Also applies to: 91-97, 207-223
📜 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 (6)
apps/meteor/app/lib/server/functions/createRoom.ts(2 hunks)apps/meteor/ee/server/hooks/federation/index.ts(3 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(4 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts(3 hunks)ee/packages/federation-matrix/src/index.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
📚 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:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/index.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/setup.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/index.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/index.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/setup.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 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:
ee/packages/federation-matrix/src/index.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.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 (3)
apps/meteor/ee/server/hooks/federation/index.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedFromUsernames(44-54)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(138-935)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/index.ts (2)
validateFederatedUsername(3-3)isFederationDomainAllowed(11-11)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowed(20-42)
apps/meteor/app/lib/server/functions/createRoom.ts (4)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedFromUsernames(44-54)ee/packages/federation-matrix/src/index.ts (2)
isFederationDomainAllowedFromUsernames(11-11)FederationMatrix(3-3)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(138-935)packages/core-services/src/index.ts (1)
FederationMatrix(199-199)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/FederationMatrix.ts
[error] 218-218: Property 'validateOutboundUser' does not exist on type 'FederationSDK'.
🔇 Additional comments (5)
ee/packages/federation-matrix/src/setup.ts (1)
100-105: Federation validation timeout wiring looks goodTimeouts are parsed safely with sensible 3s defaults and hooked cleanly under the
federation.validationconfig without affecting existing behavior.apps/meteor/app/lib/server/functions/createRoom.ts (1)
7-7: Pre‑federation allow‑list and user validation increateRoomare well‑placedThe checks run only for federated rooms with username lists, correctly gate on the domain allow‑list, and translate
FederationValidationErrorinto Meteor errors with user‑facing messages without impacting local or non‑federated rooms.Also applies to: 182-202
ee/packages/federation-matrix/src/index.ts (1)
5-5: New federation exports are coherent with the public surfaceRe‑exporting
FederationValidationErrorand the domain allow‑list helpers here cleanly exposes the new validation capabilities to callers without changing existing APIs.Also applies to: 11-11
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
227-229: Room creation rollback behavior is a clear improvementTracking
matrixRoomCreatedand attempting a best‑effortleaveRoomwhen post‑creation steps fail avoids leaving orphaned Matrix membership and is correctly guarded so cleanup only runs when a room was actually created. Errors during cleanup are logged but do not swallow the original failure, which is the right trade‑off.Also applies to: 235-237, 258-267
211-220: Let me search for the actual federation-sdk v0.3.0 API to confirm:Let me search more specifically for federation-sdk package information:
Based on my thorough codebase inspection, I can now provide the rewritten review comment:
Normalize homeserver domain before domain allowlist check
In
verifyMatrixIds,homeserverUrlincludes port (per the comment "so we keep any port in the homeserver"), but you pass the fullhomeserverUrldirectly toisFederationDomainAllowed([homeserverUrl])without stripping the port. This means the allowlist check requires admins to configuredomain:portpairs, not just domains.To align with
isFederationDomainAllowedFromUsernames, which usesextractDomainFromIdto isolate the domain, consider normalizinghomeserverUrlbefore the allowlist check:if (!(await isFederationDomainAllowed([extractDomainFromId(matrixId)]))) {This way, admins configure only the domain in the allowlist and port variations are handled transparently.
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
Outdated
Show resolved
Hide resolved
ggazzo
reviewed
Nov 25, 2025
ggazzo
reviewed
Nov 25, 2025
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
1ef7c23 to
eb46bb2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
26-42: Security issue: over-permissive domain suffix matching still present.The fallback on line 37 (
domain.endsWith(pattern)) allows unrelated domains that happen to share a suffix. For example, ifbar.comis in the allow-list,evilbar.comwould also be permitted.Apply the fix from the previous review to enforce proper domain label boundaries:
- return domain.endsWith(pattern); + // Treat a bare pattern like "bar.com" as that exact domain or any proper subdomain, + // but do not match unrelated suffixes like "evilbar.com". + return domain.endsWith(`.${pattern}`);
📜 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 (6)
apps/meteor/app/lib/server/functions/createRoom.ts(2 hunks)apps/meteor/ee/server/hooks/federation/index.ts(3 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts(3 hunks)ee/packages/federation-matrix/src/index.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/ee/server/hooks/federation/index.ts
- ee/packages/federation-matrix/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/setup.tsapps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/setup.tsapps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/setup.tsapps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 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/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/FederationMatrix.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/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/FederationMatrix.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/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (3)
apps/meteor/app/lib/server/functions/createRoom.ts (4)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(44-54)ee/packages/federation-matrix/src/index.ts (3)
isFederationDomainAllowedForUsernames(11-11)FederationMatrix(3-3)FederationValidationError(5-5)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(138-924)packages/core-services/src/index.ts (1)
FederationMatrix(199-199)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
ee/packages/federation-matrix/src/index.ts (2)
isFederationDomainAllowed(11-11)isFederationDomainAllowedForUsernames(11-11)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/index.ts (3)
validateFederatedUsername(3-3)FederationValidationError(5-5)isFederationDomainAllowed(11-11)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowed(20-42)
⏰ 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 (9)
ee/packages/federation-matrix/src/setup.ts (1)
100-105: LGTM!The validation timeout configuration is well-integrated and follows existing patterns in the file for environment variable parsing with sensible defaults.
apps/meteor/app/lib/server/functions/createRoom.ts (2)
7-7: LGTM!Imports are correctly added for the new federation validation utilities.
182-202: Well-structured federation validation flow.The sequential validation (policy check → user existence) aligns with the PR objectives. Error mapping from
FederationValidationErrortoMeteor.Errorcorrectly preserves the error code and user message for proper UI feedback. Based on learnings, throwingMeteorErrorfor user-initiated actions is the correct pattern here.ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (3)
2-2: LGTM!Correct import for extracting domains from federated user IDs.
44-54: LGTM!The function correctly filters federated usernames (containing
:), extracts their domains, and delegates toisFederationDomainAllowed. Returningtruewhen no federated users are present is the correct behavior.
92-94: Good refactor to centralized domain validation.The middleware now delegates to
isFederationDomainAllowed, ensuring consistent domain checking logic across all code paths.ee/packages/federation-matrix/src/FederationMatrix.ts (3)
11-11: LGTM!Correct import of
FederationValidationErrorfor validation error handling.
18-18: LGTM!Import added for domain allowlist validation.
892-917: LGTM - Comprehensive verification with proper error mapping.The
verifyMatrixIdsmethod now correctly:
- Checks domain allowlist before network validation (line 895)
- Validates outbound user existence via SDK (line 900)
- Maps specific error codes to appropriate verification statuses
The error code handling (lines 904-915) properly distinguishes between connection failures, user not found, and policy denials.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (2)
57-57: TODO: Extract authorization header parser if reused.The comment suggests extracting
parseMatrixAuthorizationHeaderas a shared utility if needed elsewhere.Would you like me to open an issue to track this refactoring, or help identify other locations where this parser might be useful?
6-6: Consider removing inline comments per coding guidelines.The coding guidelines for TypeScript files recommend avoiding code comments in the implementation. Most comments here explain "what" the code does rather than "why", which can be inferred from well-named functions and variables.
As per coding guidelines, consider removing comments at lines 6, 29, 41, 44, and 60. The code is self-explanatory with clear function and variable names.
Also applies to: 29-29, 41-41, 44-44, 60-60
📜 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/middlewares/isFederationDomainAllowed.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.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:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
ee/packages/federation-matrix/src/index.ts (2)
isFederationDomainAllowed(11-11)isFederationDomainAllowedForUsernames(11-11)
⏰ 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/middlewares/isFederationDomainAllowed.ts (3)
20-38: LGTM! Domain allow-list logic is secure and correct.The pattern matching correctly handles wildcards (subdomains only) and non-wildcard entries (exact + subdomains), preventing suffix-abuse attacks like
evilbar.commatchingbar.com. The use of.every()ensures fail-fast behavior when any domain is disallowed.
88-88: Clean refactoring to use shared validation.The middleware correctly delegates to
isFederationDomainAllowed, improving code reuse and maintainability while preserving the same security validation logic.
40-50: I'll verify the concerns in this review comment by examining howextractDomainFromIdis used and whether error handling is necessary.
<function_calls>
#!/bin/bashCheck extractDomainFromId usage and error handling patterns
Find all usages of extractDomainFromId in the codebase
echo "=== All usages of extractDomainFromId ==="
rg -n --type=ts 'extractDomainFromId' -B2 -A2Check for try-catch patterns around it
echo -e "\n=== Try-catch or error handling around extractDomainFromId ==="
rg -n --type=ts -B3 -A3 'try.*extractDomainFromId|extractDomainFromId.*catch|catch.*extractDomainFromId'Look for import statement to understand the source
echo -e "\n=== Import statements for extractDomainFromId ==="
#!/bin/bash # Search federation-sdk documentation or type definitions echo "=== Look for @rocket.chat/federation-sdk references ===" rg -n --type=ts '@rocket.chat/federation-sdk' -A1 | head -30
rg -n --type=ts 'import.*extractDomainFromId|extractDomainFromId.*from'
</function_calls>Check if there's any JSDoc or type info on extractDomainFromId
echo -e "\n=== Search for extractDomainFromId function definition ==="
#!/bin/bash # Examine the current file to understand context echo "=== Current file context ===" wc -l ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
rg -n --type=ts 'export.*extractDomainFromId|function extractDomainFromId|const extractDomainFromId'
</function_calls>Show the full file to understand imports and usage
cat -n ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
#!/bin/bash # Check if there are any tests for this function to understand expected behavior echo "=== Look for tests ===" fd -e test.ts -e spec.ts -e test.js | xargs rg -l 'isFederationDomainAllowedForUsernames' 2>/dev/null
</function_calls>Check for any Matrix ID validation utilities
echo -e "\n=== Look for Matrix ID or username validation patterns ==="
rg -n --type=ts 'includes.*:.*username|username.includes.:' -B2 -A2 | head -30
</function_calls>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
193-206: Type annotations and filter logic can be simplified sincemembersis alreadystring[].At line 182,
onlyUsernames(members)guarantees thatmembersisstring[]. The type annotations(member: string | IUser)on lines 196-198 are unnecessary and potentially confusing since the IUser branch is never taken.Additionally, the federated user detection here (line 197) only checks for
:, whilehasFederatedMembers(lines 140-145) checks for both:and@. WhilevalidateFederatedUserswill catch invalid formats, aligning the detection logic would improve consistency.- try { - // TODO: Use common function to extract and validate federated users - const federatedUsers = members - .filter((member: string | IUser) => (typeof member === 'string' ? member.includes(':') : member.username?.includes(':'))) - .map((member: string | IUser) => (typeof member === 'string' ? member : member.username!)); - await FederationMatrix.validateFederatedUsers(federatedUsers); + try { + const federatedUsers = members.filter((member) => member.includes(':') && member.includes('@')); + await FederationMatrix.validateFederatedUsers(federatedUsers);
📜 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/ee/server/hooks/federation/index.ts(3 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/index.ts (3)
validateFederatedUsername(3-3)FederationValidationError(5-5)isFederationDomainAllowed(11-11)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowed(20-38)
apps/meteor/ee/server/hooks/federation/index.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(40-50)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(138-924)
⏰ 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/ee/server/hooks/federation/index.ts (1)
90-97: LGTM!The domain allowlist check is correctly positioned after the authorization check and properly throws
MeteorErrorfor user-initiated actions. Based on learnings, this pattern is correct for federation permission checks in user-initiated flows.ee/packages/federation-matrix/src/FederationMatrix.ts (2)
211-229: LGTM!The
validateFederatedUsersmethod correctly:
- Validates format upfront, providing clear error messages for invalid federated usernames
- Returns early for empty arrays
- Validates each federated user via the SDK
The callers pre-filter to federated usernames before calling this method, which aligns with the expected input.
892-916: LGTM!The enhanced
verifyMatrixIdsmethod now properly:
- Enforces the domain allowlist before attempting validation (fail-fast)
- Maps SDK error codes to appropriate verification statuses
- Falls back to
UNABLE_TO_VERIFYfor unexpected errorsThe error code mapping aligns with the PR objectives for pre-federation validation (CONNECTION_FAILED → UNABLE_TO_VERIFY, USER_NOT_FOUND → UNVERIFIED, POLICY_DENIED → POLICY_DENIED).
jessicaschelly
added
the
stat: QA assured
KevLehman
previously approved these changes
Dec 18, 2025
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
d33035d to
1208660
Compare
sampaiodiego
force-pushed
the
feat/fed-errors-messaging
branch
from
1208660 to
0c44373
Compare
Contributor
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)
apps/meteor/ee/server/hooks/federation/index.ts (1)
115-142: Critical: Duplicate logic causes double permission checks and double invites.The new code block (lines 115-129) executes permission checks and invites users, but then execution falls through to the old code block (lines 132-142), which re-checks permissions and re-invites users. This creates several issues:
- Permission is checked twice (lines 116 and 132)
- Users are invited twice (lines 128 and 142) unless the inviter is federated
- The new block doesn't check if the inviter is federated (line 138 in old code), potentially causing federation loops
Expected behavior: After the new validation and invite (line 128), execution should skip the old invite logic but still create the subscription (lines 146-152).
🔎 Proposed fix
if (FederationActions.shouldPerformFederationAction(room)) { if (!(await Authorization.hasPermission(user._id, 'access-federation'))) { throw new MeteorError('error-not-authorized-federation', 'Not authorized to access federation'); } const isAllowed = await isFederationDomainAllowedForUsernames([user.username]); if (!isAllowed) { throw new MeteorError( 'federation-policy-denied', "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy.", ); } + // If inviter is federated, the invite came from an external transaction. + // Don't propagate back to Matrix (it was already processed at origin server). + if (isUserNativeFederated(inviter)) { + return; + } + await FederationMatrix.inviteUsersToRoom(room, [user.username], inviter); + + // after invite is sent we create the invite subscriptions + await Room.createUserSubscription({ + ts: new Date(), + room, + userToBeAdded: user, + inviter, + status: 'INVITED', + }); + return; } - - // TODO should we really check for "user" here? it is potentially an external user - if (!(await Authorization.hasPermission(user._id, 'access-federation'))) { - throw new MeteorError('error-not-authorized-federation', 'Not authorized to access federation'); - } - - // If inviter is federated, the invite came from an external transaction. - // Don't propagate back to Matrix (it was already processed at origin server). - if (isUserNativeFederated(inviter)) { - return; - } - - await FederationMatrix.inviteUsersToRoom(room, [user.username], inviter); - - // after invite is sent we create the invite subscriptions - // TODO this may be not needed if we receive the emit for the invite event from matrix - await Room.createUserSubscription({ - ts: new Date(), - room, - userToBeAdded: user, - inviter, - status: 'INVITED', - }); },
♻️ Duplicate comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
261-261: UseMeteorErrorinstead ofMeteor.Errorfor consistency.Lines 261 and 272 use
Meteor.Error, butMeteorErroris imported from@rocket.chat/core-services(line 1) and used consistently throughout the rest of the file (lines 46, 100, 117, 122-125, 133).Meteoris not explicitly imported.🔎 Proposed fix
const isAllowed = await isFederationDomainAllowedForUsernames(members); if (!isAllowed) { - throw new Meteor.Error( + throw new MeteorError( 'federation-policy-denied', "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy.", ); } try { const federatedUsers = members.filter((username) => username.includes(':')); await FederationMatrix.validateFederatedUsers(federatedUsers); } catch (error) { if (error instanceof FederationValidationError) { - throw new Meteor.Error(error.error, error.userMessage); + throw new MeteorError(error.error, error.userMessage); } throw error; }Also applies to: 272-272
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
apps/meteor/app/lib/server/functions/createRoom.ts(2 hunks)apps/meteor/ee/server/hooks/federation/index.ts(3 hunks)apps/meteor/package.json(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(4 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts(3 hunks)ee/packages/federation-matrix/src/index.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)packages/core-services/package.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core-services/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
- ee/packages/federation-matrix/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/setup.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 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/package.jsonee/packages/federation-matrix/src/setup.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 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/package.jsonpackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/package.jsonpackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/setup.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/setup.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
packages/core-services/src/types/IFederationMatrixService.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/createRoom.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:
packages/core-services/src/types/IFederationMatrixService.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.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:
packages/core-services/src/types/IFederationMatrixService.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.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 (3)
apps/meteor/ee/server/hooks/federation/index.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(40-50)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(144-968)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/index.ts (3)
validateFederatedUsername(3-3)FederationValidationError(5-5)isFederationDomainAllowed(11-11)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowed(20-38)
apps/meteor/app/lib/server/functions/createRoom.ts (4)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(40-50)ee/packages/federation-matrix/src/index.ts (3)
isFederationDomainAllowedForUsernames(11-11)FederationMatrix(3-3)FederationValidationError(5-5)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(144-968)packages/core-services/src/index.ts (1)
FederationMatrix(201-201)
⏰ 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)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
apps/meteor/package.json (1)
102-102: Unable to verify @rocket.chat/federation-sdk version 0.3.6 from public sources.The package
@rocket.chat/federation-sdkdoes not appear to exist in the public npm registry. Confirm the package name is correct (possibly@rocket.chat/federation-matrixinstead), and verify the version 0.3.6 is available if using a private npm registry. Runnpm view @rocket.chat/federation-sdk@0.3.6in the workspace to confirm availability locally.packages/core-services/src/types/IFederationMatrixService.ts (1)
32-32: Interface extension aligns with implementation.The new
validateFederatedUsersmethod signature matches the implementation inFederationMatrix.tsand follows the existing interface patterns where validation/mutation operations returnPromise<void>and throw on failure.ee/packages/federation-matrix/src/setup.ts (1)
98-103: Federation validation timeout configuration looks good.The new validation timeout settings follow the existing configuration patterns in this file. The 3-second defaults are reasonable for network operations.
apps/meteor/app/lib/server/functions/createRoom.ts (2)
6-6: Import additions support the new federation validation flow.The imports align with the new validation requirements and are used correctly in the subsequent code block.
194-218: Pre-federation validation logic correctly implements fail-fast behavior.The validation flow correctly:
- Checks domain allowlist before proceeding
- Validates federated users exist and are reachable
- Maps
FederationValidationErrortoMeteor.Errorfor proper client-side handlingThe TODO at line 207 appropriately flags the opportunity to consolidate federated user extraction logic. Based on learnings, this pattern of throwing
Meteor.Errorfor user-initiated federation actions is correct.ee/packages/federation-matrix/src/FederationMatrix.ts (5)
11-18: Import additions support the new validation flow.The new imports from
@rocket.chat/federation-sdkprovide the necessary types and error classes for the federation validation feature.
24-24: Domain allowlist import enables policy enforcement.This import is used in
verifyMatrixIdsfor domain policy checks.
217-234: Validation method correctly implements format and outbound checks.The implementation:
- Validates username format first (fail-fast for malformed input)
- Filters to federated users only
- Validates each user via SDK's
validateOutboundUserNote: This method expects callers to pre-filter to federated usernames only (which
createRoom.tsdoes at lines 208-210). If mixed local/federated usernames are passed, local usernames will fail the format check. The TODO increateRoom.tsacknowledges this coupling.
255-255: Simplified error logging.Minor stylistic change to error logging format.
894-918: Domain allowlist and error mapping enhance verification flow.The updated
verifyMatrixIdscorrectly:
- Checks domain against allowlist before outbound validation (line 897)
- Uses SDK's
validateOutboundUserfor reachability/existence checks (line 902)- Maps error codes to appropriate verification statuses (lines 906-916)
The error code mapping handles:
CONNECTION_FAILED→UNABLE_TO_VERIFY(network issues)USER_NOT_FOUND→UNVERIFIED(user doesn't exist)POLICY_DENIED→POLICY_DENIED(allowlist block)- Default →
UNABLE_TO_VERIFY(unknown errors)ee/packages/federation-matrix/package.json (1)
25-25: Dependency version bump aligns with federation validation features.The
@rocket.chat/federation-sdkupgrade to 0.3.6 is required to support the newvalidateOutboundUserAPI andFederationValidationErrorexport used in the federation validation flow.apps/meteor/ee/server/hooks/federation/index.ts (1)
128-128: No changes needed. TheinviteUsersToRoommethod does not throwFederationValidationError—it only usesvalidateFederatedUsernameas a conditional check, not validation that throws. Error handling is unnecessary here since the method re-throws errors generically without specifically producingFederationValidationError.Likely an incorrect or invalid review comment.
Contributor
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/26 16:39 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 26 Dec 2025 16:39:51 GMT |
sampaiodiego
requested changes
Dec 22, 2025
|
|
||
| if (!FederationActions.shouldPerformFederationAction(room)) { | ||
| return; | ||
| if (FederationActions.shouldPerformFederationAction(room)) { |
Member
There was a problem hiding this comment.
can you please elaborate on this change? it is calling FederationMatrix.inviteUsersToRoom two times now, one from inside the if and one outside, is this intended?
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
9607661 to
543db85
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
00d5e14 to
f6592e5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/ee/server/hooks/federation/index.ts (1)
254-270: Inconsistent error class usage persists; improve filtering logic.Two issues remain in this callback:
Lines 256 and 267 still use
Meteor.Errorinstead ofMeteorError, despite the past review comment marked as addressed. The file importsMeteorError(line 1) and should use it consistently.Line 263 filters federated users using
username.includes(':')as a heuristic. This is less precise than using the importedvalidateFederatedUsernamefunction, which properly validates the Matrix ID format including the leading@.🔎 Suggested fixes
const isAllowed = await isFederationDomainAllowedForUsernames(members); if (!isAllowed) { - throw new Meteor.Error( + throw new MeteorError( 'federation-policy-denied', "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy.", ); } try { - const federatedUsers = members.filter((username) => username.includes(':')); + const federatedUsers = members.filter((username) => validateFederatedUsername(username)); await FederationMatrix.validateFederatedUsers(federatedUsers); } catch (error) { if (error instanceof FederationValidationError) { - throw new Meteor.Error(error.error, error.userMessage); + throw new MeteorError(error.error, error.userMessage); } throw error; }ee/packages/federation-matrix/src/FederationMatrix.ts (1)
217-234: Critical: Pre-validation check contradicts filtering logic.The past review comment about this function appears unresolved despite being marked as addressed. The logic is contradictory:
- Lines 218-224 throw
POLICY_DENIEDif ANY username failsvalidateFederatedUsername- Lines 226-229 filter to only federated usernames and return early if none exist
This means passing mixed local/federated usernames (e.g.,
["alice", "@bob:remote.com"]) will always fail at line 218 before reaching the filter at line 226. The filter suggests the intent is to handle mixed lists gracefully, but the validation check prevents this.Based on the PR objectives and usage patterns (e.g.,
index.ts:263pre-filters before calling), the function should either:
- Remove lines 218-224 and rely solely on the filter (allow mixed lists, validate only federated usernames)
- Document that only federated usernames are accepted and rename to clarify (e.g.,
validateFederatedUsernamesOnly)Option 1 aligns with
isFederationDomainAllowedForUsernameswhich silently filters local users.🔎 Recommended fix (Option 1)
async validateFederatedUsers(usernames: string[]): Promise<void> { - const hasInvalidFederatedUsername = usernames.some((username) => !validateFederatedUsername(username)); - if (hasInvalidFederatedUsername) { - throw new FederationValidationError( - 'POLICY_DENIED', - `Invalid federated username format: ${usernames.filter((username) => !validateFederatedUsername(username)).join(', ')}. Federated usernames must follow the format @username:domain.com`, - ); - } - const federatedUsers = usernames.filter(validateFederatedUsername); if (federatedUsers.length === 0) { return; } for await (const username of federatedUsers) { await federationSDK.validateOutboundUser(userIdSchema.parse(username)); } }
🧹 Nitpick comments (2)
apps/meteor/ee/server/hooks/federation/index.ts (1)
123-129: Consider adding user existence validation before invite.The domain policy check correctly blocks invites to disallowed domains. However, according to the PR objectives, the validation shield should also verify user existence on the remote homeserver before inviting. Currently, this hook only checks the domain allowlist but doesn't call
FederationMatrix.validateFederatedUsersto verify the user exists remotely.🔎 Suggested enhancement
const isAllowed = await isFederationDomainAllowedForUsernames([user.username]); if (!isAllowed) { throw new MeteorError( 'federation-policy-denied', "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy.", ); } + + try { + if (validateFederatedUsername(user.username)) { + await FederationMatrix.validateFederatedUsers([user.username]); + } + } catch (error) { + if (error instanceof FederationValidationError) { + throw new MeteorError(error.error, error.userMessage); + } + throw error; + }ee/packages/federation-matrix/src/FederationMatrix.ts (1)
231-233: Consider parallel validation for performance.The sequential
for awaitloop validates users one at a time. While this matches the PR's "sequential validation" requirement, it could be slow when validating multiple federated users. If the order doesn't matter and early termination isn't required, consider usingPromise.allto validate in parallel.🔎 Optional optimization
- for await (const username of federatedUsers) { - await federationSDK.validateOutboundUser(userIdSchema.parse(username)); - } + await Promise.all( + federatedUsers.map((username) => + federationSDK.validateOutboundUser(userIdSchema.parse(username)) + ) + );Note: Only apply this if validation failures should report all invalid users rather than fail-fast on the first error.
📜 Review details
Configuration used: Organization 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)
apps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.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 (3)
apps/meteor/ee/server/hooks/federation/index.ts (1)
4-8: LGTM!The new imports support the pre-federation validation workflow and are used appropriately throughout the file.
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
11-18: LGTM!Import additions support the new validation workflow and are used appropriately in the updated methods.
Also applies to: 24-24
891-915: LGTM!The updated
verifyMatrixIdsmethod correctly implements the three-tier validation:
- Domain allowlist enforcement (line 893-895)
- Network reachability via
validateOutboundUser(line 898)- Comprehensive error code mapping (lines 902-914)
The error handling appropriately distinguishes between policy denials, connection failures, and user-not-found scenarios as specified in the PR objectives.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37581 +/- ##
========================================
Coverage 70.63% 70.64%
========================================
Files 3143 3143
Lines 108693 108693
Branches 19577 19541 -36
========================================
+ Hits 76779 76789 +10
Misses 29904 29904
+ Partials 2010 2000 -10
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
feat/fed-errors-messaging
branch
from
f6592e5 to
0e9ce3e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
256-259: Inconsistent error class usage:Meteor.ErrorvsMeteorErrorCatch clause variable type annotation must be 'any' or 'unknown' if specified. Use the imported
MeteorErrorclass (from line 1) for consistency with lines 46, 100, 120, and 125.Meteoris not explicitly imported in this file.🔎 Proposed fix
const isAllowed = await isFederationDomainAllowedForUsernames(members); if (!isAllowed) { - throw new Meteor.Error( + throw new MeteorError( 'federation-policy-denied', "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy.", ); } try { const federatedUsers = members.filter((username) => username.includes(':')); await FederationMatrix.validateFederatedUsers(federatedUsers); } catch (error: unknown) { if (error instanceof FederationValidationError) { - throw new Meteor.Error(error.error, error.userMessage); + throw new MeteorError(error.error, error.userMessage); } throw error; }Also applies to: 267-267
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
207-207: Address the TODO: Extract federated user detection logic.The TODO comment correctly identifies code duplication between lines 162-167 and lines 208-210. Extracting this into a shared helper function would improve maintainability.
Would you like me to generate a helper function to consolidate the federated user detection logic?
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
📚 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/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.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/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/ee/server/hooks/federation/index.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/lib/server/functions/createRoom.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/lib/server/functions/createRoom.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/lib/server/functions/createRoom.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
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (2)
apps/meteor/app/lib/server/functions/createRoom.ts (3)
ee/packages/federation-matrix/src/index.ts (3)
isFederationDomainAllowedForUsernames(11-11)FederationMatrix(3-3)FederationValidationError(5-5)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(40-50)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(144-962)
apps/meteor/ee/server/hooks/federation/index.ts (2)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedForUsernames(40-50)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(144-962)
🪛 Biome (2.1.2)
apps/meteor/app/lib/server/functions/createRoom.ts
[error] 212-212: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
apps/meteor/ee/server/hooks/federation/index.ts
[error] 265-265: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
⏰ 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 (4)
apps/meteor/ee/server/hooks/federation/index.ts (2)
123-129: Domain policy check and error handling look good.The permission check pattern, domain allowlist validation, and error propagation follow the established federation patterns correctly.
262-270: Federated user validation logic is sound.The filtering and validation of federated users with proper error mapping to
MeteorErroris correct. Theinstanceofcheck properly handles the error type narrowing.apps/meteor/app/lib/server/functions/createRoom.ts (2)
6-6: LGTM! Imports are correctly added.The new federation validation utilities are properly imported and used in the validation logic below.
194-203: LGTM! Domain allowlist enforcement is correctly implemented.The policy vetting check properly blocks disallowed domains and returns the expected error code and message as specified in the PR objectives.
Comment on lines
+208
to
+210
| const federatedUsers = members | ||
| .filter((member: string | IUser) => (typeof member === 'string' ? member.includes(':') : member.username?.includes(':'))) | ||
| .map((member: string | IUser) => (typeof member === 'string' ? member : member.username!)); |
Contributor
There was a problem hiding this comment.
Inconsistent federated user detection logic and unnecessary type annotations.
Two issues:
-
Inconsistent detection criteria: Line 164 checks for both
:AND@to identify federated users, but line 209 only checks for:. This inconsistency could cause members to be incorrectly classified. Matrix user IDs follow the format@localpart:domain, so both characters should be required. -
Unnecessary complexity: Since this code is inside the
onlyUsernames(members)type guard,membersis guaranteed to bestring[]. The type annotations(string | IUser)and the runtime type checks (typeof member === 'string') are unnecessary.
🔎 Proposed fix
- const federatedUsers = members
- .filter((member: string | IUser) => (typeof member === 'string' ? member.includes(':') : member.username?.includes(':')))
- .map((member: string | IUser) => (typeof member === 'string' ? member : member.username!));
+ const federatedUsers = members.filter((member) => member.includes(':') && member.includes('@'));🤖 Prompt for AI Agents
In apps/meteor/app/lib/server/functions/createRoom.ts around lines 208 to 210,
the federated-user detection is inconsistent and overly complex: change the
filter to require both '@' and ':' (e.g., member.includes('@') &&
member.includes(':')) to match Matrix IDs, and remove the unnecessary union
types and runtime type checks — treat members as string[] (since
onlyUsernames(members) guarantees that) and map directly from member to member.
…omainAllowedForUsernames
…rsBeforeRoomCreation
Co-authored-by: Diego Sampaio <chinello@gmail.com>
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
decfe3d to
b39e63c
Compare
ricardogarim
force-pushed
the
feat/fed-errors-messaging
branch
from
b39e63c to
312ec65
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization 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 (4)
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/errors/FederationValidationError.tsee/packages/federation-matrix/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
- ee/packages/federation-matrix/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/errors/FederationValidationError.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
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.
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/errors/FederationValidationError.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/errors/FederationValidationError.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
ee/packages/federation-matrix/src/index.ts (3)
validateFederatedUsername(3-3)FederationValidationError(7-7)isFederationDomainAllowed(13-13)ee/packages/federation-matrix/src/errors/FederationValidationError.ts (1)
FederationValidationError(2-13)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowed(21-39)
ee/packages/federation-matrix/src/errors/FederationValidationError.ts (2)
ee/packages/federation-matrix/src/index.ts (1)
FederationValidationError(7-7)packages/message-parser/src/utils.ts (1)
code(42-46)
⏰ 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/FederationMatrix.ts (1)
885-909: LGTM! Validation shield correctly implements sequential checks.The implementation properly follows the PR's three-step validation pattern:
- Domain allowlist policy check (line 887)
- Network reachability and user existence via SDK (line 892)
- Granular error code mapping for clear failure modes (lines 896-907)
The error handling correctly distinguishes between CONNECTION_FAILED, USER_NOT_FOUND, and POLICY_DENIED, enabling appropriate UI feedback.
ee/packages/federation-matrix/src/errors/FederationValidationError.ts (1)
1-13: No changes needed—the local FederationValidationError is the single source of truth.The codebase defines and exports
FederationValidationErroronly from the local file (ee/packages/federation-matrix/src/errors/FederationValidationError.ts). Theindex.tsexport on line 7 is from the local file, not from@rocket.chat/federation-sdk. No duplication exists; this is a deliberate architectural choice indicated by the comment about avoiding a broken import chain from the SDK.Likely an incorrect or invalid review comment.
Comment on lines
+211
to
+228
| async validateFederatedUsers(usernames: string[]): Promise<void> { | ||
| const hasInvalidFederatedUsername = usernames.some((username) => !validateFederatedUsername(username)); | ||
| if (hasInvalidFederatedUsername) { | ||
| throw new FederationValidationError( | ||
| 'POLICY_DENIED', | ||
| `Invalid federated username format: ${usernames.filter((username) => !validateFederatedUsername(username)).join(', ')}. Federated usernames must follow the format @username:domain.com`, | ||
| ); | ||
| } | ||
|
|
||
| const federatedUsers = usernames.filter(validateFederatedUsername); | ||
| if (federatedUsers.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| for await (const username of federatedUsers) { | ||
| await federationSDK.validateOutboundUser(userIdSchema.parse(username)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Logic inconsistency: method rejects local usernames but includes filtering for them.
The method checks if any username is invalid federated format (line 212) and throws before reaching the filter (line 220). This creates confusion:
- Lines 212-218 reject the entire call if ANY username isn't federated format (including local usernames like
"localuser") - Line 220 filters to federated usernames - but this is unreachable if any local usernames exist
- The filter suggests intent to handle mixed inputs, but the validation prevents it
The past review comment flagged this issue as addressed in commit 82fa056, but the logic still rejects mixed inputs. For example, ["localuser", "@fed:remote.com"] will throw POLICY_DENIED with a confusing message stating "localuser" is an invalid federated format.
Recommended fix: Pre-filter to federated usernames at the start, matching the behavior of isFederationDomainAllowedForUsernames:
🔎 Proposed refactor to handle mixed inputs
async validateFederatedUsers(usernames: string[]): Promise<void> {
- const hasInvalidFederatedUsername = usernames.some((username) => !validateFederatedUsername(username));
- if (hasInvalidFederatedUsername) {
- throw new FederationValidationError(
- 'POLICY_DENIED',
- `Invalid federated username format: ${usernames.filter((username) => !validateFederatedUsername(username)).join(', ')}. Federated usernames must follow the format @username:domain.com`,
- );
- }
-
const federatedUsers = usernames.filter(validateFederatedUsername);
if (federatedUsers.length === 0) {
return;
}
for await (const username of federatedUsers) {
await federationSDK.validateOutboundUser(userIdSchema.parse(username));
}
}Alternatively, if the method should strictly validate that all inputs are federated format, update the method name (e.g., validateAllUsersFederated) and remove the filter on line 220.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 211 to
228, the method currently throws if ANY username is not a federated format, but
then also filters federated usernames — causing mixed inputs (e.g.,
["localuser", "@fed:remote.com"]) to be rejected instead of processing only
federated ones. Change the logic to pre-filter federated usernames first (const
federatedUsers = usernames.filter(validateFederatedUsername)); if
federatedUsers.length === 0 return; then iterate and validate only those
federatedUsers with
federationSDK.validateOutboundUser(userIdSchema.parse(username)); remove the
initial throw for invalid formats; alternatively, if strict-all-federated
behavior is intended, rename the method to indicate that (e.g.,
validateAllUsersFederated) and keep the current throw while removing the filter.
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.
Proposed changes (including videos or screenshots)
As per FB-25, this PR implements a pre-federation validation shield that blocks outbound federation actions (invites, DMs, and room creation involving remote users) unless the destination domain is allowed, reachable, and the target user exists.
Works along with RocketChat/homeserver#305.
This introduces a sequential validation layer composed of three checks that run before any federation transaction is executed.
1. Policy vetting check (allow list enforcement)
Validates whether the remote domain is authorized according to the organization's Federation Domain Allow List.
If the domain is not allowed, the server immediately returns:
federation-policy-deniedMessage returned: "Action Blocked. Communication with one of the domains in the list is restricted by your organization's security policy."
2. Domain existence check (DNS / reachability)
Ensures that the remote homeserver domain resolves and is reachable.
If DNS lookup fails or the server is unreachable, the server returns:
federation-connection-failedMessage: "Connection Failed. The server domain could not be reached or does not support federation."
3. User existence check (protocol lookup)
Queries the remote homeserver for the given user ID.
If the remote server responds with a 404 for the user, the server blocks the request with:
federation-user-not-foundMessage: "Invitation blocked. The specified user couldn’t be found on their homeserver."
Issue(s)
Test cases
POLICY VETTING (RC ALLOW LIST)
hs1, inviting user fromhs2@admin:hs2federation-policy-denied@admin:hs2@admin:hs2POLICY_DENIED; failing on continuefederation-policy-denied/invite/invite @admin:hs2federation-policy-deniedhs1, inviting user fromhs1@admin:hs1DOMAIN EXISTENCE
@admin:hs3federation-connection-failed@admin:hs3@admin:hs3CONNECTION_FAILED/invite/invite @admin:hs3federation-connection-failedUSER EXISTENCE
@admin2:hs1federation-user-not-found@admin2:hs1@admin2:hs1USER_NOT_FOUND, continue disabled/invite/invite @admin2:hs1federation-user-not-foundSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.