slackapi / node-slack-interactive-messages

Slack Buttons, Menus, and Dialogs made simpler for Node
MIT License
133 stars 41 forks source link

Debugging "Parsing request body prohibits request signature verification" #86

Closed jackellenberger closed 5 years ago

jackellenberger commented 5 years ago

Description

I would like to add interactive messaging to my hubot plugin, but I can't seem to get action handling right. I can create an interactive prompt and I think I'm set up to receive the result (I am receiving something from slack), but the response is consistently throwing Error: Parsing request body prohibits request signature verification, which isn't a particularly descriptive error.

I think I just don't understand the point of that error being thrown - is it encouraging me to write my own middleware? Am I actually parsing the request body somewhere inadvertently? Can I write my own middleware while still taking advantage of this library's verification logic?

Here is a minimal setup for what I'm seeing: I can create an interactive message adapter, receive bot activation hooks and present the user with a message, but the action never fires correctly because expressMiddleware() activates and throws the error.

{ createMessageAdapter } = require '@slack/interactive-messages'
adapter = createMessageAdapter(process.env.SLACK_VERIFICATION_TOKEN)

module.exports = (robot) ->
  # This works fine
  robot.respond /ask me a question/, (context) ->
    context.send(
      "answer yes or no",
      attachments: [{
        text: 'yes or no',
        callback_id: 'yes_or_no',
        actions: [{
          name: 'accept',
          text: 'yes',
          value: 'accept',
          type: 'button',
          style: 'primary'
        },{
          name: 'accept',
          text: 'no',
          value: 'deny',
          type: 'button',
          style: 'danger'
        }]
      }]
    )

  # The expressMiddleware is used and throws an error
  robot.router.use('/slack/actions', adapter.expressMiddleware())
  adapter.action 'yes_or_no', (payload, respond) ->
    # We never get to this point
    console.log(payload)

Does anyone see any obvious issues with this?

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

Requirements

aoberoi commented 5 years ago

which isn't a particularly descriptive error

I totally understand. Writing good error messages is probably included when people say that naming things is one of the two hard problems in computer science. I'll try to help.

Am I actually parsing the request body somewhere inadvertently?

Wait, you've already figured it out! How astute. 😎

Okay now that we know what the error means, let's figure out how we might solve this for your app. The key lies in the Hubot source code:

https://github.com/hubotio/hubot/blob/c1c2611fc826d9419d3224f959d33b260521009c/src/robot.js#L444-L454

Hubot uses a suite of body parsers to process incoming requests, all before our middleware, or your request handler get to run any code. This is a design problem in Hubot.

Why can't this middleware deal with another part of the code (like Hubot and the middleware it adds) parsing the body beforehand? Request verifications requires access to the byte-for-byte exact body because the signature is based on that. In the process of parsing a body, some information is lost (think: JSON.parse('{"a": "b"}') results in the same value as JSON.parse(' {"a":"b"} ') but don't verify the same because the bytes are different - the latter includes spaces).

Okay, so what are our options?

  1. Advocate for the need for a change in the Hubot project. You're welcome to file an issue with them and mention that being able to remove (or reconfigure) middleware in the router would be really useful.

  2. Add an HTTP server (and/or express app) in your bot, and tell Hubot to disable its server (use the --disable-httpd command line option). This means you'll follow the instructions in our README like anyone else. You could probably use the EXPRESS_PORT or PORT as your port, so it doesn't change that behavior from Hubot. The downside to this would be that if you're using Hubot's router for other things, you will have to find a way to do those things without it (shouldn't be that hard to port to a plain express app, if you know what you're doing - and we can point you to some tutorials if not).

Thanks for sticking with us and filing this issue.

jackellenberger commented 5 years ago

Thanks for your thorough explanation @aoberoi! I'll see what I can cook up from your recommendations.

patcon commented 5 years ago

Hubot uses a suite of body parsers to process incoming requests, all before our middleware, or your request handler get to run any code. This is a design problem in Hubot.

cc @technicalpickles <3 (sorry, just a lazy cc for now, but will try to come back. pls skip if you're busy and not seeing anything that is interesting to you!)

aoberoi commented 5 years ago

@patcon Hi there 👋 Is there something we (maintainers of both this package and the hubut-slack adapter) can help with?

patcon commented 5 years ago

Thanks @aoberoi :) Just trying to figure out the minimal change to hubot core that would allow this package to work (with some upgrades): https://gitlab.olindata.com/olindata/hubot-interactive-messages/tree/master

Forked here: https://github.com/patcon/hubot-interactive-messages/tree/update-for-v1.0.0

patcon commented 5 years ago

And commenting out these 3 lines (for fellow travellers trying to get a code spike working):

https://github.com/hubotio/hubot/blob/c1c2611fc826d9419d3224f959d33b260521009c/src/robot.js#L446-L450

patcon commented 5 years ago

Any thoughts on a decent approach that might be acceptable to hubot core? I'm unclear exactly how middleware works in express, but if middleware can include other middleware, then could maybe make a wrapper middleware that skips those 3 on certain configurable conditions (like if req.headers['x-slack-signature'] exists, or something). Not sure! Any advice appreciated :)

malkomalko commented 5 years ago

You can re-arrange the order of the middleware. The following worked for me:

# do your normal setup and add your two middleware for actions/options

# create a copy of the express middleware stack
middleware = robot.router._router.stack.slice()

# move the options middleware to the front
middleware.splice(0, 0, middleware.splice(middleware.length - 1, 1)[0])

# move the actions middleware to the front
middleware.splice(0, 0, middleware.splice(middleware.length - 1, 1)[0])

# reassign the middleware stack
robot.router._router.stack = middleware