Skip to content

Commit

Permalink
Add option to disable state verification, fix redirect uri bug (#1116)
Browse files Browse the repository at this point in the history 
* Add option to disable state verification, fix redirect uri bug
  • Loading branch information
@srajiang
srajiang committed Sep 27, 2021
1 parent 2f3f499 commit 1c1eb57
Show file tree
Hide file tree
Showing 10 changed files with 843 additions and 413 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit 1c1eb57

Please sign in to comment.