From a417b5204d0f7ed5da20e2676ee676f0ffb88f1f Mon Sep 17 00:00:00 2001 From: Marcos Spessatto Defendi Date: Fri, 31 Jan 2020 19:26:11 -0300 Subject: [PATCH] Revert message properties validation (#16395) --- app/apps/server/bridges/messages.js | 2 +- app/lib/server/functions/sendMessage.js | 30 +------ app/lib/server/methods/sendMessage.js | 4 +- tests/end-to-end/api/05-chat.js | 114 +++++++++--------------- 4 files changed, 45 insertions(+), 105 deletions(-) diff --git a/app/apps/server/bridges/messages.js b/app/apps/server/bridges/messages.js index 61467c40396..1071117d180 100644 --- a/app/apps/server/bridges/messages.js +++ b/app/apps/server/bridges/messages.js @@ -15,7 +15,7 @@ export class AppMessageBridge { const convertedMessage = this.orch.getConverters().get('messages').convertAppMessage(message); - const sentMessage = executeSendMessage(convertedMessage.u._id, convertedMessage, true); + const sentMessage = executeSendMessage(convertedMessage.u._id, convertedMessage); return sentMessage._id; } diff --git a/app/lib/server/functions/sendMessage.js b/app/lib/server/functions/sendMessage.js index a596d957b04..7e0caef79a9 100644 --- a/app/lib/server/functions/sendMessage.js +++ b/app/lib/server/functions/sendMessage.js @@ -8,7 +8,6 @@ import { Apps } from '../../../apps/server'; import { Markdown } from '../../../markdown/server'; import { isURL, isRelativeURL } from '../../../utils/lib/isURL'; import { FileUpload } from '../../../file-upload/server'; -import { Users } from '../../../models/server'; /** * IMPORTANT @@ -144,42 +143,17 @@ const validateMessage = (message) => { } }; -const validateUserIdentity = (message, _id) => { - if (!message.alias && !message.avatar) { - return; - } - const forbiddenPropsToChangeWhenUserIsNotABot = ['avatar']; - const user = Users.findOneById(_id, { fields: { roles: 1, name: 1 } }); - /** - * If the query returns no user, the message has likely - * been sent by a Livechat Visitor, so we don't need to - * validate whether the sender is a bot. - */ - if (!user) { - return; - } - const userIsNotABot = !user.roles.includes('bot'); - const messageContainsAnyForbiddenProp = Object.keys(message).some((key) => forbiddenPropsToChangeWhenUserIsNotABot.includes(key)); - if (userIsNotABot && (messageContainsAnyForbiddenProp || (typeof message.alias !== 'undefined' && message.alias !== user.name))) { - throw new Error('You are not authorized to change message properties'); - } -}; - -export const sendMessage = function(user, message, room, upsert = false, trustedSender = false) { +export const sendMessage = function(user, message, room, upsert = false) { if (!user || !message || !room._id) { return false; } - const { _id, username, name } = user; - - if (!trustedSender) { - validateUserIdentity(message, _id); - } validateMessage(message); if (!message.ts) { message.ts = new Date(); } + const { _id, username, name } = user; message.u = { _id, username, diff --git a/app/lib/server/methods/sendMessage.js b/app/lib/server/methods/sendMessage.js index 637b89ef78b..827ad31ba39 100644 --- a/app/lib/server/methods/sendMessage.js +++ b/app/lib/server/methods/sendMessage.js @@ -15,7 +15,7 @@ import { RateLimiter } from '../lib'; import { canSendMessage } from '../../../authorization/server'; import { SystemLogger } from '../../../logger/server'; -export function executeSendMessage(uid, message, trustedSender = false) { +export function executeSendMessage(uid, message) { if (message.tmid && !settings.get('Threads_enabled')) { throw new Meteor.Error('error-not-allowed', 'not-allowed', { method: 'sendMessage', @@ -73,7 +73,7 @@ export function executeSendMessage(uid, message, trustedSender = false) { } metrics.messagesSent.inc(); // TODO This line needs to be moved to it's proper place. See the comments on: https://github.com/RocketChat/Rocket.Chat/pull/5736 - return sendMessage(user, message, room, false, trustedSender); + return sendMessage(user, message, room, false); } catch (error) { if (error === 'error-not-allowed') { throw new Meteor.Error('error-not-allowed'); diff --git a/tests/end-to-end/api/05-chat.js b/tests/end-to-end/api/05-chat.js index 42085bbcf56..59a52d98855 100644 --- a/tests/end-to-end/api/05-chat.js +++ b/tests/end-to-end/api/05-chat.js @@ -22,7 +22,9 @@ describe('[Chat]', function() { .set(credentials) .send({ text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', }) .expect('Content-Type', 'application/json') .expect(400) @@ -38,8 +40,10 @@ describe('[Chat]', function() { .set(credentials) .send({ channel: 'general', + alias: 'Gruggy', text: 'Sample message', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -74,7 +78,9 @@ describe('[Chat]', function() { .set(credentials) .send({ channel: 'general', + alias: 'Gruggy', text: 'Sample message', + avatar: 'http://res.guggy.com/logo_128.png', emoji: ':smirk:', attachments: [{ color: '#ff0000', @@ -99,7 +105,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -123,7 +131,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -147,7 +157,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -180,6 +192,7 @@ describe('[Chat]', function() { text: 'Sample message', emoji: ':smirk:', avatar: 'javascript:alert("xss")', + alias: 'Gruggy', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -210,7 +223,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -243,6 +258,8 @@ describe('[Chat]', function() { channel: 'general', text: 'Sample message', emoji: ':smirk:', + alias: 'Gruggy', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -268,6 +285,8 @@ describe('[Chat]', function() { channel: 'general', text: 'Sample message', emoji: ':smirk:', + alias: 'Gruggy', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -292,7 +311,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -316,7 +337,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -339,8 +362,10 @@ describe('[Chat]', function() { .set(credentials) .send({ channel: 'general', + alias: 'Gruggy', text: 'Sample message', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -367,6 +392,8 @@ describe('[Chat]', function() { channel: 'general', text: 'Sample message', emoji: ':smirk:', + alias: 'Gruggy', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -399,7 +426,7 @@ describe('[Chat]', function() { .end(done); }); - it('should throw an error when the user try to set alias and avatar and he is not a BOT user', (done) => { + it('should return statusCode 200 when postMessage successfully', (done) => { request.post(api('chat.postMessage')) .set(credentials) .send({ @@ -408,42 +435,6 @@ describe('[Chat]', function() { alias: 'Gruggy', emoji: ':smirk:', avatar: 'http://res.guggy.com/logo_128.png', - attachments: [{ - color: '#ff0000', - text: 'Yay for gruggy!', - ts: '2016-12-09T16:53:06.761Z', - thumb_url: 'http://res.guggy.com/logo_128.png', - message_link: 12, - collapsed: false, - author_name: 'Bradley Hilton', - author_link: 'https://rocket.chat/', - author_icon: 'https://avatars.githubusercontent.com/u/850391?v=3', - title: 'Attachment Example', - title_link: 'https://youtube.com', - title_link_download: 'https://youtube.com', - image_url: 'http://res.guggy.com/logo_128.png', - audio_url: 'http://www.w3schools.com/tags/horse.mp3', - video_url: 'http://www.w3schools.com/tags/movie.mp4', - fields: '', - }], - }) - .expect('Content-Type', 'application/json') - .expect(400) - .expect((res) => { - expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error'); - }) - .end(done); - }); - - - it('should return statusCode 200 when postMessage successfully', (done) => { - request.post(api('chat.postMessage')) - .set(credentials) - .send({ - channel: 'general', - text: 'Sample message', - emoji: ':smirk:', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -506,7 +497,9 @@ describe('[Chat]', function() { .send({ message: { text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', }, }) .expect('Content-Type', 'application/json') @@ -524,8 +517,10 @@ describe('[Chat]', function() { .set(credentials) .send({ channel: 'general', + alias: 'Gruggy', text: 'Sample message', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -549,7 +544,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -573,7 +570,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -597,7 +596,9 @@ describe('[Chat]', function() { .send({ channel: 'general', text: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!', @@ -624,43 +625,6 @@ describe('[Chat]', function() { }); it('should throw an error when it has some properties with the wrong type(attachments.title_link_download, attachments.fields, message_link)', (done) => { - request.post(api('chat.sendMessage')) - .set(credentials) - .send({ - message: { - channel: 'general', - text: 'Sample message', - emoji: ':smirk:', - attachments: [{ - color: '#ff0000', - text: 'Yay for gruggy!', - ts: '2016-12-09T16:53:06.761Z', - thumb_url: 'http://res.guggy.com/logo_128.png', - message_link: 12, - collapsed: false, - author_name: 'Bradley Hilton', - author_link: 'https://rocket.chat/', - author_icon: 'https://avatars.githubusercontent.com/u/850391?v=3', - title: 'Attachment Example', - title_link: 'https://youtube.com', - title_link_download: 'https://youtube.com', - image_url: 'http://res.guggy.com/logo_128.png', - audio_url: 'http://www.w3schools.com/tags/horse.mp3', - video_url: 'http://www.w3schools.com/tags/movie.mp4', - fields: '', - }], - }, - }) - .expect('Content-Type', 'application/json') - .expect(400) - .expect((res) => { - expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error'); - }) - .end(done); - }); - - it('should throw an error when the user try to set alias and avatar and he is not a BOT user', (done) => { request.post(api('chat.sendMessage')) .set(credentials) .send({ @@ -708,7 +672,9 @@ describe('[Chat]', function() { _id: message._id, rid: 'GENERAL', msg: 'Sample message', + alias: 'Gruggy', emoji: ':smirk:', + avatar: 'http://res.guggy.com/logo_128.png', attachments: [{ color: '#ff0000', text: 'Yay for gruggy!',