Add an optional handler for invalid request signature error

[dophsquare]

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]

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

[codecov]

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]

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]

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]

@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]

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]

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]

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]

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.

[dophsquare]

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

[seratch]

Thank you! LGTM