From bff756b456cbb769080631af2beb85671ff4c79c Mon Sep 17 00:00:00 2001 From: Marco Ippolito Date: Mon, 3 Jul 2023 11:50:46 +0200 Subject: [PATCH] Merge pull request from GHSA-g8x5-p9qc-cf95 * feat: pass request into checkStateFunction * feat: add state in cookies * fix: check presence * fix: check cookie only * fix: change defaultGenerateStateFunction * Update index.js Co-authored-by: Filip Skokan * fix: unsign * fix: assume stateCookie is always present * renamed state * fix: use session --------- Co-authored-by: Filip Skokan --- README.md | 9 +++---- index.js | 30 ++++++++++++++--------- package.json | 1 + test/index.test.js | 61 +++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 81 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index a73de35..15372a9 100644 --- a/README.md +++ b/README.md @@ -125,11 +125,10 @@ fastify.register(oauthPlugin, { ## Set custom state The `generateStateFunction` accepts a function to generate the `state` parameter for the OAUTH flow. This function receives the Fastify instance's `request` object as parameter. +The `state` parameter will be also set into a `httpOnly`, `sameSite: Lax` cookie. When you set it, it is required to provide the function `checkStateFunction` in order to validate the states generated. ```js - const validStates = new Set() - fastify.register(oauthPlugin, { name: 'facebookOAuth2', credentials: { @@ -146,12 +145,12 @@ When you set it, it is required to provide the function `checkStateFunction` in // custom function to generate the state generateStateFunction: (request) => { const state = request.query.customCode - validStates.add(state) + request.session.state = state return state }, // custom function to check the state is valid - checkStateFunction: (returnedState, callback) => { - if (validStates.has(returnedState)) { + checkStateFunction: (request, callback) => { + if (request.query.state === request.session.state) { callback() return } diff --git a/index.js b/index.js index c6b6f84..9562cf4 100644 --- a/index.js +++ b/index.js @@ -1,6 +1,6 @@ 'use strict' -const defaultState = require('crypto').randomBytes(10).toString('hex') +const crypto = require('crypto') const fp = require('fastify-plugin') const { AuthorizationCode } = require('simple-oauth2') @@ -10,11 +10,13 @@ const promisify = require('util').promisify const callbackify = require('util').callbackify function defaultGenerateStateFunction () { - return defaultState + return crypto.randomBytes(16).toString('base64url') } -function defaultCheckStateFunction (state, callback) { - if (state === defaultState) { +function defaultCheckStateFunction (request, callback) { + const state = request.query.state + const stateCookie = request.cookies['oauth2-redirect-state'] + if (stateCookie && state === stateCookie) { callback() return } @@ -60,6 +62,10 @@ function fastifyOauth2 (fastify, options, next) { return next(new Error('options.schema should be a object')) } + if (!fastify.hasReplyDecorator('cookie')) { + fastify.register(require('@fastify/cookie')) + } + const name = options.name const credentials = options.credentials const callbackUri = options.callbackUri @@ -73,9 +79,8 @@ function fastifyOauth2 (fastify, options, next) { const tags = options.tags || [] const schema = options.schema || { tags } - function generateAuthorizationUri (requestObject) { - const state = generateStateFunction(requestObject) - const urlOptions = Object.assign({}, generateCallbackUriParams(callbackUriParams, requestObject, scope, state), { + function generateAuthorizationUri (request, state) { + const urlOptions = Object.assign({}, generateCallbackUriParams(callbackUriParams, request, scope, state), { redirect_uri: callbackUri, scope, state @@ -86,9 +91,13 @@ function fastifyOauth2 (fastify, options, next) { } function startRedirectHandler (request, reply) { - const authorizationUri = generateAuthorizationUri(request) + const state = generateStateFunction(request) + const authorizationUri = generateAuthorizationUri(request, state) - reply.redirect(authorizationUri) + reply.setCookie('oauth2-redirect-state', state, { + httpOnly: true, + sameSite: 'lax' + }).redirect(authorizationUri) } const cbk = function (o, code, callback) { @@ -102,9 +111,8 @@ function fastifyOauth2 (fastify, options, next) { function getAccessTokenFromAuthorizationCodeFlowCallbacked (request, callback) { const code = request.query.code - const state = request.query.state - checkStateFunction(state, function (err) { + checkStateFunction(request, function (err) { if (err) { callback(err) return diff --git a/package.json b/package.json index 3949e78..84b558b 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "tsd": "^0.28.0" }, "dependencies": { + "@fastify/cookie": "^8.3.0", "fastify-plugin": "^4.0.0", "simple-oauth2": "^5.0.0" }, diff --git a/test/index.test.js b/test/index.test.js index c40c0ff..3bfc6b9 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -3,7 +3,7 @@ const t = require('tap') const nock = require('nock') const createFastify = require('fastify') - +const crypto = require('crypto') const fastifyOauth2 = require('..') nock.disableNetConnect() @@ -54,7 +54,10 @@ function makeRequests (t, fastify) { fastify.inject({ method: 'GET', - url: '/?code=my-code&state=' + state + url: '/?code=my-code&state=' + state, + cookies: { + 'oauth2-redirect-state': state + } }, function (err, responseEnd) { t.error(err) @@ -426,7 +429,6 @@ t.test('options.generateStateFunction with request', t => { }) t.test('generateAuthorizationUri redirect with request object', t => { - t.plan(4) const fastify = createFastify() fastify.register(fastifyOauth2, { @@ -448,7 +450,7 @@ t.test('generateAuthorizationUri redirect with request object', t => { }) fastify.get('/gh', function (request, reply) { - const redirectUrl = this.theName.generateAuthorizationUri(request) + const redirectUrl = this.theName.generateAuthorizationUri(request, 'generated_code') return reply.redirect(redirectUrl) }) @@ -463,6 +465,7 @@ t.test('generateAuthorizationUri redirect with request object', t => { t.equal(responseStart.statusCode, 302) const matched = responseStart.headers.location.match(/https:\/\/github\.com\/login\/oauth\/authorize\?response_type=code&client_id=my-client-id&redirect_uri=%2Fcallback&scope=notifications&state=generated_code/) t.ok(matched) + t.end() }) }) @@ -799,3 +802,53 @@ t.test('preset configuration generate-callback-uri-params', t => { t.equal(typeof fastifyOauth2[configName].authorizePath, 'string') } }) + +t.test('options.generateStateFunction with signing key', t => { + t.plan(5) + const fastify = createFastify() + + const hmacKey = 'hello' + const expectedState = crypto.createHmac('sha1', hmacKey).update('foo').digest('hex') + + fastify.register(require('@fastify/cookie')) + + fastify.register(fastifyOauth2, { + name: 'the-name', + credentials: { + client: { + id: 'my-client-id', + secret: 'my-secret' + }, + auth: fastifyOauth2.GITHUB_CONFIGURATION + }, + startRedirectPath: '/login/github', + callbackUri: '/callback', + generateStateFunction: (request) => { + const state = crypto.createHmac('sha1', hmacKey).update(request.headers.foo).digest('hex') + t.ok(request, 'the request param has been set') + return state + }, + checkStateFunction: (request) => { + const generatedState = crypto.createHmac('sha1', hmacKey).update(request.headers.foo).digest('hex') + return generatedState === request.query.state + }, + scope: ['notifications'] + }) + + t.teardown(fastify.close.bind(fastify)) + + fastify.listen({ port: 0 }, function (err) { + t.error(err) + fastify.inject({ + method: 'GET', + url: '/login/github', + query: { code: expectedState }, + headers: { foo: 'foo' } + }, function (err, responseStart) { + t.error(err) + t.equal(responseStart.statusCode, 302) + const matched = responseStart.headers.location.match(/https:\/\/github\.com\/login\/oauth\/authorize\?response_type=code&client_id=my-client-id&redirect_uri=%2Fcallback&scope=notifications&state=1e864fbd840212c1ed9ce60175d373f3a48681b2/) + t.ok(matched) + }) + }) +})