Skip to content

Commit

Permalink
Fix slackapi#324 Responding with 401 status, not 500 for signature ve…
Browse files Browse the repository at this point in the history 
…rification failures
  • Loading branch information
@seratch
seratch committed Jan 30, 2020
1 parent df4226b commit be102b2
Show file tree
Hide file tree
Showing 2 changed files with 148 additions and 62 deletions.
158 changes: 115 additions & 43 deletions src/ExpressReceiver.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,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 @@ -108,7 +119,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 @@ -183,8 +194,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 @@ -215,7 +225,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 @@ -224,25 +235,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 @@ -252,6 +318,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 @@ -262,7 +329,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 @@ -272,7 +340,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 @@ -281,19 +350,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 @@ -303,29 +372,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 @@ -339,20 +409,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 @@ -362,6 +432,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 @@ -373,6 +444,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
Original file line number Diff line number Diff line change
Expand Up @@ -180,47 +180,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,
Expand Down

0 comments on commit be102b2

Please sign in to comment.