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

Add option to disable state verification, fix redirect uri bug #1116

Merged
merged 19 commits into from
Sep 27, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
},
"dependencies": {
"@slack/logger": "^3.0.0",
"@slack/oauth": "^2.2.0",
"@slack/oauth": "^2.3.0",
"@slack/socket-mode": "^1.1.0",
"@slack/types": "^2.2.0",
"@slack/web-api": "^6.4.0",
Expand Down
28 changes: 28 additions & 0 deletions src/App.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,34 @@ describe('App', () => {
assert(fakeConversationContext.firstCall.calledWith(dummyConvoStore));
});
});
describe('with custom redirectUri supplied', () => {
it('should fail when missing installerOptions', async () => {
// Arrange
const MockApp = await importApp();

// Act
try {
new MockApp({ token: '', signingSecret: '', redirectUri: 'http://example.com/redirect' }); // eslint-disable-line no-new
assert.fail();
} catch (error: any) {
// Assert
assert.propertyVal(error, 'code', ErrorCode.AppInitializationError);
}
});
it('should fail when missing installerOptions.redirectUriPath', async () => {
// Arrange
const MockApp = await importApp();

// Act
try {
new MockApp({ token: '', signingSecret: '', redirectUri: 'http://example.com/redirect', installerOptions: {} }); // eslint-disable-line no-new
assert.fail();
} catch (error: any) {
// Assert
assert.propertyVal(error, 'code', ErrorCode.AppInitializationError);
}
});
});
it('with clientOptions', async () => {
const fakeConstructor = sinon.fake();
const overrides = mergeOverrides(withNoopAppMetadata(), {
Expand Down
15 changes: 11 additions & 4 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface AppOptions {
clientId?: HTTPReceiverOptions['clientId'];
clientSecret?: HTTPReceiverOptions['clientSecret'];
stateSecret?: HTTPReceiverOptions['stateSecret']; // required when using default stateStore
redirectUri?: HTTPReceiverOptions['redirectUri']
installationStore?: HTTPReceiverOptions['installationStore']; // default MemoryInstallationStore
scopes?: HTTPReceiverOptions['scopes'];
installerOptions?: HTTPReceiverOptions['installerOptions'];
Expand Down Expand Up @@ -231,6 +232,7 @@ export default class App {
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
redirectUri = undefined,
installationStore = undefined,
scopes = undefined,
installerOptions = undefined,
Expand All @@ -252,6 +254,7 @@ export default class App {
this.logLevel = logLevel ?? LogLevel.INFO;
}

// Set up logger
if (typeof logger === 'undefined') {
// Initialize with the default logger
const consoleLogger = new ConsoleLogger();
Expand All @@ -265,6 +268,7 @@ export default class App {
}
this.errorHandler = defaultErrorHandler(this.logger);

// Set up client options
this.clientOptions = clientOptions !== undefined ? clientOptions : {};
if (agent !== undefined && this.clientOptions.agent === undefined) {
this.clientOptions.agent = agent;
Expand All @@ -276,7 +280,7 @@ export default class App {
// only logLevel is passed
this.clientOptions.logLevel = logLevel;
} else {
// Since v3.4, WebClient starts sharing loggger with App
// Since v3.4, WebClient starts sharing logger with App
this.clientOptions.logger = this.logger;
}
// The public WebClient instance (app.client)
Expand Down Expand Up @@ -307,8 +311,8 @@ export default class App {
this.developerMode &&
this.installerOptions &&
(typeof this.installerOptions.callbackOptions === 'undefined' ||
(typeof this.installerOptions.callbackOptions !== 'undefined' &&
typeof this.installerOptions.callbackOptions.failure === 'undefined'))
(typeof this.installerOptions.callbackOptions !== 'undefined' &&
typeof this.installerOptions.callbackOptions.failure === 'undefined'))
) {
// add a custom failure callback for Developer Mode in case they are using OAuth
this.logger.debug('adding Developer Mode custom OAuth failure handler');
Expand All @@ -321,8 +325,9 @@ export default class App {
};
}

// Check for required arguments of HTTPReceiver
// Initialize receiver
if (receiver !== undefined) {
// Custom receiver
if (this.socketMode) {
throw new AppInitializationError('receiver cannot be passed when socketMode is set to true');
}
Expand All @@ -338,6 +343,7 @@ export default class App {
clientId,
clientSecret,
stateSecret,
redirectUri,
installationStore,
scopes,
logger,
Expand All @@ -363,6 +369,7 @@ export default class App {
clientId,
clientSecret,
stateSecret,
redirectUri,
installationStore,
scopes,
logger,
Expand Down
55 changes: 54 additions & 1 deletion src/receivers/ExpressReceiver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Application, IRouter, Request, Response } from 'express';
import { Readable } from 'stream';
import { EventEmitter } from 'events';
import { Override, mergeOverrides } from '../test-helpers';
import { ErrorCode, CodedError, ReceiverInconsistentStateError } from '../errors';
import { ErrorCode, CodedError, ReceiverInconsistentStateError, AppInitializationError } from '../errors';

import ExpressReceiver, {
respondToSslCheck,
Expand Down Expand Up @@ -137,6 +137,59 @@ describe('ExpressReceiver', function () {
assert((router.get as any).called);
assert((router.post as any).calledOnce);
});
it('should throw an error if redirect uri options supplied invalid or incomplete', async function () {
const clientId = 'my-clientId';
const clientSecret = 'my-clientSecret';
const signingSecret = 'secret';
const stateSecret = 'my-stateSecret';
const scopes = ['chat:write'];
const redirectUri = 'http://example.com/heyo';
const installerOptions = {
redirectUriPath: '/heyo',
};
// correct format with redirect options supplied
const receiver = new ExpressReceiver({
clientId,
clientSecret,
signingSecret,
stateSecret,
scopes,
redirectUri,
installerOptions,
});
assert.isNotNull(receiver);
// missing redirectUriPath
assert.throws(() => new ExpressReceiver({
clientId,
clientSecret,
signingSecret,
stateSecret,
scopes,
redirectUri,
}), AppInitializationError);
// inconsistent redirectUriPath
assert.throws(() => new ExpressReceiver({
clientId: 'my-clientId',
clientSecret,
signingSecret,
stateSecret,
scopes,
redirectUri,
installerOptions: {
redirectUriPath: '/hiya',
},
}), AppInitializationError);
// inconsistent redirectUri
assert.throws(() => new ExpressReceiver({
clientId: 'my-clientId',
clientSecret,
signingSecret,
stateSecret,
scopes,
redirectUri: 'http://example.com/hiya',
installerOptions,
}), AppInitializationError);
});
});

describe('#start()', function () {
Expand Down
41 changes: 31 additions & 10 deletions src/receivers/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
} from '../errors';
import { AnyMiddlewareArgs, Receiver, ReceiverEvent } from '../types';
import defaultRenderHtmlForInstallPath from './render-html-for-install-path';
import { verifyRedirectOpts } from './verify-redirect-opts';

// Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer()
const httpsOptionKeys = [
Expand Down Expand Up @@ -88,6 +89,7 @@ export interface ExpressReceiverOptions {
clientId?: string;
clientSecret?: string;
stateSecret?: InstallProviderOptions['stateSecret']; // required when using default stateStore
redirectUri?: string;
installationStore?: InstallProviderOptions['installationStore']; // default MemoryInstallationStore
scopes?: InstallURLOptions['scopes'];
installerOptions?: InstallerOptions;
Expand All @@ -98,6 +100,7 @@ export interface ExpressReceiverOptions {
// Additional Installer Options
interface InstallerOptions {
stateStore?: InstallProviderOptions['stateStore']; // default ClearStateStore
stateVerification?: InstallProviderOptions['stateVerification']; // defaults true
authVersion?: InstallProviderOptions['authVersion']; // default 'v2'
metadata?: InstallURLOptions['metadata'];
installPath?: string;
Expand Down Expand Up @@ -131,6 +134,8 @@ export default class ExpressReceiver implements Receiver {

public installer: InstallProvider | undefined = undefined;

public installerOptions?: InstallerOptions;

public constructor({
signingSecret = '',
logger = undefined,
Expand All @@ -141,6 +146,7 @@ export default class ExpressReceiver implements Receiver {
clientId = undefined,
clientSecret = undefined,
stateSecret = undefined,
redirectUri = undefined,
installationStore = undefined,
scopes = undefined,
installerOptions = {},
Expand All @@ -166,7 +172,6 @@ export default class ExpressReceiver implements Receiver {
respondToUrlVerification,
this.requestHandler.bind(this),
];

this.processBeforeResponse = processBeforeResponse;

const endpointList = typeof endpoints === 'string' ? [endpoints] : Object.values(endpoints);
Expand All @@ -175,10 +180,15 @@ export default class ExpressReceiver implements Receiver {
this.router.post(endpoint, ...expressMiddleware);
});

// Verify redirect options if supplied, throws coded error if invalid
verifyRedirectOpts({ redirectUri, redirectUriPath: installerOptions.redirectUriPath });

if (
clientId !== undefined &&
clientSecret !== undefined &&
(stateSecret !== undefined || installerOptions.stateStore !== undefined)
(installerOptions.stateVerification === false || // state store not needed
stateSecret !== undefined ||
installerOptions.stateStore !== undefined) // user provided state store
) {
this.installer = new InstallProvider({
clientId,
Expand All @@ -188,31 +198,42 @@ export default class ExpressReceiver implements Receiver {
logLevel,
logger, // pass logger that was passed in constructor, not one created locally
stateStore: installerOptions.stateStore,
stateVerification: installerOptions.stateVerification,
authVersion: installerOptions.authVersion ?? 'v2',
clientOptions: installerOptions.clientOptions,
authorizationUrl: installerOptions.authorizationUrl,
});
}

// create install url options
const installUrlOptions = {
metadata: installerOptions.metadata,
scopes: scopes ?? [],
userScopes: installerOptions.userScopes,
redirectUri,
};
// Add OAuth routes to receiver
if (this.installer !== undefined) {
const redirectUriPath = installerOptions.redirectUriPath === undefined ?
'/slack/oauth_redirect' :
installerOptions.redirectUriPath;
const { callbackOptions, stateVerification } = installerOptions;
this.router.use(redirectUriPath, async (req, res) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await this.installer!.handleCallback(req, res, installerOptions.callbackOptions);
if (stateVerification === false) {
// when stateVerification is disabled pass install options directly to handler
// since they won't be encoded in the state param of the generated url
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await this.installer!.handleCallback(req, res, callbackOptions, installUrlOptions);
} else {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await this.installer!.handleCallback(req, res, callbackOptions);
}
});

const installPath = installerOptions.installPath === undefined ? '/slack/install' : installerOptions.installPath;
this.router.get(installPath, async (_req, res, next) => {
try {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const url = await this.installer!.generateInstallUrl({
metadata: installerOptions.metadata,
scopes: scopes ?? [],
userScopes: installerOptions.userScopes,
});
const url = await this.installer!.generateInstallUrl(installUrlOptions, stateVerification);
if (installerOptions.directInstall) {
// If a Slack app sets "Direct Install URL" in the Slack app configuration,
// the installation flow of the app should start with the Slack authorize URL.
Expand Down