Fix #192 ExpressReceiver support for rawBody for signature verification

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);