Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #324 Responding with 401 status, not 500 for signature verification failures #362

Merged
merged 3 commits into from
Jan 22, 2020
Merged

Fix #324 Responding with 401 status, not 500 for signature verification failures #362

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35210b5
Fix #324 Responding with 401 status, not 500 for signature verificati…
seratch Dec 25, 2019
f15c2ed
Merge branch 'master' into issue-324
seratch Jan 9, 2020
ca651ce
Merge branch 'master' into issue-324
seratch Jan 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 115 additions & 43 deletions src/ExpressReceiver.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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);
});

Expand Down Expand Up @@ -181,8 +192,7 @@ describe('ExpressReceiver', () => {

async function runWithValidRequest(req: Request, state: any): Promise<void> {
// 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
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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<void> {
// 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<any> {
async function verifyMissingHeaderDetection(req: Request): Promise<void> {
// 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 () => {
Expand All @@ -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);
});
Expand All @@ -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);
});
Expand All @@ -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);
Expand All @@ -279,19 +348,19 @@ describe('ExpressReceiver', () => {
// ----------------------------
// verifyInvalidTimestampError

function verifyInvalidTimestampError(req: Request): Promise<any> {
async function verifyInvalidTimestampError(req: Request): Promise<void> {
// 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 () => {
Expand All @@ -301,29 +370,30 @@ 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);
});

// ----------------------------
// verifyTooOldTimestampError

function verifyTooOldTimestampError(req: Request): Promise<any> {
async function verifyTooOldTimestampError(req: Request): Promise<void> {
// 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 () => {
Expand All @@ -337,20 +407,20 @@ describe('ExpressReceiver', () => {
// ----------------------------
// verifySignatureMismatch

function verifySignatureMismatch(req: Request): Promise<any> {
async function verifySignatureMismatch(req: Request): Promise<void> {
// 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 () => {
Expand All @@ -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);
Expand All @@ -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;
Expand Down
52 changes: 33 additions & 19 deletions src/ExpressReceiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, all the possible error values have code but this is testing the existence just in case.

? `${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,
Expand Down