feat(HTTPReceiver): add invalidRequestSignatureHandler callback#2827
feat(HTTPReceiver): add invalidRequestSignatureHandler callback#2827mvanhorn wants to merge 9 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% 94.00% +0.41%
==========================================
Files 44 44
Lines 7855 7892 +37
Branches 687 698 +11
==========================================
+ Hits 7352 7419 +67
+ Misses 498 468 -30
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🦋 Changeset detectedLatest commit: 468f0bd 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 |
zimeg
left a comment
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...
| 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 |
| rawBody: string; | ||
| signature: string; | ||
| ts: number; | ||
| logger: Logger; |
There was a problem hiding this comment.
| logger: Logger; |
🪓 quibble: I'm curious for adding this too but perhaps it's better to revisit in a follow up PR to match for AwsLambdaReceiver as well? This motivates slackapi/node-slack-sdk#1135 more for me!
🔬 note: Please do let me know if implementations without it find error or strange patterns, either in calling code or this package! I'm curious for examples that we can support as well.
…er args Mirror the AwsLambdaReceiver pattern: the default handler reaches for `this.logger` directly instead of plumbing a `logger` argument through the interface. Removes `logger: Logger` from `HTTPReceiverInvalidRequestSignatureHandlerArgs`, the call site in `requestListener`, and the destructure in `defaultInvalidRequestSignatureHandler`. Per @zimeg on PR slackapi#2827, keeping the interface symmetrical with `ReceiverInvalidRequestSignatureHandlerArgs` on `AwsLambdaReceiver` leaves room for a shared interface in a follow-up PR. Tests updated to assert `logger` is no longer present on args; full HTTPReceiver suite (20 tests) passes with build, lint, and type tests green.
|
@zimeg done in 468f0bd. Dropped Leaves the eventual shared-interface refactor to the follow-up PR you mentioned in #1135. Verified: |
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).