From 56d50859d146dea014df3eca4987fc1aea07aee3 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Tue, 1 Mar 2022 07:54:49 +0900 Subject: [PATCH] Fix #1439 Add built-in StateStore implementations using server-side database --- packages/oauth/package.json | 2 +- packages/oauth/src/errors.ts | 4 + .../state-stores/clear-state-store.spec.js | 14 --- .../state-stores/clear-state-store.spec.ts | 8 ++ .../src/state-stores/clear-state-store.ts | 42 ++++++- .../src/state-stores/file-state-store.spec.ts | 10 ++ .../src/state-stores/file-state-store.ts | 109 ++++++++++++++++++ packages/oauth/src/state-stores/index.ts | 1 + packages/oauth/src/state-stores/interface.ts | 1 + packages/oauth/src/state-stores/spec-utils.ts | 91 +++++++++++++++ packages/oauth/tsconfig.json | 1 + 11 files changed, 262 insertions(+), 21 deletions(-) delete mode 100644 packages/oauth/src/state-stores/clear-state-store.spec.js create mode 100644 packages/oauth/src/state-stores/clear-state-store.spec.ts create mode 100644 packages/oauth/src/state-stores/file-state-store.spec.ts create mode 100644 packages/oauth/src/state-stores/file-state-store.ts create mode 100644 packages/oauth/src/state-stores/spec-utils.ts diff --git a/packages/oauth/package.json b/packages/oauth/package.json index 1f28bc81d..a6546ed27 100644 --- a/packages/oauth/package.json +++ b/packages/oauth/package.json @@ -31,7 +31,7 @@ "prepare": "npm run build", "build": "npm run build:clean && tsc", "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output", - "lint": "eslint --ext .ts src", + "lint": "eslint --fix --ext .ts src", "test": "npm run lint && npm run test:mocha", "test:mocha": "nyc mocha --config .mocharc.json src/*.spec.js src/**/*.spec.js src/*.spec.ts src/**/*.spec.ts", "coverage": "codecov -F oauthhelper --root=$PWD", diff --git a/packages/oauth/src/errors.ts b/packages/oauth/src/errors.ts index b1e508c42..c049208ee 100644 --- a/packages/oauth/src/errors.ts +++ b/packages/oauth/src/errors.ts @@ -10,6 +10,7 @@ export enum ErrorCode { AuthorizationError = 'slack_oauth_installer_authorization_error', GenerateInstallUrlError = 'slack_oauth_generate_url_error', MissingStateError = 'slack_oauth_missing_state', + InvalidStateError = 'slack_oauth_invalid_state', MissingCodeError = 'slack_oauth_missing_code', UnknownError = 'slack_oauth_unknown_error', } @@ -24,6 +25,9 @@ export class GenerateInstallUrlError extends Error implements CodedError { export class MissingStateError extends Error implements CodedError { public code = ErrorCode.MissingStateError; } +export class InvalidStateError extends Error implements CodedError { + public code = ErrorCode.InvalidStateError; +} export class MissingCodeError extends Error implements CodedError { public code = ErrorCode.MissingCodeError; diff --git a/packages/oauth/src/state-stores/clear-state-store.spec.js b/packages/oauth/src/state-stores/clear-state-store.spec.js deleted file mode 100644 index 1a4b05a60..000000000 --- a/packages/oauth/src/state-stores/clear-state-store.spec.js +++ /dev/null @@ -1,14 +0,0 @@ -require('mocha'); -const { assert } = require('chai'); - -const { default: ClearStateStore } = require('./clear-state-store'); - -describe('ClearStateStore', async () => { - it('should generate a state and return install options once verified', async () => { - const stateStore = new ClearStateStore('stateSecret'); - const installUrlOptions = { scopes: ['channels:read'] }; - const state = await stateStore.generateStateParam(installUrlOptions, new Date()); - const returnedInstallUrlOptions = await stateStore.verifyStateParam(new Date(), state); - assert.deepEqual(installUrlOptions, returnedInstallUrlOptions); - }); -}); diff --git a/packages/oauth/src/state-stores/clear-state-store.spec.ts b/packages/oauth/src/state-stores/clear-state-store.spec.ts new file mode 100644 index 000000000..c85967949 --- /dev/null +++ b/packages/oauth/src/state-stores/clear-state-store.spec.ts @@ -0,0 +1,8 @@ +import ClearStateStore from './clear-state-store'; +import { StateStoreChaiTestRunner } from './spec-utils'; + +const testRunner = new StateStoreChaiTestRunner({ + stateStore: new ClearStateStore('secret'), + shouldVerifyOnlyOnce: false, +}); +testRunner.enableTests('ClearStateStore'); diff --git a/packages/oauth/src/state-stores/clear-state-store.ts b/packages/oauth/src/state-stores/clear-state-store.ts index 5834a6eac..7c05e8040 100644 --- a/packages/oauth/src/state-stores/clear-state-store.ts +++ b/packages/oauth/src/state-stores/clear-state-store.ts @@ -1,23 +1,53 @@ import { sign, verify } from 'jsonwebtoken'; import { InstallURLOptions } from '../install-url-options'; import { StateStore, StateObj } from './interface'; +import { InvalidStateError } from '../errors'; // default implementation of StateStore export default class ClearStateStore implements StateStore { private stateSecret: string; - public constructor(stateSecret: string) { + private stateExpirationSeconds: number; + + public constructor( + stateSecret: string, + stateExpirationSeconds: number = 600, + ) { this.stateSecret = stateSecret; + this.stateExpirationSeconds = stateExpirationSeconds; } - public async generateStateParam(installOptions: InstallURLOptions, now: Date): Promise { - return sign({ installOptions, now: now.toJSON() }, this.stateSecret); + public async generateStateParam( + installOptions: InstallURLOptions, + now: Date, + ): Promise { + const source = { + installOptions, + now: now.toJSON(), + }; + return sign(source, this.stateSecret); } - public async verifyStateParam(_now: Date, state: string): Promise { + public async verifyStateParam( + now: Date, + state: string, + ): Promise { // decode the state using the secret - const decoded: StateObj = verify(state, this.stateSecret) as StateObj; - + let decoded: StateObj; + try { + decoded = verify(state, this.stateSecret) as StateObj; + } catch (e) { + const message = `Failed to load the data represented by the state parameter (error: ${e})`; + throw new InvalidStateError(message); + } + // Check if the state value is not too old + const generatedAt = new Date(decoded.now); + const passedSeconds = Math.floor( + (now.getTime() - generatedAt.getTime()) / 1000, + ); + if (passedSeconds > this.stateExpirationSeconds) { + throw new InvalidStateError('The state value is already expired'); + } // return installOptions return decoded.installOptions; } diff --git a/packages/oauth/src/state-stores/file-state-store.spec.ts b/packages/oauth/src/state-stores/file-state-store.spec.ts new file mode 100644 index 000000000..803d51602 --- /dev/null +++ b/packages/oauth/src/state-stores/file-state-store.spec.ts @@ -0,0 +1,10 @@ +import os from 'os'; +import { FileStateStore } from './file-state-store'; +import { StateStoreChaiTestRunner } from './spec-utils'; + +const testRunner = new StateStoreChaiTestRunner({ + stateStore: new FileStateStore({ + baseDir: os.tmpdir(), + }), +}); +testRunner.enableTests('FileStateStore'); diff --git a/packages/oauth/src/state-stores/file-state-store.ts b/packages/oauth/src/state-stores/file-state-store.ts new file mode 100644 index 000000000..d4c566c85 --- /dev/null +++ b/packages/oauth/src/state-stores/file-state-store.ts @@ -0,0 +1,109 @@ +import { homedir } from 'os'; +import fs from 'fs'; +import path from 'path'; +import { randomUUID } from 'crypto'; +import { StateStore, StateObj } from './interface'; +import { InstallURLOptions } from '../install-url-options'; +import { InvalidStateError } from '../errors'; + +export interface FileStateStoreArgs { + stateExpirationSeconds?: number; + baseDir?: string; +} + +export class FileStateStore implements StateStore { + private baseDir: string; + + private stateExpirationSeconds: number; + + public constructor(args: FileStateStoreArgs) { + this.baseDir = args.baseDir !== undefined ? + args.baseDir : + `${homedir()}/.bolt-js-oauth-states`; + this.stateExpirationSeconds = args.stateExpirationSeconds !== undefined ? + args.stateExpirationSeconds : + 600; + } + + public async generateStateParam( + installOptions: InstallURLOptions, + now: Date, + ): Promise { + const state = randomUUID(); + const source: StateObj = { + installOptions, + now, + random: Math.floor(Math.random() * 1000000), + }; + this.writeToFile(state, source); + return state; + } + + public async verifyStateParam( + now: Date, + state: string, + ): Promise { + try { + if (this.findFile(state)) { + // decode the state using the secret + let decoded: StateObj | undefined; + try { + decoded = this.readFile(state); + } catch (e) { + const message = `Failed to load the data represented by the state parameter (error: ${e})`; + throw new InvalidStateError(message); + } + if (decoded !== undefined) { + // Check if the state value is not too old + const generatedAt = new Date(decoded.now); + const passedSeconds = Math.floor( + (now.getTime() - generatedAt.getTime()) / 1000, + ); + if (passedSeconds > this.stateExpirationSeconds) { + throw new InvalidStateError('The state value is already expired'); + } + // return installOptions + return decoded.installOptions; + } + } + } finally { + this.deleteFile(state); + } + throw new InvalidStateError('The state value is already expired'); + } + + // ------------------------------------------- + // private methods + // ------------------------------------------- + + private writeToFile(filename: string, data: StateObj): void { + fs.mkdirSync(this.baseDir, { recursive: true }); + const fullpath = path.resolve(`${this.baseDir}/${filename}`); + fs.writeFileSync(fullpath, JSON.stringify(data)); + } + + private findFile(filename: string): boolean { + const fullpath = path.resolve(`${this.baseDir}/${filename}`); + return fs.existsSync(fullpath); + } + + private readFile(filename: string): StateObj | undefined { + const fullpath = path.resolve(`${this.baseDir}/${filename}`); + try { + const data = fs.readFileSync(fullpath); + if (data !== undefined) { + return JSON.parse(data.toString()); + } + return undefined; + } catch (_) { + return undefined; + } + } + + private deleteFile(filename: string): void { + const fullpath = path.resolve(`${this.baseDir}/${filename}`); + if (fs.existsSync(fullpath)) { + fs.unlinkSync(fullpath); + } + } +} diff --git a/packages/oauth/src/state-stores/index.ts b/packages/oauth/src/state-stores/index.ts index bea43cd26..87e063aa6 100644 --- a/packages/oauth/src/state-stores/index.ts +++ b/packages/oauth/src/state-stores/index.ts @@ -1,2 +1,3 @@ export { StateStore, StateObj } from './interface'; export { default as ClearStateStore } from './clear-state-store'; +export { FileStateStore, FileStateStoreArgs } from './file-state-store'; diff --git a/packages/oauth/src/state-stores/interface.ts b/packages/oauth/src/state-stores/interface.ts index 5776bf4fe..a4fea47b1 100644 --- a/packages/oauth/src/state-stores/interface.ts +++ b/packages/oauth/src/state-stores/interface.ts @@ -4,6 +4,7 @@ import { InstallURLOptions } from '../install-url-options'; export interface StateObj { now: Date; installOptions: InstallURLOptions; + random?: string | number; } export interface StateStore { diff --git a/packages/oauth/src/state-stores/spec-utils.ts b/packages/oauth/src/state-stores/spec-utils.ts new file mode 100644 index 000000000..5f8361f3e --- /dev/null +++ b/packages/oauth/src/state-stores/spec-utils.ts @@ -0,0 +1,91 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +/* eslint-disable import/no-extraneous-dependencies */ +import { assert } from 'chai'; +import { StateStore } from './interface'; +import { InstallURLOptions } from '../install-url-options'; + +export interface StateStoreChaiTestRunnerArgs { + stateStore: StateStore; + shouldVerifyOnlyOnce?: boolean; +} + +export class StateStoreChaiTestRunner { + private stateStore: StateStore; + + private shouldVerifyOnlyOnce: boolean; + + public constructor(args: StateStoreChaiTestRunnerArgs) { + this.stateStore = args.stateStore; + this.shouldVerifyOnlyOnce = args.shouldVerifyOnlyOnce === undefined ? + true : + args.shouldVerifyOnlyOnce; + } + + public async enableTests(testTarget: string): Promise { + describe(testTarget, () => { + it('should generate and verify valid state values', async () => { + const { stateStore } = this; + const options: InstallURLOptions = { + scopes: ['commands', 'chat:write'], + teamId: 'T111', + redirectUri: 'https://www.example.com/slack/oauth_redirect', + userScopes: ['search:read'], + metadata: 'the metadata', + }; + const state = await stateStore.generateStateParam(options, new Date()); + assert.isNotEmpty(state); + const result = await stateStore.verifyStateParam(new Date(), state); + assert.deepEqual(result, options); + }); + + it('should detect old state values', async () => { + const { stateStore } = this; + const installUrlOptions = { scopes: ['channels:read'] }; + const fifteenMinutesLater = new Date( + new Date().getTime() + 15 * 60 * 1000, + ); + const state = await stateStore.generateStateParam( + installUrlOptions, + new Date(), + ); + try { + await stateStore.verifyStateParam(fifteenMinutesLater, state); + assert.fail('Exception should be thrown'); + } catch (e: any) { + assert.equal(e.code, 'slack_oauth_invalid_state'); + } + }); + + if (this.shouldVerifyOnlyOnce) { + it('should detect multiple consumption', async () => { + const { stateStore } = this; + const installUrlOptions = { scopes: ['channels:read'] }; + Array.from(Array(200)).forEach(async () => { + // generate other states + await stateStore.generateStateParam(installUrlOptions, new Date()); + }); + const state = await stateStore.generateStateParam( + installUrlOptions, + new Date(), + ); + const result = await stateStore.verifyStateParam(new Date(), state); + assert.exists(result); + let expectedlyReturnedResult; + try { + expectedlyReturnedResult = await stateStore.verifyStateParam( + new Date(), + state, + ); + assert.fail('Exception should be thrown'); + } catch (e: any) { + assert.equal( + e.code, + 'slack_oauth_invalid_state', + `${state} ${JSON.stringify(expectedlyReturnedResult)}`, + ); + } + }); + } + }); + } +} diff --git a/packages/oauth/tsconfig.json b/packages/oauth/tsconfig.json index ae5123259..086630dfd 100644 --- a/packages/oauth/tsconfig.json +++ b/packages/oauth/tsconfig.json @@ -29,6 +29,7 @@ "src/**/*" ], "exclude": [ + "src/**/spec-utils.ts", "src/**/*.spec.js", "src/**/*.spec.ts", "src/**/*.js"