Fix #648 Option to disable signature verification by seratch · Pull Request #1088 · slackapi/bolt-js

seratch · 2021-08-26T06:05:13Z

Summary

This pull request resolves #648 by introducing a new option signatureVerification: boolean in App, HTTPReceiver, and ExpressReceiver. You can verify if the option works with the following example apps:

const { App, ExpressReceiver } = require('@slack/bolt');

const app = new App({
  logLevel: 'debug',
  token: process.env.SLACK_BOT_TOKEN,
  signingSecret: 'xxx',
  signatureVerification: false,
});

const app = new App({
  logLevel: 'debug',
  token: process.env.SLACK_BOT_TOKEN,
  receiver: new ExpressReceiver({
    signingSecret: 'xxx',
    signatureVerification: false,
  }),
});

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 · 2021-08-26T06:07:01Z

Codecov Report

Merging #1088 (0bf5beb) into main (2f463ce) will decrease coverage by 0.61%.
The diff coverage is 40.74%.

❗ Current head 0bf5beb differs from pull request most recent head cbb5be5. Consider uploading reports for the commit cbb5be5 to get more accurate results

@@            Coverage Diff             @@
##             main    #1088      +/-   ##
==========================================
- Coverage   70.16%   69.55%   -0.62%     
==========================================
  Files          13       13              
  Lines        1257     1281      +24     
  Branches      369      377       +8     
==========================================
+ Hits          882      891       +9     
- Misses        305      319      +14     
- Partials       70       71       +1

Impacted Files	Coverage Δ
src/receivers/verify-request.ts	`13.51% <0.00%> (-0.78%)`	⬇️
src/receivers/ExpressReceiver.ts	`59.06% <31.57%> (-3.22%)`	⬇️
src/receivers/HTTPReceiver.ts	`32.27% <66.66%> (+0.72%)`	⬆️
src/App.ts	`83.51% <100.00%> (+0.08%)`	⬆️

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 2f463ce...cbb5be5. Read the comment docs.

seratch · 2021-08-26T06:06:31Z

  next();
 };

+export function verifySignatureAndParseRawBody(


Wanted to rename the builder method to buildVerificationBodyParserMiddleware internally but we still need to keep exporting this one for backward compatibility

seratch · 2021-08-26T06:07:18Z

  signingSecret?: HTTPReceiverOptions['signingSecret'];
  endpoints?: HTTPReceiverOptions['endpoints'];
  processBeforeResponse?: HTTPReceiverOptions['processBeforeResponse'];
+  requestVerification?: HTTPReceiverOptions['requestVerification'];


There may be a better name for this flag. Let me know if you have any suggestions.

What about signatureVerification? It feels a bit more specific.

@filmaj That's also an option! But strictly speaking, the validation checks not only the signature but also others like x-slack-request-timestamp: https://api.slack.com/authentication/verifying-requests-from-slack Thus, I'm thinking that "request verification" is slightly more suitable. What do you think?

I immediately thought of signatureVerification as well. But requestVerification also makes sense to me.

Thanks for the inputs! OK, let me change the name and see how it looks!

renamed to signatureVerification ✅

filmaj

LGTM!

Do Bolt or the lower level Slack SDKs have a concept of environments, in the sense of being able to differentiate between a production Slack app, vs. one deployed to a development environment, or even one used in local test suites? The reason I ask is based on the discussion in the original issue, it seems the impetus for this feature is mostly around testing. If testing is the intended use of this feature, perhaps there could be some manner of environment checks in conjunction with this flag to ensure that users don't use this flag for a production application? Just thinking out loud; acceptable answers include "no" 😄

filmaj · 2021-08-26T17:34:16Z

  signingSecret?: HTTPReceiverOptions['signingSecret'];
  endpoints?: HTTPReceiverOptions['endpoints'];
  processBeforeResponse?: HTTPReceiverOptions['processBeforeResponse'];
+  requestVerification?: HTTPReceiverOptions['requestVerification'];


What about signatureVerification? It feels a bit more specific.

seratch · 2021-08-26T21:25:44Z

@filmaj Thanks for the comment!

Do Bolt or the lower level Slack SDKs have a concept of environments, in the sense of being able to differentiate between a production Slack app, vs. one deployed to a development environment, or even one used in local test suites? The reason I ask is based on the discussion in the original issue, it seems the impetus for this feature is mostly around testing.

This option is indeed useful for testing but the main reason why we decided to add this one in the Java SDK first is, as linked from #648, to support the use cases like having a proxy in front of our server that already checks the Slack signatures: slackapi/java-slack-sdk#689 If the motivation is only for easier testing, we might not add this functionality.

If testing is the intended use of this feature, perhaps there could be some manner of environment checks in conjunction with this flag to ensure that users don't use this flag for a production application? Just thinking out loud; acceptable answers include "no" 😄

This sounds a good idea but for the above reason, the need to disable the verification in production on the Bolt app side can be valid. We can trust users decisions and have some clear notice in the document like we do for the Java SDK: https://slack.dev/java-slack-sdk/guides/bolt-basics#customize-the-built-in-middleware-list

seratch · 2021-08-27T00:47:17Z

We are still discussing the naming requestVerification. If you have thoughts on it, feel free to write comments!

mwbrooks

Looks good to me!

Do we want to update the documentation to mention this new option?

mwbrooks · 2021-08-27T03:35:30Z

  signingSecret?: HTTPReceiverOptions['signingSecret'];
  endpoints?: HTTPReceiverOptions['endpoints'];
  processBeforeResponse?: HTTPReceiverOptions['processBeforeResponse'];
+  requestVerification?: HTTPReceiverOptions['requestVerification'];


I immediately thought of signatureVerification as well. But requestVerification also makes sense to me.

filmaj · 2021-08-27T13:19:17Z

@seratch thanks for the information and clarification! Much appreciated

seratch · 2021-08-28T00:08:43Z

Resolved conflicts ✅

seratch added the enhancement M-T: A feature request for new functionality label Aug 26, 2021

seratch added this to the 3.7.0 milestone Aug 26, 2021

seratch self-assigned this Aug 26, 2021

seratch requested review from filmaj, misscoded and srajiang August 26, 2021 06:05

seratch commented Aug 26, 2021

View reviewed changes

filmaj approved these changes Aug 26, 2021

View reviewed changes

mwbrooks approved these changes Aug 27, 2021

View reviewed changes

seratch mentioned this pull request Aug 27, 2021

Update the reference page (initialization options) for v3.7 release #1092

Closed

4 of 10 tasks

seratch added 3 commits August 28, 2021 09:28

Fix #648 Option to disable signature verification

acfd33b

Rename requestVerification to signatureVerification

ad27734

Fix lint errors

cbb5be5

seratch merged commit da3baca into slackapi:main Aug 28, 2021

seratch deleted the issue-648-request-verification branch August 28, 2021 00:32

seratch mentioned this pull request Aug 30, 2021

Fix #454 Add oauth_settings.state_validation_enabled to customize the OAuth flow slackapi/bolt-python#455

Merged

2 of 8 tasks

filmaj mentioned this pull request Apr 18, 2022

Add signature verification option to initialization reference document. #1424

Merged

Conversation

seratch commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements (place an x in each [ ])

Uh oh!

codecov Bot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seratch commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seratch commented Aug 27, 2021

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmaj commented Aug 27, 2021

Uh oh!

seratch commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seratch commented Aug 26, 2021 •

edited

Loading

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

codecov Bot commented Aug 26, 2021 •

edited

Loading

seratch commented Aug 26, 2021 •

edited

Loading