feat(HTTPReceiver): add invalidRequestSignatureHandler callback#2827
feat(HTTPReceiver): add invalidRequestSignatureHandler callback#2827mvanhorn wants to merge 7 commits intoslackapi:mainfrom
Conversation
|
Hey @mvanhorn 👋🏻 This sounds like a reasonable enhancement and thank you the explaining the motivation related to the I'll add a few maintainers to review this PR 👏🏻 In the meantime, can you please add tests for the new changes? I imagine that'll be one of the first requests from the reviewers as well. 🙇🏻 |
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Matt Van Horn <m***@m***.local>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated. |
|
Added tests in b8d77a0 - three cases covering: custom handler receives correct args on signature failure, default noop handler doesn't throw, and missing headers pass undefined for signature/ts. Full suite passes (407 tests). |
|
@mvanhorn Super amazing changes more 🧪 ✨ The most recent commit author might've been configured strange - would it be possible to force push changes with an author that can sign the CLA? 🏛️ Rambles now but slackapi/node-slack-sdk#1135 becomes most curious 👾 |
Adds an optional invalidRequestSignatureHandler to HTTPReceiver, matching the callback added to AwsLambdaReceiver in PR slackapi#2154. When signature verification fails, the handler fires with the raw body, signature header, and timestamp. Defaults to a noop. Refs slackapi#2156
Cover three scenarios: custom handler called with correct args on signature failure, default noop handler doesn't throw, and missing headers pass undefined for signature/ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b8d77a0 to
4aa2f01
Compare
|
Fixed the commit author in 4aa2f01 — both commits now use my GitHub-linked noreply address. CLA should pass on recheck. All 407 tests still passing. Re: node-slack-sdk#1135 — interesting, hadn't seen that. The invalidRequestSignatureHandler pattern here could serve as a stepping stone toward that kind of unified request validation. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2827 +/- ##
==========================================
+ Coverage 93.59% 93.99% +0.39%
==========================================
Files 44 44
Lines 7855 7878 +23
Branches 687 697 +10
==========================================
+ Hits 7352 7405 +53
+ Misses 498 468 -30
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🦋 Changeset detectedLatest commit: 3c1f24c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
@mvanhorn Thanks for the updates to authorship and quick responses! 🎆
I think this approach moves things in a solid direction and want to leave a few comments that might be useful toward slackapi/node-slack-sdk#1135 in immediate improvement 👾
In quick reference:
- Required
tsandsignaturevalues might be easier to reference in application code but I understand values from unexpected requests might be missing - The HTTP interface name might or might not match the AWS interface? I'm curious toward an eventual shared interface 👻
- The placeholder logs might be nice to move into the default implementation so overriding handlers have control of this output?
A note for documentation might also be nice to include, and brings up a callout that we could surface this as a top-level App option as well:
HTTPReceiver options can be passed into the App constructor, just like the Bolt app options.
I'm curious to your thoughts for all of these behaviors! Thanks for bringing this feature so far already too.
| export interface HTTPReceiverInvalidRequestSignatureHandlerArgs { | ||
| rawBody: string; | ||
| signature: string | undefined; | ||
| ts: number | undefined; | ||
| } |
There was a problem hiding this comment.
🐣 note: Here are findings of the adjacent implementation:
bolt-js/src/receivers/AwsLambdaReceiver.ts
Lines 51 to 57 in a8b7880
There was a problem hiding this comment.
| export interface HTTPReceiverInvalidRequestSignatureHandlerArgs { | |
| rawBody: string; | |
| signature: string | undefined; | |
| ts: number | undefined; | |
| } | |
| export interface HTTPReceiverInvalidRequestSignatureHandlerArgs { | |
| rawBody: string; | |
| signature: string; | |
| ts: number; | |
| } |
👁️🗨️ thought: I'd be curious to use default values here and perhaps matching the adjacent interface name, but I'm less confident about the second point...
| @@ -448,6 +460,13 @@ export default class HTTPReceiver implements Receiver { | |||
| const e = err as Error; | |||
| if (this.signatureVerification) { | |||
| this.logger.warn(`Failed to parse and verify the request data: ${e.message}`); | |||
There was a problem hiding this comment.
| this.logger.warn(`Failed to parse and verify the request data: ${e.message}`); |
🪓 suggestion: To move default logging to the handler function! This makes me wonder if we should include logger in the available arguments?
There was a problem hiding this comment.
🔭 note: Here's what a current output might include FWIW:
[WARN] Failed to parse and verify the request data: Failed to verify authenticity: signature mismatch
| private defaultInvalidRequestSignatureHandler(_args: HTTPReceiverInvalidRequestSignatureHandlerArgs): void { | ||
| // noop - signature verification failure is already logged and a 401 is returned |
There was a problem hiding this comment.
| private defaultInvalidRequestSignatureHandler(_args: HTTPReceiverInvalidRequestSignatureHandlerArgs): void { | |
| // noop - signature verification failure is already logged and a 401 is returned | |
| private defaultInvalidRequestSignatureHandler(args: HTTPReceiverInvalidRequestSignatureHandlerArgs): void { | |
| const { signature, ts } = args; | |
| this.logger.warn( | |
| `Invalid request signature detected (X-Slack-Signature: ${signature}, X-Slack-Request-Timestamp: ${ts})`, | |
| ); |
🔬 note: This hopes to mirror the AWS receiver implementation:
bolt-js/src/receivers/AwsLambdaReceiver.ts
Lines 369 to 375 in a8b7880
| signature: req.headers['x-slack-signature'] as string | undefined, | ||
| ts: req.headers['x-slack-request-timestamp'] ? Number(req.headers['x-slack-request-timestamp']) : undefined, |
There was a problem hiding this comment.
| signature: req.headers['x-slack-signature'] as string | undefined, | |
| ts: req.headers['x-slack-request-timestamp'] ? Number(req.headers['x-slack-request-timestamp']) : undefined, | |
| signature: req.headers['x-slack-signature'] as string, | |
| ts: Number(req.headers['x-slack-request-timestamp']), |
👾 suggestion: This might bring more strict typing?
| logLevel?: LogLevel; | ||
| processBeforeResponse?: boolean; | ||
| signatureVerification?: boolean; | ||
| invalidRequestSignatureHandler?: (args: HTTPReceiverInvalidRequestSignatureHandlerArgs) => void; |
There was a problem hiding this comment.
📚 suggestion: While it's not shared in other options right now we might add @jsdoc to this?
Let's also surface this in documentation here:
🔗 https://docs.slack.dev/tools/bolt-js/reference#receiver-options
…handler Per @zimeg: - Tighten HTTPReceiverInvalidRequestSignatureHandlerArgs: signature is now string (default '') and ts is number (default 0) when headers are missing, matching AwsLambdaReceiver's ReceiverInvalidRequestSignatureHandlerArgs shape and removing the 'string | undefined' / 'number | undefined' awkwardness in override handlers. - Add logger to the args so override handlers can emit their own structured output using the receiver's configured logger. - Move the default warn log into defaultInvalidRequestSignatureHandler so overrides can fully suppress or replace the warning instead of having it always emitted before the handler runs. Format mirrors AwsLambdaReceiver's default: 'Invalid request signature detected (X-Slack-Signature: ..., X-Slack-Request-Timestamp: ...)'. - Add jsdoc on the invalidRequestSignatureHandler option describing the default behavior, the 401 response, and the override contract. - Update the two HTTPReceiver.spec.ts test cases that previously asserted 'undefined' signature/ts to check the new defaults ('', 0) and to verify a logger is passed to the handler.
|
Thanks for the careful review @zimeg. Pushed 3c1f24c addressing most of the feedback: Interface shape (
Default handler behavior:
Docs:
Held off on:
Updated the two existing tests that asserted |
Adds invalidRequestSignatureHandler to HTTPReceiver.
Why this matters
PR #2154 added this callback to AwsLambdaReceiver. HTTPReceiver was missing it. Now HTTP apps can hook into signature verification failures too.
Changes
HTTPReceiverInvalidRequestSignatureHandlerArgsinterface. New option inHTTPReceiverOptions. Constructor wiring with noop default. Fires in the signature-verification catch block with rawBody, signature header, and timestamp.ExpressReceiver support can follow in a separate PR.
Testing
Codex ran
npm testagainst the change. All existing tests pass.Refs #2156
This contribution was developed with AI assistance (Claude Code).