feat(federation): add improved errors messaging #37581
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,8 @@ import { Logger } from '@rocket.chat/logger'; | |
| import { Users, Subscriptions, Messages, Rooms, Settings } from '@rocket.chat/models'; | ||
| import emojione from 'emojione'; | ||
|
|
||
| import { isFederationDomainAllowed } from './api/middlewares/isFederationDomainAllowed'; | ||
| import { FederationValidationError } from './errors/FederationValidationError'; | ||
| import { toExternalMessageFormat, toExternalQuoteMessageFormat } from './helpers/message.parsers'; | ||
| import { MatrixMediaService } from './services/MatrixMediaService'; | ||
|
|
||
|
|
@@ -206,6 +208,25 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS | |
| this.processEDUPresence = (await Settings.getValueById<boolean>('Federation_Service_EDU_Process_Presence')) || false; | ||
| } | ||
|
|
||
| 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)); | ||
| } | ||
| } | ||
|
Comment on lines
+211
to
+228
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:
The past review comment flagged this issue as addressed in commit 82fa056, but the logic still rejects mixed inputs. For example, Recommended fix: Pre-filter to federated usernames at the start, matching the behavior of 🔎 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., 🤖 Prompt for AI Agents |
||
|
|
||
| async createRoom(room: IRoom, owner: IUser): Promise<{ room_id: string; event_id: string }> { | ||
| if (room.t !== 'c' && room.t !== 'p') { | ||
| throw new Error('Room is not a public or private room'); | ||
|
|
@@ -215,11 +236,8 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS | |
| const matrixUserId = userIdSchema.parse(`@${owner.username}:${this.serverName}`); | ||
| const roomName = room.name || room.fname || 'Untitled Room'; | ||
|
|
||
| const matrixRoomResult = await federationSDK.createRoom(matrixUserId, roomName, room.t === 'c' ? 'public' : 'invite'); | ||
|
|
||
| await Rooms.setAsFederated(room._id, { mrid: matrixRoomResult.room_id, origin: this.serverName }); | ||
|
|
||
| // Members are NOT invited here - invites are sent via beforeAddUserToRoom callback. | ||
|
|
@@ -863,24 +881,30 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS | |
| if (!homeserverUrl) { | ||
| return [matrixId, 'UNABLE_TO_VERIFY']; | ||
| } | ||
|
|
||
| try { | ||
| // check RC domain allowlist | ||
| if (!(await isFederationDomainAllowed([homeserverUrl]))) { | ||
| return [matrixId, 'POLICY_DENIED']; | ||
| } | ||
|
|
||
| // validate using homeserver (network + user existence) | ||
| await federationSDK.validateOutboundUser(userIdSchema.parse(matrixId)); | ||
|
|
||
| return [matrixId, 'VERIFIED']; | ||
| } catch (e) { | ||
| if (e && typeof e === 'object' && 'code' in e) { | ||
| const error = e as { code: string }; | ||
| if (error.code === 'CONNECTION_FAILED') { | ||
| return [matrixId, 'UNABLE_TO_VERIFY']; | ||
| } | ||
| if (error.code === 'USER_NOT_FOUND') { | ||
| return [matrixId, 'UNVERIFIED']; | ||
| } | ||
| if (error.code === 'POLICY_DENIED') { | ||
| return [matrixId, 'POLICY_DENIED']; | ||
| } | ||
| } | ||
| return [matrixId, 'UNABLE_TO_VERIFY']; | ||
| } | ||
| }), | ||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // Local copy to avoid broken import chain in homeserver's federation-sdk | ||
| export class FederationValidationError extends Error { | ||
| public error: string; | ||
|
|
||
| constructor( | ||
| public code: 'POLICY_DENIED' | 'CONNECTION_FAILED' | 'USER_NOT_FOUND', | ||
| public userMessage: string, | ||
| ) { | ||
| super(userMessage); | ||
| this.name = 'FederationValidationError'; | ||
| this.error = `federation-${code.toLowerCase().replace(/_/g, '-')}`; | ||
| } | ||
| } |
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
🤖 Prompt for AI Agents