From a307def0975299f86556399278e8e03f7604a4e8 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Fri, 29 Oct 2021 13:00:57 +0900 Subject: [PATCH 1/2] Fix #860 Enable developers to customize the built-in receivers more --- examples/custom-properties/http.js | 23 +++- src/receivers/HTTPReceiver.spec.ts | 20 ++++ src/receivers/HTTPReceiver.ts | 165 +++++++++++++++++++++++------ 3 files changed, 174 insertions(+), 34 deletions(-) diff --git a/examples/custom-properties/http.js b/examples/custom-properties/http.js index 8ef56ce2a..cf62a31d1 100644 --- a/examples/custom-properties/http.js +++ b/examples/custom-properties/http.js @@ -9,7 +9,28 @@ const app = new App({ "headers": req.headers, "foo": "bar", }; - } + }, + // other custom handlers + dispatchErrorHandler: ({ error, logger, response }) => { + logger.error(`dispatch error: ${error}`); + response.writeHead(404); + response.write("Something is wrong!"); + response.end(); + }, + processEventErrorHandler: ({ error, logger, response }) => { + logger.error(`processEvent error: ${error}`); + // acknowledge it anyway! + response.writeHead(200); + response.end(); + return true; + }, + unhandledRequestHandler: async ({ logger, response }) => { + // acknowledge it anyway! + logger.info('Acknowledging this incoming request because 2 seconds already passed...'); + response.writeHead(200); + response.end(); + }, + unhandledRequestTimeoutMillis: 2000, // the default is 3001 }), }); diff --git a/src/receivers/HTTPReceiver.spec.ts b/src/receivers/HTTPReceiver.spec.ts index b878f7a3c..a3c077238 100644 --- a/src/receivers/HTTPReceiver.spec.ts +++ b/src/receivers/HTTPReceiver.spec.ts @@ -115,6 +115,26 @@ describe('HTTPReceiver', function () { userScopes: ['chat:write'], }, customPropertiesExtractor: (req) => ({ headers: req.headers }), + dispatchErrorHandler: ({ error, logger, response }) => { + logger.error(`An unhandled request detected: ${error}`); + response.writeHead(500); + response.write('Something is wrong!'); + response.end(); + }, + processEventErrorHandler: async ({ error, logger, response }) => { + logger.error(`processEvent error: ${error}`); + // acknowledge it anyway! + response.writeHead(200); + response.end(); + return true; + }, + unhandledRequestHandler: ({ logger, response }) => { + // acknowledge it anyway! + logger.info('Acknowledging this incoming request because 2 seconds already passed...'); + response.writeHead(200); + response.end(); + }, + unhandledRequestTimeoutMillis: 2000, // the default is 3001 }); assert.isNotNull(receiver); }); diff --git a/src/receivers/HTTPReceiver.ts b/src/receivers/HTTPReceiver.ts index 2b04a383b..9b95fb433 100644 --- a/src/receivers/HTTPReceiver.ts +++ b/src/receivers/HTTPReceiver.ts @@ -59,6 +59,7 @@ const httpsOptionKeys = [ const missingServerErrorDescription = 'The receiver cannot be started because private state was mutated. Please report this to the maintainers.'; +// All the available arguments in the constructor export interface HTTPReceiverOptions { signingSecret: string; endpoints?: string | string[]; @@ -76,7 +77,15 @@ export interface HTTPReceiverOptions { scopes?: InstallURLOptions['scopes']; installerOptions?: HTTPReceiverInstallerOptions; customPropertiesExtractor?: (request: BufferedIncomingMessage) => StringIndexed; + // NOTE: As http.RequestListener is not an async function, this cannot be async + dispatchErrorHandler?: (args: HTTPReceiverDispatchErrorHandlerArgs) => void; + processEventErrorHandler?: (args: HTTPReceiverProcessEventErrorHandlerArgs) => Promise; + // NOTE: As we use setTimeout under the hood, this cannot be async + unhandledRequestHandler?: (args: HTTPReceiverUnhandledRequestHandlerArgs) => void; + unhandledRequestTimeoutMillis?: number; } + +// All the available argument for OAuth flow enabled apps export interface HTTPReceiverInstallerOptions { installPath?: string; directInstall?: boolean; // see https://api.slack.com/start/distributing/directory#direct_install @@ -95,6 +104,85 @@ export interface HTTPReceiverInstallerOptions { port?: number; } +// The arguments for the dispatchErrorHandler, +// which handles errors occurred while dispatching a rqeuest +export interface HTTPReceiverDispatchErrorHandlerArgs { + error: Error | CodedError; + logger: Logger; + request: IncomingMessage; + response: ServerResponse; +} + +// The default dispathErrorHandler implementation: +// Developers can customize this behavior by passing dispatchErrorHandler to the constructor +// Note that it was not possible to make this function async due to the limitation of http module +function defaultDispatchErrorHandler(args: HTTPReceiverDispatchErrorHandlerArgs) { + const { error, logger, request, response } = args; + if ('code' in error) { + if (error.code === ErrorCode.HTTPReceiverDeferredRequestError) { + logger.info(`Unhandled HTTP request (${request.method}) made to ${request.url}`); + response.writeHead(404); + response.end(); + return; + } + } + logger.error(`An unexpected error occurred during a request (${request.method}) made to ${request.url}`); + logger.debug(`Error details: ${error}`); + response.writeHead(500); + response.end(); +} + +// The arguments for the processEventErrorHandler, +// which handles errors `await app.processEvent(even)` method throws +export interface HTTPReceiverProcessEventErrorHandlerArgs { + error: Error | CodedError; + logger: Logger; + request: IncomingMessage; + response: ServerResponse; + storedResponse: any; +} + +// The default processEventErrorHandler implementation: +// Developers can customize this behavior by passing processEventErrorHandler to the constructor +async function defaultProcessEventErrorHandler(args: HTTPReceiverProcessEventErrorHandlerArgs): Promise { + const { error, response, logger, storedResponse } = args; + if ('code' in error) { + // CodedError has code: string + const errorCode = (error as CodedError).code; + if (errorCode === ErrorCode.AuthorizationError) { + // authorize function threw an exception, which means there is no valid installation data + response.writeHead(401); + response.end(); + return true; + } + } + logger.error('An unhandled error occurred while Bolt processed an event'); + logger.debug(`Error details: ${error}, storedResponse: ${storedResponse}`); + response.writeHead(500); + response.end(); + return false; +} + +// The arguments for the unhandledRequestHandler, +// which deals with any unhandled incoming requests from Slack. +// (The default behavior is just printing error logs) +export interface HTTPReceiverUnhandledRequestHandlerArgs { + logger: Logger; + request: IncomingMessage; + response: ServerResponse; +} + +// The default unhandledRequestHandler implementation: +// Developers can customize this behavior by passing unhandledRequestHandler to the constructor +// Note that this method cannot be an async function to align with the implementation using setTimeout +function defaultUnhandledRequestHandler(args: HTTPReceiverUnhandledRequestHandlerArgs): void { + const { logger } = args; + logger.error( + 'An incoming event was not acknowledged within 3 seconds. ' + + 'Ensure that the ack() argument is called in a listener.', + ); +} + /** * Receives HTTP requests with Events, Slash Commands, and Actions */ @@ -137,6 +225,14 @@ export default class HTTPReceiver implements Receiver { private customPropertiesExtractor: (request: BufferedIncomingMessage) => StringIndexed; + private dispatchErrorHandler: (args: HTTPReceiverDispatchErrorHandlerArgs) => void; + + private processEventErrorHandler: (args: HTTPReceiverProcessEventErrorHandlerArgs) => Promise; + + private unhandledRequestHandler: (args: HTTPReceiverUnhandledRequestHandlerArgs) => void; + + private unhandledRequestTimeoutMillis: number; + public constructor({ signingSecret = '', endpoints = ['/slack/events'], @@ -154,6 +250,10 @@ export default class HTTPReceiver implements Receiver { scopes = undefined, installerOptions = {}, customPropertiesExtractor = (_req) => ({}), + dispatchErrorHandler = defaultDispatchErrorHandler, + processEventErrorHandler = defaultProcessEventErrorHandler, + unhandledRequestHandler = defaultUnhandledRequestHandler, + unhandledRequestTimeoutMillis = 3001, }: HTTPReceiverOptions) { // Initialize instance variables, substituting defaults for each value this.signingSecret = signingSecret; @@ -211,6 +311,10 @@ export default class HTTPReceiver implements Receiver { installerOptions.renderHtmlForInstallPath : defaultRenderHtmlForInstallPath; this.customPropertiesExtractor = customPropertiesExtractor; + this.dispatchErrorHandler = dispatchErrorHandler; + this.processEventErrorHandler = processEventErrorHandler; + this.unhandledRequestHandler = unhandledRequestHandler; + this.unhandledRequestTimeoutMillis = unhandledRequestTimeoutMillis; // Assign the requestListener property by binding the unboundRequestListener to this instance this.requestListener = this.unboundRequestListener.bind(this); @@ -248,17 +352,14 @@ export default class HTTPReceiver implements Receiver { try { this.requestListener(req, res); } catch (error) { - const e = error as any; - if (e.code === ErrorCode.HTTPReceiverDeferredRequestError) { - this.logger.info(`Unhandled HTTP request (${req.method}) made to ${req.url}`); - res.writeHead(404); - res.end(); - } else { - this.logger.error(`An unexpected error occurred during a request (${req.method}) made to ${req.url}`); - this.logger.debug(`Error details: ${e}`); - res.writeHead(500); - res.end(); - } + // You may get an error here only when the requestListener failed + // to start processing incoming requests, or your app receives a request to an unexpected path. + this.dispatchErrorHandler({ + error: error as Error | CodedError, + logger: this.logger, + request: req, + response: res, + }); } }); @@ -365,6 +466,9 @@ export default class HTTPReceiver implements Receiver { // If the request did not match the previous conditions, an error is thrown. The error can be caught by the // the caller in order to defer to other routing logic (similar to calling `next()` in connect middleware). + // If you would like to customize the HTTP repsonse for this pattern, + // implement your own dispatchErrorHandler that handles an exception + // with ErrorCode.HTTPReceiverDeferredRequestError. throw new HTTPReceiverDeferredRequestError(`Unhandled HTTP request (${method}) made to ${path}`, req, res); } @@ -424,12 +528,13 @@ export default class HTTPReceiver implements Receiver { let isAcknowledged = false; setTimeout(() => { if (!isAcknowledged) { - this.logger.error( - 'An incoming event was not acknowledged within 3 seconds. ' + - 'Ensure that the ack() argument is called in a listener.', - ); + this.unhandledRequestHandler({ + logger: this.logger, + request: req, + response: res, + }); } - }, 3001); + }, this.unhandledRequestTimeoutMillis); // Structure the ReceiverEvent let storedResponse; @@ -442,6 +547,8 @@ export default class HTTPReceiver implements Receiver { } isAcknowledged = true; if (this.processBeforeResponse) { + // In the case where processBeforeResponse: true is enabled, we don't send the HTTP response immediately. + // We hold off until the listener execution is completed. if (!response) { storedResponse = ''; } else { @@ -471,6 +578,7 @@ export default class HTTPReceiver implements Receiver { try { await this.app?.processEvent(event); if (storedResponse !== undefined) { + // in the case of processBeforeResponse: true if (typeof storedResponse === 'string') { res.writeHead(200); res.end(storedResponse); @@ -480,23 +588,14 @@ export default class HTTPReceiver implements Receiver { } this.logger.debug('stored response sent'); } - } catch (err) { - const e = err as any; - if ('code' in e) { - // CodedError has code: string - const errorCode = (e as CodedError).code; - if (errorCode === ErrorCode.AuthorizationError) { - // authorize function threw an exception, which means there is no valid installation data - res.writeHead(401); - res.end(); - isAcknowledged = true; - return; - } - } - this.logger.error('An unhandled error occurred while Bolt processed an event'); - this.logger.debug(`Error details: ${e}, storedResponse: ${storedResponse}`); - res.writeHead(500); - res.end(); + } catch (error) { + isAcknowledged = await this.processEventErrorHandler({ + error: error as Error | CodedError, + logger: this.logger, + request: req, + response: res, + storedResponse, + }); } })(); } From 9c2d8a762e1bef8b56130d23fc7720c8c0ffe35a Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 30 Oct 2021 10:06:29 +0900 Subject: [PATCH 2/2] Adjust processEventErrorHandler and add mored tests --- src/receivers/HTTPReceiver.spec.ts | 65 +++++++++++++++++++++++++++++- src/receivers/HTTPReceiver.ts | 15 +++++-- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/receivers/HTTPReceiver.spec.ts b/src/receivers/HTTPReceiver.spec.ts index a3c077238..6786446a0 100644 --- a/src/receivers/HTTPReceiver.spec.ts +++ b/src/receivers/HTTPReceiver.spec.ts @@ -7,7 +7,13 @@ import { EventEmitter } from 'events'; import { InstallProvider } from '@slack/oauth'; import { IncomingMessage, ServerResponse } from 'http'; import { Override, mergeOverrides } from '../test-helpers'; -import { AppInitializationError, CustomRouteInitializationError, HTTPReceiverDeferredRequestError } from '../errors'; +import { + AppInitializationError, + CustomRouteInitializationError, + HTTPReceiverDeferredRequestError, + CodedError, +} from '../errors'; +import { defaultDispatchErrorHandler, defaultProcessEventErrorHandler, defaultUnhandledRequestHandler } from './HTTPReceiver'; /* Testing Harness */ @@ -548,4 +554,61 @@ describe('HTTPReceiver', function () { assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError); }); }); + + describe('handlers for customization', async function () { + it('should have defaultDispatchErrorHandler', async function () { + const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage; + fakeReq.url = '/nope'; + fakeReq.headers = { host: 'localhost' }; + fakeReq.method = 'GET'; + + const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse; + fakeRes.writeHead = sinon.fake(); + fakeRes.end = sinon.fake(); + + defaultDispatchErrorHandler({ + error: { code: 'foo' } as CodedError, + logger: noopLogger, + request: fakeReq, + response: fakeRes, + }); + }); + + it('should have defaultProcessEventErrorHandler', async function () { + const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage; + fakeReq.url = '/nope'; + fakeReq.headers = { host: 'localhost' }; + fakeReq.method = 'GET'; + + const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse; + fakeRes.writeHead = sinon.fake(); + fakeRes.end = sinon.fake(); + + const result = await defaultProcessEventErrorHandler({ + error: { code: 'foo' } as CodedError, + logger: noopLogger, + request: fakeReq, + response: fakeRes, + storedResponse: undefined, + }); + assert.isFalse(result); + }); + + it('should have defaultUnhandledRequestHandler', async function () { + const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage; + fakeReq.url = '/nope'; + fakeReq.headers = { host: 'localhost' }; + fakeReq.method = 'GET'; + + const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse; + fakeRes.writeHead = sinon.fake(); + fakeRes.end = sinon.fake(); + + defaultUnhandledRequestHandler({ + logger: noopLogger, + request: fakeReq, + response: fakeRes, + }); + }); + }); }); diff --git a/src/receivers/HTTPReceiver.ts b/src/receivers/HTTPReceiver.ts index 9b95fb433..2ad02c76d 100644 --- a/src/receivers/HTTPReceiver.ts +++ b/src/receivers/HTTPReceiver.ts @@ -116,7 +116,7 @@ export interface HTTPReceiverDispatchErrorHandlerArgs { // The default dispathErrorHandler implementation: // Developers can customize this behavior by passing dispatchErrorHandler to the constructor // Note that it was not possible to make this function async due to the limitation of http module -function defaultDispatchErrorHandler(args: HTTPReceiverDispatchErrorHandlerArgs) { +export function defaultDispatchErrorHandler(args: HTTPReceiverDispatchErrorHandlerArgs): void { const { error, logger, request, response } = args; if ('code' in error) { if (error.code === ErrorCode.HTTPReceiverDeferredRequestError) { @@ -144,7 +144,9 @@ export interface HTTPReceiverProcessEventErrorHandlerArgs { // The default processEventErrorHandler implementation: // Developers can customize this behavior by passing processEventErrorHandler to the constructor -async function defaultProcessEventErrorHandler(args: HTTPReceiverProcessEventErrorHandlerArgs): Promise { +export async function defaultProcessEventErrorHandler( + args: HTTPReceiverProcessEventErrorHandlerArgs, +): Promise { const { error, response, logger, storedResponse } = args; if ('code' in error) { // CodedError has code: string @@ -175,7 +177,7 @@ export interface HTTPReceiverUnhandledRequestHandlerArgs { // The default unhandledRequestHandler implementation: // Developers can customize this behavior by passing unhandledRequestHandler to the constructor // Note that this method cannot be an async function to align with the implementation using setTimeout -function defaultUnhandledRequestHandler(args: HTTPReceiverUnhandledRequestHandlerArgs): void { +export function defaultUnhandledRequestHandler(args: HTTPReceiverUnhandledRequestHandlerArgs): void { const { logger } = args; logger.error( 'An incoming event was not acknowledged within 3 seconds. ' + @@ -589,13 +591,18 @@ export default class HTTPReceiver implements Receiver { this.logger.debug('stored response sent'); } } catch (error) { - isAcknowledged = await this.processEventErrorHandler({ + const acknowledgedByHandler = await this.processEventErrorHandler({ error: error as Error | CodedError, logger: this.logger, request: req, response: res, storedResponse, }); + if (acknowledgedByHandler) { + // If the value is false, we don't touch the value as a race condition + // with ack() call may occur especially when processBeforeResponse: false + isAcknowledged = true; + } } })(); }