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

Fix #648 Option to disable signature verification #1088

Merged
merged 3 commits into from
Aug 28, 2021
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
7 changes: 5 additions & 2 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export interface AppOptions {
signingSecret?: HTTPReceiverOptions['signingSecret'];
endpoints?: HTTPReceiverOptions['endpoints'];
processBeforeResponse?: HTTPReceiverOptions['processBeforeResponse'];
signatureVerification?: HTTPReceiverOptions['signatureVerification'];
clientId?: HTTPReceiverOptions['clientId'];
clientSecret?: HTTPReceiverOptions['clientSecret'];
stateSecret?: HTTPReceiverOptions['stateSecret']; // required when using default stateStore
Expand Down Expand Up @@ -221,6 +222,7 @@ export default class App {
ignoreSelf = true,
clientOptions = undefined,
processBeforeResponse = false,
signatureVerification = true,
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
Expand Down Expand Up @@ -337,7 +339,7 @@ export default class App {
logLevel: this.logLevel,
installerOptions: this.installerOptions,
});
} else if (signingSecret === undefined) {
} else if (signatureVerification && signingSecret === undefined) {
// No custom receiver
throw new AppInitializationError(
'Signing secret not found, so could not initialize the default receiver. Set a signing secret or use a ' +
Expand All @@ -347,9 +349,10 @@ export default class App {
this.logger.debug('Initializing HTTPReceiver');
// Create default HTTPReceiver
this.receiver = new HTTPReceiver({
signingSecret,
signingSecret: signingSecret || '',
endpoints,
processBeforeResponse,
signatureVerification,
clientId,
clientSecret,
stateSecret,
Expand Down
45 changes: 42 additions & 3 deletions src/receivers/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface ExpressReceiverOptions {
| {
[endpointType: string]: string;
};
signatureVerification?: boolean;
processBeforeResponse?: boolean;
clientId?: string;
clientSecret?: string;
Expand Down Expand Up @@ -123,6 +124,8 @@ export default class ExpressReceiver implements Receiver {

private processBeforeResponse: boolean;

private signatureVerification: boolean;

public router: IRouter;

public installer: InstallProvider | undefined = undefined;
Expand All @@ -133,6 +136,7 @@ export default class ExpressReceiver implements Receiver {
logLevel = LogLevel.INFO,
endpoints = { events: '/slack/events' },
processBeforeResponse = false,
signatureVerification = true,
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
Expand All @@ -151,8 +155,12 @@ export default class ExpressReceiver implements Receiver {
this.logger.setLevel(logLevel);
}

this.signatureVerification = signatureVerification;
const bodyParser = this.signatureVerification ?
buildVerificationBodyParserMiddleware(this.logger, signingSecret) :
buildBodyParserMiddleware(this.logger);
const expressMiddleware: RequestHandler[] = [
verifySignatureAndParseRawBody(this.logger, signingSecret),
bodyParser,
respondToSslCheck,
respondToUrlVerification,
this.requestHandler.bind(this),
Expand Down Expand Up @@ -375,12 +383,19 @@ export default class ExpressReceiver implements Receiver {
}
}

export function verifySignatureAndParseRawBody(
Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted to rename the builder method to buildVerificationBodyParserMiddleware internally but we still need to keep exporting this one for backward compatibility

logger: Logger,
signingSecret: string | (() => PromiseLike<string>),
): RequestHandler {
return buildVerificationBodyParserMiddleware(logger, signingSecret);
}

/**
* This request handler has two responsibilities:
* - Verify the request signature
* - Parse request.body and assign the successfully parsed object to it.
*/
export function verifySignatureAndParseRawBody(
function buildVerificationBodyParserMiddleware(
logger: Logger,
signingSecret: string | (() => PromiseLike<string>),
): RequestHandler {
Expand Down Expand Up @@ -468,7 +483,7 @@ function verifyRequestSignature(
* - Verify the request signature
* - Parse request.body and assign the successfully parsed object to it.
*/
function verifySignatureAndParseBody(
export function verifySignatureAndParseBody(
signingSecret: string,
body: string,
headers: Record<string, any>,
Expand All @@ -485,6 +500,30 @@ function verifySignatureAndParseBody(
return parseRequestBody(body, contentType);
}

function buildBodyParserMiddleware(logger: Logger): RequestHandler {
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();
}
try {
const { 'content-type': contentType } = req.headers;
req.body = parseRequestBody(stringBody, contentType);
} catch (error) {
if (error) {
logError(logger, 'Parsing request body failed', error);
return res.status(400).send();
}
}
return next();
};
}

function parseRequestBody(stringBody: string, contentType: string | undefined): any {
if (contentType === 'application/x-www-form-urlencoded') {
const parsedBody = querystring.parse(stringBody);
Expand Down
14 changes: 13 additions & 1 deletion src/receivers/HTTPReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export interface HTTPReceiverOptions {
logger?: Logger;
logLevel?: LogLevel;
processBeforeResponse?: boolean;
signatureVerification?: boolean;
clientId?: string;
clientSecret?: string;
stateSecret?: InstallProviderOptions['stateSecret']; // required when using default stateStore
Expand Down Expand Up @@ -92,6 +93,8 @@ export default class HTTPReceiver implements Receiver {

private processBeforeResponse: boolean;

private signatureVerification: boolean;

private app?: App;

public requestListener: RequestListener;
Expand Down Expand Up @@ -120,6 +123,7 @@ export default class HTTPReceiver implements Receiver {
logger = undefined,
logLevel = LogLevel.INFO,
processBeforeResponse = false,
signatureVerification = true,
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
Expand All @@ -130,6 +134,7 @@ export default class HTTPReceiver implements Receiver {
// Initialize instance variables, substituting defaults for each value
this.signingSecret = signingSecret;
this.processBeforeResponse = processBeforeResponse;
this.signatureVerification = signatureVerification;
this.logger = logger ??
(() => {
const defaultLogger = new ConsoleLogger();
Expand Down Expand Up @@ -317,7 +322,14 @@ export default class HTTPReceiver implements Receiver {

// Verify authenticity
try {
bufferedReq = await verifySlackAuthenticity({ signingSecret: this.signingSecret }, req);
bufferedReq = await verifySlackAuthenticity(
{
// If enabled: false, this method returns bufferredReq without verification
enabled: this.signatureVerification,
signingSecret: this.signingSecret,
},
req,
);
} catch (err) {
const e = err as any;
this.logger.warn(`Request verification failed: ${e.message}`);
Expand Down
6 changes: 6 additions & 0 deletions src/receivers/verify-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { IncomingMessage, ServerResponse } from 'http';
const verifyErrorPrefix = 'Failed to verify authenticity';

export interface VerifyOptions {
enabled?: boolean;
signingSecret: string;
nowMs?: () => number;
logger?: Logger;
Expand Down Expand Up @@ -54,6 +55,11 @@ export async function verify(
// Consume the readable stream (or use the previously consumed readable stream)
const bufferedReq = await bufferIncomingMessage(req);

if (options.enabled !== undefined && !options.enabled) {
// As the validation is disabled, immediately return the bufferred reuest
seratch marked this conversation as resolved.
Show resolved Hide resolved
return bufferedReq;
}

// Find the relevant request headers
const signature = getHeader(req, 'x-slack-signature');
const requestTimestampSec = Number(getHeader(req, 'x-slack-request-timestamp'));
Expand Down