From a1508975a52ed008422e12a279772ba2080b9f8b Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Tue, 6 Aug 2019 19:34:54 -0300 Subject: [PATCH 1/5] Revert some url validations that can be done via upload --- app/lib/server/functions/sendMessage.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/lib/server/functions/sendMessage.js b/app/lib/server/functions/sendMessage.js index 52e6187de320..1fcf31776879 100644 --- a/app/lib/server/functions/sendMessage.js +++ b/app/lib/server/functions/sendMessage.js @@ -63,7 +63,7 @@ const validateAttachmentsActions = (attachmentActions) => { type: String, text: String, url: ValidLinkParam, - image_url: ValidLinkParam, + image_url: String, is_webview: Boolean, webview_height_ratio: String, msg: String, @@ -85,17 +85,17 @@ const validateAttachment = (attachment) => { author_link: ValidLinkParam, author_icon: ValidLinkParam, title: String, - title_link: ValidLinkParam, + title_link: String, title_link_download: Boolean, image_dimensions: Object, - image_url: ValidLinkParam, + image_url: String, image_preview: String, image_type: String, image_size: Number, - audio_url: ValidLinkParam, + audio_url: String, audio_type: String, audio_size: Number, - video_url: ValidLinkParam, + video_url: String, video_type: String, video_size: Number, fields: [Match.Any], From 72c785c4ab07411bcc2f5aac0cbe09cc354c8e13 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Tue, 6 Aug 2019 21:02:32 -0300 Subject: [PATCH 2/5] Fix tests --- tests/end-to-end/api/05-chat.js | 164 -------------------------------- 1 file changed, 164 deletions(-) diff --git a/tests/end-to-end/api/05-chat.js b/tests/end-to-end/api/05-chat.js index 49f143c57e80..e250acc82821 100644 --- a/tests/end-to-end/api/05-chat.js +++ b/tests/end-to-end/api/05-chat.js @@ -125,32 +125,6 @@ describe('[Chat]', function() { .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) @@ -217,40 +191,6 @@ describe('[Chat]', function() { .end(done) ); - it('attachment.action.image_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: 'http://res.guggy.com/logo_128.png', - image_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('attachment.thumb_url', (done) => request.post(api('chat.postMessage')) .set(credentials) @@ -304,84 +244,6 @@ describe('[Chat]', function() { }) .end(done) ); - it('attachment.image_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', - image_url: 'javascript:alert("xss")', - title_link: 'http://res.guggy.com/logo_128.png', - }], - }) - .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.audio_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', - audio_url: 'javascript:alert("xss")', - title_link: 'http://res.guggy.com/logo_128.png', - }], - }) - .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.video_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', - video_url: 'javascript:alert("xss")', - title_link: 'http://res.guggy.com/logo_128.png', - }], - }) - .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) => { @@ -563,32 +425,6 @@ describe('[Chat]', function() { .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) From b1c23b538bdd3bb0b0b63f33657cd90425bba6f2 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 7 Aug 2019 11:18:32 -0300 Subject: [PATCH 3/5] Revert remove validation and add validation for file-upload --- app/file-upload/server/lib/FileUpload.js | 4 + app/file-upload/server/lib/requests.js | 2 +- .../server/methods/sendFileMessage.js | 2 +- app/lib/server/functions/sendMessage.js | 13 +- .../server/methods/sendFileLivechatMessage.js | 4 +- tests/end-to-end/api/05-chat.js | 165 ++++++++++++++++++ 6 files changed, 180 insertions(+), 10 deletions(-) diff --git a/app/file-upload/server/lib/FileUpload.js b/app/file-upload/server/lib/FileUpload.js index 337d1e865b2a..d46b40188762 100644 --- a/app/file-upload/server/lib/FileUpload.js +++ b/app/file-upload/server/lib/FileUpload.js @@ -39,6 +39,10 @@ settings.get('FileUpload_MaxFileSize', function(key, value) { export const FileUpload = { handlers: {}, + getPath() { + return '/file-upload/'; + }, + configureUploadsStore(store, name, options) { const type = name.split(':').pop(); const stores = UploadFS.getStores(); diff --git a/app/file-upload/server/lib/requests.js b/app/file-upload/server/lib/requests.js index 039231a0d5c4..80a3b4213b38 100644 --- a/app/file-upload/server/lib/requests.js +++ b/app/file-upload/server/lib/requests.js @@ -3,7 +3,7 @@ import { WebApp } from 'meteor/webapp'; import { FileUpload } from './FileUpload'; import { Uploads } from '../../../models'; -WebApp.connectHandlers.use('/file-upload/', function(req, res, next) { +WebApp.connectHandlers.use(FileUpload.getPath(), function(req, res, next) { const match = /^\/([^\/]+)\/(.*)/.exec(req.url); if (match && match[1]) { diff --git a/app/file-upload/server/methods/sendFileMessage.js b/app/file-upload/server/methods/sendFileMessage.js index cdf9aef166f9..d60d528b6f1f 100644 --- a/app/file-upload/server/methods/sendFileMessage.js +++ b/app/file-upload/server/methods/sendFileMessage.js @@ -30,7 +30,7 @@ Meteor.methods({ Uploads.updateFileComplete(file._id, Meteor.userId(), _.omit(file, '_id')); - const fileUrl = `/file-upload/${ file._id }/${ encodeURI(file.name) }`; + const fileUrl = `${ FileUpload.getPath() }${ file._id }/${ encodeURI(file.name) }`; const attachment = { title: file.name, diff --git a/app/lib/server/functions/sendMessage.js b/app/lib/server/functions/sendMessage.js index 1fcf31776879..0312b547e4a6 100644 --- a/app/lib/server/functions/sendMessage.js +++ b/app/lib/server/functions/sendMessage.js @@ -7,6 +7,7 @@ import { Messages } from '../../../models'; import { Apps } from '../../../apps/server'; import { Markdown } from '../../../markdown/server'; import { isURL } from '../../../utils/lib/isURL'; +import { FileUpload } from '../../../file-upload/server'; /** * IMPORTANT @@ -20,7 +21,7 @@ import { isURL } from '../../../utils/lib/isURL'; const ValidLinkParam = Match.Where((value) => { check(value, String); - if (!isURL(value)) { + if (!isURL(value) && !value.startsWith(FileUpload.getPath())) { throw new Error('Invalid href value provided'); } @@ -63,7 +64,7 @@ const validateAttachmentsActions = (attachmentActions) => { type: String, text: String, url: ValidLinkParam, - image_url: String, + image_url: ValidLinkParam, is_webview: Boolean, webview_height_ratio: String, msg: String, @@ -85,17 +86,17 @@ const validateAttachment = (attachment) => { author_link: ValidLinkParam, author_icon: ValidLinkParam, title: String, - title_link: String, + title_link: ValidLinkParam, title_link_download: Boolean, image_dimensions: Object, - image_url: String, + image_url: ValidLinkParam, image_preview: String, image_type: String, image_size: Number, - audio_url: String, + audio_url: ValidLinkParam, audio_type: String, audio_size: Number, - video_url: String, + video_url: ValidLinkParam, video_type: String, video_size: Number, fields: [Match.Any], diff --git a/app/livechat/server/methods/sendFileLivechatMessage.js b/app/livechat/server/methods/sendFileLivechatMessage.js index e0724d7e3188..9eaa627bb008 100644 --- a/app/livechat/server/methods/sendFileLivechatMessage.js +++ b/app/livechat/server/methods/sendFileLivechatMessage.js @@ -3,7 +3,7 @@ import { Match, check } from 'meteor/check'; import { Random } from 'meteor/random'; import { Rooms, LivechatVisitors } from '../../../models'; -import { FileUpload } from '../../../file-upload'; +import { FileUpload } from '../../../file-upload/server'; Meteor.methods({ async 'sendFileLivechatMessage'(roomId, visitorToken, file, msgData = {}) { @@ -27,7 +27,7 @@ Meteor.methods({ msg: Match.Optional(String), }); - const fileUrl = `/file-upload/${ file._id }/${ encodeURI(file.name) }`; + const fileUrl = `${ FileUpload.getPath() }${ file._id }/${ encodeURI(file.name) }`; const attachment = { title: file.name, diff --git a/tests/end-to-end/api/05-chat.js b/tests/end-to-end/api/05-chat.js index e250acc82821..8dfd3d035d47 100644 --- a/tests/end-to-end/api/05-chat.js +++ b/tests/end-to-end/api/05-chat.js @@ -125,6 +125,32 @@ describe('[Chat]', function() { .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) @@ -191,6 +217,40 @@ describe('[Chat]', function() { .end(done) ); + it('attachment.action.image_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: 'http://res.guggy.com/logo_128.png', + image_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('attachment.thumb_url', (done) => request.post(api('chat.postMessage')) .set(credentials) @@ -244,6 +304,85 @@ describe('[Chat]', function() { }) .end(done) ); + + it('attachment.image_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', + image_url: 'javascript:alert("xss")', + title_link: 'http://res.guggy.com/logo_128.png', + }], + }) + .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.audio_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', + audio_url: 'javascript:alert("xss")', + title_link: 'http://res.guggy.com/logo_128.png', + }], + }) + .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.video_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', + video_url: 'javascript:alert("xss")', + title_link: 'http://res.guggy.com/logo_128.png', + }], + }) + .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) => { @@ -425,6 +564,32 @@ describe('[Chat]', function() { .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) From 3188f0fdbef94ca685d2b2a491614bdc8798a835 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 7 Aug 2019 14:00:22 -0300 Subject: [PATCH 4/5] Add path parameter to the getPath function --- app/file-upload/server/lib/FileUpload.js | 5 ++++- app/file-upload/server/methods/sendFileMessage.js | 2 +- app/livechat/server/methods/sendFileLivechatMessage.js | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/file-upload/server/lib/FileUpload.js b/app/file-upload/server/lib/FileUpload.js index d46b40188762..298d7b5f4ad2 100644 --- a/app/file-upload/server/lib/FileUpload.js +++ b/app/file-upload/server/lib/FileUpload.js @@ -39,7 +39,10 @@ settings.get('FileUpload_MaxFileSize', function(key, value) { export const FileUpload = { handlers: {}, - getPath() { + getPath(path) { + if (path) { + return `/file-upload/${ path }`; + } return '/file-upload/'; }, diff --git a/app/file-upload/server/methods/sendFileMessage.js b/app/file-upload/server/methods/sendFileMessage.js index d60d528b6f1f..6d17cfaf1a06 100644 --- a/app/file-upload/server/methods/sendFileMessage.js +++ b/app/file-upload/server/methods/sendFileMessage.js @@ -30,7 +30,7 @@ Meteor.methods({ Uploads.updateFileComplete(file._id, Meteor.userId(), _.omit(file, '_id')); - const fileUrl = `${ FileUpload.getPath() }${ file._id }/${ encodeURI(file.name) }`; + const fileUrl = FileUpload.getPath(`${ file._id }/${ encodeURI(file.name) }`); const attachment = { title: file.name, diff --git a/app/livechat/server/methods/sendFileLivechatMessage.js b/app/livechat/server/methods/sendFileLivechatMessage.js index 9eaa627bb008..393e6b90dd68 100644 --- a/app/livechat/server/methods/sendFileLivechatMessage.js +++ b/app/livechat/server/methods/sendFileLivechatMessage.js @@ -27,7 +27,7 @@ Meteor.methods({ msg: Match.Optional(String), }); - const fileUrl = `${ FileUpload.getPath() }${ file._id }/${ encodeURI(file.name) }`; + const fileUrl = FileUpload.getPath(`${ file._id }/${ encodeURI(file.name) }`); const attachment = { title: file.name, From 6dd4fad3e553af0e98bd81a1179fba20332124c5 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 7 Aug 2019 14:10:38 -0300 Subject: [PATCH 5/5] change param --- app/file-upload/server/lib/FileUpload.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/file-upload/server/lib/FileUpload.js b/app/file-upload/server/lib/FileUpload.js index 298d7b5f4ad2..4fa5491efb2f 100644 --- a/app/file-upload/server/lib/FileUpload.js +++ b/app/file-upload/server/lib/FileUpload.js @@ -39,11 +39,8 @@ settings.get('FileUpload_MaxFileSize', function(key, value) { export const FileUpload = { handlers: {}, - getPath(path) { - if (path) { - return `/file-upload/${ path }`; - } - return '/file-upload/'; + getPath(path = '') { + return `/file-upload/${ path }`; }, configureUploadsStore(store, name, options) {