Fix #192 ExpressReceiver support for rawBody for signature verification #197

seratch · 2019-05-19T19:10:50Z

Summary

This pull request fixes issue #192 which prevents Bolt running on Google Cloud Platform.

I've extracted verifySlackRequest method from ExpressReceiver as ExpressSignatureVerifier and modified it a litle. Also, I changed the body parser parseBody in ExpressReceiver to handle rawBody. I'll add line comments to explain the intention further.

I haven't added unit tests for this change yet but I made sure the change works on real GCP environment. ~~My example app on GCP managed to respond to url_verification requests.~~
UPDATE: My example app on GCP responded to url_verification requests and handled events as I expect.

Separately from that, it seems that we still have another issue after the body parsing step. My app on GCP didn't work for event subscription (probably other requests, too) due to ack timeout errors. I've not figured out the root cause yet, but it seems to be hanging here.
^ UPDATE: Never mind. It was just my personal projects' quota issue...

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2019-05-19T19:12:53Z

Codecov Report

Merging #197 into master will increase coverage by 4.06%.
The diff coverage is 80.48%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   63.82%   67.88%   +4.06%     
==========================================
  Files           7        7              
  Lines         445      464      +19     
  Branches      122      126       +4     
==========================================
+ Hits          284      315      +31     
+ Misses        153      139      -14     
- Partials        8       10       +2

Impacted Files	Coverage Δ
src/App.ts	`83.66% <100%> (ø)`	⬆️
src/ExpressReceiver.ts	`55.23% <80%> (+23.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ca9d5...7799510. Read the comment docs.

seratch · 2019-05-19T19:11:23Z

src/ExpressReceiver.ts

@@ -40,7 +39,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
    this.server = createServer(this.app);

    const expressMiddleware: RequestHandler[] = [
-      verifySlackRequest(signingSecret),
+      ExpressSignatureVerifier.create(signingSecret),


If you prefer other names, please let me know. I don't have any strong opinions on this.

seratch · 2019-05-19T19:12:16Z

src/ExpressReceiver.ts

  } else {
-    // TODO: should we check the content type header to make sure its JSON here?


Worked on this TODO but I'm willing to revert/modify this.

seratch · 2019-05-19T19:14:50Z

src/ExpressSignatureVerifier.ts

+                        'Slack request signing verification failed. Timestamp is too old.',
+                        ErrorCode.ExpressReceiverAuthenticityError,
+                    );
+                    return next(error);


I realized that, even in this case, verifySlackRequest showed the following other errors. So, I added return here. If having return here has been intentionally avoided, let me know.

Good catch! This must have been a bug.

seratch · 2019-05-19T19:15:20Z

src/ExpressSignatureVerifier.ts

+
+                const signature = req.headers['x-slack-signature'] as string;
+                const ts = Number(req.headers['x-slack-request-timestamp']);
+                if (!signature && !ts) {


This is a newly added validation.

seratch · 2019-05-19T19:15:45Z

src/ExpressSignatureVerifier.ts

+                        'Slack request signing verification failed. Signature mismatch.',
+                        ErrorCode.ExpressReceiverAuthenticityError,
+                    );
+                    return next(error);


same as above

Again, a good catch! In the past, TypeScript has complained when I return the value returned by next(), and my solution was to just return; on its own on the next line. I'm surprised this is passing the type checker, but great!

seratch · 2019-05-19T23:10:17Z

src/ExpressReceiver.ts

+  if (preparsedRawBody) {
+      stringBody = preparsedRawBody.toString();
+  } else {
+      stringBody = rawBody(req).toString();


A bug here. I'm writing tests and will fix this code, too.

seratch · 2019-05-20T15:55:44Z

This pull request is now ready to be reviewed. I avoided doing force-push and pushed several commits. Please squash them into one when merging it.

aoberoi · 2019-05-20T21:45:46Z

src/ExpressSignatureVerifier.ts

+
+export default class ExpressSignatureVerifier {
+
+    public static create(signingSecret: string): RequestHandler {


Can you help me understand the benefit of using a class with a static method for this functionality? I totally see the benefit of encapsulating this into a testable unit of code.

In my experience with the JavaScript community, using a class in this case is not idiomatic. There's no instance with state, so this can be effectively represented with just a function, right? It seems like signingSecret is state of an "instance" of the returned function, so if we wanted to use OOP design you should initialize an instance with a signingSecret and then use a method as the interface with the Express router. But I wouldn't recommend an OOP design here. It seems like a higher order function (as create() already is) is totally fine without embedding in a class.

At that point I don't see any benefit of moving this code out of ExpressReceiver.ts, since it is tightly coupled to Express.

Thank you for the comment. Yes, I have to admit this seems to be in a very other-lang style. 😅

Anyway, as we discussed here, I'll merge this functionality into a single RequestHandler (which means merging this verification and parsing body process into one).

aoberoi · 2019-05-20T21:52:07Z

src/ExpressReceiver.ts

@@ -151,58 +150,52 @@ const respondToUrlVerification: RequestHandler = (req, res, next) => {
  next();
 };

-// TODO: this should be imported from another package


I realize this TODO comment could have been confusing, and might have led to the choice of creating an ExpressSignatureVerifier class. I'd like to clarify.

I meant that we should produce another package, maybe inside the Node Slack SDK repo, which could export a function like verifyRequestSignature(), as seen in the @slack/events-api package: https://github.com/slackapi/node-slack-sdk/blob/8d3b2fdef099306732b344c7e030dd09c82dd91b/packages/events-api/src/http-handler.js#L22-L58

It might also export other useful utilities or functions. One idea would be a to expose an express middleware like the function below this line, which uses the verifyRequestSignature() described above. If this package was distributed separately, (maybe @slack/verify-request), then people who don't use Bolt, or Express, or any other tool we provide could still use our request verification logic, and we would only have to maintain it once (instead of several places like it is today).

If this package was distributed separately, (maybe @slack/verify-request), then people who don't use Bolt, or Express, or any other tool we provide could still use our request verification logic, and we would only have to maintain it once (instead of several places like it is today).

Sounds great. In this pull request, let me go with a necessary and sufficient patch for the issue happening on GCP. Afterwards, I'd love to be involved in that improvement, too.

Great! I agree that we can come back on a separate pass to complete the extraction into a new package.

aoberoi · 2019-05-20T21:52:44Z

src/ExpressReceiver.ts

+ * This handler parses `req.body` or `req.rawBody`(on Google Could Platform)
+ * and overwrites `req.body` with the parsed JS object.
+ * Following middlewares can expect `req.body` is no long a string value.
+ */


These comments are excellent!

aoberoi · 2019-05-20T21:54:22Z

src/ExpressReceiver.ts

+  // On some environments like GCP (Google Cloud Platform),
+  // req.body can be pre-parsed and be passed as req.rawBody here
+  const preparsedRawBody: any = (req as any).rawBody;
+  if (preparsedRawBody) {


We try not to use truthiness or falsiness in conditions like this. Instead, we can use preparsedRawBody !== undefined.

aoberoi · 2019-05-20T22:06:56Z

src/ExpressReceiver.ts

+ * Following middlewares can expect `req.body` is no long a string value.
+ */
+const parseBody: RequestHandler = async (req, _res, next) => {
+  const stringBody: string = await extractRequestBodyAsString(req);


why is this being called here instead of just accessing req.body? it seems that the ExpressSignatureVerifier will call this same function first, and assign a value back to req.body.

In fact, I think there are two concepts that we're trying to separate, but they are very coupled: request verification, and body parsing. Since they are so coupled, would it make sense to implement them in one RequestHandler? The one handler could call a helper function that knows how to verify the signature given a body Buffer, and a signing secret, a current time, and the values from the request header (signature and timestamp). Once the signature is verified, the Buffer contents can be trusted, so it's safe to parse the Buffer into an object and assign the result to req.body. I feel like the intermediate string representation isn't useful, and can be eliminated.

Thank you for the clear explanation. Now I understand the point, and totally agree with the plan described here. I was also thinking that having temporary string representation is a bit confusing and actually has no benefit. I'm going to merge the two into one RequestHandler named verifySignatureAndParseBody (let me know if you have a better name for it).

aoberoi · 2019-05-20T22:10:45Z

src/ExpressReceiver.ts

  } else {
-    // TODO: should we check the content type header to make sure its JSON here?
-    req.body = JSON.parse(req.body);
+    console.warn(`Unexpected content-type detected: ${contentType}`);


This is a great idea! Let's try not to use the console global from this file, and instead use a Logger instance. Since there isn't one available directly here, maybe this sort of concern should be the responsibility of the ExpressReceiver class (we can add a logger option to the constructor).

Thanks for the suggestion. Will do.

aoberoi · 2019-05-20T22:10:52Z

src/ExpressReceiver.ts

+      // Parse this body anyway
+      req.body = JSON.parse(stringBody);
+    } catch (e) {
+      console.error(`Failed to parse body as JSON data for content-type: ${contentType}`)


Same as above.

aoberoi · 2019-05-20T22:12:07Z

src/ExpressSignatureVerifier.ts

+                        'Slack request signing verification failed. Timestamp is too old.',
+                        ErrorCode.ExpressReceiverAuthenticityError,
+                    );
+                    return next(error);


Good catch! This must have been a bug.

aoberoi · 2019-05-20T22:13:25Z

src/ExpressSignatureVerifier.ts

+                        'Slack request signing verification failed. Signature mismatch.',
+                        ErrorCode.ExpressReceiverAuthenticityError,
+                    );
+                    return next(error);


Again, a good catch! In the past, TypeScript has complained when I return the value returned by next(), and my solution was to just return; on its own on the next line. I'm surprised this is passing the type checker, but great!

aoberoi · 2019-05-20T22:14:57Z

@seratch This is really thorough and impressive. I have some comments and questions regarding the design, so I left them in this review. I did not look too closely at the tests because I think you might agree with some of the feedback, and those tests might change along with the design. Happy to hear your thoughts.

seratch · 2019-05-21T00:57:42Z

@aoberoi Thank you very much for your generous review. I've updated the PR again.

Update summary:

I merged verifySlackRequest and parseBody into unified RequestHandler named verifySignatureAndParseBody. One thing I should notice you is that I exported the function because I wanted to have tests for it. If you have suggestions or alternatives, please let me know.
I added logger as an optional arg to ExpressReceiverOptions and ExpressReceiver's constructor args + use it to output warnings/errors
The function verifyRequestSignature is not ready to be extracted as a separate module but it's not exposed to external. Therefore, we can freely change the interface when extracting it as a reusable module.

aoberoi · 2019-05-24T22:02:22Z

src/ExpressReceiver.ts

    signingSecret = '',
-    endpoints = { events: '/slack/events' },
+    logger = new ConsoleLogger(),


Can App pass in a logger when initializing this object? That way the logLevel is respected all the way down.

Thank you for pointing out this. I should have been more careful about it... Anyway, I've updated the branch to fix this.

aoberoi · 2019-05-24T22:02:57Z

This looks so good! Thanks especially for designing a testing solution for Express-related concerns, that looked tricky.

…rification

seratch requested a review from aoberoi May 19, 2019 19:10

seratch commented May 19, 2019

View reviewed changes

seratch force-pushed the issue-192-raw-body branch from 61b22ec to 698f283 Compare May 20, 2019 01:17

seratch changed the title ~~Fix #192 ExpressReceiver support for rawBody for signature verification~~ WIP: Fix #192 ExpressReceiver support for rawBody for signature verification May 20, 2019

seratch changed the title ~~WIP: Fix #192 ExpressReceiver support for rawBody for signature verification~~ Fix #192 ExpressReceiver support for rawBody for signature verification May 20, 2019

aoberoi suggested changes May 20, 2019

View reviewed changes

seratch force-pushed the issue-192-raw-body branch from 7efc90b to 6903778 Compare May 21, 2019 05:42

seratch requested a review from aoberoi May 21, 2019 05:43

seratch force-pushed the issue-192-raw-body branch from 6903778 to 6cea077 Compare May 21, 2019 06:03

aoberoi suggested changes May 24, 2019

View reviewed changes

seratch added 2 commits May 24, 2019 15:43

Fix slackapi#192 ExpressReceiver support for rawBody for signature ve…

c169a2c

…rification

Pass logger from App to ExpressReceiver

7799510

seratch force-pushed the issue-192-raw-body branch from 13ca595 to 7799510 Compare May 24, 2019 22:47

aoberoi approved these changes May 24, 2019

View reviewed changes

aoberoi merged commit 77431a7 into slackapi:master May 24, 2019

seratch deleted the issue-192-raw-body branch June 20, 2020 00:10

JozGonzalez mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities JozGonzalez/bolt-js#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #192 ExpressReceiver support for rawBody for signature verification #197

Fix #192 ExpressReceiver support for rawBody for signature verification #197

seratch commented May 19, 2019 •

edited

Loading

codecov bot commented May 19, 2019 •

edited

Loading

seratch May 19, 2019

seratch May 19, 2019

seratch May 19, 2019

aoberoi May 20, 2019

seratch May 19, 2019

seratch May 19, 2019

aoberoi May 20, 2019

seratch May 19, 2019

seratch commented May 20, 2019

aoberoi May 20, 2019

seratch May 20, 2019

aoberoi May 20, 2019

seratch May 20, 2019

aoberoi May 20, 2019

aoberoi May 20, 2019

aoberoi May 20, 2019

aoberoi May 20, 2019

seratch May 20, 2019

aoberoi May 20, 2019

seratch May 20, 2019

aoberoi May 20, 2019

aoberoi May 20, 2019

aoberoi May 20, 2019

aoberoi commented May 20, 2019 •

edited

Loading

seratch commented May 21, 2019

aoberoi May 24, 2019

seratch May 24, 2019

aoberoi commented May 24, 2019

		} else {
		// TODO: should we check the content type header to make sure its JSON here?


		export default class ExpressSignatureVerifier {

		public static create(signingSecret: string): RequestHandler {

Fix #192 ExpressReceiver support for rawBody for signature verification #197

Fix #192 ExpressReceiver support for rawBody for signature verification #197

Conversation

seratch commented May 19, 2019 • edited Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented May 19, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

seratch commented May 20, 2019

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

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

aoberoi commented May 20, 2019 • edited Loading

seratch commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi commented May 24, 2019

seratch commented May 19, 2019 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

codecov bot commented May 19, 2019 •

edited

Loading

aoberoi commented May 20, 2019 •

edited

Loading