Fix slackapi#1335 Proper use of state parameter for the OAuth CSRF pr…

…otection (slackapi#1391)
sumitparakh · Mar 28, 2022 · 0ed7874 · 0ed7874
1 parent 5e19ee1
commit 0ed7874
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 158 deletions.
diff --git a/examples/oauth-express-receiver/app.js b/examples/oauth-express-receiver/app.js
@@ -1,4 +1,4 @@
-const { App, ExpressReceiver, LogLevel } = require('@slack/bolt');
+const { App, ExpressReceiver, LogLevel, FileInstallationStore } = require('@slack/bolt');
 
 // Create an ExpressReceiver
 const receiver = new ExpressReceiver({ 
@@ -13,32 +13,7 @@ const receiver = new ExpressReceiver({
     // This flag is available in @slack/bolt v3.7 or higher
     // directInstall: true,
   },
-  installationStore: {
-    storeInstallation: async (installation) => {
-      // replace database.set so it fetches from your database
-      if (installation.isEnterpriseInstall && installation.enterprise !== undefined) {
-        // support for org wide app installation
-        return await database.set(installation.enterprise.id, installation);
-      }
-      if (installation.team !== undefined) {
-        // single team app installation
-        return await database.set(installation.team.id, installation);
-      }
-      throw new Error('Failed saving installation data to installationStore');
-    },
-    fetchInstallation: async (installQuery) => {
-      // replace database.get so it fetches from your database
-      if (installQuery.isEnterpriseInstall && installQuery.enterpriseId !== undefined) {
-        // org wide app installation lookup
-        return await database.get(installQuery.enterpriseId);
-      }
-      if (installQuery.teamId !== undefined) {
-        // single team app installation lookup
-        return await database.get(installQuery.teamId);
-      }
-      throw new Error('Failed fetching installation');
-    },
-  },
+  installationStore: new FileInstallationStore(),
 });
 
 // Create the Bolt App, using the receiver
@@ -60,6 +35,6 @@ receiver.router.get('/secret-page', (req, res) => {
 });
 
 (async () => {
-  await app.start(8080);
+  await app.start(3000);
   console.log('Express app is running');
-})();
+})();
diff --git a/examples/oauth-express-receiver/link.sh b/examples/oauth-express-receiver/link.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+current_dir=`dirname $0`
+cd ${current_dir}
+npm unlink @slack/bolt \
+  && npm i \
+  && cd ../.. \
+  && npm link \
+  && cd - \
+  && npm i \
+  && npm link @slack/bolt
diff --git a/examples/socket-mode-oauth/link.sh b/examples/socket-mode-oauth/link.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+current_dir=`dirname $0`
+cd ${current_dir}
+npm unlink @slack/bolt \
+  && npm i \
+  && cd ../.. \
+  && npm link \
+  && cd - \
+  && npm i \
+  && npm link @slack/bolt
diff --git a/package.json b/package.json
@@ -42,7 +42,7 @@
   },
   "dependencies": {
     "@slack/logger": "^3.0.0",
-    "@slack/oauth": "^2.4.0",
+    "@slack/oauth": "^2.5.0",
     "@slack/socket-mode": "^1.2.0",
     "@slack/types": "^2.4.0",
     "@slack/web-api": "^6.7.0",

diff --git a/src/receivers/ExpressReceiver.ts b/src/receivers/ExpressReceiver.ts
@@ -8,7 +8,7 @@ import querystring from 'querystring';
 import crypto from 'crypto';
 import tsscmp from 'tsscmp';
 import { Logger, ConsoleLogger, LogLevel } from '@slack/logger';
-import { InstallProvider, CallbackOptions, InstallProviderOptions, InstallURLOptions } from '@slack/oauth';
+import { InstallProvider, CallbackOptions, InstallProviderOptions, InstallURLOptions, InstallPathOptions } from '@slack/oauth';
 import App from '../App';
 import {
   ReceiverAuthenticityError,
@@ -17,7 +17,6 @@ import {
   CodedError,
 } from '../errors';
 import { AnyMiddlewareArgs, Receiver, ReceiverEvent } from '../types';
-import defaultRenderHtmlForInstallPath from './render-html-for-install-path';
 import { verifyRedirectOpts } from './verify-redirect-opts';
 import { StringIndexed } from '../types/helpers';
 import { HTTPModuleFunctions as httpFunc } from './HTTPModuleFunctions';
@@ -104,12 +103,16 @@ export interface ExpressReceiverOptions {
 interface InstallerOptions {
   stateStore?: InstallProviderOptions['stateStore']; // default ClearStateStore
   stateVerification?: InstallProviderOptions['stateVerification']; // defaults true
+  legacyStateVerification?: InstallProviderOptions['legacyStateVerification'];
+  stateCookieName?: InstallProviderOptions['stateCookieName'];
+  stateCookieExpirationSeconds?: InstallProviderOptions['stateCookieExpirationSeconds'];
   authVersion?: InstallProviderOptions['authVersion']; // default 'v2'
   metadata?: InstallURLOptions['metadata'];
   installPath?: string;
-  directInstall?: boolean; // see https://api.slack.com/start/distributing/directory#direct_install
-  renderHtmlForInstallPath?: (url: string) => string;
+  directInstall?: InstallProviderOptions['directInstall']; // see https://api.slack.com/start/distributing/directory#direct_install
+  renderHtmlForInstallPath?: InstallProviderOptions['renderHtmlForInstallPath'];
   redirectUriPath?: string;
+  installPathOptions?: InstallPathOptions;
   callbackOptions?: CallbackOptions;
   userScopes?: InstallURLOptions['userScopes'];
   clientOptions?: InstallProviderOptions['clientOptions'];
@@ -205,8 +208,13 @@ export default class ExpressReceiver implements Receiver {
         installationStore,
         logLevel,
         logger, // pass logger that was passed in constructor, not one created locally
+        directInstall: installerOptions.directInstall,
         stateStore: installerOptions.stateStore,
         stateVerification: installerOptions.stateVerification,
+        legacyStateVerification: installerOptions.legacyStateVerification,
+        stateCookieName: installerOptions.stateCookieName,
+        stateCookieExpirationSeconds: installerOptions.stateCookieExpirationSeconds,
+        renderHtmlForInstallPath: installerOptions.renderHtmlForInstallPath,
         authVersion: installerOptions.authVersion ?? 'v2',
         clientOptions: installerOptions.clientOptions,
         authorizationUrl: installerOptions.authorizationUrl,
@@ -221,6 +229,7 @@ export default class ExpressReceiver implements Receiver {
     };
     // Add OAuth routes to receiver
     if (this.installer !== undefined) {
+      const { installer } = this;
       const redirectUriPath = installerOptions.redirectUriPath === undefined ?
         '/slack/oauth_redirect' :
         installerOptions.redirectUriPath;
@@ -229,31 +238,17 @@ export default class ExpressReceiver implements Receiver {
         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);
+          await installer.handleCallback(req, res, callbackOptions, installUrlOptions);
         } else {
-          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-          await this.installer!.handleCallback(req, res, callbackOptions);
+          await installer.handleCallback(req, res, callbackOptions);
         }
       });
 
       const installPath = installerOptions.installPath === undefined ? '/slack/install' : installerOptions.installPath;
-      this.router.get(installPath, async (_req, res, next) => {
+      const { installPathOptions } = installerOptions;
+      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(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.
-            // See https://api.slack.com/start/distributing/directory#direct_install for more details.
-            res.redirect(url);
-          } else {
-            // The installation starts from a landing page served by this app.
-            const renderHtml = installerOptions.renderHtmlForInstallPath !== undefined ?
-              installerOptions.renderHtmlForInstallPath :
-              defaultRenderHtmlForInstallPath;
-            res.send(renderHtml(url));
-          }
+          await installer.handleInstallPath(req, res, installPathOptions, installUrlOptions);
         } catch (error) {
           next(error);
         }

diff --git a/src/receivers/HTTPReceiver.spec.ts b/src/receivers/HTTPReceiver.spec.ts
@@ -257,7 +257,7 @@ describe('HTTPReceiver', function () {
   });
   describe('request handling', function () {
     describe('handleInstallPathRequest()', function () {
-      it('should invoke installer generateInstallUrl if a request comes into the install path', async function () {
+      it('should invoke installer handleInstallPath if a request comes into the install path', async function () {
         // Arrange
         const installProviderStub = sinon.createStubInstance(InstallProvider);
         const overrides = mergeOverrides(
@@ -296,9 +296,7 @@ describe('HTTPReceiver', function () {
         fakeRes.end = end;
         fakeRes.setHeader = setHeader;
         await receiver.requestListener(fakeReq, fakeRes);
-        assert(installProviderStub.generateInstallUrl.calledWith(sinon.match({ metadata, scopes, userScopes })));
-        assert.isTrue(writeHead.calledWith(200));
-        assert.isTrue(setHeader.calledWith('Content-Type', 'text/html; charset=utf-8'));
+        assert(installProviderStub.handleInstallPath.calledWith(fakeReq, fakeRes));
       });
 
       it('should use a custom HTML renderer for the install path webpage', async function () {
@@ -340,9 +338,7 @@ describe('HTTPReceiver', function () {
         fakeRes.end = end;
         /* eslint-disable-next-line @typescript-eslint/await-thenable */
         await receiver.requestListener(fakeReq, fakeRes);
-        assert(installProviderStub.generateInstallUrl.calledWith(sinon.match({ metadata, scopes, userScopes })));
-        assert.isTrue(writeHead.calledWith(200));
-        assert.isTrue(end.calledWith('Hello world!'));
+        assert(installProviderStub.handleInstallPath.calledWith(fakeReq, fakeRes));
       });
 
       it('should redirect installers if directInstall is true', async function () {
@@ -383,8 +379,7 @@ describe('HTTPReceiver', function () {
         fakeRes.writeHead = writeHead;
         fakeRes.end = end;
         await receiver.requestListener(fakeReq, fakeRes);
-        assert(installProviderStub.generateInstallUrl.calledWith(sinon.match({ metadata, scopes, userScopes })));
-        assert.isTrue(writeHead.calledWith(302, sinon.match.object));
+        assert(installProviderStub.handleInstallPath.calledWith(fakeReq, fakeRes));
       });
     });
 

diff --git a/src/receivers/HTTPReceiver.ts b/src/receivers/HTTPReceiver.ts
@@ -3,13 +3,12 @@ import { createServer, Server, ServerOptions, RequestListener, IncomingMessage,
 import { createServer as createHttpsServer, Server as HTTPSServer, ServerOptions as HTTPSServerOptions } from 'https';
 import { ListenOptions } from 'net';
 import { Logger, ConsoleLogger, LogLevel } from '@slack/logger';
-import { InstallProvider, CallbackOptions, InstallProviderOptions, InstallURLOptions } from '@slack/oauth';
+import { InstallProvider, CallbackOptions, InstallProviderOptions, InstallURLOptions, InstallPathOptions } from '@slack/oauth';
 import { URL } from 'url';
 
 import { verifyRedirectOpts } from './verify-redirect-opts';
 import App from '../App';
 import { Receiver, ReceiverEvent } from '../types';
-import defaultRenderHtmlForInstallPath from './render-html-for-install-path';
 import {
   ReceiverInconsistentStateError,
   HTTPReceiverDeferredRequestError,
@@ -91,16 +90,20 @@ export interface HTTPReceiverOptions {
 // All the available argument for OAuth flow enabled apps
 export interface HTTPReceiverInstallerOptions {
   installPath?: string;
-  directInstall?: boolean; // see https://api.slack.com/start/distributing/directory#direct_install
-  renderHtmlForInstallPath?: (url: string) => string;
+  directInstall?: InstallProviderOptions['directInstall']; // see https://api.slack.com/start/distributing/directory#direct_install
+  renderHtmlForInstallPath?: InstallProviderOptions['renderHtmlForInstallPath'];
   redirectUriPath?: string;
   stateStore?: InstallProviderOptions['stateStore']; // default ClearStateStore
   stateVerification?: InstallProviderOptions['stateVerification']; // default true
+  legacyStateVerification?: InstallProviderOptions['legacyStateVerification'];
+  stateCookieName?: InstallProviderOptions['stateCookieName'];
+  stateCookieExpirationSeconds?: InstallProviderOptions['stateCookieExpirationSeconds'];
   authVersion?: InstallProviderOptions['authVersion']; // default 'v2'
   clientOptions?: InstallProviderOptions['clientOptions'];
   authorizationUrl?: InstallProviderOptions['authorizationUrl'];
   metadata?: InstallURLOptions['metadata'];
   userScopes?: InstallURLOptions['userScopes'];
+  installPathOptions?: InstallPathOptions;
   callbackOptions?: CallbackOptions;
   // This value exists here only for the compatibility with SocketModeReceiver.
   // If you use only HTTPReceiver, the top-level is recommended.
@@ -133,14 +136,12 @@ export default class HTTPReceiver implements Receiver {
 
   private installPath?: string; // always defined when installer is defined
 
-  private directInstall?: boolean; // always defined when installer is defined
-
-  private renderHtmlForInstallPath: (url: string) => string;
-
   private installRedirectUriPath?: string; // always defined when installer is defined
 
   private installUrlOptions?: InstallURLOptions; // always defined when installer is defined
 
+  private installPathOptions?: InstallPathOptions; // always defined when installer is defined
+
   private installCallbackOptions?: CallbackOptions; // always defined when installer is defined
 
   private stateVerification?: boolean; // always defined when installer is defined
@@ -212,17 +213,22 @@ export default class HTTPReceiver implements Receiver {
         installationStore,
         logger,
         logLevel,
+        directInstall: installerOptions.directInstall,
         stateStore: installerOptions.stateStore,
         stateVerification: installerOptions.stateVerification,
+        legacyStateVerification: installerOptions.legacyStateVerification,
+        stateCookieName: installerOptions.stateCookieName,
+        stateCookieExpirationSeconds: installerOptions.stateCookieExpirationSeconds,
+        renderHtmlForInstallPath: installerOptions.renderHtmlForInstallPath,
         authVersion: installerOptions.authVersion,
         clientOptions: installerOptions.clientOptions,
         authorizationUrl: installerOptions.authorizationUrl,
       });
 
       // Store the remaining instance variables that are related to using the InstallProvider
       this.installPath = installerOptions.installPath ?? '/slack/install';
-      this.directInstall = installerOptions.directInstall !== undefined && installerOptions.directInstall;
       this.installRedirectUriPath = installerOptions.redirectUriPath ?? '/slack/oauth_redirect';
+      this.installPathOptions = installerOptions.installPathOptions ?? {};
       this.installCallbackOptions = installerOptions.callbackOptions ?? {};
       this.installUrlOptions = {
         scopes: scopes ?? [],
@@ -231,9 +237,6 @@ export default class HTTPReceiver implements Receiver {
         redirectUri,
       };
     }
-    this.renderHtmlForInstallPath = installerOptions.renderHtmlForInstallPath !== undefined ?
-      installerOptions.renderHtmlForInstallPath :
-      defaultRenderHtmlForInstallPath;
     this.customPropertiesExtractor = customPropertiesExtractor;
     this.dispatchErrorHandler = dispatchErrorHandler;
     this.processEventErrorHandler = processEventErrorHandler;
@@ -375,7 +378,7 @@ export default class HTTPReceiver implements Receiver {
       // Visiting the installation endpoint
       if (path === installPath) {
         // Render installation path (containing Add to Slack button)
-        return this.handleInstallPathRequest(res);
+        return this.handleInstallPathRequest(req, res);
       }
 
       // Installation has been initiated
@@ -493,42 +496,20 @@ export default class HTTPReceiver implements Receiver {
     })();
   }
 
-  private handleInstallPathRequest(res: ServerResponse) {
+  private handleInstallPathRequest(req: IncomingMessage, res: ServerResponse) {
     // Wrapped in an async closure for ease of using await
     (async () => {
-      // NOTE: Skipping some ceremony such as content negotiation, setting informative headers, etc. These may be nice
-      // to have for completeness, but there's no clear benefit to adding them, so just keeping things simple. If a
-      // user desires a more custom page, they can always call `App.installer.generateInstallUrl()` and render their
-      // own page instead of using this one.
       try {
-        // This function is only called from within unboundRequestListener after checking that installer is defined, and
-        // when installer is defined then installUrlOptions is always defined too.
-        // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-        const [installer, installUrlOptions] = [this.installer!, this.installUrlOptions!];
-        // Generate the URL for the "Add to Slack" button.
-        const url = await installer.generateInstallUrl(installUrlOptions, this.stateVerification);
-
-        if (this.directInstall !== undefined && this.directInstall) {
-          // If a Slack app sets "Direct Install URL" in the Slack app configruation,
-          // the installation flow of the app should start with the Slack authorize URL.
-          // See https://api.slack.com/start/distributing/directory#direct_install for more details.
-          res.writeHead(302, { Location: url });
-          res.end('');
-        } else {
-          // The installation starts from a landing page served by this app.
-          // Generate HTML response body
-          const body = this.renderHtmlForInstallPath(url);
-
-          // Serve a basic HTML page including the "Add to Slack" button.
-          // Regarding headers:
-          // - Content-Length is not used because Transfer-Encoding='chunked' is automatically used.
-          res.setHeader('Content-Type', 'text/html; charset=utf-8');
-          res.writeHead(200);
-          res.end(body);
-        }
+        /* eslint-disable @typescript-eslint/no-non-null-assertion */
+        await this.installer!.handleInstallPath(
+          req,
+          res,
+          this.installPathOptions,
+          this.installUrlOptions,
+        );
       } catch (err) {
         const e = err as any;
-        this.logger.error('An unhandled error occurred while Bolt processed a request to the installation path');
+        this.logger.error(`An unhandled error occurred while Bolt processed a request to the installation path (${e.message})`);
         this.logger.debug(`Error details: ${e}`);
       }
     })();