Skip to content

Commit

Permalink
Improve message validation (#14266)
Browse files Browse the repository at this point in the history 
* Improve message validation

* Add tests for xss values

* Fix test after hook

* Add more tests
  • Loading branch information
@d-gubert @rodrigok
d-gubert authored and rodrigok committed Apr 27, 2019
1 parent e021872 commit f055d5f
Show file tree
Hide file tree
Showing 3 changed files with 264 additions and 10 deletions.
44 changes: 35 additions & 9 deletions app/lib/server/functions/sendMessage.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion app/message-attachments/client/messageAttachment.js
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
228 changes: 228 additions & 0 deletions tests/end-to-end/api/05-chat.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit f055d5f

Please sign in to comment.