From 35210b511a294a52f45382486b5445d7483fe2e9 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Wed, 25 Dec 2019 15:25:32 +0900 Subject: [PATCH] Fix #324 Responding with 401 status, not 500 for signature verification failures --- src/ExpressReceiver.spec.ts | 158 ++++++++++++++++++++++++++---------- src/ExpressReceiver.ts | 52 +++++++----- 2 files changed, 148 insertions(+), 62 deletions(-) diff --git a/src/ExpressReceiver.spec.ts b/src/ExpressReceiver.spec.ts index 3b3a58621..f5b009897 100644 --- a/src/ExpressReceiver.spec.ts +++ b/src/ExpressReceiver.spec.ts @@ -26,6 +26,17 @@ describe('ExpressReceiver', () => { setName(_name: string): void { /* noop */ }, }; + function buildResponseToVerify(result: any): Response { + return { + status: (code: number) => { + result.code = code; + return { + send: () => { result.sent = true; }, + } as any as Response; + }, + } as any as Response; + } + describe('constructor', () => { it('should accept supported arguments', async () => { const receiver = new ExpressReceiver({ @@ -106,7 +117,7 @@ describe('ExpressReceiver', () => { respondToUrlVerification(req, resp, next); // Assert - assert.equal(JSON.stringify({ challenge: 'this is it' }), JSON.stringify(sentBody)); + assert.equal(JSON.stringify(sentBody), JSON.stringify({ challenge: 'this is it' })); assert.isUndefined(errorResult); }); @@ -181,8 +192,7 @@ describe('ExpressReceiver', () => { async function runWithValidRequest(req: Request, state: any): Promise { // Arrange - // tslint:disable-next-line: no-object-literal-type-assertion - const resp = {} as Response; + const resp = buildResponseToVerify(state); const next = (error: any) => { state.error = error; }; // Act @@ -213,7 +223,8 @@ describe('ExpressReceiver', () => { req.headers['content-type'] = undefined; await runWithValidRequest(req, state); // Assert - assert.equal(state.error, 'SyntaxError: Unexpected token o in JSON at position 1'); + assert.equal(state.code, 400); + assert.equal(state.sent, true); }); it('should verify requests on GCP and then catch parse failures', async () => { @@ -222,25 +233,80 @@ describe('ExpressReceiver', () => { req.headers['content-type'] = undefined; await runWithValidRequest(req, state); // Assert - assert.equal(state.error, 'SyntaxError: Unexpected token o in JSON at position 1'); + assert.equal(state.code, 400); + assert.equal(state.sent, true); + }); + + // ---------------------------- + // verifyContentTypeAbsence + + async function verifyRequestsWithoutContentTypeHeader(req: Request): Promise { + // Arrange + 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); + 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 () => { + 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, + // 'content-type': 'application/x-www-form-urlencoded', + }; + const req = reqAsStream as Request; + await verifyRequestsWithoutContentTypeHeader(req); + }); + + it('should verify parse request body without content-type header on GCP', async () => { + const untypedReq: { [key: string]: any } = { + rawBody: body, + headers: { + 'x-slack-signature': signature, + 'x-slack-request-timestamp': requestTimestamp, + // 'content-type': 'application/x-www-form-urlencoded', + }, + }; + const req = untypedReq as Request; + await verifyRequestsWithoutContentTypeHeader(req); }); // ---------------------------- // verifyMissingHeaderDetection - function verifyMissingHeaderDetection(req: Request): Promise { + async function verifyMissingHeaderDetection(req: Request): Promise { // Arrange - // tslint:disable-next-line: no-object-literal-type-assertion - const resp = {} as Response; - let errorResult: any; - const next = (error: any) => { errorResult = error; }; + const result: any = {}; + const resp = buildResponseToVerify(result); + const next = sinon.fake(); // 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.'); - }); + await verifier(req, resp, next); + + // Assert + assert.equal(result.code, 401); + assert.equal(result.sent, true); } it('should detect headers missing signature', async () => { @@ -250,6 +316,7 @@ describe('ExpressReceiver', () => { (reqAsStream as { [key: string]: any }).headers = { // 'x-slack-signature': signature , 'x-slack-request-timestamp': requestTimestamp, + 'content-type': 'application/x-www-form-urlencoded', }; await verifyMissingHeaderDetection(reqAsStream as Request); }); @@ -260,7 +327,8 @@ describe('ExpressReceiver', () => { reqAsStream.push(null); // indicate EOF (reqAsStream as { [key: string]: any }).headers = { 'x-slack-signature': signature, - /*'x-slack-request-timestamp': requestTimestamp*/ + /*'x-slack-request-timestamp': requestTimestamp, */ + 'content-type': 'application/x-www-form-urlencoded', }; await verifyMissingHeaderDetection(reqAsStream as Request); }); @@ -270,7 +338,8 @@ describe('ExpressReceiver', () => { rawBody: body, headers: { 'x-slack-signature': signature, - /*'x-slack-request-timestamp': requestTimestamp */ + /*'x-slack-request-timestamp': requestTimestamp, */ + 'content-type': 'application/x-www-form-urlencoded', }, }; await verifyMissingHeaderDetection(untypedReq as Request); @@ -279,19 +348,19 @@ describe('ExpressReceiver', () => { // ---------------------------- // verifyInvalidTimestampError - function verifyInvalidTimestampError(req: Request): Promise { + async function verifyInvalidTimestampError(req: Request): Promise { // Arrange - // tslint:disable-next-line: no-object-literal-type-assertion - const resp = {} as Response; - let errorResult: any; - const next = (error: any) => { errorResult = error; }; + const result: any = {}; + const resp = buildResponseToVerify(result); + const next = sinon.fake(); // 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 invalid.'); - }); + await verifier(req, resp, next); + + // Assert + assert.equal(result.code, 401); + assert.equal(result.sent, true); } it('should detect invalid timestamp header', async () => { @@ -301,6 +370,7 @@ describe('ExpressReceiver', () => { (reqAsStream as { [key: string]: any }).headers = { 'x-slack-signature': signature, 'x-slack-request-timestamp': 'Hello there!', + 'content-type': 'application/x-www-form-urlencoded', }; await verifyInvalidTimestampError(reqAsStream as Request); }); @@ -308,22 +378,22 @@ describe('ExpressReceiver', () => { // ---------------------------- // verifyTooOldTimestampError - function verifyTooOldTimestampError(req: Request): Promise { + async function verifyTooOldTimestampError(req: Request): Promise { // Arrange // restore the valid clock clock.restore(); - // tslint:disable-next-line: no-object-literal-type-assertion - const resp = {} as Response; - let errorResult: any; - const next = (error: any) => { errorResult = error; }; + const result: any = {}; + const resp = buildResponseToVerify(result); + const next = sinon.fake(); // 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.'); - }); + await verifier(req, resp, next); + + // Assert + assert.equal(result.code, 401); + assert.equal(result.sent, true); } it('should detect too old timestamp', async () => { @@ -337,20 +407,20 @@ describe('ExpressReceiver', () => { // ---------------------------- // verifySignatureMismatch - function verifySignatureMismatch(req: Request): Promise { + async function verifySignatureMismatch(req: Request): Promise { // Arrange - // tslint:disable-next-line: no-object-literal-type-assertion - const resp = {} as Response; - let errorResult: any; - const next = (error: any) => { errorResult = error; }; + const result: any = {}; + const resp = buildResponseToVerify(result); + const next = sinon.fake(); // 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.'); - }); + await verifier(req, resp, next); + + // Assert + assert.equal(result.code, 401); + assert.equal(result.sent, true); } it('should detect signature mismatch', async () => { @@ -360,6 +430,7 @@ describe('ExpressReceiver', () => { (reqAsStream as { [key: string]: any }).headers = { 'x-slack-signature': signature, 'x-slack-request-timestamp': requestTimestamp + 10, + 'content-type': 'application/x-www-form-urlencoded', }; const req = reqAsStream as Request; await verifySignatureMismatch(req); @@ -371,6 +442,7 @@ describe('ExpressReceiver', () => { headers: { 'x-slack-signature': signature, 'x-slack-request-timestamp': requestTimestamp + 10, + 'content-type': 'application/x-www-form-urlencoded', }, }; const req = untypedReq as Request; diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 7a810ef3c..50130651c 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -176,47 +176,61 @@ export function verifySignatureAndParseBody( logger: Logger, signingSecret: string, ): RequestHandler { - return async (req, _res, next) => { - try { - // *** 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(); - } + return async (req, res, next) => { + + 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(); + } + // *** Request verification *** + try { const { 'x-slack-signature': signature, 'x-slack-request-timestamp': requestTimestamp, - 'content-type': contentType, } = 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. + // *** 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. + 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(); } catch (error) { - return next(error); + // Deny a bad request + logError(logger, 'Parsing request body failed', error); + return res.status(400).send(); } }; } +function logError(logger: Logger, message: string, error: any): void { + const logMessage = ('code' in error) + ? `${message} (code: ${error.code}, message: ${error.message})` + : `${message} (error: ${error})`; + logger.warn(logMessage); +} + // TODO: this should be imported from another package async function verifyRequestSignature( signingSecret: string,