Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .changeset/public-gifts-lose.md
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);
},
}),
});
```
39 changes: 38 additions & 1 deletion src/receivers/HTTPReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

}
Comment on lines +37 to +42
Copy link
Copy Markdown
Member

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:

export interface ReceiverInvalidRequestSignatureHandlerArgs {
rawBody: string;
signature: string;
ts: number;
awsEvent: AwsEvent;
awsResponse: Promise<AwsResponse>;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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...


// Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer()
const httpsOptionKeys = [
'ALPNProtocols',
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 @jsdoc to this?

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
Expand Down Expand Up @@ -137,6 +154,8 @@ export default class HTTPReceiver implements Receiver {

private signatureVerification: boolean;

private invalidRequestSignatureHandler: (args: HTTPReceiverInvalidRequestSignatureHandlerArgs) => void;

private app?: App;

public requestListener: RequestListener;
Expand Down Expand Up @@ -178,6 +197,7 @@ export default class HTTPReceiver implements Receiver {
logLevel = LogLevel.INFO,
processBeforeResponse = false,
signatureVerification = true,
invalidRequestSignatureHandler,
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
Expand All @@ -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 ??
(() => {
Expand Down Expand Up @@ -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}`);
}
Expand Down Expand Up @@ -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})`,
);
}
}
121 changes: 121 additions & 0 deletions test/unit/receivers/HTTPReceiver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,127 @@ describe('HTTPReceiver', () => {
});
});

describe('invalidRequestSignatureHandler', () => {
it('should call the custom handler when signature verification fails', async () => {
const spy = sinon.spy();
const fakeParseAndVerify = sinon.fake.rejects(new Error('Signature mismatch'));
const fakeBuildNoBodyResponse = sinon.fake();

const overridesWithFakeVerify = mergeOverrides(overrides, {
'./HTTPModuleFunctions': {
parseAndVerifyHTTPRequest: fakeParseAndVerify,
parseHTTPRequestBody: sinon.fake(),
buildNoBodyResponse: fakeBuildNoBodyResponse,
'@noCallThru': true,
},
});

const HTTPReceiver = importHTTPReceiver(overridesWithFakeVerify);
const receiver = new HTTPReceiver({
signingSecret: 'secret',
logger: noopLogger,
invalidRequestSignatureHandler: spy,
});
assert.isNotNull(receiver);

const fakeReq = sinon.createStubInstance(IncomingMessage) as unknown as IncomingMessage;
fakeReq.url = '/slack/events';
fakeReq.method = 'POST';
fakeReq.headers = {
'x-slack-signature': 'v0=bad',
'x-slack-request-timestamp': '1234567890',
};
(fakeReq as IncomingMessage & { rawBody?: string }).rawBody = '{"token":"test"}';

const fakeRes = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

receiver.requestListener(fakeReq, fakeRes);

// Wait for the async closure inside handleIncomingEvent to settle
await new Promise((resolve) => setTimeout(resolve, 50));

assert(spy.calledOnce, 'invalidRequestSignatureHandler should be called once');
const args = spy.firstCall.args[0];
assert.equal(args.rawBody, '{"token":"test"}');
assert.equal(args.signature, 'v0=bad');
assert.equal(args.ts, 1234567890);
assert.isDefined(args.logger, 'logger should be passed to the handler');
});

it('should use the default noop handler when no custom handler is provided', async () => {
const fakeParseAndVerify = sinon.fake.rejects(new Error('Signature mismatch'));
const fakeBuildNoBodyResponse = sinon.fake();

const overridesWithFakeVerify = mergeOverrides(overrides, {
'./HTTPModuleFunctions': {
parseAndVerifyHTTPRequest: fakeParseAndVerify,
parseHTTPRequestBody: sinon.fake(),
buildNoBodyResponse: fakeBuildNoBodyResponse,
'@noCallThru': true,
},
});

const HTTPReceiver = importHTTPReceiver(overridesWithFakeVerify);
const receiver = new HTTPReceiver({
signingSecret: 'secret',
logger: noopLogger,
});

const fakeReq = sinon.createStubInstance(IncomingMessage) as unknown as IncomingMessage;
fakeReq.url = '/slack/events';
fakeReq.method = 'POST';
fakeReq.headers = {};

const fakeRes = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

// Should not throw even without a custom handler
receiver.requestListener(fakeReq, fakeRes);
await new Promise((resolve) => setTimeout(resolve, 50));

sinon.assert.calledOnce(fakeBuildNoBodyResponse);
sinon.assert.calledWith(fakeBuildNoBodyResponse, fakeRes, 401);
});

it('should pass empty signature and zero ts when headers are missing', async () => {
const spy = sinon.spy();
const fakeParseAndVerify = sinon.fake.rejects(new Error('Signature mismatch'));
const fakeBuildNoBodyResponse = sinon.fake();

const overridesWithFakeVerify = mergeOverrides(overrides, {
'./HTTPModuleFunctions': {
parseAndVerifyHTTPRequest: fakeParseAndVerify,
parseHTTPRequestBody: sinon.fake(),
buildNoBodyResponse: fakeBuildNoBodyResponse,
'@noCallThru': true,
},
});

const HTTPReceiver = importHTTPReceiver(overridesWithFakeVerify);
const receiver = new HTTPReceiver({
signingSecret: 'secret',
logger: noopLogger,
invalidRequestSignatureHandler: spy,
});

const fakeReq = sinon.createStubInstance(IncomingMessage) as unknown as IncomingMessage;
fakeReq.url = '/slack/events';
fakeReq.method = 'POST';
fakeReq.headers = {};

const fakeRes = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

receiver.requestListener(fakeReq, fakeRes);
await new Promise((resolve) => setTimeout(resolve, 50));

assert(spy.calledOnce);
const args = spy.firstCall.args[0];
assert.equal(args.rawBody, '');
assert.equal(args.signature, '');
assert.equal(args.ts, 0);
assert.isDefined(args.logger);
});
});

it("should throw if request doesn't match install path, redirect URI path, or custom routes", async () => {
const installProviderStub = sinon.createStubInstance(InstallProvider);
const HTTPReceiver = importHTTPReceiver(overrides);
Expand Down