From be1323e69bf1293fb89cabfc6534593b8be86a63 Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Fri, 17 Jan 2020 17:59:49 -0500 Subject: [PATCH 1/8] Convert receivers to App#processEvent --- src/App.spec.ts | 344 +++++++++++++++++++++++---------- src/App.ts | 69 +++---- src/ExpressReceiver.spec.ts | 31 ++- src/ExpressReceiver.ts | 157 ++++----------- src/conversation-store.spec.ts | 8 +- src/errors.spec.ts | 40 ++++ src/errors.ts | 56 +++++- src/index.ts | 5 +- src/middleware/builtin.spec.ts | 6 +- src/middleware/builtin.ts | 14 +- src/receiver.spec.ts | 124 ++++++++++++ src/receiver.ts | 103 ++++++++++ src/test-helpers.ts | 12 +- src/types/helpers.ts | 4 +- src/types/index.ts | 1 - src/types/middleware.ts | 6 - src/types/receiver.ts | 23 --- src/types/utilities.ts | 3 +- 18 files changed, 649 insertions(+), 357 deletions(-) create mode 100644 src/errors.spec.ts create mode 100644 src/receiver.spec.ts create mode 100644 src/receiver.ts delete mode 100644 src/types/receiver.ts diff --git a/src/App.spec.ts b/src/App.spec.ts index 89de7fb1b..b745821d8 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -1,17 +1,36 @@ // tslint:disable:no-implicit-dependencies import 'mocha'; -import { EventEmitter } from 'events'; import sinon, { SinonSpy } from 'sinon'; import { assert } from 'chai'; import { Override, mergeOverrides, createFakeLogger, delay } from './test-helpers'; import rewiremock from 'rewiremock'; -import { ErrorCode } from './errors'; -import { Receiver, ReceiverEvent, SayFn, NextMiddleware } from './types'; +import { ErrorCode, UnknownError } from './errors'; +import { SayFn, NextMiddleware } from './types'; +import { Receiver, ReceiverEvent } from './receiver'; import { ConversationStore } from './conversation-store'; import { LogLevel } from '@slack/logger'; import App, { ViewConstraints } from './App'; import { WebClientOptions, WebClient } from '@slack/web-api'; +// TODO: swap out rewiremock for proxyquire to see if it saves execution time +// Utility functions +const noop = (() => Promise.resolve(undefined)); +const noopMiddleware = async ({ next }: { next: NextMiddleware; }) => { await next(); }; +const noopAuthorize = (() => Promise.resolve({})); + +// Dummies (values that have no real behavior but pass through the system opaquely) +function createDummyReceiverEvent(): ReceiverEvent { + // NOTE: this is a degenerate ReceiverEvent that would successfully pass through the App. it happens to look like a + // IncomingEventType.Event + return { + body: { + event: { + }, + }, + ack: noop, + }; +} + describe('App', () => { describe('constructor', () => { // TODO: test when the single team authorization results fail. that should still succeed but warn. it also means @@ -82,7 +101,7 @@ describe('App', () => { const App = await importApp(); // tslint:disable-line:variable-name // Act - const app = new App({ receiver: createFakeReceiver(), authorize: noopAuthorize }); + const app = new App({ receiver: new FakeReceiver(), authorize: noopAuthorize }); // Assert assert.instanceOf(app, App); @@ -195,17 +214,17 @@ describe('App', () => { describe('#start', () => { it('should pass calls through to receiver', async () => { // Arrange - const dummyReturn = Symbol(); const dummyParams = [Symbol(), Symbol()]; - const fakeReceiver = createFakeReceiver(sinon.fake.resolves(dummyReturn)); + const fakeReceiver = new FakeReceiver(); const App = await importApp(); // tslint:disable-line:variable-name // Act const app = new App({ receiver: fakeReceiver, authorize: noopAuthorize }); + sinon.spy(app, 'start'); const actualReturn = await app.start(...dummyParams); // Assert - assert.equal(actualReturn, dummyReturn); + assert.deepEqual(actualReturn, dummyParams); assert.deepEqual(dummyParams, fakeReceiver.start.firstCall.args); }); }); @@ -213,9 +232,8 @@ describe('App', () => { describe('#stop', () => { it('should pass calls through to receiver', async () => { // Arrange - const dummyReturn = Symbol(); - const dummyParams = [Symbol(), Symbol()]; - const fakeReceiver = createFakeReceiver(undefined, sinon.fake.resolves(dummyReturn)); + const dummyParams = [1, 2]; + const fakeReceiver = new FakeReceiver(); const App = await importApp(); // tslint:disable-line:variable-name // Act @@ -223,7 +241,7 @@ describe('App', () => { const actualReturn = await app.stop(...dummyParams); // Assert - assert.equal(actualReturn, dummyReturn); + assert.deepEqual(actualReturn, dummyParams); assert.deepEqual(dummyParams, fakeReceiver.stop.firstCall.args); }); }); @@ -234,7 +252,7 @@ describe('App', () => { let dummyAuthorizationResult: { botToken: string, botId: string }; beforeEach(() => { - fakeReceiver = createFakeReceiver(); + fakeReceiver = new FakeReceiver(); fakeErrorHandler = sinon.fake(); dummyAuthorizationResult = { botToken: '', botId: '' }; }); @@ -260,16 +278,15 @@ describe('App', () => { // Act const app = new App({ receiver: fakeReceiver, logger: fakeLogger, authorize: noopAuthorize }); app.use(fakeMiddleware); - for (const event of invalidReceiverEvents) { - fakeReceiver.emit('message', event); - } + await Promise.all(invalidReceiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert(fakeErrorHandler.notCalled); assert(fakeMiddleware.notCalled); assert.isAtLeast(fakeLogger.warn.callCount, invalidReceiverEvents.length); }); - it('should warn, send to global error handler, and skip when a receiver event fails authorization', async () => { + + it('should warn and skip when a receiver event fails authorization', async () => { // Arrange const fakeLogger = createFakeLogger(); const fakeMiddleware = sinon.fake(noopMiddleware); @@ -285,16 +302,13 @@ describe('App', () => { }); app.use(fakeMiddleware); app.error(fakeErrorHandler); - fakeReceiver.emit('message', dummyReceiverEvent); - await delay(); + await fakeReceiver.sendEvent(dummyReceiverEvent); // Assert assert(fakeMiddleware.notCalled); assert(fakeLogger.warn.called); - assert.instanceOf(fakeErrorHandler.firstCall.args[0], Error); - assert.propertyVal(fakeErrorHandler.firstCall.args[0], 'code', ErrorCode.AuthorizationError); - assert.propertyVal(fakeErrorHandler.firstCall.args[0], 'original', dummyAuthorizationError); }); + describe('global middleware', () => { let fakeFirstMiddleware: SinonSpy; let fakeSecondMiddleware: SinonSpy; @@ -315,22 +329,11 @@ describe('App', () => { fakeFirstMiddleware = sinon.fake(noopMiddleware); fakeSecondMiddleware = sinon.fake(noopMiddleware); - app = new App({ receiver: fakeReceiver, authorize: sinon.fake.resolves(dummyAuthorizationResult) }); - }); - - it('should process receiver events in order of #use', async () => { - // Act - app.use(fakeFirstMiddleware); - app.use(fakeSecondMiddleware); - app.error(fakeErrorHandler); - fakeReceiver.emit('message', dummyReceiverEvent); - await delay(); - - // Assert - assert(fakeErrorHandler.notCalled); - assert(fakeFirstMiddleware.calledOnce); - assert(fakeFirstMiddleware.calledBefore(fakeSecondMiddleware)); - assert(fakeSecondMiddleware.calledOnce); + app = new App({ + logger: createFakeLogger(), + receiver: fakeReceiver, + authorize: sinon.fake.resolves(dummyAuthorizationResult), + }); }); it('should error if next called multiple times', async () => { @@ -342,18 +345,147 @@ describe('App', () => { }); app.use(fakeSecondMiddleware); app.error(fakeErrorHandler); - fakeReceiver.emit('message', dummyReceiverEvent); - await delay(); + await fakeReceiver.sendEvent(dummyReceiverEvent); // Assert assert.instanceOf(fakeErrorHandler.firstCall.args[0], Error); }); + + it('correctly waits for async listeners', async () => { + let changed = false; + + app.use(async ({ next }) => { + await delay(100); + changed = true; + + await next(); + }); + + await fakeReceiver.sendEvent(dummyReceiverEvent); + assert.isTrue(changed); + assert(fakeErrorHandler.notCalled); + }); + + it('throws errors which can be caught by upstream async listeners', async () => { + const thrownError = new Error('Error handling the message :('); + let caughtError; + + app.use(async ({ next }) => { + try { + await next(); + } catch (err) { + caughtError = err; + } + }); + + app.use(async () => { + throw thrownError; + }); + + app.error(fakeErrorHandler); + + await fakeReceiver.sendEvent(dummyReceiverEvent); + + assert.equal(caughtError, thrownError); + assert(fakeErrorHandler.notCalled); + }); + + it('calls async middleware in declared order', async () => { + const message = ':wave:'; + let middlewareCount = 0; + + const assertOrderMiddleware = (orderDown: number, orderUp: number) => async ({ next }: any) => { + await delay(100); + middlewareCount += 1; + assert.equal(middlewareCount, orderDown); + await next(); + middlewareCount += 1; + assert.equal(middlewareCount, orderUp); + }; + + app.use(assertOrderMiddleware(1, 8)); + app.message(message, assertOrderMiddleware(3, 6), assertOrderMiddleware(4, 5)); + app.use(assertOrderMiddleware(2, 7)); + app.error(fakeErrorHandler); + + await fakeReceiver.sendEvent({ + ...dummyReceiverEvent, + body: { + type: 'event_callback', + event: { + type: 'message', + text: message, + }, + }, + }); + + assert.equal(middlewareCount, 8); + assert(fakeErrorHandler.notCalled); + }); + + it('should, on error, ack an error, then global error handler', async () => { + const error = new Error('Everything is broke, you probably should restart, if not then good luck'); + let callCount = 0; + + const assertOrder = (order: number) => { + callCount += 1; + assert.equal(callCount, order); + }; + + app.use(() => { + assertOrder(1); + throw error; + }); + + const event = { + ...dummyReceiverEvent, + ack: (actualError: Error): Promise => { + if (actualError instanceof Error) { + assert.instanceOf(actualError, UnknownError); + assert.equal(actualError.message, error.message); + assertOrder(2); + } + + return Promise.resolve(); + }, + }; + + app.error(async (actualError) => { + assert.instanceOf(actualError, UnknownError); + assert.equal(actualError.message, error.message); + assertOrder(3); + await delay(); // Make this async to make sure error handlers can be tested + }); + + await fakeReceiver.sendEvent(event); + + assert.equal(callCount, 3); + }); + + it('with a default global error handler, rejects App#ProcessEvent', async () => { + const error = new Error('The worst has happened, bot is beyond saving, always hug servers'); + let actualError; + + app.use(() => { + throw error; + }); + + try { + await fakeReceiver.sendEvent(dummyReceiverEvent); + } catch (err) { + actualError = err; + } + + assert.instanceOf(actualError, UnknownError); + assert.equal(actualError.message, error.message); + }); }); describe('middleware and listener arguments', () => { let fakeErrorHandler: SinonSpy; const dummyChannelId = 'CHANNEL_ID'; let overrides: Override; + const baseEvent = createDummyReceiverEvent(); function buildOverrides(secondOverrides: Override[]): Override { fakeErrorHandler = sinon.fake(); @@ -367,22 +499,22 @@ describe('App', () => { } describe('routing', () => { - function createReceiverEvents(): ReceiverEvent[] { return [ { // IncomingEventType.Event (app.event) + ...baseEvent, body: { event: {}, }, - ack: noop, }, { // IncomingEventType.Command (app.command) + ...baseEvent, body: { command: '/COMMAND_NAME', }, - ack: noop, }, { // IncomingEventType.Action (app.action) + ...baseEvent, body: { type: 'block_actions', actions: [{ @@ -392,9 +524,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.Action (app.action) + ...baseEvent, body: { type: 'message_action', callback_id: 'message_action_callback_id', @@ -402,9 +534,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.Action (app.action) + ...baseEvent, body: { type: 'message_action', callback_id: 'another_message_action_callback_id', @@ -412,9 +544,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.Action (app.action) + ...baseEvent, body: { type: 'interactive_message', callback_id: 'interactive_message_callback_id', @@ -423,9 +555,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.Action with dialog submission (app.action) + ...baseEvent, body: { type: 'dialog_submission', callback_id: 'dialog_submission_callback_id', @@ -433,9 +565,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.Action for an external_select block (app.options) + ...baseEvent, body: { type: 'block_suggestion', action_id: 'external_select_action_id', @@ -444,9 +576,9 @@ describe('App', () => { team: {}, actions: [], }, - ack: noop, }, { // IncomingEventType.Action for "data_source": "external" in dialogs (app.options) + ...baseEvent, body: { type: 'dialog_suggestion', callback_id: 'dialog_suggestion_callback_id', @@ -455,9 +587,9 @@ describe('App', () => { user: {}, team: {}, }, - ack: noop, }, { // IncomingEventType.ViewSubmitAction (app.view) + ...baseEvent, body: { type: 'view_submission', channel: {}, @@ -467,9 +599,9 @@ describe('App', () => { callback_id: 'view_callback_id', }, }, - ack: noop, }, { + ...baseEvent, body: { type: 'view_closed', channel: {}, @@ -479,9 +611,9 @@ describe('App', () => { callback_id: 'view_callback_id', }, }, - ack: noop, }, { + ...baseEvent, body: { type: 'event_callback', token: 'XXYYZZ', @@ -494,7 +626,6 @@ describe('App', () => { text: 'hello friends!', }, }, - ack: noop, }, ]; } @@ -558,8 +689,7 @@ describe('App', () => { assert.isTrue(fakeLogger.error.called); app.error(fakeErrorHandler); - dummyReceiverEvents.forEach(dummyEvent => fakeReceiver.emit('message', dummyEvent)); - await delay(); + await Promise.all(dummyReceiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.equal(actionFn.callCount, 5); @@ -586,7 +716,7 @@ describe('App', () => { await respond(responseText); }); app.error(fakeErrorHandler); - fakeReceiver.emit('message', { // IncomingEventType.Action (app.action) + await fakeReceiver.sendEvent({ // IncomingEventType.Action (app.action) body: { type: 'block_actions', response_url: responseUrl, @@ -599,7 +729,6 @@ describe('App', () => { }, ack: noop, }); - await delay(); // Assert assert(fakeErrorHandler.notCalled); @@ -623,7 +752,7 @@ describe('App', () => { await respond(responseObject); }); app.error(fakeErrorHandler); - fakeReceiver.emit('message', { // IncomingEventType.Action (app.action) + await fakeReceiver.sendEvent({ // IncomingEventType.Action (app.action) body: { type: 'block_actions', response_url: responseUrl, @@ -636,7 +765,6 @@ describe('App', () => { }, ack: noop, }); - await delay(); // Assert assert.equal(fakeAxiosPost.callCount, 1); @@ -687,8 +815,7 @@ describe('App', () => { ]; // Act - receiverEvents.forEach(event => fakeReceiver.emit('message', event)); - await delay(); + await Promise.all(receiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.isTrue(fakeLogger.info.called); @@ -753,8 +880,7 @@ describe('App', () => { const receiverEvents = [event, event, event]; // Act - receiverEvents.forEach(event => fakeReceiver.emit('message', event)); - await delay(); + await Promise.all(receiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.isUndefined(app.client.token); @@ -774,16 +900,17 @@ describe('App', () => { return [ // IncomingEventType.Event with channel in payload { + ...baseEvent, body: { event: { channel: channelId, }, team_id: 'TEAM_ID', }, - ack: noop, }, // IncomingEventType.Event with channel in item { + ...baseEvent, body: { event: { item: { @@ -792,19 +919,19 @@ describe('App', () => { }, team_id: 'TEAM_ID', }, - ack: noop, }, // IncomingEventType.Command { + ...baseEvent, body: { command: '/COMMAND_NAME', channel_id: channelId, team_id: 'TEAM_ID', }, - ack: noop, }, // IncomingEventType.Action from block action, interactive message, or message action { + ...baseEvent, body: { actions: [{}], channel: { @@ -817,10 +944,10 @@ describe('App', () => { id: 'TEAM_ID', }, }, - ack: noop, }, // IncomingEventType.Action from dialog submission { + ...baseEvent, body: { type: 'dialog_submission', channel: { @@ -833,7 +960,6 @@ describe('App', () => { id: 'TEAM_ID', }, }, - ack: noop, }, ]; } @@ -855,8 +981,7 @@ describe('App', () => { await say(dummyMessage); }); app.error(fakeErrorHandler); - dummyReceiverEvents.forEach(dummyEvent => fakeReceiver.emit('message', dummyEvent)); - await delay(); + await Promise.all(dummyReceiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.equal(fakePostMessage.callCount, dummyReceiverEvents.length); @@ -886,8 +1011,7 @@ describe('App', () => { await say(dummyMessage); }); app.error(fakeErrorHandler); - dummyReceiverEvents.forEach(dummyEvent => fakeReceiver.emit('message', dummyEvent)); - await delay(); + await Promise.all(dummyReceiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.equal(fakePostMessage.callCount, dummyReceiverEvents.length); @@ -906,6 +1030,7 @@ describe('App', () => { return [ // IncomingEventType.Options from block action { + ...baseEvent, body: { type: 'block_suggestion', channel: { @@ -918,10 +1043,10 @@ describe('App', () => { id: 'TEAM_ID', }, }, - ack: noop, }, // IncomingEventType.Options from interactive message or dialog { + ...baseEvent, body: { name: 'select_field_name', channel: { @@ -934,16 +1059,15 @@ describe('App', () => { id: 'TEAM_ID', }, }, - ack: noop, }, // IncomingEventType.Event without a channel context { + ...baseEvent, body: { event: { }, team_id: 'TEAM_ID', }, - ack: noop, }, ]; } @@ -964,12 +1088,35 @@ describe('App', () => { // called assertionAggregator(); }); - dummyReceiverEvents.forEach(dummyEvent => fakeReceiver.emit('message', dummyEvent)); - await delay(); + + await Promise.all(dummyReceiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.equal(assertionAggregator.callCount, dummyReceiverEvents.length); }); + + it('should handle failures through the App\'s global error handler', async () => { + // Arrange + const fakePostMessage = sinon.fake.rejects(new Error('fake error')); + const overrides = buildOverrides([withPostMessage(fakePostMessage)]); + const App = await importApp(overrides); // tslint:disable-line:variable-name + + const dummyMessage = { text: 'test' }; + const dummyReceiverEvents = createChannelContextualReceiverEvents(dummyChannelId); + + // Act + const app = new App({ receiver: fakeReceiver, authorize: sinon.fake.resolves(dummyAuthorizationResult) }); + app.use(async (args) => { + // By definition, these events should all produce a say function, so we cast args.say into a SayFn + const say = (args as any).say as SayFn; + await say(dummyMessage); + }); + app.error(fakeErrorHandler); + await Promise.all(dummyReceiverEvents.map(event => fakeReceiver.sendEvent(event))); + + // Assert + assert.equal(fakeErrorHandler.callCount, dummyReceiverEvents.length); + }); }); }); }); @@ -1065,37 +1212,20 @@ function withConversationContext(spy: SinonSpy): Override { } // Fakes -type FakeReceiver = SinonSpy & EventEmitter & { - start: SinonSpy, ReturnType>; - stop: SinonSpy, ReturnType>; -}; - -function createFakeReceiver( - startSpy: SinonSpy = sinon.fake.resolves(undefined), - stopSpy: SinonSpy = sinon.fake.resolves(undefined), -): FakeReceiver { - const mock = new EventEmitter(); - (mock as FakeReceiver).start = startSpy; - (mock as FakeReceiver).stop = stopSpy; - return mock as FakeReceiver; -} +class FakeReceiver implements Receiver { + private bolt: App | undefined; -// Dummies (values that have no real behavior but pass through the system opaquely) -function createDummyReceiverEvent(): ReceiverEvent { - // NOTE: this is a degenerate ReceiverEvent that would successfully pass through the App. it happens to look like a - // IncomingEventType.Event - return { - body: { - event: { - }, - }, - ack: noop, - }; -} + public init = (bolt: App) => { this.bolt = bolt; }; -// Utility functions -const noop = (() => Promise.resolve(undefined)); -const noopMiddleware = async ({ next }: { next: NextMiddleware; }) => { await next(); }; -const noopAuthorize = (() => Promise.resolve({})); + public start = sinon.fake((...params: any[]): Promise => { + return Promise.resolve([...params]); + }); -// TODO: swap out rewiremock for proxyquire to see if it saves execution time + public stop = sinon.fake((...params: any[]): Promise => { + return Promise.resolve([...params]); + }); + + public async sendEvent(event: ReceiverEvent): Promise { + return this.bolt?.processEvent(event); + } +} diff --git a/src/App.ts b/src/App.ts index 73a29048e..9d695f078 100644 --- a/src/App.ts +++ b/src/App.ts @@ -39,12 +39,15 @@ import { BlockAction, InteractiveMessage, SlackViewAction, - Receiver, - ReceiverEvent, RespondArguments, } from './types'; import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers'; -import { ErrorCode, CodedError, errorWithCode, asCodedError } from './errors'; +import { Receiver, ReceiverEvent } from './receiver'; +import { + CodedError, + asCodedError, + AppInitializationError, +} from './errors'; import { MiddlewareContext } from './types/middleware'; const packageJson = require('../package.json'); // tslint:disable-line:no-require-imports no-var-requires @@ -72,7 +75,7 @@ export { LogLevel, Logger } from '@slack/logger'; export interface Authorize { ( source: AuthorizeSourceData, - body: ReceiverEvent['body'], + body: AnyMiddlewareArgs['body'], ): Promise; } @@ -107,7 +110,7 @@ export interface ViewConstraints { } export interface ErrorHandler { - (error: CodedError): void; + (error: CodedError): Promise; } class WebClientPool { @@ -195,16 +198,14 @@ export default class App { if (token !== undefined) { if (authorize !== undefined) { - throw errorWithCode( + throw new AppInitializationError( `Both token and authorize options provided. ${tokenUsage}`, - ErrorCode.AppInitializationError, ); } this.authorize = singleTeamAuthorization(this.client, { botId, botUserId, botToken: token }); } else if (authorize === undefined) { - throw errorWithCode( + throw new AppInitializationError( `No token and no authorize options provided. ${tokenUsage}`, - ErrorCode.AppInitializationError, ); } else { this.authorize = authorize; @@ -219,20 +220,15 @@ export default class App { } else { // No custom receiver if (signingSecret === undefined) { - throw errorWithCode( - 'Signing secret not found, so could not initialize the default receiver. Set a signing secret or use a ' + - 'custom receiver.', - ErrorCode.AppInitializationError, + throw new AppInitializationError( + 'Signing secret not found, so could not initialize the default receiver. Set a signing secret or use a ' + + 'custom receiver.', ); - } else { - // Create default ExpressReceiver - this.receiver = new ExpressReceiver({ signingSecret, logger, endpoints, agent, clientTls }); } + // Create default ExpressReceiver + this.receiver = new ExpressReceiver({ signingSecret, logger, endpoints, agent, clientTls }); } - - // Subscribe to messages and errors from the receiver - this.receiver.on('message', message => this.onIncomingEvent(message)); - this.receiver.on('error', error => this.onGlobalError(error)); + this.receiver.init(this); // Conditionally use a global middleware that ignores events (including messages) that are sent from this app if (ignoreSelf) { @@ -408,7 +404,7 @@ export default class App { /** * Handles events from the receiver */ - private async onIncomingEvent(event: ReceiverEvent): Promise { + public async processEvent(event: ReceiverEvent): Promise { const { body, ack } = event; // TODO: when generating errors (such as in the say utility) it may become useful to capture the current context, // or even all of the args, as properties of the error. This would give error handling code some ability to deal @@ -426,13 +422,15 @@ export default class App { // Initialize context (shallow copy to enforce object identity separation) const source = buildSource(type, conversationId, bodyArg); - const authorizeResult = await (this.authorize(source, bodyArg).catch((error) => { - this.onGlobalError(authorizationErrorFromOriginal(error)); - })); - if (authorizeResult === undefined) { + let authorizeResult; + + try { + authorizeResult = await this.authorize(source, bodyArg); + } catch (error) { this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); return; } + const context: Context = { ...authorizeResult }; // Factory for say() utility @@ -553,15 +551,16 @@ export default class App { ...(listenerArgs as MiddlewareContext), }); } catch (error) { - this.onGlobalError(error); + await event.ack(asCodedError(error)); + return this.handleError(error); } } /** * Global error handler. The final destination for all errors (hopefully). */ - private onGlobalError(error: Error): void { - this.errorHandler(asCodedError(error)); + public handleError(error: Error): Promise { + return this.errorHandler(asCodedError(error)); } } @@ -618,6 +617,8 @@ function isBlockActionOrInteractiveMessageBody( function defaultErrorHandler(logger: Logger): ErrorHandler { return (error) => { logger.error(error); + + throw error; }; } @@ -648,15 +649,3 @@ function selectToken(context: Context): string | undefined { /* Instrumentation */ addAppMetadata({ name: packageJson.name, version: packageJson.version }); - -/* Error handling helpers */ -function authorizationErrorFromOriginal(original: Error): AuthorizationError { - const error = errorWithCode('Authorization of incoming event did not succeed.', ErrorCode.AuthorizationError); - (error as AuthorizationError).original = original; - return error as AuthorizationError; -} - -export interface AuthorizationError extends CodedError { - code: ErrorCode.AuthorizationError; - original: Error; -} diff --git a/src/ExpressReceiver.spec.ts b/src/ExpressReceiver.spec.ts index d7f1ba7bd..72bd791b0 100644 --- a/src/ExpressReceiver.spec.ts +++ b/src/ExpressReceiver.spec.ts @@ -1,6 +1,5 @@ // tslint:disable:no-implicit-dependencies import 'mocha'; - import { Logger, LogLevel } from '@slack/logger'; import { assert } from 'chai'; import { Request, Response } from 'express'; @@ -11,11 +10,10 @@ import { Readable } from 'stream'; import ExpressReceiver, { respondToSslCheck, respondToUrlVerification, - verifySignatureAndParseBody, + verifySignatureAndParseRawBody, } from './ExpressReceiver'; describe('ExpressReceiver', () => { - const noopLogger: Logger = { debug(..._msg: any[]): void { /* noop */ }, info(..._msg: any[]): void { /* noop */ }, @@ -58,6 +56,7 @@ describe('ExpressReceiver', () => { signingSecret: 'my-secret', logger: noopLogger, }); + await receiver.start(9999); await receiver.stop(); }); @@ -102,7 +101,7 @@ describe('ExpressReceiver', () => { }); }); - describe('url_verification requset handler', () => { + describe('url_verification request handler', () => { it('should handle valid requests', async () => { // Arrange // tslint:disable-next-line: no-object-literal-type-assertion @@ -141,7 +140,7 @@ describe('ExpressReceiver', () => { }); }); - describe('verifySignatureAndParseBody', () => { + describe('verifySignatureAndParseRawBody', () => { let clock: SinonFakeTimers; @@ -196,7 +195,7 @@ describe('ExpressReceiver', () => { const next = (error: any) => { state.error = error; }; // Act - const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); await verifier(req, resp, next); } @@ -245,24 +244,15 @@ describe('ExpressReceiver', () => { const result: any = {}; const resp = buildResponseToVerify(result); - let error: string = ''; - let warn: string = ''; - const logger = { - error: (msg: string) => { error = msg; }, - warn: (msg: string) => { warn = msg; }, - } as any as Logger; - const next = sinon.fake(); // Act - const verifier = verifySignatureAndParseBody(logger, signingSecret); + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); await verifier(req, resp, next); // Assert assert.equal(result.code, 400); assert.equal(result.sent, true); - assert.equal(error, 'Failed to parse body as JSON data for content-type: undefined'); - assert.equal(warn, 'Parsing request body failed (error: SyntaxError: Unexpected token o in JSON at position 1)'); } it('should fail to parse request body without content-type header', async () => { @@ -301,7 +291,7 @@ describe('ExpressReceiver', () => { const next = sinon.fake(); // Act - const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); await verifier(req, resp, next); // Assert @@ -355,7 +345,8 @@ describe('ExpressReceiver', () => { const next = sinon.fake(); // Act - const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); await verifier(req, resp, next); // Assert @@ -388,7 +379,7 @@ describe('ExpressReceiver', () => { const next = sinon.fake(); // Act - const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); await verifier(req, resp, next); // Assert @@ -414,7 +405,7 @@ describe('ExpressReceiver', () => { const next = sinon.fake(); // Act - const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret); verifier(req, resp, next); await verifier(req, resp, next); diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 9aff1e52a..9368934c6 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -1,20 +1,17 @@ -import { EventEmitter } from 'events'; -import { Receiver, ReceiverEvent, ReceiverAckTimeoutError } from './types'; import { createServer, Server, Agent } from 'http'; import { SecureContextOptions } from 'tls'; import express, { Request, Response, Application, RequestHandler, NextFunction } from 'express'; import rawBody from 'raw-body'; -import querystring from 'querystring'; -import crypto from 'crypto'; -import tsscmp from 'tsscmp'; -import { ErrorCode, errorWithCode } from './errors'; +import App from './App'; +import { verifySignatureAndParseBody, Receiver, ReceiverEvent } from './receiver'; +import { ReceiverAuthenticityError } from './errors'; import { Logger, ConsoleLogger } from '@slack/logger'; // TODO: we throw away the key names for endpoints, so maybe we should use this interface. is it better for migrations? // if that's the reason, let's document that with a comment. export interface ExpressReceiverOptions { signingSecret: string; - logger?: Logger; + logger: Logger; endpoints?: string | { [endpointType: string]: string; }; @@ -25,27 +22,26 @@ export interface ExpressReceiverOptions { /** * Receives HTTP requests with Events, Slash Commands, and Actions */ -export default class ExpressReceiver extends EventEmitter implements Receiver { +export default class ExpressReceiver implements Receiver { /* Express app */ public app: Application; private server: Server; + private bolt: App | undefined; constructor({ signingSecret = '', logger = new ConsoleLogger(), endpoints = { events: '/slack/events' }, }: ExpressReceiverOptions) { - super(); - this.app = express(); this.app.use(this.errorHandler.bind(this)); // TODO: what about starting an https server instead of http? what about other options to create the server? this.server = createServer(this.app); const expressMiddleware: RequestHandler[] = [ - verifySignatureAndParseBody(logger, signingSecret), + verifySignatureAndParseRawBody(logger, signingSecret), respondToSslCheck, respondToUrlVerification, this.requestHandler.bind(this), @@ -57,10 +53,10 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { } } - private requestHandler(req: Request, res: Response): void { + private async requestHandler(req: Request, res: Response): Promise { let timer: NodeJS.Timer | undefined = setTimeout( () => { - this.emit('error', receiverAckTimeoutError( + this.bolt?.handleError(new ReceiverAuthenticityError( 'An incoming event was not acknowledged before the timeout. ' + 'Ensure that the ack() argument is called in your listeners.', )); @@ -69,14 +65,15 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { 2800, ); const event: ReceiverEvent = { - body: req.body as { [key: string]: any }, - ack: async (response: any): Promise => { - // TODO: if app tries acknowledging more than once, emit a warning + body: req.body, + ack: async (response): Promise => { if (timer !== undefined) { clearTimeout(timer); timer = undefined; - if (!response) { + if (response instanceof Error) { + res.send(500); + } else if (!response) { res.send(''); } else if (typeof response === 'string') { res.send(response); @@ -87,10 +84,14 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { }, }; - this.emit('message', event); + await this.bolt?.processEvent(event); + } + + public init(bolt: App): void { + this.bolt = bolt; } - // TODO: the arguments should be defined as the arguments of Server#listen() + // TODO: the arguments should be defined as the arguments of Server#listen() // TODO: the return value should be defined as a type that both http and https servers inherit from, or a union public start(port: number): Promise { return new Promise((resolve, reject) => { @@ -124,7 +125,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { } private errorHandler(err: any, _req: Request, _res: Response, next: NextFunction): void { - this.emit('error', err); + this.bolt?.handleError(err); // Forward to express' default error handler (which knows how to print stack traces in development) next(err); } @@ -151,7 +152,7 @@ export const respondToUrlVerification: RequestHandler = (req, res, next) => { * - Verify the request signature * - Parse request.body and assign the successfully parsed object to it. */ -export function verifySignatureAndParseBody( +export function verifySignatureAndParseRawBody( logger: Logger, signingSecret: string, ): RequestHandler { @@ -167,24 +168,6 @@ export function verifySignatureAndParseBody( stringBody = (await rawBody(req)).toString(); } - // *** Request verification *** - try { - const { - 'x-slack-signature': signature, - 'x-slack-request-timestamp': requestTimestamp, - } = req.headers; - await verifyRequestSignature( - signingSecret, - stringBody, - signature as string | undefined, - requestTimestamp as string | undefined, - ); - } catch (error) { - // Deny the request as something wrong with the signature - logError(logger, 'Request verification failed', error); - return res.status(401).send(); - } - // *** Parsing body *** // As the verification passed, parse the body as an object and assign it to req.body // Following middlewares can expect `req.body` is already a parsed one. @@ -192,14 +175,20 @@ export function verifySignatureAndParseBody( try { // This handler parses `req.body` or `req.rawBody`(on Google Could Platform) // and overwrites `req.body` with the parsed JS object. - const contentType = req.headers['content-type']; - req.body = parseRequestBody(logger, stringBody, contentType); - return next(); + req.body = verifySignatureAndParseBody(signingSecret, stringBody, req.headers); } catch (error) { - // Deny a bad request - logError(logger, 'Parsing request body failed', error); - return res.status(400).send(); + if (error) { + if (error instanceof ReceiverAuthenticityError) { + logError(logger, 'Request verification failed', error); + return res.status(401).send(); + } + + logError(logger, 'Parsing request body failed', error); + return res.status(400).send(); + } } + + return next(); }; } @@ -209,81 +198,3 @@ function logError(logger: Logger, message: string, error: any): void { : `${message} (error: ${error})`; logger.warn(logMessage); } - -// TODO: this should be imported from another package -async function verifyRequestSignature( - signingSecret: string, - body: string, - signature: string | undefined, - requestTimestamp: string | undefined, -): Promise { - if (signature === undefined || requestTimestamp === undefined) { - throw errorWithCode( - 'Slack request signing verification failed. Some headers are missing.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - } - - const ts = Number(requestTimestamp); - if (isNaN(ts)) { - throw errorWithCode( - 'Slack request signing verification failed. Timestamp is invalid.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - } - - // Divide current date to match Slack ts format - // Subtract 5 minutes from current time - const fiveMinutesAgo = Math.floor(Date.now() / 1000) - (60 * 5); - - if (ts < fiveMinutesAgo) { - throw errorWithCode( - 'Slack request signing verification failed. Timestamp is too old.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - } - - const hmac = crypto.createHmac('sha256', signingSecret); - const [version, hash] = signature.split('='); - hmac.update(`${version}:${ts}:${body}`); - - if (!tsscmp(hash, hmac.digest('hex'))) { - throw errorWithCode( - 'Slack request signing verification failed. Signature mismatch.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - } -} - -function parseRequestBody( - logger: Logger, - stringBody: string, - contentType: string | undefined): any { - if (contentType === 'application/x-www-form-urlencoded') { - const parsedBody = querystring.parse(stringBody); - if (typeof parsedBody.payload === 'string') { - return JSON.parse(parsedBody.payload); - } - return parsedBody; - } - - if (contentType === 'application/json') { - return JSON.parse(stringBody); - } - - logger.warn(`Unexpected content-type detected: ${contentType}`); - try { - // Parse this body anyway - return JSON.parse(stringBody); - } catch (e) { - logger.error(`Failed to parse body as JSON data for content-type: ${contentType}`); - throw e; - } - -} - -function receiverAckTimeoutError(message: string): ReceiverAckTimeoutError { - const error = new Error(message); - (error as ReceiverAckTimeoutError).code = ErrorCode.ReceiverAckTimeoutError; - return (error as ReceiverAckTimeoutError); -} diff --git a/src/conversation-store.spec.ts b/src/conversation-store.spec.ts index 09419fee0..3e03825ce 100644 --- a/src/conversation-store.spec.ts +++ b/src/conversation-store.spec.ts @@ -2,13 +2,19 @@ import 'mocha'; import { assert, AssertionError } from 'chai'; import sinon, { SinonSpy } from 'sinon'; -import { Override, createFakeLogger, delay, wrapToResolveOnFirstCall } from './test-helpers'; +import { Override, createFakeLogger, wrapToResolveOnFirstCall } from './test-helpers'; import rewiremock from 'rewiremock'; import { ConversationStore } from './conversation-store'; import { AnyMiddlewareArgs, NextMiddleware, Context } from './types'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; +function delay(ms: number = 0): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + describe('conversationContext middleware', () => { it('should forward events that have no conversation ID', async () => { // Arrange diff --git a/src/errors.spec.ts b/src/errors.spec.ts new file mode 100644 index 000000000..b06e6e5ec --- /dev/null +++ b/src/errors.spec.ts @@ -0,0 +1,40 @@ +// tslint:disable:no-implicit-dependencies +import { assert } from 'chai'; +import { + asCodedError, + ErrorCode, + CodedError, + AppInitializationError, + AuthorizationError, + ContextMissingPropertyError, + ReceiverAckTimeoutError, + ReceiverAuthenticityError, + UnknownError, +} from './errors'; + +describe('Errors', () => { + + it('has errors matching codes', () => { + const errorMap = { + [ErrorCode.AppInitializationError]: new AppInitializationError(), + [ErrorCode.AuthorizationError]: new AuthorizationError('auth failed', new Error('auth failed')), + [ErrorCode.ContextMissingPropertyError]: new ContextMissingPropertyError('foo', "can't find foo"), + [ErrorCode.ReceiverAckTimeoutError]: new ReceiverAckTimeoutError(), + [ErrorCode.ReceiverAuthenticityError]: new ReceiverAuthenticityError(), + [ErrorCode.UnknownError]: new UnknownError(new Error('It errored')), + }; + + Object.entries(errorMap).forEach(([code, error]) => { + assert.equal((error as CodedError).code, code); + }); + }); + + it('wraps non-coded errors', () => { + assert.instanceOf(asCodedError(new Error('AHH!')), UnknownError); + }); + + it('passes coded errors through', () => { + const error = new AppInitializationError(); + assert.equal(asCodedError(error), error); + }); +}); diff --git a/src/errors.ts b/src/errors.ts index 2be369911..8a935c08f 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -10,7 +10,7 @@ export enum ErrorCode { ReceiverAckTimeoutError = 'slack_bolt_receiver_ack_timeout_error', - ExpressReceiverAuthenticityError = 'slack_bolt_express_receiver_authenticity_error', + ReceiverAuthenticityError = 'slack_bolt_receiver_authenticity_error', /** * This value is used to assign to errors that occur inside the framework but do not have a code, to keep interfaces @@ -19,16 +19,54 @@ export enum ErrorCode { UnknownError = 'slack_bolt_unknown_error', } -export function errorWithCode(message: string, code: ErrorCode): CodedError { - const error = new Error(message); - (error as CodedError).code = code; - return error as CodedError; -} - export function asCodedError(error: CodedError | Error): CodedError { if ((error as CodedError).code !== undefined) { return error as CodedError; } - (error as CodedError).code = ErrorCode.UnknownError; - return error as CodedError; + + return new UnknownError(error); +} + +export class AppInitializationError extends Error implements CodedError { + public code = ErrorCode.AppInitializationError; +} + +export class AuthorizationError extends Error implements CodedError { + public code = ErrorCode.AuthorizationError; + public original: Error; + + constructor(message: string, original: Error) { + super(message); + + this.original = original; + } +} + +export class ContextMissingPropertyError extends Error implements CodedError { + public code = ErrorCode.ContextMissingPropertyError; + public missingProperty: string; + + constructor(missingProperty: string, message: string) { + super(message); + this.missingProperty = missingProperty; + } +} + +export class ReceiverAckTimeoutError extends Error implements CodedError { + public code = ErrorCode.ReceiverAckTimeoutError; +} + +export class ReceiverAuthenticityError extends Error implements CodedError { + public code = ErrorCode.ReceiverAuthenticityError; +} + +export class UnknownError extends Error implements CodedError { + public code = ErrorCode.UnknownError; + public original: Error; + + constructor(original: Error) { + super(original.message); + + this.original = original; + } } diff --git a/src/index.ts b/src/index.ts index d62e1af3d..6339d405a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,20 +9,19 @@ export { Authorize, AuthorizeSourceData, AuthorizeResult, - AuthorizationError, ActionConstraints, LogLevel, Logger, } from './App'; -export { ErrorCode } from './errors'; - export { default as ExpressReceiver, ExpressReceiverOptions, } from './ExpressReceiver'; +export * from './errors'; export * from './middleware/builtin'; +export * from './receiver'; export * from './types'; export { diff --git a/src/middleware/builtin.spec.ts b/src/middleware/builtin.spec.ts index 7ca1cd50f..9ec486cff 100644 --- a/src/middleware/builtin.spec.ts +++ b/src/middleware/builtin.spec.ts @@ -2,7 +2,7 @@ import 'mocha'; import { assert } from 'chai'; import sinon from 'sinon'; -import { ErrorCode } from '../errors'; +import { ErrorCode, ContextMissingPropertyError } from '../errors'; import { Override, wrapToResolveOnFirstCall, createFakeLogger } from '../test-helpers'; import rewiremock from 'rewiremock'; import { @@ -274,7 +274,7 @@ describe('ignoreSelf()', () => { context: { botUserId: fakeBotUserId, botId: fakeBotUserId }, } as unknown as MemberJoinedOrLeftChannelMiddlewareArgs; - const { ignoreSelf: getIgnoreSelfMiddleware, contextMissingPropertyError } = await importBuiltin(); + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); // Act const middleware = getIgnoreSelfMiddleware(); @@ -287,7 +287,7 @@ describe('ignoreSelf()', () => { } // Assert - const expectedError = contextMissingPropertyError( + const expectedError = new ContextMissingPropertyError( 'botId', 'Cannot ignore events from the app without a bot ID. Ensure authorize callback returns a botId.', ); diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index 8c4a041c3..7cc4d552a 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -16,11 +16,10 @@ import { DialogSubmitAction, MessageAction, BlockElementAction, - ContextMissingPropertyError, SlackViewAction, } from '../types'; import { ActionConstraints, ViewConstraints } from '../App'; -import { ErrorCode, errorWithCode } from '../errors'; +import { ContextMissingPropertyError } from '../errors'; /** * Middleware that filters out any event that isn't an action @@ -241,7 +240,7 @@ export function ignoreSelf(): Middleware { return async (args) => { // When context does not have a botId in it, then this middleware cannot perform its job. Bail immediately. if (args.context.botId === undefined) { - throw contextMissingPropertyError( + throw new ContextMissingPropertyError( 'botId', 'Cannot ignore events from the app without a bot ID. Ensure authorize callback returns a botId.', ); @@ -296,7 +295,7 @@ export function directMention(): Middleware> return async ({ message, context, next }) => { // When context does not have a botUserId in it, then this middleware cannot perform its job. Bail immediately. if (context.botUserId === undefined) { - throw contextMissingPropertyError( + throw new ContextMissingPropertyError( 'botUserId', 'Cannot match direct mentions of the app without a bot user ID. Ensure authorize callback returns a botUserId.', ); @@ -358,10 +357,3 @@ function isEventArgs( ): args is SlackEventMiddlewareArgs { return (args as SlackEventMiddlewareArgs).event !== undefined; } - -export function contextMissingPropertyError(propertyName: string, message?: string): ContextMissingPropertyError { - const m = message === undefined ? `Context missing property: ${propertyName}` : message; - const error = errorWithCode(m, ErrorCode.ContextMissingPropertyError); - (error as ContextMissingPropertyError).missingProperty = propertyName; - return error as ContextMissingPropertyError; -} diff --git a/src/receiver.spec.ts b/src/receiver.spec.ts new file mode 100644 index 000000000..b8810aeb6 --- /dev/null +++ b/src/receiver.spec.ts @@ -0,0 +1,124 @@ +// tslint:disable:no-implicit-dependencies +import { assert } from 'chai'; +import querystring from 'querystring'; +import sinon, { SinonFakeTimers } from 'sinon'; +import { parseRequestBody } from '../dist'; +import { CodedError, ReceiverAuthenticityError } from './errors'; +import { verifyRequestSignature } from './receiver'; +import { StringIndexed } from './types/helpers'; + +describe('receiver', () => { + describe('verifyRequestSignature', () => { + let clock: SinonFakeTimers; + + beforeEach(() => { + // requestTimestamp = 1531420618 means this timestamp + clock = sinon.useFakeTimers(new Date('Thu Jul 12 2018 11:36:58 GMT-0700').getTime()); + }); + + afterEach(() => { + clock.restore(); + }); + + // These values are example data in the official doc + // https://api.slack.com/docs/verifying-requests-from-slack + const signingSecret = '8f742231b10e8888abcd99yyyzzz85a5'; + const signature = 'v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503'; + const requestTimestamp = 1531420618; + const body = 'token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c'; + const headers = { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp, + }; + + function catchVerifyErrors(headers: StringIndexed): CodedError | undefined { + const { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp, + } = headers; + + let error; + + try { + verifyRequestSignature(signingSecret, body, signature, requestTimestamp); + } catch (err) { + error = err; + } + + return error; + } + + it('should verify requests', () => { + const error = catchVerifyErrors(headers); + // Assert + assert.isUndefined(error); + }); + + it('should detect headers missing signature', () => { + const error = catchVerifyErrors({ + // 'x-slack-signature': signature , + 'x-slack-request-timestamp': requestTimestamp, + }) as CodedError; + assert.instanceOf(error, ReceiverAuthenticityError); + assert.include(error.message, 'Some headers are missing.'); + }); + + it('should detect headers missing timestamp', () => { + const error = catchVerifyErrors({ + 'x-slack-signature': signature, + /*'x-slack-request-timestamp': requestTimestamp*/ + }); + + assert.instanceOf(error, ReceiverAuthenticityError); + assert.include(error?.message, 'Some headers are missing.'); + }); + + it('should detect invalid timestamp header', () => { + const error = catchVerifyErrors({ + ...headers, + 'x-slack-request-timestamp': 'Hello there!', + }); + + assert.instanceOf(error, ReceiverAuthenticityError); + assert.include(error?.message, 'Timestamp is invalid.'); + }); + + it('should detect too old timestamp', () => { + const error = catchVerifyErrors({ + ...headers, + 'x-slack-request-timestamp': 0, + }); + + assert.instanceOf(error, ReceiverAuthenticityError); + assert.include(error?.message, 'Timestamp is too old.'); + }); + + it('should detect signature mismatch', () => { + const error = catchVerifyErrors({ + ...headers, + 'x-slack-request-timestamp': requestTimestamp + 10, + }); + + assert.instanceOf(error, ReceiverAuthenticityError); + assert.include(error?.message, 'Signature mismatch.'); + }); + }); + + describe('parseRequestBody', () => { + const stringBody = 'token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c'; + const jsonBody = querystring.parse(stringBody); + + it('parses form-urlencoded bodies', () => { + assert.deepEqual(parseRequestBody(stringBody, 'application/x-www-form-urlencoded'), jsonBody); + }); + + it('parses form-urlencoded bodies with a payload', () => { + const payload = `payload=${JSON.stringify(jsonBody)}`; + assert.deepEqual(parseRequestBody(payload, 'application/x-www-form-urlencoded'), jsonBody); + }); + + it('parses json bodies', () => { + assert.deepEqual(parseRequestBody(JSON.stringify(jsonBody), 'application/json'), jsonBody); + }); + }); +}); diff --git a/src/receiver.ts b/src/receiver.ts new file mode 100644 index 000000000..f7083acf0 --- /dev/null +++ b/src/receiver.ts @@ -0,0 +1,103 @@ +import crypto from 'crypto'; +import tsscmp from 'tsscmp'; +import querystring from 'querystring'; +import App from './App'; +import { AckFn, AnyMiddlewareArgs } from './types'; +import { ReceiverAuthenticityError } from './errors'; + +export interface ReceiverEvent { + body: Record; + // TODO: there should maybe be some more help for implementors of Receiver to know what kind of argument the AckFn + // is expected to deal with. + ack: AckFn; +} + +export interface Receiver { + init(app: App): void; + start(...args: any[]): Promise; + stop(...args: any[]): Promise; +} + +export function verifyRequestSignature( + signingSecret: string, + body: string, + signature: string | undefined, + requestTimestamp: string | undefined, +): void { + if (signature === undefined || requestTimestamp === undefined) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Some headers are missing.', + ); + } + + const ts = Number(requestTimestamp); + if (isNaN(ts)) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Timestamp is invalid.', + ); + } + + // Divide current date to match Slack ts format + // Subtract 5 minutes from current time + const fiveMinutesAgo = Math.floor(Date.now() / 1000) - (60 * 5); + + if (ts < fiveMinutesAgo) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Timestamp is too old.', + ); + } + + const hmac = crypto.createHmac('sha256', signingSecret); + const [version, hash] = signature.split('='); + hmac.update(`${version}:${ts}:${body}`); + + if (!tsscmp(hash, hmac.digest('hex'))) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Signature mismatch.', + ); + } +} + +/** + * This request handler has two responsibilities: + * - Verify the request signature + * - Parse request.body and assign the successfully parsed object to it. + */ +export function verifySignatureAndParseBody( + signingSecret: string, + body: string, + headers: Record, +): AnyMiddlewareArgs['body'] { + // *** Request verification *** + const { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp, + 'content-type': contentType, + } = headers; + + verifyRequestSignature( + signingSecret, + body, + signature, + requestTimestamp, + ); + + return parseRequestBody(body, contentType); +} + +export function parseRequestBody( + stringBody: string, + contentType: string | undefined, +): any { + if (contentType === 'application/x-www-form-urlencoded') { + const parsedBody = querystring.parse(stringBody); + + if (typeof parsedBody.payload === 'string') { + return JSON.parse(parsedBody.payload); + } + + return parsedBody; + } + + return JSON.parse(stringBody); +} diff --git a/src/test-helpers.ts b/src/test-helpers.ts index 4cad7b9ef..2e930387e 100644 --- a/src/test-helpers.ts +++ b/src/test-helpers.ts @@ -2,6 +2,12 @@ import sinon, { SinonSpy } from 'sinon'; import { Logger } from '@slack/logger'; +export function delay(ms: number = 0): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + export interface Override { [packageName: string]: { [exportName: string]: any; @@ -63,12 +69,6 @@ export function createFakeLogger(): FakeLogger { }; } -export function delay(ms: number = 0): Promise { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); -} - export function wrapToResolveOnFirstCall void>( original: T, timeoutMs: number = 1000, diff --git a/src/types/helpers.ts b/src/types/helpers.ts index c2ebb50c0..c589d2b71 100644 --- a/src/types/helpers.ts +++ b/src/types/helpers.ts @@ -1,9 +1,7 @@ /** * Extend this interface to build a type that is treated as an open set of properties, where each key is a string. */ -export interface StringIndexed { - [key: string]: any; -} +export type StringIndexed = Record; /** * Type function which removes the index signature for the type `T` diff --git a/src/types/index.ts b/src/types/index.ts index 2d05a8ded..3af090c70 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -5,4 +5,3 @@ export * from './command'; export * from './events'; export * from './options'; export * from './view'; -export * from './receiver'; diff --git a/src/types/middleware.ts b/src/types/middleware.ts index c06fa76ff..63588c76c 100644 --- a/src/types/middleware.ts +++ b/src/types/middleware.ts @@ -4,7 +4,6 @@ import { SlackActionMiddlewareArgs } from './actions'; import { SlackCommandMiddlewareArgs } from './command'; import { SlackOptionsMiddlewareArgs } from './options'; import { SlackViewMiddlewareArgs } from './view'; -import { CodedError, ErrorCode } from '../errors'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; @@ -33,8 +32,3 @@ export interface Middleware { } export type NextMiddleware = () => Promise; - -export interface ContextMissingPropertyError extends CodedError { - code: ErrorCode.ContextMissingPropertyError; - missingProperty: string; -} diff --git a/src/types/receiver.ts b/src/types/receiver.ts deleted file mode 100644 index 5951060e7..000000000 --- a/src/types/receiver.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { AckFn } from '../types'; -import { StringIndexed } from './helpers'; -import { CodedError, ErrorCode } from '../errors'; - -export interface ReceiverEvent { - body: StringIndexed; - // TODO: there should maybe be some more help for implementors of Receiver to know what kind of argument the AckFn - // is expected to deal with. - ack: AckFn; -} - -export interface Receiver { - on(event: 'message', listener: (event: ReceiverEvent) => void): unknown; - // strictly speaking, Error | ReceiverAckTimeoutError is just Error since the latter is a subtype of the former, but - // being explicit here is good for documentation purposes - on(event: 'error', listener: (error: Error | ReceiverAckTimeoutError) => void): unknown; - start(...args: any[]): Promise; - stop(...args: any[]): Promise; -} - -export interface ReceiverAckTimeoutError extends CodedError { - code: ErrorCode.ReceiverAckTimeoutError; -} diff --git a/src/types/utilities.ts b/src/types/utilities.ts index 5563ba8ac..07a1e5000 100644 --- a/src/types/utilities.ts +++ b/src/types/utilities.ts @@ -1,5 +1,6 @@ import { ChatPostMessageArguments, WebAPICallResult } from '@slack/web-api'; import { KnownKeys } from './helpers'; +import { CodedError } from '../errors'; // The say() utility function binds the message to the same channel as the incoming message that triggered the // listener. Therefore, specifying the `channel` argument is not required. @@ -23,5 +24,5 @@ export interface RespondFn { } export interface AckFn { - (response?: Response): Promise; + (response?: Response | CodedError): Promise; } From 224ce32b9ea1500d2a1751ab5b86c8f1479577f6 Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Mon, 17 Feb 2020 21:24:23 -0500 Subject: [PATCH 2/8] Move tls client ops to App out of ExpressReceiver --- src/App.ts | 8 +++++--- src/ExpressReceiver.spec.ts | 5 ----- src/ExpressReceiver.ts | 5 +---- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/App.ts b/src/App.ts index 9d695f078..c4ef3e3a7 100644 --- a/src/App.ts +++ b/src/App.ts @@ -1,3 +1,5 @@ +import { Agent } from 'http'; +import { SecureContextOptions } from 'tls'; import util from 'util'; import { WebClient, @@ -55,8 +57,8 @@ const packageJson = require('../package.json'); // tslint:disable-line:no-requir export interface AppOptions { signingSecret?: ExpressReceiverOptions['signingSecret']; endpoints?: ExpressReceiverOptions['endpoints']; - agent?: ExpressReceiverOptions['agent']; // also WebClientOptions['agent'] - clientTls?: ExpressReceiverOptions['clientTls']; // also WebClientOptions['tls'] + agent?: Agent; + clientTls?: Pick; convoStore?: ConversationStore | false; token?: AuthorizeResult['botToken']; // either token or authorize botId?: AuthorizeResult['botId']; // only used when authorize is not defined, shortcut for fetching @@ -226,7 +228,7 @@ export default class App { ); } // Create default ExpressReceiver - this.receiver = new ExpressReceiver({ signingSecret, logger, endpoints, agent, clientTls }); + this.receiver = new ExpressReceiver({ signingSecret, logger, endpoints }); } this.receiver.init(this); diff --git a/src/ExpressReceiver.spec.ts b/src/ExpressReceiver.spec.ts index 72bd791b0..e25deb8bd 100644 --- a/src/ExpressReceiver.spec.ts +++ b/src/ExpressReceiver.spec.ts @@ -3,7 +3,6 @@ import 'mocha'; import { Logger, LogLevel } from '@slack/logger'; import { assert } from 'chai'; import { Request, Response } from 'express'; -import { Agent } from 'http'; import sinon, { SinonFakeTimers } from 'sinon'; import { Readable } from 'stream'; @@ -41,10 +40,6 @@ describe('ExpressReceiver', () => { signingSecret: 'my-secret', logger: noopLogger, endpoints: { events: '/custom-endpoint' }, - agent: new Agent({ - maxSockets: 999, - }), - clientTls: undefined, }); assert.isNotNull(receiver); }); diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 9368934c6..226cf9748 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -1,5 +1,4 @@ -import { createServer, Server, Agent } from 'http'; -import { SecureContextOptions } from 'tls'; +import { createServer, Server } from 'http'; import express, { Request, Response, Application, RequestHandler, NextFunction } from 'express'; import rawBody from 'raw-body'; import App from './App'; @@ -15,8 +14,6 @@ export interface ExpressReceiverOptions { endpoints?: string | { [endpointType: string]: string; }; - agent?: Agent; - clientTls?: Pick; } /** From 6c9cb11917dd2c35db3c27d8b07b2fc62cf407fd Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Tue, 18 Feb 2020 08:21:58 -0500 Subject: [PATCH 3/8] Make express receiver's logger optional --- src/ExpressReceiver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 226cf9748..c01e3e1a3 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -10,7 +10,7 @@ import { Logger, ConsoleLogger } from '@slack/logger'; // if that's the reason, let's document that with a comment. export interface ExpressReceiverOptions { signingSecret: string; - logger: Logger; + logger?: Logger; endpoints?: string | { [endpointType: string]: string; }; From 8faa0b5928b8482299d35aa28198914051d4a055 Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Thu, 20 Feb 2020 13:02:19 -0500 Subject: [PATCH 4/8] Fix broken tests from merge --- src/App.spec.ts | 3 +-- src/receiver.spec.ts | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 90f3f7e57..7474c5d60 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -863,8 +863,7 @@ describe('App', () => { ]; // Act - receiverEvents.forEach(event => fakeReceiver.emit('message', event)); - await delay(); + await Promise.all(receiverEvents.map(event => fakeReceiver.sendEvent(event))); // Assert assert.isTrue(fakeLogger.info.called); diff --git a/src/receiver.spec.ts b/src/receiver.spec.ts index b8810aeb6..b88d24538 100644 --- a/src/receiver.spec.ts +++ b/src/receiver.spec.ts @@ -2,9 +2,8 @@ import { assert } from 'chai'; import querystring from 'querystring'; import sinon, { SinonFakeTimers } from 'sinon'; -import { parseRequestBody } from '../dist'; import { CodedError, ReceiverAuthenticityError } from './errors'; -import { verifyRequestSignature } from './receiver'; +import { verifyRequestSignature, parseRequestBody } from './receiver'; import { StringIndexed } from './types/helpers'; describe('receiver', () => { From 2313c4692d06478cca152ccd9d3241b6ac2a61cd Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Fri, 13 Mar 2020 15:11:15 -0400 Subject: [PATCH 5/8] Revert receiver refactor --- src/App.spec.ts | 3 +- src/App.ts | 3 +- src/ExpressReceiver.ts | 89 ++++++++++++++++++++++++++++- src/index.ts | 1 - src/receiver.spec.ts | 123 ----------------------------------------- src/receiver.ts | 103 ---------------------------------- src/types/index.ts | 1 + src/types/receiver.ts | 16 ++++++ 8 files changed, 108 insertions(+), 231 deletions(-) delete mode 100644 src/receiver.spec.ts delete mode 100644 src/receiver.ts create mode 100644 src/types/receiver.ts diff --git a/src/App.spec.ts b/src/App.spec.ts index 7474c5d60..6bd0817ca 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -5,8 +5,7 @@ import { assert } from 'chai'; import { Override, mergeOverrides, createFakeLogger, delay } from './test-helpers'; import rewiremock from 'rewiremock'; import { ErrorCode, UnknownError } from './errors'; -import { SayFn, NextMiddleware } from './types'; -import { Receiver, ReceiverEvent } from './receiver'; +import { Receiver, ReceiverEvent, SayFn, NextMiddleware } from './types'; import { ConversationStore } from './conversation-store'; import { LogLevel } from '@slack/logger'; import App, { ViewConstraints } from './App'; diff --git a/src/App.ts b/src/App.ts index 7357bae42..7916286cd 100644 --- a/src/App.ts +++ b/src/App.ts @@ -41,10 +41,11 @@ import { BlockAction, InteractiveMessage, SlackViewAction, + Receiver, + ReceiverEvent, RespondArguments, } from './types'; import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers'; -import { Receiver, ReceiverEvent } from './receiver'; import { CodedError, asCodedError, diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index c01e3e1a3..6376ea571 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -1,8 +1,11 @@ +import { AnyMiddlewareArgs, Receiver, ReceiverEvent } from './types'; import { createServer, Server } from 'http'; import express, { Request, Response, Application, RequestHandler, NextFunction } from 'express'; import rawBody from 'raw-body'; +import querystring from 'querystring'; +import crypto from 'crypto'; +import tsscmp from 'tsscmp'; import App from './App'; -import { verifySignatureAndParseBody, Receiver, ReceiverEvent } from './receiver'; import { ReceiverAuthenticityError } from './errors'; import { Logger, ConsoleLogger } from '@slack/logger'; @@ -195,3 +198,87 @@ function logError(logger: Logger, message: string, error: any): void { : `${message} (error: ${error})`; logger.warn(logMessage); } + +function verifyRequestSignature( + signingSecret: string, + body: string, + signature: string | undefined, + requestTimestamp: string | undefined, +): void { + if (signature === undefined || requestTimestamp === undefined) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Some headers are missing.', + ); + } + + const ts = Number(requestTimestamp); + if (isNaN(ts)) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Timestamp is invalid.', + ); + } + + // Divide current date to match Slack ts format + // Subtract 5 minutes from current time + const fiveMinutesAgo = Math.floor(Date.now() / 1000) - (60 * 5); + + if (ts < fiveMinutesAgo) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Timestamp is too old.', + ); + } + + const hmac = crypto.createHmac('sha256', signingSecret); + const [version, hash] = signature.split('='); + hmac.update(`${version}:${ts}:${body}`); + + if (!tsscmp(hash, hmac.digest('hex'))) { + throw new ReceiverAuthenticityError( + 'Slack request signing verification failed. Signature mismatch.', + ); + } +} + +/** + * This request handler has two responsibilities: + * - Verify the request signature + * - Parse request.body and assign the successfully parsed object to it. + */ +function verifySignatureAndParseBody( + signingSecret: string, + body: string, + headers: Record, +): AnyMiddlewareArgs['body'] { + // *** Request verification *** + const { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp, + 'content-type': contentType, + } = headers; + + verifyRequestSignature( + signingSecret, + body, + signature, + requestTimestamp, + ); + + return parseRequestBody(body, contentType); +} + +function parseRequestBody( + stringBody: string, + contentType: string | undefined, +): any { + if (contentType === 'application/x-www-form-urlencoded') { + const parsedBody = querystring.parse(stringBody); + + if (typeof parsedBody.payload === 'string') { + return JSON.parse(parsedBody.payload); + } + + return parsedBody; + } + + return JSON.parse(stringBody); +} diff --git a/src/index.ts b/src/index.ts index 6339d405a..84431a28d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -21,7 +21,6 @@ export { export * from './errors'; export * from './middleware/builtin'; -export * from './receiver'; export * from './types'; export { diff --git a/src/receiver.spec.ts b/src/receiver.spec.ts deleted file mode 100644 index b88d24538..000000000 --- a/src/receiver.spec.ts +++ /dev/null @@ -1,123 +0,0 @@ -// tslint:disable:no-implicit-dependencies -import { assert } from 'chai'; -import querystring from 'querystring'; -import sinon, { SinonFakeTimers } from 'sinon'; -import { CodedError, ReceiverAuthenticityError } from './errors'; -import { verifyRequestSignature, parseRequestBody } from './receiver'; -import { StringIndexed } from './types/helpers'; - -describe('receiver', () => { - describe('verifyRequestSignature', () => { - let clock: SinonFakeTimers; - - beforeEach(() => { - // requestTimestamp = 1531420618 means this timestamp - clock = sinon.useFakeTimers(new Date('Thu Jul 12 2018 11:36:58 GMT-0700').getTime()); - }); - - afterEach(() => { - clock.restore(); - }); - - // These values are example data in the official doc - // https://api.slack.com/docs/verifying-requests-from-slack - const signingSecret = '8f742231b10e8888abcd99yyyzzz85a5'; - const signature = 'v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503'; - const requestTimestamp = 1531420618; - const body = 'token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c'; - const headers = { - 'x-slack-signature': signature, - 'x-slack-request-timestamp': requestTimestamp, - }; - - function catchVerifyErrors(headers: StringIndexed): CodedError | undefined { - const { - 'x-slack-signature': signature, - 'x-slack-request-timestamp': requestTimestamp, - } = headers; - - let error; - - try { - verifyRequestSignature(signingSecret, body, signature, requestTimestamp); - } catch (err) { - error = err; - } - - return error; - } - - it('should verify requests', () => { - const error = catchVerifyErrors(headers); - // Assert - assert.isUndefined(error); - }); - - it('should detect headers missing signature', () => { - const error = catchVerifyErrors({ - // 'x-slack-signature': signature , - 'x-slack-request-timestamp': requestTimestamp, - }) as CodedError; - assert.instanceOf(error, ReceiverAuthenticityError); - assert.include(error.message, 'Some headers are missing.'); - }); - - it('should detect headers missing timestamp', () => { - const error = catchVerifyErrors({ - 'x-slack-signature': signature, - /*'x-slack-request-timestamp': requestTimestamp*/ - }); - - assert.instanceOf(error, ReceiverAuthenticityError); - assert.include(error?.message, 'Some headers are missing.'); - }); - - it('should detect invalid timestamp header', () => { - const error = catchVerifyErrors({ - ...headers, - 'x-slack-request-timestamp': 'Hello there!', - }); - - assert.instanceOf(error, ReceiverAuthenticityError); - assert.include(error?.message, 'Timestamp is invalid.'); - }); - - it('should detect too old timestamp', () => { - const error = catchVerifyErrors({ - ...headers, - 'x-slack-request-timestamp': 0, - }); - - assert.instanceOf(error, ReceiverAuthenticityError); - assert.include(error?.message, 'Timestamp is too old.'); - }); - - it('should detect signature mismatch', () => { - const error = catchVerifyErrors({ - ...headers, - 'x-slack-request-timestamp': requestTimestamp + 10, - }); - - assert.instanceOf(error, ReceiverAuthenticityError); - assert.include(error?.message, 'Signature mismatch.'); - }); - }); - - describe('parseRequestBody', () => { - const stringBody = 'token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c'; - const jsonBody = querystring.parse(stringBody); - - it('parses form-urlencoded bodies', () => { - assert.deepEqual(parseRequestBody(stringBody, 'application/x-www-form-urlencoded'), jsonBody); - }); - - it('parses form-urlencoded bodies with a payload', () => { - const payload = `payload=${JSON.stringify(jsonBody)}`; - assert.deepEqual(parseRequestBody(payload, 'application/x-www-form-urlencoded'), jsonBody); - }); - - it('parses json bodies', () => { - assert.deepEqual(parseRequestBody(JSON.stringify(jsonBody), 'application/json'), jsonBody); - }); - }); -}); diff --git a/src/receiver.ts b/src/receiver.ts deleted file mode 100644 index f7083acf0..000000000 --- a/src/receiver.ts +++ /dev/null @@ -1,103 +0,0 @@ -import crypto from 'crypto'; -import tsscmp from 'tsscmp'; -import querystring from 'querystring'; -import App from './App'; -import { AckFn, AnyMiddlewareArgs } from './types'; -import { ReceiverAuthenticityError } from './errors'; - -export interface ReceiverEvent { - body: Record; - // TODO: there should maybe be some more help for implementors of Receiver to know what kind of argument the AckFn - // is expected to deal with. - ack: AckFn; -} - -export interface Receiver { - init(app: App): void; - start(...args: any[]): Promise; - stop(...args: any[]): Promise; -} - -export function verifyRequestSignature( - signingSecret: string, - body: string, - signature: string | undefined, - requestTimestamp: string | undefined, -): void { - if (signature === undefined || requestTimestamp === undefined) { - throw new ReceiverAuthenticityError( - 'Slack request signing verification failed. Some headers are missing.', - ); - } - - const ts = Number(requestTimestamp); - if (isNaN(ts)) { - throw new ReceiverAuthenticityError( - 'Slack request signing verification failed. Timestamp is invalid.', - ); - } - - // Divide current date to match Slack ts format - // Subtract 5 minutes from current time - const fiveMinutesAgo = Math.floor(Date.now() / 1000) - (60 * 5); - - if (ts < fiveMinutesAgo) { - throw new ReceiverAuthenticityError( - 'Slack request signing verification failed. Timestamp is too old.', - ); - } - - const hmac = crypto.createHmac('sha256', signingSecret); - const [version, hash] = signature.split('='); - hmac.update(`${version}:${ts}:${body}`); - - if (!tsscmp(hash, hmac.digest('hex'))) { - throw new ReceiverAuthenticityError( - 'Slack request signing verification failed. Signature mismatch.', - ); - } -} - -/** - * This request handler has two responsibilities: - * - Verify the request signature - * - Parse request.body and assign the successfully parsed object to it. - */ -export function verifySignatureAndParseBody( - signingSecret: string, - body: string, - headers: Record, -): AnyMiddlewareArgs['body'] { - // *** Request verification *** - const { - 'x-slack-signature': signature, - 'x-slack-request-timestamp': requestTimestamp, - 'content-type': contentType, - } = headers; - - verifyRequestSignature( - signingSecret, - body, - signature, - requestTimestamp, - ); - - return parseRequestBody(body, contentType); -} - -export function parseRequestBody( - stringBody: string, - contentType: string | undefined, -): any { - if (contentType === 'application/x-www-form-urlencoded') { - const parsedBody = querystring.parse(stringBody); - - if (typeof parsedBody.payload === 'string') { - return JSON.parse(parsedBody.payload); - } - - return parsedBody; - } - - return JSON.parse(stringBody); -} diff --git a/src/types/index.ts b/src/types/index.ts index 3af090c70..2d05a8ded 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -5,3 +5,4 @@ export * from './command'; export * from './events'; export * from './options'; export * from './view'; +export * from './receiver'; diff --git a/src/types/receiver.ts b/src/types/receiver.ts new file mode 100644 index 000000000..c92ecc95e --- /dev/null +++ b/src/types/receiver.ts @@ -0,0 +1,16 @@ +import App from '../App'; +import { AckFn } from '../types'; +import { StringIndexed } from './helpers'; + +export interface ReceiverEvent { + body: StringIndexed; + // TODO: there should maybe be some more help for implementors of Receiver to know what kind of argument the AckFn + // is expected to deal with. + ack: AckFn; +} + +export interface Receiver { + init(app: App): void; + start(...args: any[]): Promise; + stop(...args: any[]): Promise; +} From 3d19d48991ed22181b8b80dd0790242fd14922bd Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Fri, 13 Mar 2020 15:23:12 -0400 Subject: [PATCH 6/8] Restore uneeded change --- src/test-helpers.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test-helpers.ts b/src/test-helpers.ts index 2e930387e..4cad7b9ef 100644 --- a/src/test-helpers.ts +++ b/src/test-helpers.ts @@ -2,12 +2,6 @@ import sinon, { SinonSpy } from 'sinon'; import { Logger } from '@slack/logger'; -export function delay(ms: number = 0): Promise { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); -} - export interface Override { [packageName: string]: { [exportName: string]: any; @@ -69,6 +63,12 @@ export function createFakeLogger(): FakeLogger { }; } +export function delay(ms: number = 0): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + export function wrapToResolveOnFirstCall void>( original: T, timeoutMs: number = 1000, From e9b53d90d5f758da25e826ebee4804ca2ea95771 Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Mon, 16 Mar 2020 13:54:11 -0400 Subject: [PATCH 7/8] Address review feedback --- src/App.spec.ts | 49 ++++++++++++---------------------- src/App.ts | 8 +++--- src/ExpressReceiver.ts | 22 +++++++++------ src/conversation-store.spec.ts | 8 +----- src/errors.ts | 10 ++++++- src/types/utilities.ts | 3 +-- 6 files changed, 47 insertions(+), 53 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 6bd0817ca..22c810036 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -213,17 +213,18 @@ describe('App', () => { describe('#start', () => { it('should pass calls through to receiver', async () => { // Arrange + const dummyReturn = Symbol(); const dummyParams = [Symbol(), Symbol()]; const fakeReceiver = new FakeReceiver(); const App = await importApp(); // tslint:disable-line:variable-name + const app = new App({ receiver: fakeReceiver, authorize: noopAuthorize }); + fakeReceiver.start = sinon.fake.returns(dummyReturn); // Act - const app = new App({ receiver: fakeReceiver, authorize: noopAuthorize }); - sinon.spy(app, 'start'); const actualReturn = await app.start(...dummyParams); // Assert - assert.deepEqual(actualReturn, dummyParams); + assert.deepEqual(actualReturn, dummyReturn); assert.deepEqual(dummyParams, fakeReceiver.start.firstCall.args); }); }); @@ -231,16 +232,18 @@ describe('App', () => { describe('#stop', () => { it('should pass calls through to receiver', async () => { // Arrange - const dummyParams = [1, 2]; + const dummyReturn = Symbol(); + const dummyParams = [Symbol(), Symbol()]; const fakeReceiver = new FakeReceiver(); const App = await importApp(); // tslint:disable-line:variable-name + fakeReceiver.stop = sinon.fake.returns(dummyReturn); // Act const app = new App({ receiver: fakeReceiver, authorize: noopAuthorize }); const actualReturn = await app.stop(...dummyParams); // Assert - assert.deepEqual(actualReturn, dummyParams); + assert.deepEqual(actualReturn, dummyReturn); assert.deepEqual(dummyParams, fakeReceiver.stop.firstCall.args); }); }); @@ -277,8 +280,8 @@ describe('App', () => { // Act const app = new App({ receiver: fakeReceiver, logger: fakeLogger, authorize: noopAuthorize }); app.use(fakeMiddleware); - await Promise.all(invalidReceiverEvents.map(event => fakeReceiver.sendEvent(event))); + // Assert assert(fakeErrorHandler.notCalled); assert(fakeMiddleware.notCalled); @@ -393,6 +396,11 @@ describe('App', () => { const message = ':wave:'; let middlewareCount = 0; + /** + * Middleware that, when calls, asserts that it was called in the correct order + * @param orderDown The order it should be called when processing middleware down the stack + * @param orderUp The order it should be called when processing middleware up the stack + */ const assertOrderMiddleware = (orderDown: number, orderUp: number) => async ({ next }: any) => { await delay(100); middlewareCount += 1; @@ -422,43 +430,20 @@ describe('App', () => { assert(fakeErrorHandler.notCalled); }); - it('should, on error, ack an error, then global error handler', async () => { + it('should, on error call the global error handler', async () => { const error = new Error('Everything is broke, you probably should restart, if not then good luck'); - let callCount = 0; - - const assertOrder = (order: number) => { - callCount += 1; - assert.equal(callCount, order); - }; app.use(() => { - assertOrder(1); throw error; }); - const event = { - ...dummyReceiverEvent, - ack: (actualError: Error): Promise => { - if (actualError instanceof Error) { - assert.instanceOf(actualError, UnknownError); - assert.equal(actualError.message, error.message); - assertOrder(2); - } - - return Promise.resolve(); - }, - }; - app.error(async (actualError) => { assert.instanceOf(actualError, UnknownError); assert.equal(actualError.message, error.message); - assertOrder(3); await delay(); // Make this async to make sure error handlers can be tested }); - await fakeReceiver.sendEvent(event); - - assert.equal(callCount, 3); + await fakeReceiver.sendEvent(dummyReceiverEvent); }); it('with a default global error handler, rejects App#ProcessEvent', async () => { @@ -1143,7 +1128,7 @@ describe('App', () => { assert.equal(assertionAggregator.callCount, dummyReceiverEvents.length); }); - it('should handle failures through the App\'s global error handler', async () => { + it("should handle failures through the App's global error handler", async () => { // Arrange const fakePostMessage = sinon.fake.rejects(new Error('fake error')); const overrides = buildOverrides([withPostMessage(fakePostMessage)]); diff --git a/src/App.ts b/src/App.ts index 7916286cd..f64f54fe1 100644 --- a/src/App.ts +++ b/src/App.ts @@ -244,7 +244,6 @@ export default class App { }); } } - this.receiver.init(this); // Conditionally use a global middleware that ignores events (including messages) that are sent from this app if (ignoreSelf) { @@ -257,6 +256,9 @@ export default class App { const store: ConversationStore = convoStore === undefined ? new MemoryStore() : convoStore; this.use(conversationContext(store, this.logger)); } + + // Should be last to avoid exposing partially initialized app + this.receiver.init(this); } /** @@ -444,6 +446,7 @@ export default class App { authorizeResult = await this.authorize(source, bodyArg); } catch (error) { this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); + this.handleError(error); return; } @@ -567,7 +570,6 @@ export default class App { ...(listenerArgs as MiddlewareContext), }); } catch (error) { - await event.ack(asCodedError(error)); return this.handleError(error); } } @@ -634,7 +636,7 @@ function defaultErrorHandler(logger: Logger): ErrorHandler { return (error) => { logger.error(error); - throw error; + return Promise.reject(error); }; } diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 6376ea571..7230a338b 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -6,7 +6,7 @@ import querystring from 'querystring'; import crypto from 'crypto'; import tsscmp from 'tsscmp'; import App from './App'; -import { ReceiverAuthenticityError } from './errors'; +import { ReceiverAuthenticityError, ReceiverAckTimeoutError, ReceiverMultipleAckError } from './errors'; import { Logger, ConsoleLogger } from '@slack/logger'; // TODO: we throw away the key names for endpoints, so maybe we should use this interface. is it better for migrations? @@ -54,9 +54,9 @@ export default class ExpressReceiver implements Receiver { } private async requestHandler(req: Request, res: Response): Promise { - let timer: NodeJS.Timer | undefined = setTimeout( + let timer: NodeJS.Timeout | undefined = setTimeout( () => { - this.bolt?.handleError(new ReceiverAuthenticityError( + this.bolt?.handleError(new ReceiverAckTimeoutError( 'An incoming event was not acknowledged before the timeout. ' + 'Ensure that the ack() argument is called in your listeners.', )); @@ -64,6 +64,7 @@ export default class ExpressReceiver implements Receiver { }, 2800, ); + const event: ReceiverEvent = { body: req.body, ack: async (response): Promise => { @@ -71,27 +72,32 @@ export default class ExpressReceiver implements Receiver { clearTimeout(timer); timer = undefined; - if (response instanceof Error) { - res.send(500); - } else if (!response) { + if (!response) { res.send(''); } else if (typeof response === 'string') { res.send(response); } else { res.json(response); } + } else { + this.bolt?.handleError(new ReceiverMultipleAckError()); } }, }; - await this.bolt?.processEvent(event); + try { + await this.bolt?.processEvent(event); + } catch (err) { + res.send(500); + throw err; + } } public init(bolt: App): void { this.bolt = bolt; } - // TODO: the arguments should be defined as the arguments of Server#listen() + // TODO: the arguments should be defined as the arguments of Server#listen() // TODO: the return value should be defined as a type that both http and https servers inherit from, or a union public start(port: number): Promise { return new Promise((resolve, reject) => { diff --git a/src/conversation-store.spec.ts b/src/conversation-store.spec.ts index 3e03825ce..09419fee0 100644 --- a/src/conversation-store.spec.ts +++ b/src/conversation-store.spec.ts @@ -2,19 +2,13 @@ import 'mocha'; import { assert, AssertionError } from 'chai'; import sinon, { SinonSpy } from 'sinon'; -import { Override, createFakeLogger, wrapToResolveOnFirstCall } from './test-helpers'; +import { Override, createFakeLogger, delay, wrapToResolveOnFirstCall } from './test-helpers'; import rewiremock from 'rewiremock'; import { ConversationStore } from './conversation-store'; import { AnyMiddlewareArgs, NextMiddleware, Context } from './types'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; -function delay(ms: number = 0): Promise { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); -} - describe('conversationContext middleware', () => { it('should forward events that have no conversation ID', async () => { // Arrange diff --git a/src/errors.ts b/src/errors.ts index 8a935c08f..f8150e9c6 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -9,7 +9,7 @@ export enum ErrorCode { ContextMissingPropertyError = 'slack_bolt_context_missing_property_error', ReceiverAckTimeoutError = 'slack_bolt_receiver_ack_timeout_error', - + ReceiverAckTwiceError = 'slack_bolt_receiver_ack_twice_error', ReceiverAuthenticityError = 'slack_bolt_receiver_authenticity_error', /** @@ -56,6 +56,14 @@ export class ReceiverAckTimeoutError extends Error implements CodedError { public code = ErrorCode.ReceiverAckTimeoutError; } +export class ReceiverMultipleAckError extends Error implements CodedError { + public code = ErrorCode.ReceiverAckTimeoutError; + + constructor() { + super("The receiver's `ack` function was called multiple times."); + } +} + export class ReceiverAuthenticityError extends Error implements CodedError { public code = ErrorCode.ReceiverAuthenticityError; } diff --git a/src/types/utilities.ts b/src/types/utilities.ts index 07a1e5000..5563ba8ac 100644 --- a/src/types/utilities.ts +++ b/src/types/utilities.ts @@ -1,6 +1,5 @@ import { ChatPostMessageArguments, WebAPICallResult } from '@slack/web-api'; import { KnownKeys } from './helpers'; -import { CodedError } from '../errors'; // The say() utility function binds the message to the same channel as the incoming message that triggered the // listener. Therefore, specifying the `channel` argument is not required. @@ -24,5 +23,5 @@ export interface RespondFn { } export interface AckFn { - (response?: Response | CodedError): Promise; + (response?: Response): Promise; } From b25ad277e6e94c94fede44d110c8952f403fc6b9 Mon Sep 17 00:00:00 2001 From: Michael Barlock Date: Tue, 17 Mar 2020 14:16:44 -0400 Subject: [PATCH 8/8] Fix broken promise --- src/App.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/App.ts b/src/App.ts index f64f54fe1..cfd7bb497 100644 --- a/src/App.ts +++ b/src/App.ts @@ -446,8 +446,7 @@ export default class App { authorizeResult = await this.authorize(source, bodyArg); } catch (error) { this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); - this.handleError(error); - return; + return this.handleError(error); } const context: Context = { ...authorizeResult };