Fix slackapi#192 ExpressReceiver support for rawBody for signature ve…

…rification

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<any> {
+ // 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<any> {
+ // 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<any> {
+ // 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);
+ });
+
+ });
+
+});

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,57 +153,114 @@ 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);
  }
  };
 }
 
-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<void> {
+ 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);