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 Dec 25, 2019
1 parent 449b880 commit e9bd645
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 48 deletions.
107 changes: 61 additions & 46 deletions src/ExpressReceiver.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ describe('ExpressReceiver', () => {
setName(_name: string): void { },
};

function buildResponseToVerify(result: any): Response {
return {
status: (code: number) => {
result.code = code;
return {
send: () => { result.sent = true; }
} as Response;
},
} as Response;
}

describe('verifySignatureAndParseBody', () => {

let clock: SinonFakeTimers;
Expand All @@ -45,7 +56,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',
};
const req = reqAsStream as Request;
return req;
Expand All @@ -56,7 +68,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',
}
};
const req = untypedReq as Request;
Expand All @@ -66,46 +79,45 @@ describe('ExpressReceiver', () => {
// ----------------------------
// runWithValidRequest

async function runWithValidRequest(req: Request, errorResult: any) {
async function runWithValidRequest(req: Request): Promise<void> {
// Arrange
const resp = {} as Response;
const next = (error: any) => { errorResult = error; };
const result: any = {};
const resp = buildResponseToVerify(result);
const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);
await verifier(req, resp, next);
return errorResult;

// Assert
assert.isUndefined(result.code);
assert.isUndefined(result.sent);
}

it('should verify requests', async () => {
let errorResult: any;
runWithValidRequest(buildExpressRequest(), errorResult);
// Assert
assert.isUndefined(errorResult);
await runWithValidRequest(buildExpressRequest());
});

it('should verify requests on GCP', async () => {
let errorResult: any;
runWithValidRequest(buildGCPRequest(), errorResult);
// Assert
assert.isUndefined(errorResult);
await runWithValidRequest(buildGCPRequest());
});

// ----------------------------
// verifyMissingHeaderDetection

function verifyMissingHeaderDetection(req: Request): Promise<any> {
async function verifyMissingHeaderDetection(req: Request): Promise<void> {
// Arrange
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(401, result.code);
assert.equal(true, result.sent);
}

it('should detect headers missing signature', async () => {
Expand Down Expand Up @@ -144,18 +156,19 @@ describe('ExpressReceiver', () => {
// ----------------------------
// verifyInvalidTimestampError

function verifyInvalidTimestampError(req: Request): Promise<any> {
async function verifyInvalidTimestampError(req: Request): Promise<void> {
// Arrange
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(401, result.code);
assert.equal(true, result.sent);
}

it('should detect invalid timestamp header', async () => {
Expand All @@ -172,21 +185,22 @@ describe('ExpressReceiver', () => {
// ----------------------------
// verifyTooOldTimestampError

function verifyTooOldTimestampError(req: Request): Promise<any> {
async function verifyTooOldTimestampError(req: Request): Promise<void> {
// Arrange
// restore the valid clock
clock.restore();

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(401, result.code);
assert.equal(true, result.sent);
}

it('should detect too old timestamp', async () => {
Expand All @@ -200,19 +214,20 @@ describe('ExpressReceiver', () => {
// ----------------------------
// verifySignatureMismatch

function verifySignatureMismatch(req: Request): Promise<any> {
async function verifySignatureMismatch(req: Request): Promise<void> {
// Arrange
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(401, result.code);
assert.equal(true, result.sent);
}

it('should detect signature mismatch', async () => {
Expand Down
11 changes: 9 additions & 2 deletions src/ExpressReceiver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export function verifySignatureAndParseBody(
logger: Logger,
signingSecret: string,
): RequestHandler {
return async (req, _res, next) => {
return async (req, res, next) => {
try {
// *** Request verification ***
let stringBody: string;
Expand Down Expand Up @@ -211,8 +211,15 @@ export function verifySignatureAndParseBody(
req.body = parseRequestBody(logger, stringBody, contentType);

return next();

} catch (error) {
return next(error);
// Deny the request as something wrong with the signature
if ('code' in error) {
logger.warn(`Request verification failed (code: ${error.code}, message: ${error.message})`);
} else {
logger.warn(`Request verification failed (error: ${error})`);
}
return res.status(401).send();
}
};
}
Expand Down

0 comments on commit e9bd645

Please sign in to comment.