Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage - Closes#1017 #1020

ishantiw · 2019-01-18T14:25:38Z

Description

Add sanitization functions to handle protocol RPC requests and messages

Review checklist

The PR resolves Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage #1017
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

shuse2 · 2019-01-19T12:41:16Z

packages/lisk-p2p/src/sanitization.ts

@@ -92,3 +97,32 @@ export const sanitizePeerInfoList = (
 		throw new InvalidRPCResponse('Invalid response type');
 	}
 };
+
+export const sanitizeRPCRequest = (request: unknown): ProtocolRPCRequest => {


instead of sanitize, can we use validate?

shuse2 · 2019-01-19T12:42:04Z

packages/lisk-p2p/src/errors.ts

@@ -69,3 +69,17 @@ export class RequestFailError extends VError {
 		this.name = 'RequestFailError';
 	}
 }
+
+export class InvalidRPCRequest extends VError {


prefer InvalidRPCRequestError

shuse2 · 2019-01-19T12:42:15Z

packages/lisk-p2p/src/errors.ts

+	}
+}
+
+export class InvalidProtocolMessage extends VError {


prefer InvalidProtocolMessageError

…pc and message

ishantiw · 2019-01-21T14:21:33Z

Addressed all the comments and rebased. Also, I have included the changes related to socketcluster-server library in #1021, where I forgot to upgrade the version of the library

jondubois · 2019-01-21T15:47:54Z

packages/lisk-p2p/src/peer.ts


-				return;
+				this.emit(EVENT_REQUEST_RECEIVED, request);


It's dangerous to emit within a try block because event listeners which are declared outside this class will be called synchronously; so if a listener is attached to the Peer from outside; for example using peer.on(EVENT_REQUEST_RECEIVED, myListenerFunction); it means that if myListenerFunction throws an error or has a bug then it will cause the catch block below to execute and this.emit(EVENT_INVALID_REQUEST_RECEIVED, packet); will be called even though the request is fine; makes it very hard to debug.

An alternative approach might be to return from inside the catch block just after calling this.emit(EVENT_INVALID_REQUEST_RECEIVED, packet); and on the next line outside the catch block (which will only run if request is valid), we can emit the request with this.emit(EVENT_REQUEST_RECEIVED, request);.

It's easy to do with let but may be kind of tricky to do with only const. Maybe it's worth separating the validation logic from the sanitization logic and only have the try-catch block around the validation part?

Another option would be to emit asynchronously (e.g. inside a setTimeout/setImmediate) but that is more ugly and computationally expensive.

jondubois · 2019-01-21T15:49:44Z

packages/lisk-p2p/src/peer.ts

-				return;
+			try {
+				const protocolMessage = sanitizeProtocolMessage(packet);
+				this.emit(EVENT_MESSAGE_RECEIVED, protocolMessage);


Same problem as above.

jondubois · 2019-01-21T16:00:53Z

packages/lisk-p2p/src/sanitization.ts

+		throw new InvalidRPCRequestError('Invalid request');
+	}
+
+	const rpcRequest = request as P2PRequest;


We can't cast the request object into a P2PRequest because P2PRequest is a usable request object which can be responded to using its request.end(...) or request.error(...) method. Maybe this validation/sanitization function should just return a validated/sanitized ProtocolRPCRequestPacket; that's also more consistent with the sanitizeProtocolMessage function below. We will have to instantiate the P2PRequest as a separate step (not part of validation/sanitization); it needs the SocketCluster respond callback as third argument to its constructor.

shuse2 · 2019-01-21T21:28:53Z

packages/lisk-p2p/src/sanitization.ts

+	return rpcRequest;
+};
+
+export const sanitizeProtocolMessage = (message: unknown): ProtocolMessagePacket => {


name of this should also be validate?

actually before, all the functions were prefixed as process or validate. But later @jondubois renamed all of them to sanitize including the file name because these functions are not only validating but type casting or sanitizing it to some protocol type or preferred data structure.

Mmm we have similar functionality in transaction with validating and the checking so maybe it’s better to standardize?

Okay then I think it makes sense to just use validate here too 👍

shuse2 · 2019-01-21T21:29:27Z

packages/lisk-p2p/src/sanitization.ts

@@ -14,8 +14,14 @@
 */


also, filename should be consistent, and all the function in this file?

same here, should we rename it back to validate prefix? what do you think @jondubois ?

…ization.ts -> validation.ts in test

ishantiw · 2019-01-22T15:02:24Z

Addressed @jondubois @shuse2

ishantiw added the 👁️ pending review label Jan 18, 2019

ishantiw self-assigned this Jan 18, 2019

ishantiw requested review from jondubois and shuse2 January 18, 2019 14:25

shuse2 suggested changes Jan 19, 2019

View reviewed changes

shuse2 added 🏗️ in progress and removed 👁️ pending review labels Jan 19, 2019

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch 2 times, most recently from a92cadf to e69a997 Compare January 21, 2019 13:46

ishantiw added 👁️ pending review and removed 🏗️ in progress labels Jan 21, 2019

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch from 94a7c5e to 463eb33 Compare January 21, 2019 13:58

ishantiw added 3 commits January 21, 2019 15:17

♻️ Create sanitization functions and use it in peer handle protocol r…

e3441f0

…pc and message

💅 Change custom error names and rpc function name

7a5f7f4

⬆️ Upgrade socketcluster-server to 14.2.1

b60e132

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch from 615b99b to 4bd298d Compare January 21, 2019 14:20

🏠 Update imports related to sanitization

c3883cb

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch from 4bd298d to c3883cb Compare January 21, 2019 14:24

🏠 Resolve conflicts of interface name changes

ef16b96

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch from 8d748e1 to ef16b96 Compare January 21, 2019 14:39

jondubois suggested changes Jan 21, 2019

View reviewed changes

ishantiw added 🏗️ in progress and removed 👁️ pending review labels Jan 21, 2019

shuse2 suggested changes Jan 21, 2019

View reviewed changes

ishantiw added 2 commits January 22, 2019 15:49

♻️ Refactor emit event in handlers and rename sanitize->validate

dd16e9d

♻️ Use ProtocolRPCRequestPacket instead of P2PRequest and rename sant…

9461ea0

…ization.ts -> validation.ts in test

ishantiw force-pushed the 1017-add_more_funcs_in_sanitization_to_handler branch from 3c419c0 to 9461ea0 Compare January 22, 2019 15:01

ishantiw added the 👁️ pending review label Jan 22, 2019

ishantiw removed the 🏗️ in progress label Jan 22, 2019

jondubois approved these changes Jan 22, 2019

View reviewed changes

shuse2 approved these changes Jan 22, 2019

View reviewed changes

shuse2 merged commit 5a82de1 into add_p2p_experiment Jan 22, 2019

shuse2 deleted the 1017-add_more_funcs_in_sanitization_to_handler branch January 22, 2019 22:29

LiskArchive added ✅ ready and removed 👁️ pending review labels Jun 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage - Closes#1017 #1020

Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage - Closes#1017 #1020

ishantiw commented Jan 18, 2019

shuse2 Jan 19, 2019

shuse2 Jan 19, 2019

shuse2 Jan 19, 2019

ishantiw commented Jan 21, 2019 •

edited

Loading

jondubois Jan 21, 2019

jondubois Jan 21, 2019

jondubois Jan 21, 2019 •

edited

Loading

shuse2 Jan 21, 2019

ishantiw Jan 22, 2019

shuse2 Jan 22, 2019

ishantiw Jan 22, 2019

shuse2 Jan 21, 2019

ishantiw Jan 22, 2019

ishantiw commented Jan 22, 2019

Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage - Closes#1017 #1020

Add functions in sanitization to handler ProtocolRPCRequest and ProtocolMessage - Closes#1017 #1020

Conversation

ishantiw commented Jan 18, 2019

Description

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondubois Jan 21, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Jan 22, 2019

ishantiw commented Jan 21, 2019 •

edited

Loading

jondubois Jan 21, 2019 •

edited

Loading