Skip to content
This repository has been archived by the owner on Jul 24, 2019. It is now read-only.

verifyRequestSignature does not consider body-parser's req.rawBody #85

Closed
4 of 9 tasks
ErwinAI opened this issue Jan 12, 2019 · 7 comments
Closed
4 of 9 tasks

verifyRequestSignature does not consider body-parser's req.rawBody #85

ErwinAI opened this issue Jan 12, 2019 · 7 comments

Comments

@ErwinAI
Copy link
Contributor

ErwinAI commented Jan 12, 2019

Description

The current implementation of verifying the request signature does not take into account that body-parser package does expose a raw body as the request.rawBody variable.

Currently, both the node-slack-events-api and the node-slack-interactive-messages api use the same code. I've tried using both API's with Google Cloud Functions (and Firebase Functions, built on top of that service). Unfortunately, the request is pre-parsed before it even reaches the Cloud Function. This causes the slack middleware to deny attaching the listener. (adapter.js L75).

The raw-body package is used in both node-slack-events-api and the node-slack-interactive-messages api, which takes the request object as a parameter. This package does not work with body-parser's request.rawBody.

Perhaps make use of body-parser's ability to provide a raw body and check if it's there. If not, only then use the raw-body package? This will allow developers to use body-parser in their code, but won't break any existing code depending on node-slack-events-api and node-slack-interactive-messages.

It will also allow Google Cloud Functions (and Firebase Functions) to be used, which probably opens up some interesting opportunities for Slack API.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • 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.
  • I've searched for any related issues and avoided creating a duplicate issue.

Reproducible in:

@slack/events-api version: 2.1.1 (latest)

node version: any

OS version(s): any

Attachments:

https://stackoverflow.com/questions/18710225/node-js-get-raw-request-body-using-express
https://stackoverflow.com/questions/42950561/how-can-i-get-the-raw-request-body-in-a-google-cloud-function
https://cloud.google.com/functions/docs/writing/http#handling_content_types
https://cloud.google.com/functions/docs/writing/http#parsing_http_requests

@aoberoi
Copy link
Contributor

aoberoi commented Jan 15, 2019

Thanks for raising this issue.

I’m hoping you can help clarify something for me. When you say that GCP pre-parses the request body before the application handles it, are you saying that it uses the body-parser middleware to perform the parsing? I can’t find any reference to this in the documentation you linked to above.

I think we could check forreq.rawBody before emitting an error to address this problem.

@aoberoi
Copy link
Contributor

aoberoi commented Jan 15, 2019

Okay I think I understand now. GCP pre-parses the request body (it doesn’t matter how) and then places the raw Buffer on the req.rawBody property.

The issue doesn’t really have anything to do with the body-parser middleware. It’s just that we should look for a rawBody before failing.

@ErwinAI
Copy link
Contributor Author

ErwinAI commented Jan 16, 2019

@aoberoi The parsing method that GCP uses is indeed irrelevant for a solution; only the fact that the raw body is available under req.rawBody is important. Not a significant change, except that in http-handler.js:151, the getRawBody method only takes a request and doesn't check for a rawBody. I'll think about it and propose a change in a few days.

@aoberoi
Copy link
Contributor

aoberoi commented Mar 7, 2019

#90 addressed this!

@anthonysapien
Copy link

Hi, I think I just had trouble with a similar issue with Serverless and AWS Lamda. On Lambda, res.body is populated but it's actually the raw body, so this part fails:

if (req.body && !req.rawBody) {
var error = new Error('Parsing request body prohibits request signature verification');

We may need to check the type of req.body to make sure it's not parsed JSON.

@aoberoi
Copy link
Contributor

aoberoi commented Apr 21, 2019

@anthonysapien thanks for catching this. can you help point me to some documentation from Serverless or AWS Lambda where I can find details about how the body is created? it would be good for reference.

i think we can then create a separate issue (in the new repo for this project) as an enhancement specific to AWS Lambda support and/or Serverless framework support.

@anthonysapien
Copy link

anthonysapien commented Apr 22, 2019

Hi @aoberoi I'm pretty new to both. Here are some docs related to both Serverless and AWS Lambda:
https://serverless.com/framework/docs/providers/aws/

It looks like in this setup, the request.body is actually the rawBody. I.e., it's not JSON.

Your example should run into the issue:
https://github.com/slackapi/node-slack-events-api/tree/master/examples/greet-and-react

Unfortunately, I don't have insight into why Serverless or AWS have set it up that way. I suspect AWS because everything works with a local Serverless environment.

I think your check's intention is to make sure that the request.body is not already parsed JSON, so checking if (typeof req.body === "object" && !req.rawBody) might fix the problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants