From 4c29c350adf4c7deeb8eff0c7dd918b5d6175792 Mon Sep 17 00:00:00 2001 From: Julio Araujo Date: Tue, 7 Jul 2026 13:00:42 +0200 Subject: [PATCH 1/4] fix: team conversion permissions checked incorrectly --- apps/meteor/server/api/v1/channels.ts | 10 ++-- apps/meteor/server/services/team/service.ts | 4 ++ apps/meteor/tests/end-to-end/api/channels.ts | 23 ++++++++ apps/meteor/tests/end-to-end/api/teams.ts | 58 ++++++++++++++++++++ 4 files changed, 90 insertions(+), 5 deletions(-) diff --git a/apps/meteor/server/api/v1/channels.ts b/apps/meteor/server/api/v1/channels.ts index 56484cc598b3c..5e84a6b08271f 100644 --- a/apps/meteor/server/api/v1/channels.ts +++ b/apps/meteor/server/api/v1/channels.ts @@ -46,7 +46,7 @@ import { executeUnarchiveRoom } from '../../../app/lib/server/methods/unarchiveR import { getUserMentionsByChannel } from '../../../app/mentions/server/methods/getUserMentionsByChannel'; import { settings } from '../../../app/settings/server'; import { normalizeMessagesForUser } from '../../../app/utils/server/lib/normalizeMessagesForUser'; -import { hasPermissionAsync } from '../../lib/authorization/hasPermission'; +import { hasAllPermissionAsync, hasPermissionAsync } from '../../lib/authorization/hasPermission'; import { eraseRoom } from '../../lib/eraseRoom'; import { findUsersOfRoom } from '../../lib/findUsersOfRoom'; import { openRoom } from '../../lib/openRoom'; @@ -522,10 +522,6 @@ API.v1.addRoute( return API.v1.failure('The parameter "channelId" or "channelName" is required'); } - if (channelId && !(await hasPermissionAsync(this.userId, 'edit-room', channelId))) { - return API.v1.forbidden(); - } - const room = await findChannelByIdOrName({ params: channelId !== undefined ? { roomId: channelId } : { roomName: channelName }, userId: this.userId, @@ -535,6 +531,10 @@ API.v1.addRoute( return API.v1.failure('Channel not found'); } + if (!(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room._id))) { + return API.v1.forbidden(); + } + const subscriptions = await Subscriptions.findByRoomId(room._id, { projection: { 'u._id': 1 }, }); diff --git a/apps/meteor/server/services/team/service.ts b/apps/meteor/server/services/team/service.ts index 551e264d0e968..d37810d91b7e7 100644 --- a/apps/meteor/server/services/team/service.ts +++ b/apps/meteor/server/services/team/service.ts @@ -40,6 +40,10 @@ export class TeamService extends ServiceClassInternal implements ITeamService { protected name = 'team'; async create(uid: string, { team, room = { name: team.name, extraData: {} }, members, owner }: ITeamCreateParams): Promise { + if (room.id && !(await Authorization.hasAllPermission(uid, ['create-team', 'edit-room'], room.id))) { + throw new Error('error-no-owner-channel'); + } + if (!(await checkUsernameAvailability(team.name))) { throw new Error('team-name-already-exists'); } diff --git a/apps/meteor/tests/end-to-end/api/channels.ts b/apps/meteor/tests/end-to-end/api/channels.ts index 16dbbd836f2b4..0e3cb33d79700 100644 --- a/apps/meteor/tests/end-to-end/api/channels.ts +++ b/apps/meteor/tests/end-to-end/api/channels.ts @@ -3991,6 +3991,29 @@ describe('[Channels]', () => { }); }); + it('should return 403 when a user without edit-room permission on the channel tries to convert it to a team using channelName', async () => { + await updatePermission('create-team', ['admin', 'user']); + await updatePermission('edit-room', ['admin', 'owner', 'moderator']); + + const outsiderChannel = (await createRoom({ type: 'c', name: `channel.convertToTeam.outsider.test.${Date.now()}` })).body.channel; + const outsiderUser = await createUser(); + const outsiderCredentials = await login(outsiderUser.username, password); + + try { + await request + .post(api('channels.convertToTeam')) + .set(outsiderCredentials) + .send({ channelName: outsiderChannel.name }) + .expect(403) + .expect((res) => { + expect(res.body).to.have.a.property('success', false); + }); + } finally { + await deleteRoom({ type: 'c', roomId: outsiderChannel._id }); + await deleteUser(outsiderUser); + } + }); + it(`should return an error when the channel's name and id are sent as parameter`, (done) => { void request .post(api('channels.convertToTeam')) diff --git a/apps/meteor/tests/end-to-end/api/teams.ts b/apps/meteor/tests/end-to-end/api/teams.ts index 35ddbaae0b5d9..09de8ac9e2d46 100644 --- a/apps/meteor/tests/end-to-end/api/teams.ts +++ b/apps/meteor/tests/end-to-end/api/teams.ts @@ -249,6 +249,64 @@ describe('[Teams]', () => { }); }); }); + + describe('/teams.create - existing room ownership check', () => { + let roomOwner: TestUser; + let roomOwnerCredentials: Credentials; + let attacker: TestUser; + let attackerCredentials: Credentials; + let targetRoom: IRoom; + const teamName = `test-team-hijack-${Date.now()}`; + + before(async () => { + [roomOwner, attacker] = await Promise.all([createUser(), createUser()]); + [roomOwnerCredentials, attackerCredentials] = await Promise.all([ + login(roomOwner.username, password), + login(attacker.username, password), + ]); + targetRoom = (await createRoom({ type: 'c', name: `test-room-hijack-${Date.now()}`, credentials: roomOwnerCredentials })).body + .channel; + }); + + before(() => updatePermission('create-team', ['admin', 'user'])); + + after(async () => { + await Promise.all([ + deleteRoom({ type: 'c', roomId: targetRoom._id }), + deleteUser(roomOwner), + deleteUser(attacker), + updatePermission('create-team', ['admin', 'user']), + ]); + }); + + it('should not allow a user with no ownership/moderation of a room to hijack it into a new team by passing room.id', async () => { + await request + .post(api('teams.create')) + .set(attackerCredentials) + .send({ + name: teamName, + type: 0, + room: { id: targetRoom._id }, + }) + .expect('Content-Type', 'application/json') + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error', 'error-no-owner-channel'); + }); + + await request + .get(api('channels.info')) + .set(credentials) + .query({ roomId: targetRoom._id }) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body.channel).to.not.have.property('teamId'); + expect(res.body.channel).to.not.have.property('teamMain'); + }); + }); + }); }); describe('/teams.convertToChannel', () => { From 3a4fe65ddb0f6a9313881e2da76d587322306ad9 Mon Sep 17 00:00:00 2001 From: Julio Araujo Date: Tue, 7 Jul 2026 13:45:04 +0200 Subject: [PATCH 2/4] Add changeset --- .changeset/quiet-teams-guard.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quiet-teams-guard.md diff --git a/.changeset/quiet-teams-guard.md b/.changeset/quiet-teams-guard.md new file mode 100644 index 0000000000000..ac9a8af178b0a --- /dev/null +++ b/.changeset/quiet-teams-guard.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Ensures room permission checks are applied consistently regardless of how the room is identified when converting a channel to a team or creating a team from an existing room From d6e00e22fa6944082a969d4e7e2dd2b79d74d312 Mon Sep 17 00:00:00 2001 From: Julio Araujo Date: Tue, 7 Jul 2026 17:43:21 +0200 Subject: [PATCH 3/4] Change tests --- apps/meteor/tests/end-to-end/api/channels.ts | 29 ++++++++++++-------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/apps/meteor/tests/end-to-end/api/channels.ts b/apps/meteor/tests/end-to-end/api/channels.ts index 0e3cb33d79700..363f18886aa61 100644 --- a/apps/meteor/tests/end-to-end/api/channels.ts +++ b/apps/meteor/tests/end-to-end/api/channels.ts @@ -3991,15 +3991,25 @@ describe('[Channels]', () => { }); }); - it('should return 403 when a user without edit-room permission on the channel tries to convert it to a team using channelName', async () => { - await updatePermission('create-team', ['admin', 'user']); - await updatePermission('edit-room', ['admin', 'owner', 'moderator']); + describe('when a user without edit-room permission on the channel tries to convert it to a team', () => { + let outsiderChannel: IRoom; + let outsiderUser: TestUser; + let outsiderCredentials: Credentials; + + before(async () => { + await updatePermission('create-team', ['admin', 'user']); + await updatePermission('edit-room', ['admin', 'owner', 'moderator']); + + outsiderChannel = (await createRoom({ type: 'c', name: `channel.convertToTeam.outsider.test.${Date.now()}` })).body.channel; + outsiderUser = await createUser(); + outsiderCredentials = await login(outsiderUser.username, password); + }); - const outsiderChannel = (await createRoom({ type: 'c', name: `channel.convertToTeam.outsider.test.${Date.now()}` })).body.channel; - const outsiderUser = await createUser(); - const outsiderCredentials = await login(outsiderUser.username, password); + after(async () => { + await Promise.all([deleteRoom({ type: 'c', roomId: outsiderChannel._id }), deleteUser(outsiderUser)]); + }); - try { + it('should return 403 when using channelName', async () => { await request .post(api('channels.convertToTeam')) .set(outsiderCredentials) @@ -4008,10 +4018,7 @@ describe('[Channels]', () => { .expect((res) => { expect(res.body).to.have.a.property('success', false); }); - } finally { - await deleteRoom({ type: 'c', roomId: outsiderChannel._id }); - await deleteUser(outsiderUser); - } + }); }); it(`should return an error when the channel's name and id are sent as parameter`, (done) => { From 405bb7dd85d90ba05a334577b8002c78ab38f13e Mon Sep 17 00:00:00 2001 From: Julio Araujo Date: Tue, 7 Jul 2026 18:07:22 +0200 Subject: [PATCH 4/4] Move the checks to the API layer --- apps/meteor/server/api/v1/teams.ts | 7 ++++++- apps/meteor/server/services/team/service.ts | 4 ---- apps/meteor/tests/end-to-end/api/teams.ts | 3 +-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/meteor/server/api/v1/teams.ts b/apps/meteor/server/api/v1/teams.ts index 2dc46fb77ad30..8fbb73137db1a 100644 --- a/apps/meteor/server/api/v1/teams.ts +++ b/apps/meteor/server/api/v1/teams.ts @@ -30,7 +30,7 @@ import { escapeRegExp } from '@rocket.chat/string-helpers'; import { canAccessRoomAsync } from '../../../app/authorization/server'; import { settings } from '../../../app/settings/server'; -import { hasPermissionAsync, hasAtLeastOnePermissionAsync } from '../../lib/authorization/hasPermission'; +import { hasPermissionAsync, hasAtLeastOnePermissionAsync, hasAllPermissionAsync } from '../../lib/authorization/hasPermission'; import { eraseRoom } from '../../lib/eraseRoom'; import { removeUserFromRoom } from '../../lib/rooms/removeUserFromRoom'; import type { ExtractRoutesFromAPI } from '../ApiClass'; @@ -125,11 +125,16 @@ const teamsEndpoints = API.v1 }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, }, }, async function action() { const { name, type, members, room, owner } = this.bodyParams; + if (room?.id && !(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room.id))) { + return API.v1.forbidden(); + } + const team = await Team.create(this.userId, { team: { name, diff --git a/apps/meteor/server/services/team/service.ts b/apps/meteor/server/services/team/service.ts index d37810d91b7e7..551e264d0e968 100644 --- a/apps/meteor/server/services/team/service.ts +++ b/apps/meteor/server/services/team/service.ts @@ -40,10 +40,6 @@ export class TeamService extends ServiceClassInternal implements ITeamService { protected name = 'team'; async create(uid: string, { team, room = { name: team.name, extraData: {} }, members, owner }: ITeamCreateParams): Promise { - if (room.id && !(await Authorization.hasAllPermission(uid, ['create-team', 'edit-room'], room.id))) { - throw new Error('error-no-owner-channel'); - } - if (!(await checkUsernameAvailability(team.name))) { throw new Error('team-name-already-exists'); } diff --git a/apps/meteor/tests/end-to-end/api/teams.ts b/apps/meteor/tests/end-to-end/api/teams.ts index 09de8ac9e2d46..d99f2817d28e6 100644 --- a/apps/meteor/tests/end-to-end/api/teams.ts +++ b/apps/meteor/tests/end-to-end/api/teams.ts @@ -289,10 +289,9 @@ describe('[Teams]', () => { room: { id: targetRoom._id }, }) .expect('Content-Type', 'application/json') - .expect(400) + .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'error-no-owner-channel'); }); await request