From 8d48d10fde09eb95d458e3b3b26f5baddccfe266 Mon Sep 17 00:00:00 2001 From: Julio A <52619625+julio-cfa@users.noreply.github.com> Date: Fri, 23 Aug 2024 16:36:17 +0200 Subject: [PATCH] fix: imported fixes (#33136) --- .changeset/orange-clocks-wait.md | 5 + .../functions/getModifiedHttpHeaders.ts | 20 ++ apps/meteor/app/lib/server/lib/debug.js | 3 +- .../meteor/app/livechat/server/api/v1/room.ts | 107 +++++---- .../app/livechat/server/api/v1/visitor.ts | 211 +++++++++--------- .../tests/end-to-end/api/00-miscellaneous.js | 154 ++++++++++++- .../functions/getModifiedHttpHeaders.tests.ts | 59 +++++ 7 files changed, 407 insertions(+), 152 deletions(-) create mode 100644 .changeset/orange-clocks-wait.md create mode 100644 apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts create mode 100644 apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts diff --git a/.changeset/orange-clocks-wait.md b/.changeset/orange-clocks-wait.md new file mode 100644 index 000000000000..eacb88108a0f --- /dev/null +++ b/.changeset/orange-clocks-wait.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/lib/server/functions/getModifiedHttpHeaders.ts b/apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts new file mode 100644 index 000000000000..e62727814de3 --- /dev/null +++ b/apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts @@ -0,0 +1,20 @@ +export const getModifiedHttpHeaders = (httpHeaders: Record) => { + const modifiedHttpHeaders = { ...httpHeaders }; + + if ('x-auth-token' in modifiedHttpHeaders) { + modifiedHttpHeaders['x-auth-token'] = '[redacted]'; + } + + if (modifiedHttpHeaders.cookie) { + const cookies = modifiedHttpHeaders.cookie.split('; '); + const modifiedCookies = cookies.map((cookie: string) => { + if (cookie.startsWith('rc_token=')) { + return 'rc_token=[redacted]'; + } + return cookie; + }); + modifiedHttpHeaders.cookie = modifiedCookies.join('; '); + } + + return modifiedHttpHeaders; +}; diff --git a/apps/meteor/app/lib/server/lib/debug.js b/apps/meteor/app/lib/server/lib/debug.js index aaa492e80337..cbf38528579f 100644 --- a/apps/meteor/app/lib/server/lib/debug.js +++ b/apps/meteor/app/lib/server/lib/debug.js @@ -7,6 +7,7 @@ import _ from 'underscore'; import { getMethodArgs } from '../../../../server/lib/logger/logPayloads'; import { metrics } from '../../../metrics/server'; import { settings } from '../../../settings/server'; +import { getModifiedHttpHeaders } from '../functions/getModifiedHttpHeaders'; const logger = new Logger('Meteor'); @@ -41,7 +42,7 @@ const traceConnection = (enable, filter, prefix, name, connection, userId) => { console.log(name, { id: connection.id, clientAddress: connection.clientAddress, - httpHeaders: connection.httpHeaders, + httpHeaders: getModifiedHttpHeaders(connection.httpHeaders), userId, }); } else { diff --git a/apps/meteor/app/livechat/server/api/v1/room.ts b/apps/meteor/app/livechat/server/api/v1/room.ts index d2a76e53926f..09b998c9351e 100644 --- a/apps/meteor/app/livechat/server/api/v1/room.ts +++ b/apps/meteor/app/livechat/server/api/v1/room.ts @@ -32,61 +32,70 @@ import { findVisitorInfo } from '../lib/visitors'; const isAgentWithInfo = (agentObj: ILivechatAgent | { hiddenInfo: boolean }): agentObj is ILivechatAgent => !('hiddenInfo' in agentObj); -API.v1.addRoute('livechat/room', { - async get() { - // I'll temporary use check for validation, as validateParams doesnt support what's being done here - const extraCheckParams = await onCheckRoomParams({ - token: String, - rid: Match.Maybe(String), - agentId: Match.Maybe(String), - }); - - check(this.queryParams, extraCheckParams as any); - - const { token, rid: roomId, agentId, ...extraParams } = this.queryParams; - - const guest = token && (await findGuest(token)); - if (!guest) { - throw new Error('invalid-token'); - } - - let room: IOmnichannelRoom | null; - if (!roomId) { - room = await LivechatRooms.findOneOpenByVisitorToken(token, {}); - if (room) { - return API.v1.success({ room, newRoom: false }); - } - - let agent: SelectedAgent | undefined; - const agentObj = agentId && (await findAgent(agentId)); - if (agentObj) { - if (isAgentWithInfo(agentObj)) { - const { username = undefined } = agentObj; - agent = { agentId, username }; - } else { - agent = { agentId }; - } +API.v1.addRoute( + 'livechat/room', + { + rateLimiterOptions: { + numRequestsAllowed: 5, + intervalTimeInMS: 60000, + }, + }, + { + async get() { + // I'll temporary use check for validation, as validateParams doesnt support what's being done here + const extraCheckParams = await onCheckRoomParams({ + token: String, + rid: Match.Maybe(String), + agentId: Match.Maybe(String), + }); + + check(this.queryParams, extraCheckParams as any); + + const { token, rid: roomId, agentId, ...extraParams } = this.queryParams; + + const guest = token && (await findGuest(token)); + if (!guest) { + throw new Error('invalid-token'); } - const rid = Random.id(); - const roomInfo = { - source: { - type: isWidget(this.request.headers) ? OmnichannelSourceType.WIDGET : OmnichannelSourceType.API, - }, - }; + let room: IOmnichannelRoom | null; + if (!roomId) { + room = await LivechatRooms.findOneOpenByVisitorToken(token, {}); + if (room) { + return API.v1.success({ room, newRoom: false }); + } - const newRoom = await getRoom({ guest, rid, agent, roomInfo, extraParams }); - return API.v1.success(newRoom); - } + let agent: SelectedAgent | undefined; + const agentObj = agentId && (await findAgent(agentId)); + if (agentObj) { + if (isAgentWithInfo(agentObj)) { + const { username = undefined } = agentObj; + agent = { agentId, username }; + } else { + agent = { agentId }; + } + } + + const rid = Random.id(); + const roomInfo = { + source: { + type: isWidget(this.request.headers) ? OmnichannelSourceType.WIDGET : OmnichannelSourceType.API, + }, + }; - const froom = await LivechatRooms.findOneOpenByRoomIdAndVisitorToken(roomId, token, {}); - if (!froom) { - throw new Error('invalid-room'); - } + const newRoom = await getRoom({ guest, rid, agent, roomInfo, extraParams }); + return API.v1.success(newRoom); + } - return API.v1.success({ room: froom, newRoom: false }); + const froom = await LivechatRooms.findOneOpenByRoomIdAndVisitorToken(roomId, token, {}); + if (!froom) { + throw new Error('invalid-room'); + } + + return API.v1.success({ room: froom, newRoom: false }); + }, }, -}); +); // Note: use this route if a visitor is closing a room // If a RC user(like eg agent) is closing a room, use the `livechat/room.closeByUser` route diff --git a/apps/meteor/app/livechat/server/api/v1/visitor.ts b/apps/meteor/app/livechat/server/api/v1/visitor.ts index 9c19f5bbdec8..f4a8e57b93cf 100644 --- a/apps/meteor/app/livechat/server/api/v1/visitor.ts +++ b/apps/meteor/app/livechat/server/api/v1/visitor.ts @@ -9,117 +9,126 @@ import { settings } from '../../../../settings/server'; import { Livechat as LivechatTyped } from '../../lib/LivechatTyped'; import { findGuest, normalizeHttpHeaderData } from '../lib/livechat'; -API.v1.addRoute('livechat/visitor', { - async post() { - check(this.bodyParams, { - visitor: Match.ObjectIncluding({ - token: String, - name: Match.Maybe(String), - email: Match.Maybe(String), - department: Match.Maybe(String), - phone: Match.Maybe(String), - username: Match.Maybe(String), - customFields: Match.Maybe([ - Match.ObjectIncluding({ - key: String, - value: String, - overwrite: Boolean, - }), - ]), - }), - }); +API.v1.addRoute( + 'livechat/visitor', + { + rateLimiterOptions: { + numRequestsAllowed: 5, + intervalTimeInMS: 60000, + }, + }, + { + async post() { + check(this.bodyParams, { + visitor: Match.ObjectIncluding({ + token: String, + name: Match.Maybe(String), + email: Match.Maybe(String), + department: Match.Maybe(String), + phone: Match.Maybe(String), + username: Match.Maybe(String), + customFields: Match.Maybe([ + Match.ObjectIncluding({ + key: String, + value: String, + overwrite: Boolean, + }), + ]), + }), + }); - const { customFields, id, token, name, email, department, phone, username, connectionData } = this.bodyParams.visitor; + const { customFields, id, token, name, email, department, phone, username, connectionData } = this.bodyParams.visitor; - if (!token?.trim()) { - throw new Meteor.Error('error-invalid-token', 'Token cannot be empty', { method: 'livechat/visitor' }); - } + if (!token?.trim()) { + throw new Meteor.Error('error-invalid-token', 'Token cannot be empty', { method: 'livechat/visitor' }); + } - const guest = { - token, - ...(id && { id }), - ...(name && { name }), - ...(email && { email }), - ...(department && { department }), - ...(username && { username }), - ...(connectionData && { connectionData }), - ...(phone && typeof phone === 'string' && { phone: { number: phone as string } }), - connectionData: normalizeHttpHeaderData(this.request.headers), - }; - - const visitorId = await LivechatTyped.registerGuest(guest); - - let visitor: ILivechatVisitor | null = await VisitorsRaw.findOneEnabledById(visitorId, {}); - if (visitor) { - const extraQuery = await callbacks.run('livechat.applyRoomRestrictions', {}); - // If it's updating an existing visitor, it must also update the roomInfo - const rooms = await LivechatRooms.findOpenByVisitorToken(visitor?.token, {}, extraQuery).toArray(); - await Promise.all( - rooms.map( - (room: IRoom) => - visitor && - LivechatTyped.saveRoomInfo(room, { - _id: visitor._id, - name: visitor.name, - phone: visitor.phone?.[0]?.phoneNumber, - livechatData: visitor.livechatData as { [k: string]: string }, - }), - ), - ); - } + const guest = { + token, + ...(id && { id }), + ...(name && { name }), + ...(email && { email }), + ...(department && { department }), + ...(username && { username }), + ...(connectionData && { connectionData }), + ...(phone && typeof phone === 'string' && { phone: { number: phone as string } }), + connectionData: normalizeHttpHeaderData(this.request.headers), + }; + + const visitorId = await LivechatTyped.registerGuest(guest); + + let visitor: ILivechatVisitor | null = await VisitorsRaw.findOneEnabledById(visitorId, {}); + if (visitor) { + const extraQuery = await callbacks.run('livechat.applyRoomRestrictions', {}); + // If it's updating an existing visitor, it must also update the roomInfo + const rooms = await LivechatRooms.findOpenByVisitorToken(visitor?.token, {}, extraQuery).toArray(); + await Promise.all( + rooms.map( + (room: IRoom) => + visitor && + LivechatTyped.saveRoomInfo(room, { + _id: visitor._id, + name: visitor.name, + phone: visitor.phone?.[0]?.phoneNumber, + livechatData: visitor.livechatData as { [k: string]: string }, + }), + ), + ); + } + + if (customFields && Array.isArray(customFields) && customFields.length > 0) { + const keys = customFields.map((field) => field.key); + const errors: string[] = []; - if (customFields && Array.isArray(customFields) && customFields.length > 0) { - const keys = customFields.map((field) => field.key); - const errors: string[] = []; - - const processedKeys = await Promise.all( - await LivechatCustomField.findByIdsAndScope>(keys, 'visitor', { - projection: { _id: 1 }, - }) - .map(async (field) => { - const customField = customFields.find((f) => f.key === field._id); - if (!customField) { - return; - } - - const { key, value, overwrite } = customField; - // TODO: Change this to Bulk update - if (!(await VisitorsRaw.updateLivechatDataByToken(token, key, value, overwrite))) { - errors.push(key); - } - - return key; + const processedKeys = await Promise.all( + await LivechatCustomField.findByIdsAndScope>(keys, 'visitor', { + projection: { _id: 1 }, }) - .toArray(), - ); - - if (processedKeys.length !== keys.length) { - LivechatTyped.logger.warn({ - msg: 'Some custom fields were not processed', - visitorId, - missingKeys: keys.filter((key) => !processedKeys.includes(key)), - }); + .map(async (field) => { + const customField = customFields.find((f) => f.key === field._id); + if (!customField) { + return; + } + + const { key, value, overwrite } = customField; + // TODO: Change this to Bulk update + if (!(await VisitorsRaw.updateLivechatDataByToken(token, key, value, overwrite))) { + errors.push(key); + } + + return key; + }) + .toArray(), + ); + + if (processedKeys.length !== keys.length) { + LivechatTyped.logger.warn({ + msg: 'Some custom fields were not processed', + visitorId, + missingKeys: keys.filter((key) => !processedKeys.includes(key)), + }); + } + + if (errors.length > 0) { + LivechatTyped.logger.error({ + msg: 'Error updating custom fields', + visitorId, + errors, + }); + throw new Error('error-updating-custom-fields'); + } + + visitor = await VisitorsRaw.findOneEnabledById(visitorId, {}); } - if (errors.length > 0) { - LivechatTyped.logger.error({ - msg: 'Error updating custom fields', - visitorId, - errors, - }); - throw new Error('error-updating-custom-fields'); + if (!visitor) { + throw new Meteor.Error('error-saving-visitor', 'An error ocurred while saving visitor'); } - visitor = await VisitorsRaw.findOneEnabledById(visitorId, {}); - } - - if (!visitor) { - throw new Meteor.Error('error-saving-visitor', 'An error ocurred while saving visitor'); - } - - return API.v1.success({ visitor }); + return API.v1.success({ visitor }); + }, }, -}); +); API.v1.addRoute('livechat/visitor/:token', { async get() { diff --git a/apps/meteor/tests/end-to-end/api/00-miscellaneous.js b/apps/meteor/tests/end-to-end/api/00-miscellaneous.js index 1fe04b9b380a..4defa22fe84e 100644 --- a/apps/meteor/tests/end-to-end/api/00-miscellaneous.js +++ b/apps/meteor/tests/end-to-end/api/00-miscellaneous.js @@ -2,7 +2,8 @@ import { TEAM_TYPE } from '@rocket.chat/core-typings'; import { expect } from 'chai'; import { after, before, describe, it } from 'mocha'; -import { getCredentials, api, request, credentials } from '../../data/api-data.js'; +import { getCredentials, api, request, credentials, methodCall } from '../../data/api-data'; +import { sleep } from '../../data/livechat/utils'; import { updatePermission, updateSetting } from '../../data/permissions.helper'; import { createRoom, deleteRoom } from '../../data/rooms.helper'; import { createTeam, deleteTeam } from '../../data/teams.helper'; @@ -694,4 +695,155 @@ describe('miscellaneous', function () { .end(done); }); }); + + describe('[/stdout.queue]', () => { + let testUser; + let testUsername; + let testUserPassword; + before(async () => { + testUser = await createUser(); + testUsername = testUser.username; + testUserPassword = password; + await updateSetting('Log_Trace_Methods', true); + await updateSetting('Log_Level', '2'); + await sleep(4000); + + // populate the logs by sending method calls + const populateLogsPromises = []; + populateLogsPromises.push( + request + .post(methodCall('getRoomRoles')) + .set(credentials) + .set('Cookie', `rc_token=${credentials['X-Auth-Token']}`) + .send({ + message: JSON.stringify({ + method: 'getRoomRoles', + params: ['GENERAL'], + id: 'id', + msg: 'method', + }), + }), + ); + + populateLogsPromises.push( + request + .post(methodCall('getRoomRoles')) + .set(credentials) + .set('Cookie', `rc_token=${credentials['X-Auth-Token']}`) + .send({ + message: JSON.stringify({ + method: 'getRoomRoles', + params: ['GENERAL'], + id: 'id', + msg: 'method', + }), + }), + ); + + populateLogsPromises.push( + request + .post(methodCall('private-settings:get')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'private-settings/get', + params: [ + { + $date: new Date().getTime(), + }, + ], + id: 'id', + msg: 'method', + }), + }), + ); + + populateLogsPromises.push( + request.post(api('login')).send({ + user: { + username: testUsername, + }, + password: testUserPassword, + }), + ); + + await Promise.all(populateLogsPromises); + }); + + after(async () => { + await Promise.all([updateSetting('Log_Trace_Methods', false), updateSetting('Log_Level', '0'), deleteUser(testUser)]); + }); + + it('if log trace enabled, x-auth-token should be redacted', async () => { + await request + .get(api('stdout.queue')) + .set(credentials) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('queue').that.is.an('array'); + + const { queue } = res.body; + let foundRedactedToken = false; + + for (const log of queue) { + if (log.string.includes("'x-auth-token': '[redacted]'")) { + foundRedactedToken = true; + break; + } + } + + expect(foundRedactedToken).to.be.true; + }); + }); + + it('if log trace enabled, rc_token should be redacted', async () => { + await request + .get(api('stdout.queue')) + .set(credentials) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('queue').that.is.an('array'); + + const { queue } = res.body; + let foundRedactedCookie = false; + + for (const log of queue) { + if (log.string.includes('rc_token=[redacted]')) { + foundRedactedCookie = true; + break; + } + } + + expect(foundRedactedCookie).to.be.true; + }); + }); + + it('should not return user token anywhere in the log stream', async () => { + await request + .get(api('stdout.queue')) + .set(credentials) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('queue').that.is.an('array'); + + const { queue } = res.body; + let foundTokenValue = false; + + for (const log of queue) { + if (log.string.includes(credentials['X-Auth-Token'])) { + foundTokenValue = true; + break; + } + } + + expect(foundTokenValue).to.be.false; + }); + }); + }); }); diff --git a/apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts new file mode 100644 index 000000000000..5130bbe59a99 --- /dev/null +++ b/apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts @@ -0,0 +1,59 @@ +import { expect } from 'chai'; + +import { getModifiedHttpHeaders } from '../../../../../../app/lib/server/functions/getModifiedHttpHeaders'; + +describe('getModifiedHttpHeaders', () => { + it('should redact x-auth-token if present', () => { + const inputHeaders = { + 'x-auth-token': '12345', + 'some-other-header': 'value', + }; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result['x-auth-token']).to.equal('[redacted]'); + expect(result['some-other-header']).to.equal('value'); + }); + + it('should not modify headers if x-auth-token is not present', () => { + const inputHeaders = { + 'some-other-header': 'value', + }; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result).to.deep.equal(inputHeaders); + }); + + it('should redact rc_token in cookies if present', () => { + const inputHeaders = { + cookie: 'session_id=abc123; rc_token=98765; other_cookie=value', + }; + const expectedCookies = 'session_id=abc123; rc_token=[redacted]; other_cookie=value'; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result.cookie).to.equal(expectedCookies); + }); + + it('should not modify cookies if rc_token is not present', () => { + const inputHeaders = { + cookie: 'session_id=abc123; other_cookie=value', + }; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result.cookie).to.equal(inputHeaders.cookie); + }); + + it('should return headers unchanged if neither x-auth-token nor cookie are present', () => { + const inputHeaders = { + 'some-other-header': 'value', + }; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result).to.deep.equal(inputHeaders); + }); + + it('should handle cases with both x-auth-token and rc_token in cookie', () => { + const inputHeaders = { + 'x-auth-token': '12345', + 'cookie': 'session_id=abc123; rc_token=98765; other_cookie=value', + }; + const expectedCookies = 'session_id=abc123; rc_token=[redacted]; other_cookie=value'; + const result = getModifiedHttpHeaders(inputHeaders); + expect(result['x-auth-token']).to.equal('[redacted]'); + expect(result.cookie).to.equal(expectedCookies); + }); +});