[BREAK] Validate incoming message schema #9922

MarcosSpessatto · 2018-02-27T18:19:33Z

We now verify the message object, when being sent, matches our expected schema. Should it not, we throw an error.

rafaelks · 2018-02-27T18:22:58Z

packages/rocketchat-api/server/v1/chat.js

+				check(attachment, Match.ObjectIncluding({
+					color: Match.Maybe(String),
+					text: Match.Maybe(String),
+					ts: Match.Maybe(String),


ts is a String? 🤔

rafaelks · 2018-02-27T18:23:29Z

packages/rocketchat-api/server/v1/chat.js

+
+		const validateBodyParams = (bodyParams) => {
+			check(bodyParams, Match.ObjectIncluding({
+				roomId: Match.Maybe(String),


Isn't this one REQUIRED?

rafaelks · 2018-02-27T18:24:06Z

tests/end-to-end/api/05-chat.js

@@ -32,7 +32,7 @@ describe('[Chat]', function() {
 					author_icon: 'https://avatars.githubusercontent.com/u/850391?v=3',
 					title: 'Attachment Example',
 					title_link: 'https://youtube.com',
-					title_link_download: 'https://rocket.chat/download',
+					title_link_download: true,


I think we can have more tests here, with lots of different variations and some fail case. Can you do that, please?

MarcosSpessatto · 2018-02-27T20:25:59Z

Can you see again @rafaelks ?

rafaelks · 2018-02-27T21:33:44Z

@MarcosSpessatto Looking great, thanks for tests. LGTM.

rafaelks · 2018-02-27T21:34:09Z

I approved, but I would that someone else also revised the JS code, please. @RocketChat/core

MarcosSpessatto · 2018-02-28T21:48:05Z

@rafaelks, can you take a look again please? I have added the same verification in chat.sendMessage too.

graywolf336

Instead of applying this verification to just these rest api methods, can we instead apply it to the code which these use? As these are duplicated code and too low level when we should instead be applying these at a higher level. Because with these changes, I can use the real time api and bypass the verification. Also, the webhooks would be able to bypass is verification.

rodrigok · 2018-03-01T13:01:51Z

@graywolf336 should he add the validation to the RocketChat.sendMessage function https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-lib/server/functions/sendMessage.js#L3 ?

And why the webhooks should be able to bypass the verification? I don't agree with that.

graywolf336 · 2018-03-01T14:28:25Z

Yes.

No, I'm saying with the way this pull request currently is they would be able to but my request was to not allow them to. :)

graywolf336 · 2018-04-19T18:43:57Z

packages/rocketchat-lib/server/functions/sendMessage.js

+		if (Array.isArray(message.attachments) && message.attachments.length) {
+			validateBodyAttachments(message.attachments);
+		}
+	} catch (error) {


Why are we catching an error here to then throw the same error? Let's not do that. Either allow the error to be thrown or throw a customized error.

graywolf336 · 2018-04-19T18:44:52Z

packages/rocketchat-lib/server/functions/sendMessage.js

+	if (message.ts == null) {
+		message.ts = new Date();
+	}
+	message.u = _.pick(user, ['_id', 'username', 'name']);


Instead of relying on underscore for this, let's just do it ourselves. :)

graywolf336 · 2018-04-19T18:45:53Z

packages/rocketchat-lib/server/functions/sendMessage.js

+		throw error;
+	}
+
+	if (message.ts == null) {


Can we change this to if(!message.ts) {?

graywolf336 · 2018-04-19T18:46:43Z

packages/rocketchat-lib/server/functions/sendMessage.js

@@ -1,10 +1,69 @@
 import _ from 'underscore';

+const validateBodyAttachments = (attachments) => {


As we are now going to be validating the messages and attachments, do we have some place where we document the schema in our documentation?

It seems we don't have @graywolf336, only in the endpoint docs. :(

Any recommendations for where to place it?

cc: @MartinSchoeler

Hey @MartinSchoeler, any ideas?

graywolf336 · 2018-04-19T18:47:11Z

packages/rocketchat-lib/server/functions/sendMessage.js

 RocketChat.sendMessage = function(user, message, room, upsert = false) {
 	if (!user || !message || !room._id) {
 		return false;
 	}

+	try {
+		check(message, Match.ObjectIncluding({
+			msg: Match.Maybe(String),


Let's also check the _id field, since it can be provided.

graywolf336 · 2018-04-19T18:47:59Z

packages/rocketchat-lib/server/methods/sendMessage.js

@@ -10,6 +10,10 @@ Meteor.methods({
 			});
 		}

+		if (!message.rid) {
+			throw new Error('The \'rid\' param, must be provided');


Maybe say The 'rid' property on the message object is missing.? Does that make more sense?

…-api-chat-postmessage-validations * commit 'a9fb4da5c847a456990a5d60369f0f52ff4a8bd8': (137 commits) Remove "secret" from REST endpoint /settings.oauth response [FIX] Directory sort and column sizes were wrong (#10403) [FIX] Add oauth services missing fields, and indicate whether the oauth service is customized (#10299) Show error message when email verification fails (#10446) Correct the column positions in the directory search for users (#10454) Fixed custom fields misalignment in registration form (#10463) [FIX] Unique identifier file not really being unique (#10341) [OTHER] More Listeners for Apps & Utilize Promises inside Apps (#10335) [FIX] Empty panel after changing a user's username (#10404) [FIX] Russian translation of "False" (#10418) [FIX] Links being embedded inside of blockquotes (#10496) [FIX] The 'channel.messages' REST API Endpoint error (#10485) [OTHER] Develop sync (#10487) [FIX] Button on user info contextual bar scrolling with the content (#10358) [FIX] "Idle Time Limit" using milliseconds instead of seconds (#9824) [NEW] Body of the payload on an incoming webhook is included on the request object (#10259) [FIX] Missing i18n translation key for "Unread" (#10387) [FIX] Owner unable to delete channel or group from APIs (#9729) [NEW] REST endpoint to recover forgotten password (#10371) Add REST endpoint chat.reportMessage, to report a message (#10354) ...

…d try catch unnecessary

…unction

graywolf336 · 2018-04-20T15:44:14Z

@RocketChat/core Thoughts on renaming this to [BREAK] prefix? As we are possibly going to be breaking people's API calls which are passing invalid data types to us.

sampaiodiego · 2018-04-20T15:58:15Z

@graywolf336 I agree.

graywolf336 · 2018-04-20T16:46:17Z

Changed to [BREAK] prefix.

graywolf336 · 2018-04-20T22:54:48Z

packages/rocketchat-lib/server/functions/sendMessage.js

@@ -1,10 +1,66 @@
-import _ from 'underscore';
+const validateAttachmentsFields = attachmentFields => {


@ggazzo why did you change the code style from parenthesis around the arguments to not?

because we dont need ( )in this case :)

Addressed

tbaehr · 2018-05-14T14:43:11Z

You are breaking a lot of stuff (e.g. webhooks) by now enforcing title on attachments

Exception in callback of async function: Error: Match error: Missing key 'title' at check (/opt/Rocket.Chat/programs/server/packages/check.js:68:17) at validateAttachmentsFields (/opt/Rocket.Chat/programs/server/packages/rocketchat_lib.js:5283:3)

This is not really a good way to do so!

graywolf336 · 2018-05-19T02:10:51Z

@tbaehr Did you open an issue so we can get this fixed?

Edit: Looks like the title property is optional due to this: https://github.com/RocketChat/Rocket.Chat/pull/9922/files#diff-b18deaa461a67499aa1867414e83204eR20

tbaehr · 2018-05-24T13:27:25Z

check(attachmentFields, Match.ObjectIncluding({
```
  short: Match.Maybe(Boolean),
```
```
  title: String,
```
```
  value: String
```
}));

does not mean optional, but before it was not checked so it could be left out, didnt make a separate Bug

graywolf336 · 2018-05-24T14:11:56Z

@tbaehr That is because those two properties are actually required when you are providing a field on the attachment. This is because of the way the mobile applications and everything are setup to display the attachment fields.

Added validation in chat.postMessage endpoint

c7a169e

MarcosSpessatto added type: bug type: improvement feat: api feat: rest labels Feb 27, 2018

engelgabriel temporarily deployed to rocket-chat-pr-9922 February 27, 2018 18:19 Inactive

MarcosSpessatto mentioned this pull request Feb 27, 2018

[DOCS & BUG] API to upload files accepts "title_link_download" as String #9878

Closed

rafaelks reviewed Feb 27, 2018

View reviewed changes

rafaelks suggested changes Feb 27, 2018

View reviewed changes

fix required parameters and add more tests cases.

b42d221

engelgabriel temporarily deployed to rocket-chat-pr-9922 February 27, 2018 20:24 Inactive

rafaelks previously approved these changes Feb 27, 2018

View reviewed changes

MarcosSpessatto added the stat: in progress label Feb 28, 2018

MarcosSpessatto changed the title ~~Added validation in chat.postMessage endpoint~~ Added validation in chat.postMessage and chat.sendMessage endpoints Feb 28, 2018

Added the same validation to chat.sendMessage method too.

c8d047a

engelgabriel temporarily deployed to rocket-chat-pr-9922 February 28, 2018 21:46 Inactive

graywolf336 suggested changes Mar 1, 2018

View reviewed changes

MarcosSpessatto changed the title ~~Added validation in chat.postMessage and chat.sendMessage endpoints~~ [WIP][FIX] Added validation in chat.postMessage and chat.sendMessage endpoints Mar 1, 2018

changed validation level, set in sendMessage method

ad512fb

engelgabriel temporarily deployed to rocket-chat-pr-9922 March 2, 2018 14:49 Inactive

Merge branch 'develop' into fix-rest-api-chat-postmessage-validations

2c2f4e7

engelgabriel temporarily deployed to rocket-chat-pr-9922 March 27, 2018 13:58 Inactive

sampaiodiego added this to the 0.64.0 milestone Apr 3, 2018

MarcosSpessatto changed the title ~~[WIP][FIX] Added validation in chat.postMessage and chat.sendMessage endpoints~~ [FIX] Added validation in chat.postMessage and chat.sendMessage endpoints Apr 19, 2018

MarcosSpessatto removed the stat: in progress label Apr 19, 2018

graywolf336 previously requested changes Apr 19, 2018

View reviewed changes

graywolf336 changed the title ~~[FIX] Added validation in chat.postMessage and chat.sendMessage endpoints~~ [NEW] Validate incoming message objects Apr 19, 2018

graywolf336 changed the title ~~[NEW] Validate incoming message objects~~ [NEW] Validate incoming message schema Apr 19, 2018

MarcosSpessatto added 2 commits April 19, 2018 16:11

Improvements in the sendMessage function, removed underscore uses, an…

be4a6ef

…d try catch unnecessary

MarcosSpessatto dismissed rafaelks’s stale review via be4a6ef April 19, 2018 19:52

engelgabriel temporarily deployed to rocket-chat-pr-9922 April 19, 2018 19:52 Inactive

MarcosSpessatto added 2 commits April 20, 2018 10:37

Merge branch 'develop' into fix-rest-api-chat-postmessage-validations

e957d88

Fix test in chat.sendMessage endpoint, and fix error in sendMessage f…

c770bf4

…unction

engelgabriel temporarily deployed to rocket-chat-pr-9922 April 20, 2018 15:06 Inactive

graywolf336 changed the title ~~[NEW] Validate incoming message schema~~ [BREAK] Validate incoming message schema Apr 20, 2018

fix review

d948c97

ggazzo deployed to rocket-chat-pr-9922 April 20, 2018 22:41 View deployment

ggazzo approved these changes Apr 20, 2018

View reviewed changes

graywolf336 reviewed Apr 20, 2018

View reviewed changes

rodrigok merged commit 816d110 into develop Apr 21, 2018

rodrigok deleted the fix-rest-api-chat-postmessage-validations branch April 21, 2018 01:57

rodrigok mentioned this pull request Apr 28, 2018

Release 0.64.0 #10613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAK] Validate incoming message schema #9922

[BREAK] Validate incoming message schema #9922

MarcosSpessatto commented Feb 27, 2018 •

edited by graywolf336

Loading

rafaelks Feb 27, 2018

rafaelks Feb 27, 2018

rafaelks Feb 27, 2018

MarcosSpessatto commented Feb 27, 2018

rafaelks commented Feb 27, 2018

rafaelks commented Feb 27, 2018

MarcosSpessatto commented Feb 28, 2018

graywolf336 left a comment

rodrigok commented Mar 1, 2018

graywolf336 commented Mar 1, 2018

graywolf336 Apr 19, 2018

graywolf336 Apr 19, 2018

graywolf336 Apr 19, 2018

graywolf336 Apr 19, 2018

MarcosSpessatto Apr 19, 2018 •

edited

Loading

graywolf336 Apr 19, 2018

graywolf336 Apr 20, 2018

graywolf336 Apr 19, 2018

graywolf336 Apr 19, 2018

graywolf336 commented Apr 20, 2018

sampaiodiego commented Apr 20, 2018

graywolf336 commented Apr 20, 2018

graywolf336 Apr 20, 2018

ggazzo Apr 21, 2018

tbaehr commented May 14, 2018

graywolf336 commented May 19, 2018 •

edited

Loading

tbaehr commented May 24, 2018

graywolf336 commented May 24, 2018

		@@ -1,10 +1,69 @@
		import _ from 'underscore';

		const validateBodyAttachments = (attachments) => {

		@@ -1,10 +1,66 @@
		import _ from 'underscore';
		const validateAttachmentsFields = attachmentFields => {

[BREAK] Validate incoming message schema #9922

[BREAK] Validate incoming message schema #9922

Conversation

MarcosSpessatto commented Feb 27, 2018 • edited by graywolf336 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosSpessatto commented Feb 27, 2018

rafaelks commented Feb 27, 2018

rafaelks commented Feb 27, 2018

MarcosSpessatto commented Feb 28, 2018

graywolf336 left a comment

Choose a reason for hiding this comment

rodrigok commented Mar 1, 2018

graywolf336 commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosSpessatto Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graywolf336 commented Apr 20, 2018

sampaiodiego commented Apr 20, 2018

graywolf336 commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbaehr commented May 14, 2018

graywolf336 commented May 19, 2018 • edited Loading

tbaehr commented May 24, 2018

graywolf336 commented May 24, 2018

MarcosSpessatto commented Feb 27, 2018 •

edited by graywolf336

Loading

MarcosSpessatto Apr 19, 2018 •

edited

Loading

graywolf336 commented May 19, 2018 •

edited

Loading