-
Notifications
You must be signed in to change notification settings - Fork 441
feat(HTTPReceiver): add invalidRequestSignatureHandler callback #2827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
3ba7a2b
4aa2f01
c2bdaf9
5c78be9
137fe26
6ceb696
3c1f24c
436ae8d
468f0bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,12 @@ import type { ParamsIncomingMessage } from './ParamsIncomingMessage'; | |||||||||
| import { type CustomRoute, type ReceiverRoutes, buildReceiverRoutes } from './custom-routes'; | ||||||||||
| import { verifyRedirectOpts } from './verify-redirect-opts'; | ||||||||||
|
|
||||||||||
| export interface HTTPReceiverInvalidRequestSignatureHandlerArgs { | ||||||||||
| rawBody: string; | ||||||||||
| signature: string | undefined; | ||||||||||
| ts: number | undefined; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer() | ||||||||||
| const httpsOptionKeys = [ | ||||||||||
| 'ALPNProtocols', | ||||||||||
|
|
@@ -81,6 +87,7 @@ export interface HTTPReceiverOptions { | |||||||||
| logLevel?: LogLevel; | ||||||||||
| processBeforeResponse?: boolean; | ||||||||||
| signatureVerification?: boolean; | ||||||||||
| invalidRequestSignatureHandler?: (args: HTTPReceiverInvalidRequestSignatureHandlerArgs) => void; | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📚 suggestion: While it's not shared in other options right now we might add Let's also surface this in documentation here: 🔗 https://docs.slack.dev/tools/bolt-js/reference#receiver-options |
||||||||||
| clientId?: string; | ||||||||||
| clientSecret?: string; | ||||||||||
| stateSecret?: InstallProviderOptions['stateSecret']; // required when using default stateStore | ||||||||||
|
|
@@ -137,6 +144,8 @@ export default class HTTPReceiver implements Receiver { | |||||||||
|
|
||||||||||
| private signatureVerification: boolean; | ||||||||||
|
|
||||||||||
| private invalidRequestSignatureHandler: (args: HTTPReceiverInvalidRequestSignatureHandlerArgs) => void; | ||||||||||
|
|
||||||||||
| private app?: App; | ||||||||||
|
|
||||||||||
| public requestListener: RequestListener; | ||||||||||
|
|
@@ -178,6 +187,7 @@ export default class HTTPReceiver implements Receiver { | |||||||||
| logLevel = LogLevel.INFO, | ||||||||||
| processBeforeResponse = false, | ||||||||||
| signatureVerification = true, | ||||||||||
| invalidRequestSignatureHandler, | ||||||||||
| clientId = undefined, | ||||||||||
| clientSecret = undefined, | ||||||||||
| stateSecret = undefined, | ||||||||||
|
|
@@ -195,6 +205,8 @@ export default class HTTPReceiver implements Receiver { | |||||||||
| this.signingSecret = signingSecret; | ||||||||||
| this.processBeforeResponse = processBeforeResponse; | ||||||||||
| this.signatureVerification = signatureVerification; | ||||||||||
| this.invalidRequestSignatureHandler = | ||||||||||
| invalidRequestSignatureHandler ?? this.defaultInvalidRequestSignatureHandler.bind(this); | ||||||||||
| this.logger = | ||||||||||
| logger ?? | ||||||||||
| (() => { | ||||||||||
|
|
@@ -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}`); | ||||||||||
|
zimeg marked this conversation as resolved.
Outdated
|
||||||||||
| const requestWithRawBody = req as IncomingMessage & { rawBody?: string }; | ||||||||||
| const rawBody = typeof requestWithRawBody.rawBody === 'string' ? requestWithRawBody.rawBody : ''; | ||||||||||
| this.invalidRequestSignatureHandler({ | ||||||||||
| rawBody, | ||||||||||
| signature: req.headers['x-slack-signature'] as string | undefined, | ||||||||||
| ts: req.headers['x-slack-request-timestamp'] ? Number(req.headers['x-slack-request-timestamp']) : undefined, | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
👾 suggestion: This might bring more strict typing? |
||||||||||
| }); | ||||||||||
| } else { | ||||||||||
| this.logger.warn(`Failed to parse the request body: ${e.message}`); | ||||||||||
| } | ||||||||||
|
|
@@ -565,4 +584,8 @@ export default class HTTPReceiver implements Receiver { | |||||||||
| installer.handleCallback(req, res, installCallbackOptions).catch(errorHandler); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private defaultInvalidRequestSignatureHandler(_args: HTTPReceiverInvalidRequestSignatureHandlerArgs): void { | ||||||||||
| // noop - signature verification failure is already logged and a 401 is returned | ||||||||||
|
zimeg marked this conversation as resolved.
Outdated
|
||||||||||
| } | ||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐣 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ 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...