Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion apps/meteor/ee/server/api/federation.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,51 @@
import type { IFederationMatrixService } from '@rocket.chat/core-services';
import { Logger } from '@rocket.chat/logger';
import { ajv } from '@rocket.chat/rest-typings';
import type express from 'express';
import { WebApp } from 'meteor/webapp';

import { API } from '../../../app/api/server';
import { isRunningMs } from '../../../server/lib/isRunningMs';

const logger = new Logger('FederationRoutes');

export async function registerFederationRoutes(federationService: IFederationMatrixService): Promise<void> {
let federationService: IFederationMatrixService | undefined;
API.v1.get(
'/federation/matrixIds.verify',
{
authRequired: true,
query: ajv.compile<{
matrixIds: string[];
}>({
type: 'object',
properties: {
matrixIds: { type: 'array', items: { type: 'string' } },
},
}),
response: {
200: ajv.compile<{
results: { [key: string]: string };
}>({
type: 'object',
properties: {
results: { type: 'object', additionalProperties: { type: 'string' } },
},
}),
},
},
async function () {
const { matrixIds } = this.queryParams;
if (!federationService) {
throw new Error('Federation service not registered');
}
return API.v1.success({
results: await federationService.verifyMatrixIds(matrixIds),
});
Comment on lines +17 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make matrixIds mandatory in the query schema

Right now the AJV schema doesn’t mark matrixIds as required, so a request without that parameter is accepted and matrixIds is undefined. The handler then calls federationService.verifyMatrixIds(matrixIds), which blows up at runtime because it tries to iterate over undefined. Please make the field required (and consider rejecting unexpected extras) so we respond with a 400 instead of a 500:

 		query: ajv.compile<{
 			matrixIds: string[];
 		}>({
 			type: 'object',
 			properties: {
 				matrixIds: { type: 'array', items: { type: 'string' } },
 			},
+			required: ['matrixIds'],
+			additionalProperties: false,
 		}),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
query: ajv.compile<{
matrixIds: string[];
}>({
type: 'object',
properties: {
matrixIds: { type: 'array', items: { type: 'string' } },
},
}),
response: {
200: ajv.compile<{
results: { [key: string]: string };
}>({
type: 'object',
properties: {
results: { type: 'object', additionalProperties: { type: 'string' } },
},
}),
},
},
async function () {
const { matrixIds } = this.queryParams;
if (!federationService) {
throw new Error('Federation service not registered');
}
return API.v1.success({
results: await federationService.verifyMatrixIds(matrixIds),
});
query: ajv.compile<{
matrixIds: string[];
}>({
type: 'object',
properties: {
matrixIds: { type: 'array', items: { type: 'string' } },
},
required: ['matrixIds'],
additionalProperties: false,
}),
🤖 Prompt for AI Agents
In apps/meteor/ee/server/api/federation.ts around lines 17 to 43, the AJV query
schema does not mark matrixIds as required so matrixIds can be undefined at
runtime; update the query schema to include required: ['matrixIds'], tighten the
matrixIds property to be an array of strings (optionally with minItems: 1 if
empty should be rejected), and set additionalProperties: false on the query
object to reject unexpected extras; this ensures AJV returns a 400 for
missing/extra params and prevents passing undefined into
federationService.verifyMatrixIds.

},
);

export async function registerFederationRoutes(f: IFederationMatrixService): Promise<void> {
federationService = f;
if (isRunningMs()) {
return;
}
Expand Down
47 changes: 47 additions & 0 deletions ee/packages/federation-matrix/src/FederationMatrix.ts
Original file line number Diff line number Diff line change
Expand Up @@ -944,4 +944,51 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS

void this.homeserverServices.edu.sendTypingNotification(room.federation.mrid, userMui, isTyping);
}

async verifyMatrixIds(matrixIds: string[]): Promise<{ [key: string]: string }> {
const results = Object.fromEntries(
await Promise.all(
matrixIds.map(async (matrixId) => {
// Split only on the first ':' (after the leading '@') so we keep any port in the homeserver
const separatorIndex = matrixId.indexOf(':', 1);
if (separatorIndex === -1) {
return [matrixId, 'UNABLE_TO_VERIFY'];
}
const userId = matrixId.slice(0, separatorIndex);
const homeserverUrl = matrixId.slice(separatorIndex + 1);

if (homeserverUrl === this.serverName) {
const user = await Users.findOneByUsername(userId.slice(1));
return [matrixId, user ? 'VERIFIED' : 'UNVERIFIED'];
}

if (!homeserverUrl) {
return [matrixId, 'UNABLE_TO_VERIFY'];
}
try {
const result = await this.homeserverServices.request.get<
| {
avatar_url: string;
displayname: string;
}
| {
errcode: string;
error: string;
}
>(homeserverUrl, `/_matrix/federation/v1/query/profile`, { user_id: matrixId });

if ('errcode' in result && result.errcode === 'M_NOT_FOUND') {
return [matrixId, 'UNVERIFIED'];
}

return [matrixId, 'VERIFIED'];
} catch (e) {
Comment on lines +980 to +985
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Do not mark errored Matrix profile queries as VERIFIED

Any response that still contains an errcode (e.g., M_FORBIDDEN, M_UNAUTHORIZED, M_LIMIT_EXCEEDED) currently falls through this branch and is treated as VERIFIED. That gives a false positive: a homeserver telling us it could not supply the profile is strong evidence that we failed to verify the ID. Please treat every non-M_NOT_FOUND errcode as UNABLE_TO_VERIFY instead of VERIFIED.

-						if ('errcode' in result && result.errcode === 'M_NOT_FOUND') {
-							return [matrixId, 'UNVERIFIED'];
-						}
-
-						return [matrixId, 'VERIFIED'];
+						if ('errcode' in result) {
+							if (result.errcode === 'M_NOT_FOUND') {
+								return [matrixId, 'UNVERIFIED'];
+							}
+
+							return [matrixId, 'UNABLE_TO_VERIFY'];
+						}

Committable suggestion skipped: line range outside the PR's diff.

return [matrixId, 'UNABLE_TO_VERIFY'];
}
}),
),
);

return results;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ export interface IFederationMatrixService {
): Promise<void>;
inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: IUser): Promise<void>;
notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>;
verifyMatrixIds(matrixIds: string[]): Promise<{ [key: string]: string }>;
}
Loading