Add an optional handler for invalid request signature error #2154

dophsquare · 2024-07-01T20:48:11Z

Summary

Describe the goal of this PR. Mention any related Issue numbers.

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.

salesforce-cla · 2024-07-01T20:48:15Z

Thanks for the contribution! Before we can merge this, we need @dophsquare to sign the Salesforce Inc. Contributor License Agreement.

codecov · 2024-07-01T22:32:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.07%. Comparing base (8e49f7b) to head (3fa248c).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2154      +/-   ##
==========================================
+ Coverage   82.00%   82.07%   +0.06%     
==========================================
  Files          18       18              
  Lines        1539     1545       +6     
  Branches      442      443       +1     
==========================================
+ Hits         1262     1268       +6     
  Misses        179      179              
  Partials       98       98

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zimeg

Hey @dophsquare 👋 Thanks for sending this in with tests to verify correctness - that's super appreciated! 🙌

This PR is looking great but I'm wondering if we need to support more than just AwsLambdaReceiver with this change? Open to thoughts on this but I'd push for supporting all receivers that verify signatures in this PR for completeness ✨

We'll also want to include this option in documentation - perhaps with some JSDoc too - so that it can be discovered easily on slack.dev! 📚

Hopefully this sounds alright to you, but please let me know if you think otherwise or have trouble setting up documentation or anything else!

zimeg · 2024-07-01T22:34:51Z

src/receivers/HTTPModuleFunctions.ts

+export interface ReceiverInvalidRequestSignatureHandlerArgs {
+  error: Error;
+}


Unsure if this is the most ideal place for an export - it seems fine but open to other suggestions!

Placing it here does also raise question around if we want to include these checks for this.invalidRequestSignatureHandler in other receivers like the ExpressReceiver and HTTPReceiver. I'm thinking now'd be a good time to do so to avoid confusion around similar App initializations with a different receiver.

dophsquare · 2024-07-01T23:57:01Z

ReceiverInvalidRequestSignatureHandlerArgs

Hi @zimeg thanks for reviewing. I'll add the documentation. I think I can add a similar handler to HTTPReceiver. For ExpressReceiver, if I read correctly, there's already a way to handle error through ReceiverAuthenticityError.

https://github.com/slackapi/bolt-js/blob/main/src/receivers/ExpressReceiver.ts#L501

It seems to be incompatible using error handlers. Let me know what you think.

zimeg · 2024-07-02T00:18:39Z

@dophsquare for sure! Thanks for the fast follow up too 🙏

For ExpressReceiver, if I read correctly, there's already a way to handle error through ReceiverAuthenticityError.

https://github.com/slackapi/bolt-js/blob/main/src/receivers/ExpressReceiver.ts#L501

The thrown ReceiverAuthenticityError gives us a way to catch this error from some calling function to log (or otherwise process things) as needed. It's a bit tangled, but it's also a shorthand for returning errors 😉

I was thinking the same invalidRequestSignatureHandler pattern can be used after logging errors but before returning a response for the HTTPReceiver and ExpressReceiver handlers. The explicit Error type shouldn't prevent the invalid signature handler since it's a void function, but please correct me if that's not right! 😅

seratch

Thanks for suggesting this and taking the time to try to implement it!

seratch · 2024-07-02T00:32:11Z

src/receivers/AwsLambdaReceiver.ts

@@ -7,6 +7,7 @@ import App from '../App';
 import { Receiver, ReceiverEvent } from '../types/receiver';
 import { ReceiverMultipleAckError } from '../errors';
 import { StringIndexed } from '../types/helpers';
+import { ReceiverInvalidRequestSignatureHandlerArgs } from './HTTPModuleFunctions';


It's fine to have this args type for HTTPReceiver and ExpressReceiver as they both are compatible with Node.js HTTP modules. However, AWS Lambda does not receive the same. It receives AWS-specific data structure.

Therefore, please avoid importing this into this class even though it just works right now. Instead, you can define the specific args type for this receiver. I will mention the details in the following comment.

seratch · 2024-07-02T00:39:17Z

src/receivers/AwsLambdaReceiver.ts

          this.logger.info(`Invalid request signature detected (X-Slack-Signature: ${signature}, X-Slack-Request-Timestamp: ${ts})`);
+          if (this.invalidRequestSignatureHandler) {
+            this.invalidRequestSignatureHandler({
+              error: new Error('Invalid request signature'),
+            });
+          }
          return Promise.resolve({ statusCode: 401, body: '' });


These lines of code should be extracted as defaultInvalidRequestSignatureHandler within this source file. Returning Promise<AwsResponse> would be better here. No need to make it compatible with HTTPFuntions as this class does not use it at all.

When adding the same to HTTPFunctions, the compatibility with others should be well-considered. The return type should be void for it.

Regarding the args type, the one for AwsLambdaReceiver should have not only the exception but also other available ones like rawBody, signature, ts for logging / error analysis on the developer side. For HTTPFunctions, others like req: IncomingMessage, res: ServerResponse will be necessary for flexibility.

dophsquare · 2024-07-02T18:05:02Z

Hi @seratch, thanks for clarifying the differences between the AWS receiver and the HTTP receiver. I refactored the code based on your suggestions. Can you check if I'm in the right direction?

@zimeg, I tried to add a similar handler to HTTPReceiver. But the signature verification seems a bit different. There isn't a spec test for signature verification. I think it's beyond my understanding of the codebase to implement.

seratch

Thanks again for taking the time to work on this

seratch · 2024-07-03T00:23:13Z

src/receivers/AwsLambdaReceiver.ts

-          this.logger.info(`Invalid request signature detected (X-Slack-Signature: ${signature}, X-Slack-Request-Timestamp: ${ts})`);
+          this.invalidRequestSignatureHandler({ rawBody, signature, ts });
          return Promise.resolve({ statusCode: 401, body: '' });


Rather than just wrapping the logging part, the next line Promise.resolve({ statusCode: 401, body: '' }); should be included in the callback too. Some developers may want to customize the status code, headeres, and body content. If we provide the callback for this, the pattern needs to be supported too like we do for other similar callbacks for HTTP/ExpressReceiver.

Hi @seratch I updated a code. Can you have a look?

seratch

Thank you! LGTM

salesforce-cla bot added the cla:missing label Jul 1, 2024

dophsquare mentioned this pull request Jul 1, 2024

How to handle errors with invalid signing secret? #2143

Closed

salesforce-cla bot added cla:signed and removed cla:missing labels Jul 1, 2024

zimeg added enhancement M-T: A feature request for new functionality semver:minor labels Jul 1, 2024

zimeg added this to the 3.20.0 milestone Jul 1, 2024

zimeg linked an issue Jul 1, 2024 that may be closed by this pull request

How to handle errors with invalid signing secret? #2143

Closed

zimeg reviewed Jul 1, 2024

View reviewed changes

seratch reviewed Jul 2, 2024

View reviewed changes

dophsquare force-pushed the main branch from 1775635 to 04820f1 Compare July 2, 2024 18:00

seratch requested changes Jul 3, 2024

View reviewed changes

Add an optional handler for invalid request signature error

3fa248c

dophsquare force-pushed the main branch from 04820f1 to 3fa248c Compare July 3, 2024 21:30

seratch mentioned this pull request Jul 4, 2024

Add invalidRequestSignatureHandler to HTTP/ExpressReceiver #2156

Open

seratch approved these changes Jul 4, 2024

View reviewed changes

seratch merged commit b12db71 into slackapi:main Jul 4, 2024
9 checks passed

This was referenced Sep 5, 2024

[Snyk] Upgrade @slack/bolt from 3.19.0 to 3.21.0 Jrc356/avokudos#37

Closed

[Snyk] Upgrade @slack/bolt from 3.19.0 to 3.21.1 Jrc356/avokudos#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an optional handler for invalid request signature error #2154

Add an optional handler for invalid request signature error #2154

dophsquare commented Jul 1, 2024

salesforce-cla bot commented Jul 1, 2024

codecov bot commented Jul 1, 2024 •

edited

Loading

zimeg left a comment

zimeg Jul 1, 2024

dophsquare commented Jul 1, 2024

zimeg commented Jul 2, 2024

seratch left a comment

seratch Jul 2, 2024 •

edited

Loading

seratch Jul 2, 2024

dophsquare commented Jul 2, 2024

seratch left a comment

seratch Jul 3, 2024

dophsquare Jul 3, 2024

seratch left a comment

Add an optional handler for invalid request signature error #2154

Add an optional handler for invalid request signature error #2154

Conversation

dophsquare commented Jul 1, 2024

Summary

Requirements (place an x in each [ ])

salesforce-cla bot commented Jul 1, 2024

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

zimeg left a comment

Choose a reason for hiding this comment

zimeg Jul 1, 2024

Choose a reason for hiding this comment

dophsquare commented Jul 1, 2024

zimeg commented Jul 2, 2024

seratch left a comment

Choose a reason for hiding this comment

seratch Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

seratch Jul 2, 2024

Choose a reason for hiding this comment

dophsquare commented Jul 2, 2024

seratch left a comment

Choose a reason for hiding this comment

seratch Jul 3, 2024

Choose a reason for hiding this comment

dophsquare Jul 3, 2024

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

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

codecov bot commented Jul 1, 2024 •

edited

Loading

seratch Jul 2, 2024 •

edited

Loading