From f8f406f2148696bb9f760c2e72a1628912abce6b Mon Sep 17 00:00:00 2001 From: Kush Kumar Date: Sun, 5 Apr 2026 08:06:05 +0000 Subject: [PATCH 1/3] fix: Implement avatar validation for federated users and add corresponding tests --- .../src/events/member.spec.ts | 146 ++++++++++++++++++ .../federation-matrix/src/events/member.ts | 23 ++- 2 files changed, 165 insertions(+), 4 deletions(-) create mode 100644 ee/packages/federation-matrix/src/events/member.spec.ts diff --git a/ee/packages/federation-matrix/src/events/member.spec.ts b/ee/packages/federation-matrix/src/events/member.spec.ts new file mode 100644 index 0000000000000..f448186365038 --- /dev/null +++ b/ee/packages/federation-matrix/src/events/member.spec.ts @@ -0,0 +1,146 @@ +import { Room, Upload } from '@rocket.chat/core-services'; +import { federationSDK } from '@rocket.chat/federation-sdk'; +import { Rooms, Subscriptions, Users } from '@rocket.chat/models'; + +import { member } from './member'; + +jest.mock('lodash.debounce', () => ({ + __esModule: true, + default: (fn: (...args: unknown[]) => unknown) => fn, +})); + +const onMembershipEvent = jest.fn(); + +jest.mock('@rocket.chat/federation-sdk', () => ({ + federationSDK: { + eventEmitterService: { + on: (...args: unknown[]) => onMembershipEvent(...args), + }, + queryProfile: jest.fn(), + downloadFromRemoteServer: jest.fn(), + getConfig: jest.fn().mockReturnValue('local.server'), + }, +})); + +jest.mock('@rocket.chat/models', () => ({ + Rooms: { + findOneFederatedByMrid: jest.fn(), + }, + Subscriptions: { + findOneByRoomIdAndUserId: jest.fn(), + }, + Users: { + findOneByUsername: jest.fn(), + setFederationAvatarUrlById: jest.fn(), + setName: jest.fn(), + }, +})); + +jest.mock('@rocket.chat/core-services', () => ({ + Room: { + updateDirectMessageRoomName: jest.fn(), + performAcceptRoomInvite: jest.fn(), + }, + Upload: { + setUserAvatar: jest.fn(), + resetUserAvatar: jest.fn(), + }, +})); + +describe('member avatar validation', () => { + const queryProfileMock = federationSDK.queryProfile as jest.MockedFunction; + const downloadFromRemoteServerMock = federationSDK.downloadFromRemoteServer as jest.MockedFunction< + typeof federationSDK.downloadFromRemoteServer + >; + const findOneByUsernameMock = Users.findOneByUsername as jest.MockedFunction; + const setFederationAvatarUrlByIdMock = Users.setFederationAvatarUrlById as jest.MockedFunction; + const findOneFederatedByMridMock = Rooms.findOneFederatedByMrid as jest.MockedFunction; + const findOneByRoomIdAndUserIdMock = Subscriptions.findOneByRoomIdAndUserId as jest.MockedFunction< + typeof Subscriptions.findOneByRoomIdAndUserId + >; + const setUserAvatarMock = Upload.setUserAvatar as jest.MockedFunction; + const resetUserAvatarMock = Upload.resetUserAvatar as jest.MockedFunction; + + beforeEach(() => { + jest.clearAllMocks(); + onMembershipEvent.mockClear(); + + member(); + }); + + function getHandler() { + const [, handler] = onMembershipEvent.mock.calls[0]; + return handler as ({ event }: { event: any }) => Promise; + } + + async function emitJoinEvent({ avatarUrl }: { avatarUrl?: string | null }) { + const handler = getHandler(); + + const event = { + room_id: '!room:remote.server', + state_key: '@alice:remote.server', + content: { + membership: 'join', + ...(avatarUrl !== undefined ? { avatar_url: avatarUrl } : {}), + }, + }; + + await handler({ event }); + } + + it('skips avatar download when current remote avatar matches stored federation avatar URL', async () => { + findOneByUsernameMock.mockResolvedValue({ + _id: 'u1', + username: '@alice:remote.server', + name: 'Alice', + federation: { avatarUrl: 'mxc://remote.server/same' }, + } as any); + findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); + findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); + queryProfileMock.mockResolvedValue({ avatar_url: 'mxc://remote.server/same' } as any); + + await emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' }); + + expect(downloadFromRemoteServerMock).not.toHaveBeenCalled(); + expect(setUserAvatarMock).not.toHaveBeenCalled(); + expect(resetUserAvatarMock).not.toHaveBeenCalled(); + expect(setFederationAvatarUrlByIdMock).not.toHaveBeenCalled(); + }); + + it('downloads and stores avatar when current remote avatar changed', async () => { + findOneByUsernameMock.mockResolvedValue({ + _id: 'u1', + username: '@alice:remote.server', + name: 'Alice', + federation: { avatarUrl: 'mxc://remote.server/old' }, + } as any); + findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); + findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); + queryProfileMock.mockResolvedValue({ avatar_url: 'mxc://remote.server/new' } as any); + downloadFromRemoteServerMock.mockResolvedValue(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0, 0, 0, 0])); + + await emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' }); + + expect(downloadFromRemoteServerMock).toHaveBeenCalledWith('remote.server', 'new'); + expect(setUserAvatarMock).toHaveBeenCalledTimes(1); + expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', 'mxc://remote.server/new'); + }); + + it('falls back to event payload when profile query fails', async () => { + findOneByUsernameMock.mockResolvedValue({ + _id: 'u1', + username: '@alice:remote.server', + name: 'Alice', + federation: { avatarUrl: 'mxc://remote.server/old' }, + } as any); + findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); + findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); + queryProfileMock.mockRejectedValue(new Error('profile unavailable')); + downloadFromRemoteServerMock.mockResolvedValue(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0, 0, 0, 0])); + + await emitJoinEvent({ avatarUrl: 'mxc://remote.server/fallback' }); + + expect(downloadFromRemoteServerMock).toHaveBeenCalledWith('remote.server', 'fallback'); + expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', 'mxc://remote.server/fallback'); + }); +}); diff --git a/ee/packages/federation-matrix/src/events/member.ts b/ee/packages/federation-matrix/src/events/member.ts index 00ed97bc6b5ed..2d1aaf0cff3c1 100644 --- a/ee/packages/federation-matrix/src/events/member.ts +++ b/ee/packages/federation-matrix/src/events/member.ts @@ -14,14 +14,25 @@ import { MatrixMediaService } from '../services/MatrixMediaService'; const logger = new Logger('federation-matrix:member'); +async function getCurrentFederatedAvatarUrl(userId: string, fallbackAvatarUrl: string | null): Promise { + try { + const profile = await federationSDK.queryProfile(userId); + + return profile?.avatar_url ?? null; + } catch (error) { + logger.warn({ err: error, userId, msg: 'Failed to query current federated profile avatar, falling back to event payload' }); + return fallbackAvatarUrl; + } +} + async function downloadAndSetAvatar(user: IUser, avatarUrl: string | null): Promise { try { // if no avatarUrl is provided, it means the user removed his avatar, so we need to set an empty avatar to remove the avatar from their side as well if (!avatarUrl) { await Upload.resetUserAvatar(user); + await Users.setFederationAvatarUrlById(user._id, ''); return; } - if (!avatarUrl?.startsWith('mxc://')) { return; } @@ -66,8 +77,8 @@ async function downloadAndSetAvatar(user: IUser, avatarUrl: string | null): Prom return; } - // TODO need to perform a validation to check if the user actually changed avatar await Upload.setUserAvatar(user, buffer, contentType, 'rest'); + await Users.setFederationAvatarUrlById(user._id, avatarUrl); } catch (error) { logger.error({ err: error, user: user.username, msg: `Error downloading/setting avatar for user` }); } @@ -297,8 +308,12 @@ async function handleJoin({ // handle avatar updates to membership events if (senderServerName !== federationSDK.getConfig('serverName')) { - // TODO if there is no avatar_url we may want to validate first if we should remove the user avatar because if may be dealing with an old join event, and the user may have changed their avatar since then, so we need to check if the avatar_url is different from the current one before removing it - void downloadAndSetAvatarDebounced(joiningUser._id, joiningUser, content.avatar_url || null); + const currentAvatarUrl = await getCurrentFederatedAvatarUrl(userId, content.avatar_url || null); + const storedAvatarUrl = joiningUser.federation?.avatarUrl || null; + + if (currentAvatarUrl !== storedAvatarUrl) { + void downloadAndSetAvatarDebounced(joiningUser._id, joiningUser, currentAvatarUrl); + } } // updates user name whenever we receive a join event, because Matrix sends a new join event with the updated display name whenever a user changes their display name From 334ccbce743ce27fd430a2f2a0704209db50de0c Mon Sep 17 00:00:00 2001 From: Kush Kumar Date: Sun, 5 Apr 2026 08:17:38 +0000 Subject: [PATCH 2/3] fix: Add README for avatar validation implementation for federated users --- .changeset/good-fireants-rush.md | 5 +++++ .../src/events/README-avatar-validation-39908.md | 0 2 files changed, 5 insertions(+) create mode 100644 .changeset/good-fireants-rush.md create mode 100644 ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md diff --git a/.changeset/good-fireants-rush.md b/.changeset/good-fireants-rush.md new file mode 100644 index 0000000000000..2e6446bec43c9 --- /dev/null +++ b/.changeset/good-fireants-rush.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/federation-matrix': patch +--- + +Implement avatar validation for federated users diff --git a/ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md b/ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md new file mode 100644 index 0000000000000..e69de29bb2d1d From 3b4389a4ddba33c6c9ee5498bd013fd78bb0caad Mon Sep 17 00:00:00 2001 From: Kush Kumar Date: Sun, 5 Apr 2026 08:31:13 +0000 Subject: [PATCH 3/3] fix: Enhance avatar URL handling and validation in federated member events --- .../events/README-avatar-validation-39908.md | 0 .../src/events/member.spec.ts | 47 +++++++++++++++++++ .../federation-matrix/src/events/member.ts | 27 ++++++++--- 3 files changed, 68 insertions(+), 6 deletions(-) delete mode 100644 ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md diff --git a/ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md b/ee/packages/federation-matrix/src/events/README-avatar-validation-39908.md deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/ee/packages/federation-matrix/src/events/member.spec.ts b/ee/packages/federation-matrix/src/events/member.spec.ts index f448186365038..e8e9e83407c5c 100644 --- a/ee/packages/federation-matrix/src/events/member.spec.ts +++ b/ee/packages/federation-matrix/src/events/member.spec.ts @@ -143,4 +143,51 @@ describe('member avatar validation', () => { expect(downloadFromRemoteServerMock).toHaveBeenCalledWith('remote.server', 'fallback'); expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', 'mxc://remote.server/fallback'); }); + + it('does not treat missing avatar_url as removal when profile query fails', async () => { + findOneByUsernameMock.mockResolvedValue({ + _id: 'u1', + username: '@alice:remote.server', + name: 'Alice', + federation: { avatarUrl: 'mxc://remote.server/old' }, + } as any); + findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); + findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); + queryProfileMock.mockRejectedValue(new Error('profile unavailable')); + + await emitJoinEvent({}); + + expect(downloadFromRemoteServerMock).not.toHaveBeenCalled(); + expect(setUserAvatarMock).not.toHaveBeenCalled(); + expect(resetUserAvatarMock).not.toHaveBeenCalled(); + expect(setFederationAvatarUrlByIdMock).not.toHaveBeenCalled(); + }); + + it('coalesces concurrent profile lookups for the same user', async () => { + findOneByUsernameMock.mockResolvedValue({ + _id: 'u1', + username: '@alice:remote.server', + name: 'Alice', + federation: { avatarUrl: 'mxc://remote.server/same' }, + } as any); + findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); + findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); + + let resolveProfile: ((value: { avatar_url: string }) => void) | undefined; + const pendingProfile = new Promise<{ avatar_url: string }>((resolve) => { + resolveProfile = resolve; + }); + + queryProfileMock.mockImplementation(() => pendingProfile as any); + + const firstJoin = emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' }); + const secondJoin = emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' }); + + resolveProfile?.({ avatar_url: 'mxc://remote.server/same' }); + + await Promise.all([firstJoin, secondJoin]); + + expect(queryProfileMock).toHaveBeenCalledTimes(1); + expect(downloadFromRemoteServerMock).not.toHaveBeenCalled(); + }); }); diff --git a/ee/packages/federation-matrix/src/events/member.ts b/ee/packages/federation-matrix/src/events/member.ts index 2d1aaf0cff3c1..77889d3f4cc49 100644 --- a/ee/packages/federation-matrix/src/events/member.ts +++ b/ee/packages/federation-matrix/src/events/member.ts @@ -14,14 +14,28 @@ import { MatrixMediaService } from '../services/MatrixMediaService'; const logger = new Logger('federation-matrix:member'); -async function getCurrentFederatedAvatarUrl(userId: string, fallbackAvatarUrl: string | null): Promise { - try { - const profile = await federationSDK.queryProfile(userId); +const inFlightFederatedAvatarLookupsByUserId = new Map>(); + +async function getCurrentFederatedAvatarUrl( + userId: string, + fallbackAvatarUrl: string | null | undefined, +): Promise { + let lookupPromise = inFlightFederatedAvatarLookupsByUserId.get(userId); + + if (!lookupPromise) { + lookupPromise = federationSDK.queryProfile(userId).then((profile) => profile?.avatar_url); + inFlightFederatedAvatarLookupsByUserId.set(userId, lookupPromise); + } - return profile?.avatar_url ?? null; + try { + return await lookupPromise; } catch (error) { logger.warn({ err: error, userId, msg: 'Failed to query current federated profile avatar, falling back to event payload' }); return fallbackAvatarUrl; + } finally { + if (inFlightFederatedAvatarLookupsByUserId.get(userId) === lookupPromise) { + inFlightFederatedAvatarLookupsByUserId.delete(userId); + } } } @@ -308,10 +322,11 @@ async function handleJoin({ // handle avatar updates to membership events if (senderServerName !== federationSDK.getConfig('serverName')) { - const currentAvatarUrl = await getCurrentFederatedAvatarUrl(userId, content.avatar_url || null); + const fallbackAvatarUrl = 'avatar_url' in content ? content.avatar_url ?? null : undefined; + const currentAvatarUrl = await getCurrentFederatedAvatarUrl(userId, fallbackAvatarUrl); const storedAvatarUrl = joiningUser.federation?.avatarUrl || null; - if (currentAvatarUrl !== storedAvatarUrl) { + if (currentAvatarUrl !== undefined && currentAvatarUrl !== storedAvatarUrl) { void downloadAndSetAvatarDebounced(joiningUser._id, joiningUser, currentAvatarUrl); } }