From c169a2c6514443e88bd3f0c0c026e88f3d83a2e3 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sun, 19 May 2019 11:51:52 -0700 Subject: [PATCH 1/2] Fix #192 ExpressReceiver support for rawBody for signature verification --- src/ExpressReceiver.spec.ts | 204 ++++++++++++++++++++++++++++++++++++ src/ExpressReceiver.ts | 137 +++++++++++++++++------- 2 files changed, 302 insertions(+), 39 deletions(-) create mode 100644 src/ExpressReceiver.spec.ts diff --git a/src/ExpressReceiver.spec.ts b/src/ExpressReceiver.spec.ts new file mode 100644 index 000000000..e222218a8 --- /dev/null +++ b/src/ExpressReceiver.spec.ts @@ -0,0 +1,204 @@ +// tslint:disable:no-implicit-dependencies +import 'mocha'; +import { assert } from 'chai'; +import { Request, Response } from 'express'; +import { verifySignatureAndParseBody } from './ExpressReceiver'; +import sinon, { SinonFakeTimers } from 'sinon'; +import { Readable } from 'stream'; +import { Logger, LogLevel } from '@slack/logger'; + +describe('ExpressReceiver', () => { + + const noopLogger: Logger = { + debug(..._msg: any[]): void { }, + info(..._msg: any[]): void { }, + warn(..._msg: any[]): void { }, + error(..._msg: any[]): void { }, + setLevel(_level: LogLevel): void { }, + setName(_name: string): void { }, + }; + + describe('verifySignatureAndParseBody', () => { + + let clock: SinonFakeTimers; + + beforeEach(function () { + // requestTimestamp = 1531420618 means this timestamp + clock = sinon.useFakeTimers(new Date('Thu Jul 12 2018 11:36:58 GMT-0700').getTime()); + }); + + afterEach(function () { + 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'; + + function buildExpressRequest(): Request { + const reqAsStream = new Readable(); + reqAsStream.push(body); + reqAsStream.push(null); // indicate EOF + (reqAsStream as { [key: string]: any }).headers = { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp + }; + const req = reqAsStream as Request; + return req; + } + + function buildGCPRequest(): Request { + const untypedReq: { [key: string]: any } = { + rawBody: body, + headers: { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp + } + }; + const req = untypedReq as Request; + return req; + } + + // ---------------------------- + // runWithValidRequest + + async function runWithValidRequest(req: Request, errorResult: any) { + // Arrange + const resp = {} as Response; + const next = (error: any) => { errorResult = error; }; + + // Act + const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + await verifier(req, resp, next); + return errorResult; + } + + it('should verify requests', async () => { + let errorResult: any; + runWithValidRequest(buildExpressRequest(), errorResult); + // Assert + assert.isUndefined(errorResult); + }); + + it('should verify requests on GCP', async () => { + let errorResult: any; + runWithValidRequest(buildGCPRequest(), errorResult); + // Assert + assert.isUndefined(errorResult); + }); + + // ---------------------------- + // verifyMissingHeaderDetection + + function verifyMissingHeaderDetection(req: Request): Promise { + // Arrange + const resp = {} as Response; + let errorResult: any; + const next = (error: any) => { errorResult = error; }; + + // Act + const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + return verifier(req, resp, next).then((_: any) => { + // Assert + assert.equal(errorResult, 'Error: Slack request signing verification failed. Some headers are missing.'); + }) + } + + it('should detect headers missing', async () => { + const reqAsStream = new Readable(); + reqAsStream.push(body); + reqAsStream.push(null); // indicate EOF + (reqAsStream as { [key: string]: any }).headers = { + 'x-slack-signature': signature /*, + 'x-slack-request-timestamp': requestTimestamp */ + }; + await verifyMissingHeaderDetection(reqAsStream as Request); + }); + + it('should detect headers missing on GCP', async () => { + const untypedReq: { [key: string]: any } = { + rawBody: body, + headers: { + 'x-slack-signature': signature /*, + 'x-slack-request-timestamp': requestTimestamp */ + } + }; + await verifyMissingHeaderDetection(untypedReq as Request); + }); + + // ---------------------------- + // verifyTooOldTimestampError + + function verifyTooOldTimestampError(req: Request): Promise { + // Arrange + // restore the valid clock + clock.restore(); + + const resp = {} as Response; + let errorResult: any; + const next = (error: any) => { errorResult = error; }; + + // Act + const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + return verifier(req, resp, next).then((_: any) => { + // Assert + assert.equal(errorResult, 'Error: Slack request signing verification failed. Timestamp is too old.'); + }); + } + + it('should detect too old tiestamp', async () => { + await verifyTooOldTimestampError(buildExpressRequest()); + }); + + it('should detect too old tiestamp on GCP', async () => { + await verifyTooOldTimestampError(buildGCPRequest()); + }); + + // ---------------------------- + // verifySingnatureMismatch + + function verifySingnatureMismatch(req: Request): Promise { + // Arrange + const resp = {} as Response; + let errorResult: any; + const next = (error: any) => { errorResult = error; }; + + // Act + const verifier = verifySignatureAndParseBody(noopLogger, signingSecret); + verifier(req, resp, next); + return verifier(req, resp, next).then((_: any) => { + // Assert + assert.equal(errorResult, 'Error: Slack request signing verification failed. Signature mismatch.'); + }); + } + + it('should detect signature mismatch', async () => { + const reqAsStream = new Readable(); + reqAsStream.push(body); + reqAsStream.push(null); // indicate EOF + (reqAsStream as { [key: string]: any }).headers = { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp + 10 + }; + const req = reqAsStream as Request; + await verifySingnatureMismatch(req); + }); + + it('should detect signature mismatch on GCP', async () => { + const untypedReq: { [key: string]: any } = { + rawBody: body, + headers: { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp + 10 + } + }; + const req = untypedReq as Request; + await verifySingnatureMismatch(req); + }); + + }); + +}); \ No newline at end of file diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 1667caaf3..4032f8ce7 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -4,15 +4,17 @@ import { createServer, Server } from 'http'; import express, { Request, Response, Application, RequestHandler, NextFunction } from 'express'; import axios from 'axios'; import rawBody from 'raw-body'; +import querystring from 'querystring'; import crypto from 'crypto'; import tsscmp from 'tsscmp'; -import querystring from 'querystring'; import { ErrorCode, errorWithCode } 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; endpoints?: string | { [endpointType: string]: string; }; @@ -28,9 +30,10 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { private server: Server; - constructor ({ + constructor({ signingSecret = '', - endpoints = { events: '/slack/events' }, + logger = new ConsoleLogger(), + endpoints = { events: '/slack/events' } }: ExpressReceiverOptions) { super(); @@ -40,8 +43,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver { this.server = createServer(this.app); const expressMiddleware: RequestHandler[] = [ - verifySlackRequest(signingSecret), - parseBody, + verifySignatureAndParseBody(logger, signingSecret), respondToSslCheck, respondToUrlVerification, this.requestHandler.bind(this), @@ -151,40 +153,44 @@ const respondToUrlVerification: RequestHandler = (req, res, next) => { next(); }; -// TODO: this should be imported from another package -function verifySlackRequest(signingSecret: string): RequestHandler { - return async (req , _res, next) => { +/** + * This request handler has two responsibilities: + * - Verify the request signature + * - Parse request.body and assign the successfully parsed object to it. + */ +export function verifySignatureAndParseBody( + logger: Logger, + signingSecret: string): RequestHandler { + return async (req, _res, next) => { try { - const body: string = (await rawBody(req)).toString(); + // *** Request verification *** + let stringBody: string; + // On some environments like GCP (Google Cloud Platform), + // req.body can be pre-parsed and be passed as req.rawBody here + const preparsedRawBody: any = (req as any).rawBody; + if (preparsedRawBody !== undefined) { + stringBody = preparsedRawBody.toString(); + } else { + stringBody = (await rawBody(req)).toString(); + } const signature = req.headers['x-slack-signature'] as string; const ts = Number(req.headers['x-slack-request-timestamp']); - // 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) { - const error = errorWithCode( - 'Slack request signing verification failed. Timestamp is too old.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - next(error); + try { + await verifyRequestSignature(signingSecret, stringBody, signature, ts); + } catch (e) { + return next(e); } - const hmac = crypto.createHmac('sha256', signingSecret); - const [version, hash] = signature.split('='); - hmac.update(`${version}:${ts}:${body}`); + // *** 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. - if (!tsscmp(hash, hmac.digest('hex'))) { - const error = errorWithCode( - 'Slack request signing verification failed. Signature mismatch.', - ErrorCode.ExpressReceiverAuthenticityError, - ); - next(error); - } + // 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); - // Verification passed, assign string body back to request and resume - req.body = body; next(); } catch (error) { next(error); @@ -192,16 +198,69 @@ function verifySlackRequest(signingSecret: string): RequestHandler { }; } -const parseBody: RequestHandler = (req, _res, next) => { - if (req.headers['content-type'] === 'application/x-www-form-urlencoded') { - const parsedBody = querystring.parse(req.body); - req.body = (typeof parsedBody.payload === 'string') ? JSON.parse(parsedBody.payload) : parsedBody; +// TODO: this should be imported from another package +async function verifyRequestSignature( + signingSecret: string, + body: string, + signature: string, + requestTimestamp: number): Promise { + if (!signature || !requestTimestamp) { + const error = errorWithCode( + 'Slack request signing verification failed. Some headers are missing.', + ErrorCode.ExpressReceiverAuthenticityError, + ); + throw error; + } + + // Divide current date to match Slack ts format + // Subtract 5 minutes from current time + const fiveMinutesAgo = Math.floor(Date.now() / 1000) - (60 * 5); + + if (requestTimestamp < fiveMinutesAgo) { + const error = errorWithCode( + 'Slack request signing verification failed. Timestamp is too old.', + ErrorCode.ExpressReceiverAuthenticityError, + ); + throw error; + } + + const hmac = crypto.createHmac('sha256', signingSecret); + const [version, hash] = signature.split('='); + hmac.update(`${version}:${requestTimestamp}:${body}`); + + if (!tsscmp(hash, hmac.digest('hex'))) { + const error = errorWithCode( + 'Slack request signing verification failed. Signature mismatch.', + ErrorCode.ExpressReceiverAuthenticityError, + ); + throw error; + } +} + +function parseRequestBody( + logger: Logger, + stringBody: string, + contentType: string | undefined) { + if (contentType === 'application/x-www-form-urlencoded') { + const parsedBody = querystring.parse(stringBody); + if (typeof parsedBody.payload === 'string') { + return JSON.parse(parsedBody.payload); + } else { + return parsedBody; + } + } else if (contentType === 'application/json') { + return JSON.parse(stringBody); } else { - // TODO: should we check the content type header to make sure its JSON here? - req.body = JSON.parse(req.body); + 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; + } } - next(); -}; +} function receiverAckTimeoutError(message: string): ReceiverAckTimeoutError { const error = new Error(message); From 779951028eb2ba791a3940000343f233cb4aedee Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Fri, 24 May 2019 15:42:24 -0700 Subject: [PATCH 2/2] Pass logger from App to ExpressReceiver --- src/App.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/App.ts b/src/App.ts index 0cd1822e6..4178ed037 100644 --- a/src/App.ts +++ b/src/App.ts @@ -158,7 +158,7 @@ export default class App { // Check for required arguments of ExpressReceiver if (signingSecret !== undefined) { - this.receiver = new ExpressReceiver({ signingSecret, endpoints }); + this.receiver = new ExpressReceiver({ signingSecret, logger, endpoints }); } else if (receiver === undefined) { // Check for custom receiver throw errorWithCode(