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.

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

🧹 mostly low-hanging ts lint fixes, ts lint as part of test #339

Merged
merged 8 commits into from
Dec 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ called with arguments that make it easy to build a rich app.
types of data are only available outside the event payload itself, such as `api_app_id`, `authed_users`, etc. This
argument should rarely be needed, but for completeness it is provided here.

The arguments are grouped into properties of one object, so that its easier to pick just the ones your listener needs
The arguments are grouped into properties of one object, so that it's easier to pick just the ones your listener needs
(using
[object destructuring](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Unpacking_fields_from_objects_passed_as_function_parameter)).
Here is an example where the app sends a simple response, so there's no need for most of these arguments:
Expand Down Expand Up @@ -173,7 +173,7 @@ app.action({ callbackId: 'my_dialog_callback' }, async ({ action, ack }) => {

## Handling errors

If an error occurs in a listener function, its strongly recommended to handle it directly. There are a few cases where
If an error occurs in a listener function, it's strongly recommended to handle it directly. There are a few cases where
those errors may occur after your listener function has returned (such as when calling `say()` or `respond()`, or
forgetting to call `ack()`). In these cases, your app will be notified about the error in an error handler function.
Your app should register an error handler using the `App#error(fn)` method.
Expand Down Expand Up @@ -239,7 +239,7 @@ app.message('whoami', ({ say, context }) => { say(`User Details: ${JSON.stringif
})();

// Authentication middleware - Calls Acme identity provider to associate the incoming event with the user who sent it
// Its a function just like listeners, but it also uses the next argument
// It's a function just like listeners, but it also uses the next argument
function authWithAcme({ payload, context, say, next }) {
const slackUserId = payload.user;

Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"build": "tsc",
"build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
"lint": "tslint --project .",
"test": "nyc mocha --config .mocharc.json \"src/**/*.spec.ts\"",
"mocha": "nyc mocha --config .mocharc.json \"src/**/*.spec.ts\"",
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
"test": "npm run lint && npm run mocha",
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
"coverage": "codecov"
},
"repository": "slackapi/bolt",
Expand Down
47 changes: 23 additions & 24 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export interface AuthorizeResult {
}

export interface ActionConstraints {
type?: string,
type?: string;
block_id?: string | RegExp;
action_id?: string | RegExp;
callback_id?: string | RegExp;
Expand Down Expand Up @@ -407,29 +407,28 @@ export default class App {
// Set body and payload (this value will eventually conform to AnyMiddlewareArgs)
// NOTE: the following doesn't work because... distributive?
// const listenerArgs: Partial<AnyMiddlewareArgs> = {
const listenerArgs:
Pick<AnyMiddlewareArgs, 'body' | 'payload'> & {
/** Say function might be set below */
say?: SayFn
/** Respond function might be set below */
respond?: RespondFn,
/** Ack function might be set below */
ack?: AckFn<any>,
} = {
body: bodyArg,
payload:
(type === IncomingEventType.Event) ?
(bodyArg as SlackEventMiddlewareArgs['body']).event :
(type === IncomingEventType.ViewAction) ?
(bodyArg as SlackViewMiddlewareArgs['body']).view :
(type === IncomingEventType.Action &&
isBlockActionOrInteractiveMessageBody(bodyArg as SlackActionMiddlewareArgs['body'])) ?
(bodyArg as SlackActionMiddlewareArgs<BlockAction | InteractiveMessage>['body']).actions[0] :
(bodyArg as (
Exclude<AnyMiddlewareArgs, SlackEventMiddlewareArgs | SlackActionMiddlewareArgs | SlackViewMiddlewareArgs> |
SlackActionMiddlewareArgs<Exclude<SlackAction, BlockAction | InteractiveMessage>>
)['body']),
};
const listenerArgs: Pick<AnyMiddlewareArgs, 'body' | 'payload'> & {
/** Say function might be set below */
say?: SayFn
/** Respond function might be set below */
respond?: RespondFn,
/** Ack function might be set below */
ack?: AckFn<any>,
} = {
body: bodyArg,
payload:
(type === IncomingEventType.Event) ?
(bodyArg as SlackEventMiddlewareArgs['body']).event :
(type === IncomingEventType.ViewAction) ?
(bodyArg as SlackViewMiddlewareArgs['body']).view :
(type === IncomingEventType.Action &&
isBlockActionOrInteractiveMessageBody(bodyArg as SlackActionMiddlewareArgs['body'])) ?
(bodyArg as SlackActionMiddlewareArgs<BlockAction | InteractiveMessage>['body']).actions[0] :
(bodyArg as (
Exclude<AnyMiddlewareArgs, SlackEventMiddlewareArgs | SlackActionMiddlewareArgs | SlackViewMiddlewareArgs> |
SlackActionMiddlewareArgs<Exclude<SlackAction, BlockAction | InteractiveMessage>>
)['body']),
};
aoberoi marked this conversation as resolved.
Show resolved Hide resolved

// Set aliases
if (type === IncomingEventType.Event) {
Expand Down
57 changes: 48 additions & 9 deletions src/ExpressReceiver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,24 @@ describe('ExpressReceiver', () => {
})
}

it('should detect headers missing', async () => {
it('should detect headers missing signature', 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 */
// 'x-slack-signature': signature ,
'x-slack-request-timestamp': requestTimestamp
};
await verifyMissingHeaderDetection(reqAsStream as Request);
});
aoberoi marked this conversation as resolved.
Show resolved Hide resolved

it('should detect headers missing timestamp', 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);
});
Expand All @@ -130,6 +141,34 @@ describe('ExpressReceiver', () => {
await verifyMissingHeaderDetection(untypedReq as Request);
});

// ----------------------------
// verifyInvalidTimestampError

function verifyInvalidTimestampError(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. Timestamp is invalid.');
});
}

it('should detect invalid timestamp 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': 'Hello there!',
};
await verifyInvalidTimestampError(reqAsStream as Request);
});
aoberoi marked this conversation as resolved.
Show resolved Hide resolved

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

Expand All @@ -150,18 +189,18 @@ describe('ExpressReceiver', () => {
});
}

it('should detect too old tiestamp', async () => {
it('should detect too old timestamp', async () => {
await verifyTooOldTimestampError(buildExpressRequest());
});

it('should detect too old tiestamp on GCP', async () => {
it('should detect too old timestamp on GCP', async () => {
await verifyTooOldTimestampError(buildGCPRequest());
});

// ----------------------------
// verifySingnatureMismatch
// verifySignatureMismatch

function verifySingnatureMismatch(req: Request): Promise<any> {
function verifySignatureMismatch(req: Request): Promise<any> {
// Arrange
const resp = {} as Response;
let errorResult: any;
Expand All @@ -185,7 +224,7 @@ describe('ExpressReceiver', () => {
'x-slack-request-timestamp': requestTimestamp + 10
};
const req = reqAsStream as Request;
await verifySingnatureMismatch(req);
await verifySignatureMismatch(req);
});

it('should detect signature mismatch on GCP', async () => {
Expand All @@ -197,7 +236,7 @@ describe('ExpressReceiver', () => {
}
};
const req = untypedReq as Request;
await verifySingnatureMismatch(req);
await verifySignatureMismatch(req);
});

});
Expand Down
84 changes: 48 additions & 36 deletions src/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
return new Promise((resolve, reject) => {
// TODO: what about synchronous errors?
this.server.close((error) => {
if (error) {
if (error !== undefined) {
reject(error);
return;
}
Expand Down Expand Up @@ -174,7 +174,8 @@ const respondToUrlVerification: RequestHandler = (req, res, next) => {
*/
export function verifySignatureAndParseBody(
logger: Logger,
signingSecret: string): RequestHandler {
signingSecret: string,
): RequestHandler {
return async (req, _res, next) => {
try {
// *** Request verification ***
Expand All @@ -187,27 +188,31 @@ export function verifySignatureAndParseBody(
} else {
stringBody = (await rawBody(req)).toString();
}
const signature = req.headers['x-slack-signature'] as string;
const ts = Number(req.headers['x-slack-request-timestamp']);

try {
await verifyRequestSignature(signingSecret, stringBody, signature, ts);
} catch (e) {
return next(e);
}
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,
);
aoberoi marked this conversation as resolved.
Show resolved Hide resolved

// *** 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.

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

next();
return next();
} catch (error) {
next(error);
return next(error);
}
};
}
Expand All @@ -216,63 +221,70 @@ export function verifySignatureAndParseBody(
async function verifyRequestSignature(
signingSecret: string,
body: string,
signature: string,
requestTimestamp: number): Promise<void> {
if (!signature || !requestTimestamp) {
const error = errorWithCode(
signature: string | undefined,
requestTimestamp: string | undefined,
): Promise<void> {
if (signature === undefined || requestTimestamp === undefined) {
throw errorWithCode(
'Slack request signing verification failed. Some headers are missing.',
ErrorCode.ExpressReceiverAuthenticityError,
);
throw error;
}

const ts = Number(requestTimestamp);
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
if (isNaN(ts)) {
throw errorWithCode(
'Slack request signing verification failed. Timestamp is invalid.',
ErrorCode.ExpressReceiverAuthenticityError,
);
}

// 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(
if (ts < fiveMinutesAgo) {
throw 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}`);
hmac.update(`${version}:${ts}:${body}`);

if (!tsscmp(hash, hmac.digest('hex'))) {
const error = errorWithCode(
throw errorWithCode(
'Slack request signing verification failed. Signature mismatch.',
ErrorCode.ExpressReceiverAuthenticityError,
);
throw error;
}
}

function parseRequestBody(
logger: Logger,
stringBody: string,
contentType: string | undefined) {
contentType: string | undefined): any {
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 parsedBody;
}

if (contentType === 'application/json') {
return JSON.parse(stringBody);
} else {
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;
}
}

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;
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/conversation-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ export function conversationContext<ConversationState = any>(
.catch((error) => {
logger.debug(`Conversation context not loaded: ${error.message}`);
})
.then(next);
.then(next)
.catch(error => logger.debug(error.message));
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
} else {
logger.debug('No conversation ID for incoming event');
next();
Expand Down
4 changes: 2 additions & 2 deletions src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function getTypeAndConversation(body: any): { type?: IncomingEventType, c
type: IncomingEventType.Event,
conversationId:
eventBody.event.channel !== undefined ? eventBody.event.channel :
eventBody.event.item !== undefined ? eventBody.event.item.channel : undefined,
eventBody.event.item !== undefined ? eventBody.event.item.channel : undefined,
};
}
if (body.command !== undefined) {
Expand All @@ -44,7 +44,7 @@ export function getTypeAndConversation(body: any): { type?: IncomingEventType, c
const optionsBody = (body as SlackOptionsMiddlewareArgs<OptionsSource>['body']);
return {
type: IncomingEventType.Options,
conversationId: optionsBody.channel ? optionsBody.channel.id : undefined,
conversationId: optionsBody.channel !== undefined ? optionsBody.channel.id : undefined,
};
}
if (body.actions !== undefined || body.type === 'dialog_submission' || body.type === 'message_action') {
Expand Down