diff --git a/.changeset/lovely-bats-buy.md b/.changeset/lovely-bats-buy.md new file mode 100644 index 0000000000000..eacb88108a0f7 --- /dev/null +++ b/.changeset/lovely-bats-buy.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates) diff --git a/apps/meteor/app/threads/server/methods/getThreadMessages.ts b/apps/meteor/app/threads/server/methods/getThreadMessages.ts index ab5e8a5e3f00d..67d8019af6f35 100644 --- a/apps/meteor/app/threads/server/methods/getThreadMessages.ts +++ b/apps/meteor/app/threads/server/methods/getThreadMessages.ts @@ -31,6 +31,10 @@ Meteor.methods({ }); } + if (typeof tmid !== 'string') { + throw new Meteor.Error('error-invalid-message', 'Invalid message', { method: 'getThreadMessages' }); + } + const thread = await Messages.findOneById(tmid); if (!thread) { return []; @@ -48,15 +52,15 @@ Meteor.methods({ } await callbacks.run('beforeReadMessages', thread.rid, user._id); - await readThread({ userId: user._id, rid: thread.rid, tmid }); + await readThread({ userId: user._id, rid: thread.rid, tmid: thread._id }); - const result = await Messages.findVisibleThreadByThreadId(tmid, { + const result = await Messages.findVisibleThreadByThreadId(thread._id, { ...(skip && { skip }), ...(limit && { limit }), sort: { ts: -1 }, }).toArray(); - callbacks.runAsync('afterReadMessages', room, { uid: user._id, tmid }); + callbacks.runAsync('afterReadMessages', room, { uid: user._id, tmid: thread._id }); return [thread, ...result]; }, diff --git a/apps/meteor/app/threads/server/methods/getThreadsList.ts b/apps/meteor/app/threads/server/methods/getThreadsList.ts index c51449ff2d4d3..cd31eda648548 100644 --- a/apps/meteor/app/threads/server/methods/getThreadsList.ts +++ b/apps/meteor/app/threads/server/methods/getThreadsList.ts @@ -27,6 +27,10 @@ Meteor.methods({ throw new Meteor.Error('error-not-allowed', 'Threads Disabled', { method: 'getThreadsList' }); } + if (typeof rid !== 'string') { + throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'getThreadsList' }); + } + const user = await Meteor.userAsync(); const room = await Rooms.findOneById(rid); @@ -34,6 +38,6 @@ Meteor.methods({ throw new Meteor.Error('error-not-allowed', 'Not Allowed', { method: 'getThreadsList' }); } - return Messages.findThreadsByRoomId(rid, skip, limit).toArray(); + return Messages.findThreadsByRoomId(room._id, skip, limit).toArray(); }, }); diff --git a/apps/meteor/server/modules/notifications/notifications.module.ts b/apps/meteor/server/modules/notifications/notifications.module.ts index 8f051d3f1e264..eb636d22d7ac9 100644 --- a/apps/meteor/server/modules/notifications/notifications.module.ts +++ b/apps/meteor/server/modules/notifications/notifications.module.ts @@ -240,7 +240,11 @@ export class NotificationsModule { ...args: [{ action: string; params: { callId: string; uid: string; rid: string } }] | [IUserDataEvent] ) { const [roomId, e] = eventName.split('/') as [string, 'video-conference' | 'userData']; - if (this.userId && (await Subscriptions.countByRoomIdAndUserId(roomId, this.userId)) > 0) { + if ( + this.userId && + ['video-conference', 'userData'].includes(e) && + (await Subscriptions.countByRoomIdAndUserId(roomId, this.userId)) > 0 + ) { const subscriptions: ISubscription[] = await Subscriptions.findByRoomIdAndNotUserId(roomId, this.userId, { projection: { 'u._id': 1, '_id': 0 }, }).toArray(); diff --git a/apps/meteor/tests/unit/server/modules/notifications/notifications.module.spec.ts b/apps/meteor/tests/unit/server/modules/notifications/notifications.module.spec.ts index dd44c53dfa57f..61a42846071ff 100644 --- a/apps/meteor/tests/unit/server/modules/notifications/notifications.module.spec.ts +++ b/apps/meteor/tests/unit/server/modules/notifications/notifications.module.spec.ts @@ -19,61 +19,135 @@ class TestStreamer extends Streamer { } const validateActionStub = sinon.stub(); const processSerializedSignalStub = sinon.stub(); +const countByRoomIdAndUserIdStub = sinon.stub(); +const findSubscriptionsExcludingUserStub = sinon.stub(); const { NotificationsModule } = proxyquire.noCallThru().load('../../../../../server/modules/notifications/notifications.module', { '@rocket.chat/core-services': { VideoConf: { validateAction: validateActionStub }, MediaCall: { processSerializedSignal: processSerializedSignalStub }, }, + '@rocket.chat/models': { + Subscriptions: { + countByRoomIdAndUserId: countByRoomIdAndUserIdStub, + findByRoomIdAndNotUserId: findSubscriptionsExcludingUserStub, + }, + Rooms: {}, + Users: {}, + }, }); -describe('NotificationsModule notify-user allowWrite', () => { +describe('NotificationsModule', () => { let notifications: any; - // `isWriteAllowed` is public on the concrete Streamer class but not on the IStreamer - // interface that `streamUser` is typed as, so cast to reach it. - const writeAllowed = (eventName: string, ...args: unknown[]) => - (notifications.streamUser as unknown as Streamer<'notify-user'>).isWriteAllowed({ userId: 'userId' } as any, eventName, args); - beforeEach(() => { - validateActionStub.reset(); - validateActionStub.resolves(true); - processSerializedSignalStub.reset(); - processSerializedSignalStub.resolves(undefined); - notifications = new NotificationsModule(TestStreamer as any); notifications.configure(); }); - afterEach(() => { - Object.keys(StreamerCentral.instances).forEach((name) => delete StreamerCentral.instances[name]); - }); + describe('notify-user allowWrite', () => { + // `isWriteAllowed` is public on the concrete Streamer class but not on the IStreamer + // interface that `streamUser` is typed as, so cast to reach it. + const writeAllowed = (eventName: string, ...args: unknown[]) => + (notifications.streamUser as unknown as Streamer<'notify-user'>).isWriteAllowed({ userId: 'userId' } as any, eventName, args); + + beforeEach(() => { + validateActionStub.reset(); + validateActionStub.resolves(true); + processSerializedSignalStub.reset(); + processSerializedSignalStub.resolves(undefined); + countByRoomIdAndUserIdStub.reset(); + findSubscriptionsExcludingUserStub.reset(); + }); - ['force_logout', 'notification', 'message', 'uiInteraction', 'subscriptions-changed', 'webdav', 'banners'].forEach((event) => { - it(`should deny a logged-in client writing "${event}" to another user's stream`, async () => { - expect(await writeAllowed(`victim/${event}`, { foo: 'bar' })).to.equal(false); + afterEach(() => { + Object.keys(StreamerCentral.instances).forEach((name) => delete StreamerCentral.instances[name]); }); - }); - it("should deny writes even to the client's own stream", async () => { - expect(await writeAllowed(`userId/force_logout`, undefined)).to.equal(false); - }); + ['force_logout', 'notification', 'message', 'uiInteraction', 'subscriptions-changed', 'webdav', 'banners'].forEach((event) => { + it(`should deny a logged-in client writing "${event}" to another user's stream`, async () => { + expect(await writeAllowed(`victim/${event}`, { foo: 'bar' })).to.equal(false); + }); + }); + + it("should deny writes even to the client's own stream", async () => { + expect(await writeAllowed(`userId/force_logout`, undefined)).to.equal(false); + }); + + it('should accept "video-conference" and delegate authorization to VideoConf.validateAction', async () => { + const result = await writeAllowed(`userId/video-conference`, { + action: 'call-start', + params: { callId: '123', uid: '456', rid: '789' }, + }); - it('should accept "video-conference" and delegate authorization to VideoConf.validateAction', async () => { - const result = await writeAllowed(`userId/video-conference`, { - action: 'call-start', - params: { callId: '123', uid: '456', rid: '789' }, + expect(result).to.be.true; + expect(validateActionStub.calledOnceWith('call-start', 'userId', { callId: '123', uid: '456', rid: '789' })).to.be.true; }); - expect(result).to.be.true; - expect(validateActionStub.calledOnceWith('call-start', 'userId', { callId: '123', uid: '456', rid: '789' })).to.be.true; + it('should process "media-calls" signals server-side and never broadcast them', async () => { + const signal = '{"type":"offer"}'; + const result = await writeAllowed(`userId/media-calls`, signal); + + expect(result).to.equal(false); + expect(processSerializedSignalStub.calledOnceWith('userId', signal)).to.be.true; + }); }); - it('should process "media-calls" signals server-side and never broadcast them', async () => { - const signal = '{"type":"offer"}'; - const result = await writeAllowed(`userId/media-calls`, signal); + describe('notify-room-users allowWrite', () => { + const writeAllowed = (eventName: string, ...args: unknown[]) => + (notifications.streamRoomUsers as unknown as Streamer<'notify-room-users'>).isWriteAllowed( + { userId: 'attacker' } as any, + eventName, + args, + ); + + beforeEach(() => { + countByRoomIdAndUserIdStub.reset(); + countByRoomIdAndUserIdStub.resolves(1); // attacker is subscribed to the room + findSubscriptionsExcludingUserStub.reset(); + findSubscriptionsExcludingUserStub.returns({ toArray: async () => [{ u: { _id: 'victim' } }] }); + }); - expect(result).to.equal(false); - expect(processSerializedSignalStub.calledOnceWith('userId', signal)).to.be.true; + afterEach(() => { + Object.keys(StreamerCentral.instances).forEach((name) => delete StreamerCentral.instances[name]); + }); + + ['force_logout', 'notification', 'message', 'uiInteraction', 'subscriptions-changed', 'webdav', 'banners'].forEach((event) => { + it(`should deny and not relay an arbitrary "${event}" event to other room members`, async () => { + const emitSpy = sinon.spy(notifications.streamUser, 'emit'); + + const result = await writeAllowed(`room1/${event}`, { foo: 'bar' }); + + expect(result).to.equal(false); + expect(findSubscriptionsExcludingUserStub.called).to.equal(false); + expect(emitSpy.called).to.equal(false); + }); + }); + + it('should not relay anything for a user not subscribed to the room', async () => { + countByRoomIdAndUserIdStub.resolves(0); + const emitSpy = sinon.spy(notifications.streamUser, 'emit'); + + const result = await writeAllowed('room1/video-conference', { + action: 'call-start', + params: { callId: '123', uid: 'victim', rid: 'room1' }, + }); + + expect(result).to.equal(false); + expect(findSubscriptionsExcludingUserStub.called).to.equal(false); + expect(emitSpy.called).to.equal(false); + }); + + ['video-conference', 'userData'].forEach((event) => { + it(`should relay "${event}" event to other room members`, async () => { + const emitSpy = sinon.spy(notifications.streamUser, 'emit'); + + const result = await writeAllowed(`room1/${event}`, { foo: 'bar' }); + + expect(result).to.equal(false); + expect(findSubscriptionsExcludingUserStub.called).to.equal(true); + expect(emitSpy.called).to.equal(true); + }); + }); }); });