-
Notifications
You must be signed in to change notification settings - Fork 427
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 all commits
3ba7a2b
4aa2f01
c2bdaf9
5c78be9
137fe26
6ceb696
3c1f24c
436ae8d
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 |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| --- | ||
| "@slack/bolt": minor | ||
| --- | ||
|
|
||
| feat(HTTPReceiver): add invalidRequestSignatureHandler callback | ||
|
|
||
| Details of a failed request can be parsed and logged with the customized `invalidRequestSignatureHandler` callback for the `HTTPReceiver` receiver: | ||
|
|
||
| ```javascript | ||
| import { App, HTTPReceiver } from "@slack/bolt"; | ||
|
|
||
| const app = new App({ | ||
| token: process.env.SLACK_BOT_TOKEN, | ||
| receiver: new HTTPReceiver({ | ||
| signingSecret: "unexpectedvalue", | ||
| invalidRequestSignatureHandler: (args) => { | ||
| app.logger.warn(args); | ||
| }, | ||
| }), | ||
| }); | ||
| ``` |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,13 @@ 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; | ||||||||||||||||||||||||||||||||||||
| ts: number; | ||||||||||||||||||||||||||||||||||||
| logger: Logger; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+37
to
+42
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. 🐣 note: Here are findings of the adjacent implementation: bolt-js/src/receivers/AwsLambdaReceiver.ts Lines 51 to 57 in a8b7880
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
👁️🗨️ 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... |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer() | ||||||||||||||||||||||||||||||||||||
| const httpsOptionKeys = [ | ||||||||||||||||||||||||||||||||||||
| 'ALPNProtocols', | ||||||||||||||||||||||||||||||||||||
|
|
@@ -81,6 +88,16 @@ export interface HTTPReceiverOptions { | |||||||||||||||||||||||||||||||||||
| logLevel?: LogLevel; | ||||||||||||||||||||||||||||||||||||
| processBeforeResponse?: boolean; | ||||||||||||||||||||||||||||||||||||
| signatureVerification?: boolean; | ||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Called when an incoming request fails signature verification. Override to | ||||||||||||||||||||||||||||||||||||
| * emit custom telemetry, return a specific response body, or suppress the | ||||||||||||||||||||||||||||||||||||
| * default warn log. The receiver still returns `401 Unauthorized` to the | ||||||||||||||||||||||||||||||||||||
| * client regardless of what the handler does. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * Defaults to a handler that logs a warning with the received | ||||||||||||||||||||||||||||||||||||
| * `x-slack-signature` and `x-slack-request-timestamp` values. | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| 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 +154,8 @@ export default class HTTPReceiver implements Receiver { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private signatureVerification: boolean; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private invalidRequestSignatureHandler: (args: HTTPReceiverInvalidRequestSignatureHandlerArgs) => void; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private app?: App; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| public requestListener: RequestListener; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -178,6 +197,7 @@ export default class HTTPReceiver implements Receiver { | |||||||||||||||||||||||||||||||||||
| logLevel = LogLevel.INFO, | ||||||||||||||||||||||||||||||||||||
| processBeforeResponse = false, | ||||||||||||||||||||||||||||||||||||
| signatureVerification = true, | ||||||||||||||||||||||||||||||||||||
| invalidRequestSignatureHandler, | ||||||||||||||||||||||||||||||||||||
| clientId = undefined, | ||||||||||||||||||||||||||||||||||||
| clientSecret = undefined, | ||||||||||||||||||||||||||||||||||||
| stateSecret = undefined, | ||||||||||||||||||||||||||||||||||||
|
|
@@ -195,6 +215,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 ?? | ||||||||||||||||||||||||||||||||||||
| (() => { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -447,7 +469,14 @@ export default class HTTPReceiver implements Receiver { | |||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||
| const e = err as Error; | ||||||||||||||||||||||||||||||||||||
| if (this.signatureVerification) { | ||||||||||||||||||||||||||||||||||||
| this.logger.warn(`Failed to parse and verify the request data: ${e.message}`); | ||||||||||||||||||||||||||||||||||||
| 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) ?? '', | ||||||||||||||||||||||||||||||||||||
| ts: Number(req.headers['x-slack-request-timestamp']) || 0, | ||||||||||||||||||||||||||||||||||||
| logger: this.logger, | ||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| this.logger.warn(`Failed to parse the request body: ${e.message}`); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
@@ -565,4 +594,12 @@ export default class HTTPReceiver implements Receiver { | |||||||||||||||||||||||||||||||||||
| installer.handleCallback(req, res, installCallbackOptions).catch(errorHandler); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private defaultInvalidRequestSignatureHandler(args: HTTPReceiverInvalidRequestSignatureHandlerArgs): void { | ||||||||||||||||||||||||||||||||||||
| const { signature, ts, logger } = args; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| logger.warn( | ||||||||||||||||||||||||||||||||||||
| `Invalid request signature detected (X-Slack-Signature: ${signature}, X-Slack-Request-Timestamp: ${ts})`, | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
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.
🪓 quibble: I'm curious for adding this too but perhaps it's better to revisit in a follow up PR to match for
AwsLambdaReceiveras 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.