From 1c8e0df1867045f926b9ef2fc4c2fdf9452df323 Mon Sep 17 00:00:00 2001 From: Abhinav Kumar Date: Fri, 11 Apr 2025 13:50:37 +0530 Subject: [PATCH 1/3] fix: incoming webhooks sends messages by non team members in public channels under private teams Signed-off-by: Abhinav Kumar --- .../getRoomByNameOrIdWithOptionToJoin.ts | 4 +- .../end-to-end/api/incoming-integrations.ts | 267 +++++++++++++++++- 2 files changed, 268 insertions(+), 3 deletions(-) diff --git a/apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts b/apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts index 1dd803f8b13c5..e1aeabe1b46a5 100644 --- a/apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts +++ b/apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts @@ -1,8 +1,8 @@ +import { Room } from '@rocket.chat/core-services'; import type { IRoom, IUser, RoomType } from '@rocket.chat/core-typings'; import { Rooms, Users } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor'; -import { addUserToRoom } from './addUserToRoom'; import { isObject } from '../../../../lib/utils/isObject'; import { createDirectMessage } from '../../../../server/methods/createDirectMessage'; @@ -88,7 +88,7 @@ export const getRoomByNameOrIdWithOptionToJoin = async ({ // If the room type is channel and joinChannel has been passed, try to join them // if they can't join the room, this will error out! if (room.t === 'c' && joinChannel) { - await addUserToRoom(room._id, user); + await Room.join({ room, user }); } return room; diff --git a/apps/meteor/tests/end-to-end/api/incoming-integrations.ts b/apps/meteor/tests/end-to-end/api/incoming-integrations.ts index 3e8769adda08a..cfac6064f6f93 100644 --- a/apps/meteor/tests/end-to-end/api/incoming-integrations.ts +++ b/apps/meteor/tests/end-to-end/api/incoming-integrations.ts @@ -1,5 +1,6 @@ import type { Credentials } from '@rocket.chat/api-client'; -import type { IIntegration, IMessage, IRoom, IUser } from '@rocket.chat/core-typings'; +import { TEAM_TYPE } from '@rocket.chat/core-typings'; +import type { AtLeast, IIntegration, IMessage, IRoom, ITeam, IUser } from '@rocket.chat/core-typings'; import { Random } from '@rocket.chat/random'; import { assert, expect } from 'chai'; import { after, before, describe, it } from 'mocha'; @@ -8,6 +9,7 @@ import { getCredentials, api, request, credentials } from '../../data/api-data'; import { createIntegration, removeIntegration } from '../../data/integration.helper'; import { updatePermission } from '../../data/permissions.helper'; import { createRoom, deleteRoom } from '../../data/rooms.helper'; +import { createTeam, deleteTeam } from '../../data/teams.helper'; import { password } from '../../data/user'; import type { TestUser } from '../../data/users.helper'; import { createUser, deleteUser, login } from '../../data/users.helper'; @@ -811,4 +813,267 @@ describe('[Incoming Integrations]', () => { }); }); }); + + describe('Additional Tests for Message Delivery Permissions', () => { + let nonMemberUser: IUser; + let privateTeam: ITeam; + let publicChannelInPrivateTeam: IRoom; + let privateRoom: IRoom; + let publicRoom: IRoom; + let integration2: IIntegration; + let integration3: IIntegration; + let integration4: IIntegration; + + before(async () => { + nonMemberUser = await createUser({ username: `g_${Random.id()}` }); + privateTeam = await createTeam(credentials, `private.team.${Random.id()}`, TEAM_TYPE.PRIVATE); + + const [publicInPrivateResponse, privateRoomResponse, publicRoomResponse] = await Promise.all([ + createRoom({ + type: 'c', + name: `teamPrivate.publicChannel.${Random.id()}`, + credentials, + extraData: { + teamId: privateTeam._id, + }, + }), + createRoom({ + type: 'p', + name: `privateChannel.${Random.id()}`, + credentials, + }), + createRoom({ + type: 'c', + name: `publicChannel.${Random.id()}`, + credentials, + }), + ]); + + publicChannelInPrivateTeam = publicInPrivateResponse.body.channel; + privateRoom = privateRoomResponse.body.group; + publicRoom = publicRoomResponse.body.channel; + + await updatePermission('manage-incoming-integrations', ['admin']); + await request + .post(api('integrations.create')) + .set(credentials) + .send({ + type: 'webhook-incoming', + name: 'Incoming test 2 - Sending Messages', + enabled: true, + alias: 'Incoming test 2 - Sending Messages', + username: nonMemberUser.username as string, + scriptEnabled: false, + overrideDestinationChannelEnabled: false, + channel: `#${privateRoom.fname}`, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('integration').and.to.be.an('object'); + integration2 = res.body.integration; + }); + + await request + .post(api('integrations.create')) + .set(credentials) + .send({ + type: 'webhook-incoming', + name: 'Incoming test 3 - Sending Messages', + enabled: true, + alias: 'Incoming test 3 - Sending Messages', + username: nonMemberUser.username as string, + scriptEnabled: false, + overrideDestinationChannelEnabled: false, + channel: `#${publicChannelInPrivateTeam.fname}`, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('integration').and.to.be.an('object'); + integration3 = res.body.integration; + }); + await request + .post(api('integrations.create')) + .set(credentials) + .send({ + type: 'webhook-incoming', + name: 'Incoming test 4 - Sending Messages', + enabled: true, + alias: 'Incoming test 4 - Sending Messages', + username: nonMemberUser.username as string, + scriptEnabled: false, + overrideDestinationChannelEnabled: false, + channel: `#${publicRoom.fname}`, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('integration').and.to.be.an('object'); + integration4 = res.body.integration; + }); + }); + + after(async () => { + await Promise.all( + [publicRoom, privateRoom, publicChannelInPrivateTeam].map((room) => deleteRoom({ type: room.t as 'c' | 'p', roomId: room._id })), + ); + await deleteTeam(credentials, privateTeam.name); + await deleteUser(nonMemberUser); + await Promise.all([ + ...[integration2, integration3, integration4].map((integration) => + request.post(api('integrations.remove')).set(credentials).send({ + integrationId: integration._id, + type: integration.type, + }), + ), + updatePermission('manage-incoming-integrations', ['admin']), + ]); + }); + + it('should not send a message to a private rooms on behalf of a non member', async () => { + const successfulMesssage = `Message sent successfully at #${Random.id()}`; + await request + .post(`/hooks/${integration2._id}/${integration2.token}`) + .send({ + text: successfulMesssage, + }) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('error', 'error-not-allowed'); + }); + await request + .get(api('groups.messages')) + .set(credentials) + .query({ + roomId: privateRoom._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('messages').and.to.be.an('array'); + expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).not.to.be.true; + }); + }); + + it('should not add non member to private rooms when sending message', async () => { + const successfulMesssage = `Message sent successfully at #${Random.id()}`; + await request + .post(`/hooks/${integration2._id}/${integration2.token}`) + .send({ + text: successfulMesssage, + }) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('error', 'error-not-allowed'); + }); + await request + .get(api('groups.members')) + .set(credentials) + .query({ + roomId: privateRoom._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('members').and.to.be.an('array'); + expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).not.to.be.true; + }); + }); + + it('should not send a message to public channel of a private team on behalf of a non team member', async () => { + const successfulMesssage = `Message sent successfully at #${Random.id()}`; + await request + .post(`/hooks/${integration3._id}/${integration3.token}`) + .send({ + text: successfulMesssage, + }) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('error', 'error-not-allowed'); + }); + await request + .get(api('channels.messages')) + .set(credentials) + .query({ + roomId: publicChannelInPrivateTeam._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('messages').and.to.be.an('array'); + expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).not.to.be.true; + }); + }); + + it('should not add non team member to the public channel in a private team when sending message', async () => { + const successfulMesssage = `Message sent successfully at #${Random.id()}`; + await request + .post(`/hooks/${integration3._id}/${integration3.token}`) + .send({ + text: successfulMesssage, + }) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('error', 'error-not-allowed'); + }); + await request + .get(api('channels.members')) + .set(credentials) + .query({ + roomId: publicChannelInPrivateTeam._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('members').and.to.be.an('array'); + expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).not.to.be.true; + }); + }); + + it('should send messages from non-members to public rooms and add them as room members', async () => { + const successfulMesssage = `Message sent successfully at #${Random.id()}`; + await request + .post(`/hooks/${integration4._id}/${integration4.token}`) + .send({ + text: successfulMesssage, + }) + .expect(200); + + await request + .get(api('channels.messages')) + .set(credentials) + .query({ + roomId: publicRoom._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('messages').and.to.be.an('array'); + expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).to.be.true; + }); + + await request + .get(api('channels.members')) + .set(credentials) + .query({ + roomId: publicRoom._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('members').and.to.be.an('array'); + expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).to.be.true; + }); + }); + }); }); From dee3067e36118707fec04f87de44230b860a7a9c Mon Sep 17 00:00:00 2001 From: Abhinav Kumar Date: Fri, 11 Apr 2025 13:53:00 +0530 Subject: [PATCH 2/3] added changeset Signed-off-by: Abhinav Kumar --- .changeset/healthy-insects-cheer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/healthy-insects-cheer.md diff --git a/.changeset/healthy-insects-cheer.md b/.changeset/healthy-insects-cheer.md new file mode 100644 index 0000000000000..3c8446154963c --- /dev/null +++ b/.changeset/healthy-insects-cheer.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes an issue where the incoming webhooks integration allowed messages to be sent to public channels under private teams by users who were not members of the team. From 407a17ec3bb99e5327abe8a9f28866a9b79ae1bc Mon Sep 17 00:00:00 2001 From: Abhinav Kumar Date: Wed, 16 Apr 2025 12:21:16 +0530 Subject: [PATCH 3/3] minor changes Signed-off-by: Abhinav Kumar --- .../meteor/app/lib/server/functions/addUserToRoom.ts | 4 ++++ .../tests/end-to-end/api/incoming-integrations.ts | 12 ++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/apps/meteor/app/lib/server/functions/addUserToRoom.ts b/apps/meteor/app/lib/server/functions/addUserToRoom.ts index 2997acaf5924c..a97293e783577 100644 --- a/apps/meteor/app/lib/server/functions/addUserToRoom.ts +++ b/apps/meteor/app/lib/server/functions/addUserToRoom.ts @@ -13,6 +13,10 @@ import { settings } from '../../../settings/server'; import { getDefaultSubscriptionPref } from '../../../utils/lib/getDefaultSubscriptionPref'; import { notifyOnRoomChangedById, notifyOnSubscriptionChangedById } from '../lib/notifyListener'; +/** + * This function adds user to the given room. + * Caution - It does not validates if the user has permission to join room + */ export const addUserToRoom = async function ( rid: string, user: Pick | string, diff --git a/apps/meteor/tests/end-to-end/api/incoming-integrations.ts b/apps/meteor/tests/end-to-end/api/incoming-integrations.ts index cfac6064f6f93..34b459dcdf931 100644 --- a/apps/meteor/tests/end-to-end/api/incoming-integrations.ts +++ b/apps/meteor/tests/end-to-end/api/incoming-integrations.ts @@ -956,7 +956,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('messages').and.to.be.an('array'); - expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).not.to.be.true; + expect((res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).to.be.undefined; }); }); @@ -982,7 +982,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('members').and.to.be.an('array'); - expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).not.to.be.true; + expect((res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).to.be.undefined; }); }); @@ -1008,7 +1008,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('messages').and.to.be.an('array'); - expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).not.to.be.true; + expect((res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).to.be.undefined; }); }); @@ -1034,7 +1034,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('members').and.to.be.an('array'); - expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).not.to.be.true; + expect((res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).to.be.undefined; }); }); @@ -1058,7 +1058,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('messages').and.to.be.an('array'); - expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).to.be.true; + expect((res.body.messages as IMessage[]).find((m) => m.msg === successfulMesssage)).not.to.be.undefined; }); await request @@ -1072,7 +1072,7 @@ describe('[Incoming Integrations]', () => { .expect((res) => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('members').and.to.be.an('array'); - expect(!!(res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).to.be.true; + expect((res.body.members as AtLeast[]).find((m) => m._id === nonMemberUser._id)).not.to.be.undefined; }); }); });