From f055d5f92e58f543d2513a2a17f8c32b28a17c58 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Fri, 26 Apr 2019 23:50:12 -0300 Subject: [PATCH] Improve message validation (#14266) * Improve message validation * Add tests for xss values * Fix test after hook * Add more tests --- app/lib/server/functions/sendMessage.js | 44 +++- .../client/messageAttachment.js | 2 +- tests/end-to-end/api/05-chat.js | 228 ++++++++++++++++++ 3 files changed, 264 insertions(+), 10 deletions(-) diff --git a/app/lib/server/functions/sendMessage.js b/app/lib/server/functions/sendMessage.js index 9ff6fa6b432b..e629f2858a1b 100644 --- a/app/lib/server/functions/sendMessage.js +++ b/app/lib/server/functions/sendMessage.js @@ -6,6 +6,25 @@ import { Messages } from '../../../models'; import { Apps } from '../../../apps/server'; import { Markdown } from '../../../markdown/server'; +/** + * IMPORTANT + * + * This validator prevents malicious href values + * intending to run arbitrary js code in anchor tags. + * You should use it whenever the value you're checking + * is going to be rendered in the href attribute of a + * link. + */ +const ValidHref = Match.Where((value) => { + check(value, String); + + if (/^javascript:/i.test(value)) { + throw new Error('Invalid href value provided'); + } + + return true; +}); + const objectMaybeIncluding = (types) => Match.Where((value) => { Object.keys(types).forEach((field) => { if (value[field] != null) { @@ -38,7 +57,7 @@ const validateAttachmentsActions = (attachmentActions) => { check(attachmentActions, objectMaybeIncluding({ type: String, text: String, - url: String, + url: ValidHref, image_url: String, is_webview: Boolean, webview_height_ratio: String, @@ -55,13 +74,13 @@ const validateAttachment = (attachment) => { thumb_url: String, button_alignment: String, actions: [Match.Any], - message_link: String, + message_link: ValidHref, collapsed: Boolean, author_name: String, - author_link: String, + author_link: ValidHref, author_icon: String, title: String, - title_link: String, + title_link: ValidHref, title_link_download: Boolean, image_dimensions: Object, image_url: String, @@ -88,11 +107,7 @@ const validateAttachment = (attachment) => { const validateBodyAttachments = (attachments) => attachments.map(validateAttachment); -export const sendMessage = function(user, message, room, upsert = false) { - if (!user || !message || !room._id) { - return false; - } - +const validateMessage = (message) => { check(message, objectMaybeIncluding({ _id: String, msg: String, @@ -106,6 +121,14 @@ export const sendMessage = function(user, message, room, upsert = false) { if (Array.isArray(message.attachments) && message.attachments.length) { validateBodyAttachments(message.attachments); } +}; + +export const sendMessage = function(user, message, room, upsert = false) { + if (!user || !message || !room._id) { + return false; + } + + validateMessage(message); if (!message.ts) { message.ts = new Date(); @@ -143,6 +166,9 @@ export const sendMessage = function(user, message, room, upsert = false) { if (typeof result === 'object') { message = Object.assign(message, result); + + // Some app may have inserted malicious/invalid values in the message, let's check it again + validateMessage(message); } } diff --git a/app/message-attachments/client/messageAttachment.js b/app/message-attachments/client/messageAttachment.js index b095260c042d..148db49fde2e 100644 --- a/app/message-attachments/client/messageAttachment.js +++ b/app/message-attachments/client/messageAttachment.js @@ -1,7 +1,7 @@ import { Meteor } from 'meteor/meteor'; import { DateFormat } from '../../lib'; import { Template } from 'meteor/templating'; -import { getUserPreference, getURL } from '../../utils'; +import { getUserPreference, getURL } from '../../utils/client'; import { Users } from '../../models'; import { renderMessageBody } from '../../ui-utils'; diff --git a/tests/end-to-end/api/05-chat.js b/tests/end-to-end/api/05-chat.js index 31526bc030f6..65eda7a4bd22 100644 --- a/tests/end-to-end/api/05-chat.js +++ b/tests/end-to-end/api/05-chat.js @@ -73,6 +73,120 @@ describe('[Chat]', function() { .end(done); }); + describe('should throw an error when the sensitive properties contain malicious XSS values', () => { + + it('attachment.message_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + thumb_url: 'http://res.guggy.com/logo_128.png', + message_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.author_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + thumb_url: 'http://res.guggy.com/logo_128.png', + author_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.title_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + title: 'Attachment Example', + title_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.action.url', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + title: 'Attachment Example', + title_link: 'https://youtube.com', + actions: [ + { + type: 'button', + text: 'Text', + url: 'javascript:alert("xss")', + }, + ], + }], + }) + .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 properties (attachments.fields.title, attachments.fields.value) are with the wrong type', (done) => { request.post(api('chat.postMessage')) .set(credentials) @@ -201,6 +315,120 @@ describe('[Chat]', function() { .end(done); }); + describe('should throw an error when the sensitive properties contain malicious XSS values', () => { + + it('attachment.message_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + thumb_url: 'http://res.guggy.com/logo_128.png', + message_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.author_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + thumb_url: 'http://res.guggy.com/logo_128.png', + author_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.title_link', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + title: 'Attachment Example', + title_link: 'javascript:alert("xss")', + }], + }) + .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('attachment.action.url', (done) => + request.post(api('chat.postMessage')) + .set(credentials) + .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!', + ts: '2016-12-09T16:53:06.761Z', + title: 'Attachment Example', + title_link: 'https://youtube.com', + actions: [ + { + type: 'button', + text: 'Text', + url: 'javascript:alert("xss")', + }, + ], + }], + }) + .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 it has some properties with the wrong type(attachments.title_link_download, attachments.fields, message_link)', (done) => { request.post(api('chat.sendMessage')) .set(credentials)