From e6e437e28bbc6038aebd761defd12db08b0c13e1 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 27 Jan 2020 12:56:15 -0800 Subject: [PATCH 1/8] Cleanup in login() method on request context --- lib/context.js | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/context.js b/lib/context.js index 70365de0..b3f2b2f6 100644 --- a/lib/context.js +++ b/lib/context.js @@ -46,37 +46,38 @@ class ResponseContext { return urlJoin(this._config.baseURL, this._config.redirectUriPath); } - async login(params = {}) { + async login(options = {}) { const next = cb(this._next).once(); const req = this._req; const res = this._res; const config = this._config; - const client = req.openid.client; - const authorizeParams = config.authorizationParams; + + // Determine landing page after login callback. + let returnTo = this._config.baseURL; + if (options.returnTo) { + returnTo = options.returnTo; + } else if (req.method === 'GET' && req.originalUrl) { + returnTo = req.originalUrl; + } + const transientOpts = { legacySameSiteCookie: config.legacySameSiteCookie, sameSite: config.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' }; try { - let returnTo; - if (params.returnTo) { - returnTo = params.returnTo; - } else if (req.method === 'GET') { - returnTo = req.originalUrl; - } else { - returnTo = this._config.baseURL; - } - - // TODO: Store this in state transient.store('returnTo', res, Object.assign({value: returnTo}, transientOpts)); - const authParams = Object.assign({ - nonce: transient.store('nonce', res, transientOpts), - state: transient.store('state', res, transientOpts), - redirect_uri: this.getRedirectUri() - }, authorizeParams, params.authorizationParams || {}); + const authParams = Object.assign( + { + nonce: transient.store('nonce', res, transientOpts), + state: transient.store('state', res, transientOpts), + redirect_uri: this.getRedirectUri() + }, + config.authorizationParams, + options.authorizationParams || {} + ); const authorizationUrl = client.authorizationUrl(authParams); res.redirect(authorizationUrl); @@ -91,11 +92,11 @@ class ResponseContext { const res = this._res; let returnURL = params.returnTo || req.query.returnTo || this._config.postLogoutRedirectUri; - + if (url.parse(returnURL).host === null) { returnURL = urlJoin(this._config.baseURL, returnURL); } - + if (!req.isAuthenticated()) { return res.redirect(returnURL); } From ec08ef302c61d25bd6f71b7db81ff0876d66b9f2 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 27 Jan 2020 15:43:24 -0800 Subject: [PATCH 2/8] Failing tests for new post-login redirect handling --- test/callback_route_form_post.tests.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/callback_route_form_post.tests.js b/test/callback_route_form_post.tests.js index 50229c48..06977120 100644 --- a/test/callback_route_form_post.tests.js +++ b/test/callback_route_form_post.tests.js @@ -9,6 +9,7 @@ const expressOpenid = require('..'); const server = require('./fixture/server'); const cert = require('./fixture/cert'); const clientID = '__test_client_id__'; +const expectedDefaultState = Buffer('{returnTo:"https://example.org"}').toString('base64'); function testCase(params) { return () => { @@ -184,8 +185,7 @@ describe('callback routes response_type: id_token, response_mode: form_post', fu describe('when nonce is missing from cookies', testCase({ cookies: { - state: '__test_state__', - returnTo: '/return-to' + state: '__test_state__' }, body: { state: '__test_state__', @@ -200,12 +200,11 @@ describe('callback routes response_type: id_token, response_mode: form_post', fu describe('when id_token is valid', testCase({ cookies: { - _state: '__test_state__', - _nonce: '__test_nonce__', - _returnTo: '/return-to' + _state: expectedDefaultState, + _nonce: '__test_nonce__' }, body: { - state: '__test_state__', + state: expectedDefaultState, id_token: makeIdToken() }, assertions() { @@ -214,7 +213,7 @@ describe('callback routes response_type: id_token, response_mode: form_post', fu }); it('should redirect to the intended url', function() { - assert.equal(this.response.headers['location'], '/return-to'); + assert.equal(this.response.headers['location'], 'https://example.org'); }); it('should contain the claims in the current session', function() { From 6289919386b661a6d1ef2869dc7eced954923906 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 27 Jan 2020 14:35:40 -0800 Subject: [PATCH 3/8] Add ability to generate custom login state Added the config key getLoginState to accept a function for generating custom state values. By default, this will store the post-callback redirect URL and a nonce to ensure unique values. This can be used to store information about the request before redirecting to login. --- index.d.ts | 5 +++++ lib/config.js | 2 ++ lib/context.js | 22 ++++++++----------- lib/hooks/getLoginState.js | 43 ++++++++++++++++++++++++++++++++++++++ lib/transientHandler.js | 8 +++---- middleware/auth.js | 15 +++++++++++-- 6 files changed, 76 insertions(+), 19 deletions(-) create mode 100644 lib/hooks/getLoginState.js diff --git a/index.d.ts b/index.d.ts index 2b51cc2c..8436dcf6 100644 --- a/index.d.ts +++ b/index.d.ts @@ -68,6 +68,11 @@ interface ConfigParams { */ errorOnRequiredAuth?: boolean; + /** + * Function that returns a transient state value for `res.openid.login()`. + */ + getLoginState?: (req: Request, config: object) => string; + /** * Function that returns the profile for `req.openid.user`. */ diff --git a/lib/config.js b/lib/config.js index b01a856f..d709af85 100644 --- a/lib/config.js +++ b/lib/config.js @@ -1,5 +1,6 @@ const Joi = require('@hapi/joi'); const clone = require('clone'); +const { generate: getLoginState } = require('./hooks/getLoginState'); const getUser = require('./hooks/getUser'); const handleCallback = require('./hooks/handleCallback'); @@ -54,6 +55,7 @@ const paramsSchema = Joi.object({ ), clockTolerance: Joi.number().optional().default(60), errorOnRequiredAuth: Joi.boolean().optional().default(false), + getLoginState: Joi.function().optional().default(() => getLoginState), getUser: Joi.function().optional().default(() => getUser), handleCallback: Joi.function().optional().default(() => handleCallback), httpOptions: Joi.object().optional(), diff --git a/lib/context.js b/lib/context.js index b3f2b2f6..bd681fb3 100644 --- a/lib/context.js +++ b/lib/context.js @@ -1,9 +1,10 @@ const cb = require('cb'); const url = require('url'); const urlJoin = require('url-join'); +const { TokenSet } = require('openid-client'); + const transient = require('./transientHandler'); const { get: getClient } = require('./client'); -const { TokenSet } = require('openid-client'); class RequestContext { constructor(config, req, res, next) { @@ -53,30 +54,25 @@ class ResponseContext { const config = this._config; const client = req.openid.client; - // Determine landing page after login callback. - let returnTo = this._config.baseURL; - if (options.returnTo) { - returnTo = options.returnTo; - } else if (req.method === 'GET' && req.originalUrl) { - returnTo = req.originalUrl; - } + options = Object.assign({returnTo: this._config.baseURL, authorizationParams: {}}, options); + options.authorizationParams = Object.assign({}, config.authorizationParams, options.authorizationParams); const transientOpts = { legacySameSiteCookie: config.legacySameSiteCookie, - sameSite: config.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' + sameSite: options.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' }; + const stateTransientOpts = Object.assign({}, transientOpts, {value: config.getLoginState(req, options)}); + try { - transient.store('returnTo', res, Object.assign({value: returnTo}, transientOpts)); const authParams = Object.assign( { nonce: transient.store('nonce', res, transientOpts), - state: transient.store('state', res, transientOpts), + state: transient.store('state', res, stateTransientOpts), redirect_uri: this.getRedirectUri() }, - config.authorizationParams, - options.authorizationParams || {} + options.authorizationParams ); const authorizationUrl = client.authorizationUrl(authParams); diff --git a/lib/hooks/getLoginState.js b/lib/hooks/getLoginState.js new file mode 100644 index 00000000..62e3b2a0 --- /dev/null +++ b/lib/hooks/getLoginState.js @@ -0,0 +1,43 @@ +const { createNonce } = require('../transientHandler'); +const { encode: base64encode, decode: base64decode } = require('base64url'); + +module.exports.generate = generateLoginState; +module.exports.prepare = prepareLoginState; +module.exports.decode = decodeLoginState; + +/** + * Generate a unique state value for use during login transactions. + * + * @param {RequestHandler} req + * @param {object} options + */ +function generateLoginState(req, options) { + let state = { + returnTo: options.returnTo, + nonce: createNonce() + }; + + if (req.method === 'GET' && req.originalUrl) { + state.returnTo = req.originalUrl; + } + + return prepareLoginState(state); +} + +/** + * Prepare a state object to send. + * + * @param {object} stateObject + */ +function prepareLoginState(stateObject) { + return base64encode(JSON.stringify(stateObject)); +} + +/** + * Decode a state value. + * + * @param {string} state + */ +function decodeLoginState(state) { + return JSON.parse(base64decode(state)); +} diff --git a/lib/transientHandler.js b/lib/transientHandler.js index ee27b5ac..69083264 100644 --- a/lib/transientHandler.js +++ b/lib/transientHandler.js @@ -1,5 +1,9 @@ const crypto = require('crypto'); +exports.store = store; +exports.getOnce = getOnce; +exports.createNonce = createNonce; + /** * Set a cookie with a value or a generated nonce. * @@ -84,7 +88,3 @@ function createNonce() { function deleteCookie(name, res) { res.cookie(name, '', {maxAge: 0}); } - -exports.store = store; -exports.getOnce = getOnce; -exports.createNonce = createNonce; diff --git a/middleware/auth.js b/middleware/auth.js index d63af041..7c98da9a 100644 --- a/middleware/auth.js +++ b/middleware/auth.js @@ -9,6 +9,7 @@ const requiresAuth = require('./requiresAuth'); const transient = require('../lib/transientHandler'); const { RequestContext, ResponseContext } = require('../lib/context'); const appSession = require('../lib/appSession'); +const {decode: decodeLoginState} = require('../lib/hooks/getLoginState'); const enforceLeadingSlash = (path) => { return '/' === path.split('')[0] ? path : '/' + path; @@ -83,13 +84,15 @@ module.exports = function (params) { const redirectUri = res.openid.getRedirectUri(); const client = req.openid.client; + req.openidState = transient.getOnce('state', req, res, transientOpts); + let tokenSet; try { const callbackParams = client.callbackParams(req); tokenSet = await client.callback(redirectUri, callbackParams, { nonce: transient.getOnce('nonce', req, res, transientOpts), - state: transient.getOnce('state', req, res, transientOpts), + state: req.openidState, response_type: authorizeParams.response_type, }); } catch (err) { @@ -115,7 +118,15 @@ module.exports = function (params) { }, config.handleCallback, function (req, res) { - const returnTo = transient.getOnce('returnTo', req, res, transientOpts) || config.baseURL; + + let stateDecoded; + try { + stateDecoded = decodeLoginState(req.openidState); + } catch (err) { + stateDecoded = {}; + } + + const returnTo = stateDecoded.returnTo || config.baseURL; res.redirect(returnTo); } ); From 1a5e6e995708dda0d07dda191d4018712afd2515 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 27 Jan 2020 17:21:19 -0800 Subject: [PATCH 4/8] Add tests for custom state functionality --- lib/hooks/getLoginState.js | 8 +-- test/auth.tests.js | 135 ++++++++++++++++++++++++------------- 2 files changed, 92 insertions(+), 51 deletions(-) diff --git a/lib/hooks/getLoginState.js b/lib/hooks/getLoginState.js index 62e3b2a0..98b0c160 100644 --- a/lib/hooks/getLoginState.js +++ b/lib/hooks/getLoginState.js @@ -12,15 +12,11 @@ module.exports.decode = decodeLoginState; * @param {object} options */ function generateLoginState(req, options) { - let state = { - returnTo: options.returnTo, + const state = { + returnTo: options.returnTo || req.originalUrl, nonce: createNonce() }; - if (req.method === 'GET' && req.originalUrl) { - state.returnTo = req.originalUrl; - } - return prepareLoginState(state); } diff --git a/test/auth.tests.js b/test/auth.tests.js index 3fa81466..bb31fa9c 100644 --- a/test/auth.tests.js +++ b/test/auth.tests.js @@ -6,6 +6,7 @@ const request = require('request-promise-native').defaults({ }); const expressOpenid = require('..'); +const { prepare: encodeState, decode: decodeState } = require('../lib/hooks/getLoginState'); const server = require('./fixture/server'); const filterRoute = (method, path) => { return r => r.route && @@ -29,19 +30,24 @@ const getCookieFromResponse = (res, cookieName) => { return cookieValuePart.split('=')[1]; }; +const defaultConfig = { + appSessionSecret: '__test_session_secret__', + clientID: '__test_client_id__', + baseURL: 'https://example.org', + issuerBaseURL: 'https://test.auth0.com', + required: false +}; + +function getRouter (customConfig) { + return expressOpenid.auth(Object.assign({}, defaultConfig, customConfig)); +} + describe('auth', function() { describe('default', () => { - let baseUrl, router; before(async function() { - router = expressOpenid.auth({ - appSessionSecret: '__test_session_secret__', - clientID: '__test_client_id__', - baseURL: 'https://example.org', - issuerBaseURL: 'https://test.auth0.com', - required: false - }); + router = getRouter(); baseUrl = await server.create(router); }); @@ -86,17 +92,10 @@ describe('auth', function() { let baseUrl, router; before(async function() { - router = expressOpenid.auth({ - appSessionSecret: '__test_session_secret__', - clientID: '__test_client_id__', - baseURL: 'https://example.org', - issuerBaseURL: 'https://test.auth0.com', - authorizationParams: { - response_mode: undefined, - response_type: 'none', - }, - required: false - }); + router = getRouter({authorizationParams: { + response_mode: undefined, + response_type: 'none', + }}); baseUrl = await server.create(router); }); @@ -124,16 +123,11 @@ describe('auth', function() { }); describe('response_type=code', () => { - let baseUrl; - let router; + let baseUrl, router; before(async function() { - router = expressOpenid.auth({ - appSessionSecret: '__test_session_secret__', - clientID: '__test_client_id__', + router = getRouter({ clientSecret: '__test_client_secret__', - baseURL: 'https://example.org', - issuerBaseURL: 'https://test.auth0.com', authorizationParams: { response_mode: undefined, response_type: 'code', @@ -142,7 +136,6 @@ describe('auth', function() { baseUrl = await server.create(router); }); - it('should redirect to the authorize url properly on /login', async function() { const cookieJar = request.jar(); const res = await request.get('/login', { cookieJar, baseUrl, followRedirect: false }); @@ -171,20 +164,13 @@ describe('auth', function() { }); describe('response_type=id_token', () => { - let router; - let baseUrl; + let baseUrl, router; before(async function() { - router = expressOpenid.auth({ - appSessionSecret: '__test_session_secret__', - clientID: '__test_client_id__', - baseURL: 'https://example.org', - issuerBaseURL: 'https://test.auth0.com', - authorizationParams: { - response_mode: undefined, - response_type: 'id_token', - } - }); + router = getRouter({authorizationParams: { + response_mode: undefined, + response_type: 'id_token', + }}); baseUrl = await server.create(router); }); @@ -214,19 +200,15 @@ describe('auth', function() { }); describe('custom path values', () => { - let baseUrl, router; before(async function() { - router = expressOpenid.auth({ - appSessionSecret: '__test_session_secret__', - clientID: '__test_client_id__', - baseURL: 'https://example.org', - issuerBaseURL: 'https://test.auth0.com', + router = getRouter({ redirectUriPath: 'custom-callback', loginPath: 'custom-login', logoutPath: 'custom-logout', }); + baseUrl = await server.create(router); }); @@ -254,4 +236,67 @@ describe('auth', function() { }); }); + + describe('custom login parameter values', () => { + + it('should redirect to the authorize url properly on /login', async function() { + const router = getRouter({routes: false}); + router.get('/login', (req, res) => res.openid.login({ + returnTo: 'https://example.org/custom-redirect', + authorizationParams: { + response_type: 'code', + response_mode: 'query', + scope: 'openid email', + } + })); + const baseUrl = await server.create(router); + + const cookieJar = request.jar(); + const res = await request.get('/login', { cookieJar, baseUrl, followRedirect: false }); + assert.equal(res.statusCode, 302); + + const parsed = url.parse(res.headers.location, true); + + assert.equal(parsed.hostname, 'test.auth0.com'); + assert.equal(parsed.pathname, '/authorize'); + assert.equal(parsed.query.scope, 'openid email'); + assert.equal(parsed.query.response_type, 'code'); + assert.equal(parsed.query.response_mode, 'query'); + assert.equal(parsed.query.redirect_uri, 'https://example.org/callback'); + assert.property(parsed.query, 'nonce'); + + const decodedState = decodeState(parsed.query.state); + + assert.equal(decodedState.returnTo, 'https://example.org/custom-redirect'); + assert.isTrue(decodedState.nonce.length >= 16); + assert.notEqual(decodedState.nonce, parsed.query.nonce); + }); + + }); + + describe('custom state building', () => { + + it('should use a custom state builder', async function() { + const router = getRouter({getLoginState: (req, opts) => { + return encodeState({ + returnTo: opts.returnTo + '/custom-page', + nonce: '__test_nonce__', + customProp: '__test_custom_prop__', + }); + }}); + const baseUrl = await server.create(router); + + const cookieJar = request.jar(); + const res = await request.get('/login', { cookieJar, baseUrl, followRedirect: false }); + assert.equal(res.statusCode, 302); + + const parsed = url.parse(res.headers.location, true); + const decodedState = decodeState(parsed.query.state); + + assert.equal(decodedState.returnTo, 'https://example.org/custom-page'); + assert.equal(decodedState.nonce, '__test_nonce__'); + assert.equal(decodedState.customProp, '__test_custom_prop__'); + }); + + }); }); From 0d3f7bc450884fa3278436aecd38d4e491d873cb Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 27 Jan 2020 20:51:01 -0800 Subject: [PATCH 5/8] Add state example and move encode/decode to RequestHandler --- EXAMPLES.md | 28 ++++++++++++++++++++++++++++ lib/context.js | 19 +++++++++++++++++++ lib/hooks/getLoginState.js | 23 +---------------------- middleware/auth.js | 3 +-- test/auth.tests.js | 26 ++++++++++++++++---------- 5 files changed, 65 insertions(+), 34 deletions(-) diff --git a/EXAMPLES.md b/EXAMPLES.md index 0724c4d7..d9700d09 100644 --- a/EXAMPLES.md +++ b/EXAMPLES.md @@ -231,3 +231,31 @@ app.use(auth({ } })); ``` + +## 8. Custom state handling + +If your application needs to keep track of the request state before redirecting to log in, you can use the built-in state handling. By default, this library stores the post-callback redirect URL in a state object (along with a generated nonce) that is converted to a string, base64 encoded, and verified during callback (see [our documentation](https://auth0.com/docs/protocols/oauth2/oauth-state) for general information about this parameter). + +You can define a `getLoginState` configuration key set to a function that takes an Express `RequestHandler` and an options object and returned a URL-safe string representation: + +```js +const crypto = require('crypto'); + +app.use(auth({ + getLoginState: function (req, options) { + const state = { + // Property used by the library for redirecting after logging in. + returnTo: '/custom-return-path', + // Required to make sure the state parameter can't be replayed. + nonce: crypto.randomBytes(32).toString('hex'); + // Additional properties as needed. + customProperty: req.someProperty, + }; + return req.openid.encodeState(state); + }, + handleCallback: function (req, res, next) { + const decodedState = req.openid.decodeState(eq.openidState); + // decodedState.customProperty now available to use. + } +})); +``` diff --git a/lib/context.js b/lib/context.js index bd681fb3..2951f998 100644 --- a/lib/context.js +++ b/lib/context.js @@ -2,6 +2,7 @@ const cb = require('cb'); const url = require('url'); const urlJoin = require('url-join'); const { TokenSet } = require('openid-client'); +const { encode: base64encode, decode: base64decode } = require('base64url'); const transient = require('./transientHandler'); const { get: getClient } = require('./client'); @@ -29,6 +30,24 @@ class RequestContext { this.user = await this._config.getUser(this._req, this._config); } + + /** + * Prepare a state object to send. + * + * @param {object} stateObject + */ + encodeState(stateObject) { + return base64encode(JSON.stringify(stateObject)); + } + + /** + * Decode a state value. + * + * @param {string} state + */ + decodeState(state) { + return JSON.parse(base64decode(state)); + } } class ResponseContext { diff --git a/lib/hooks/getLoginState.js b/lib/hooks/getLoginState.js index 98b0c160..d5353730 100644 --- a/lib/hooks/getLoginState.js +++ b/lib/hooks/getLoginState.js @@ -1,9 +1,6 @@ const { createNonce } = require('../transientHandler'); -const { encode: base64encode, decode: base64decode } = require('base64url'); module.exports.generate = generateLoginState; -module.exports.prepare = prepareLoginState; -module.exports.decode = decodeLoginState; /** * Generate a unique state value for use during login transactions. @@ -17,23 +14,5 @@ function generateLoginState(req, options) { nonce: createNonce() }; - return prepareLoginState(state); -} - -/** - * Prepare a state object to send. - * - * @param {object} stateObject - */ -function prepareLoginState(stateObject) { - return base64encode(JSON.stringify(stateObject)); -} - -/** - * Decode a state value. - * - * @param {string} state - */ -function decodeLoginState(state) { - return JSON.parse(base64decode(state)); + return req.openid.encodeState(state); } diff --git a/middleware/auth.js b/middleware/auth.js index 7c98da9a..59369196 100644 --- a/middleware/auth.js +++ b/middleware/auth.js @@ -126,8 +126,7 @@ module.exports = function (params) { stateDecoded = {}; } - const returnTo = stateDecoded.returnTo || config.baseURL; - res.redirect(returnTo); + res.redirect(stateDecoded.returnTo || config.baseURL); } ); diff --git a/test/auth.tests.js b/test/auth.tests.js index bb31fa9c..24da9525 100644 --- a/test/auth.tests.js +++ b/test/auth.tests.js @@ -6,8 +6,8 @@ const request = require('request-promise-native').defaults({ }); const expressOpenid = require('..'); -const { prepare: encodeState, decode: decodeState } = require('../lib/hooks/getLoginState'); const server = require('./fixture/server'); + const filterRoute = (method, path) => { return r => r.route && r.route.path === path && @@ -240,15 +240,19 @@ describe('auth', function() { describe('custom login parameter values', () => { it('should redirect to the authorize url properly on /login', async function() { + let decodeState; const router = getRouter({routes: false}); - router.get('/login', (req, res) => res.openid.login({ - returnTo: 'https://example.org/custom-redirect', - authorizationParams: { - response_type: 'code', - response_mode: 'query', - scope: 'openid email', - } - })); + router.get('/login', (req, res) => { + decodeState = req.openid.decodeState; + res.openid.login({ + returnTo: 'https://example.org/custom-redirect', + authorizationParams: { + response_type: 'code', + response_mode: 'query', + scope: 'openid email', + } + }); + }); const baseUrl = await server.create(router); const cookieJar = request.jar(); @@ -277,8 +281,10 @@ describe('auth', function() { describe('custom state building', () => { it('should use a custom state builder', async function() { + let decodeState; const router = getRouter({getLoginState: (req, opts) => { - return encodeState({ + decodeState = req.openid.decodeState; + return req.openid.encodeState({ returnTo: opts.returnTo + '/custom-page', nonce: '__test_nonce__', customProp: '__test_custom_prop__', From ed683244f48b37ef4595b8f58f69e7972b41f03d Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Tue, 28 Jan 2020 10:09:33 -0800 Subject: [PATCH 6/8] Clean up req.openid.login and expand example --- EXAMPLES.md | 8 +++++--- index.d.ts | 2 +- lib/context.js | 32 ++++++++++++++++++++------------ 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/EXAMPLES.md b/EXAMPLES.md index d9700d09..46c12b3a 100644 --- a/EXAMPLES.md +++ b/EXAMPLES.md @@ -234,9 +234,9 @@ app.use(auth({ ## 8. Custom state handling -If your application needs to keep track of the request state before redirecting to log in, you can use the built-in state handling. By default, this library stores the post-callback redirect URL in a state object (along with a generated nonce) that is converted to a string, base64 encoded, and verified during callback (see [our documentation](https://auth0.com/docs/protocols/oauth2/oauth-state) for general information about this parameter). +If your application needs to keep track of the request state before redirecting to log in, you can use the built-in state handling. By default, this library stores the post-callback redirect URL in a state object (along with a generated nonce) that is converted to a string, base64 encoded, and verified during callback (see [our documentation](https://auth0.com/docs/protocols/oauth2/oauth-state) for general information about this parameter). This state object can be added to and used during callback. -You can define a `getLoginState` configuration key set to a function that takes an Express `RequestHandler` and an options object and returned a URL-safe string representation: +You can define a `getLoginState` configuration key set to a function that takes an Express `RequestHandler` and an options object and returns a URL-safe string representation: ```js const crypto = require('crypto'); @@ -251,11 +251,13 @@ app.use(auth({ // Additional properties as needed. customProperty: req.someProperty, }; + // This value will be sent in a URL parameter so it should be transfer-safe. return req.openid.encodeState(state); }, handleCallback: function (req, res, next) { const decodedState = req.openid.decodeState(eq.openidState); - // decodedState.customProperty now available to use. + // The decodedState.customProperty is now available to use. + // Call next() to redirect to decodedState.returnTo. } })); ``` diff --git a/index.d.ts b/index.d.ts index 8436dcf6..4ca9b648 100644 --- a/index.d.ts +++ b/index.d.ts @@ -69,7 +69,7 @@ interface ConfigParams { errorOnRequiredAuth?: boolean; /** - * Function that returns a transient state value for `res.openid.login()`. + * Function that returns a URL-safe state value for `res.openid.login()`. */ getLoginState?: (req: Request, config: object) => string; diff --git a/lib/context.js b/lib/context.js index 2951f998..68182131 100644 --- a/lib/context.js +++ b/lib/context.js @@ -73,26 +73,34 @@ class ResponseContext { const config = this._config; const client = req.openid.client; - options = Object.assign({returnTo: this._config.baseURL, authorizationParams: {}}, options); - options.authorizationParams = Object.assign({}, config.authorizationParams, options.authorizationParams); + options = { + returnTo: this._config.baseURL, + authorizationParams: {}, + ...options + }; + + options.authorizationParams = { + redirect_uri: this.getRedirectUri(), + ...config.authorizationParams, + ...options.authorizationParams + }; const transientOpts = { legacySameSiteCookie: config.legacySameSiteCookie, sameSite: options.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' }; - const stateTransientOpts = Object.assign({}, transientOpts, {value: config.getLoginState(req, options)}); + const stateTransientOpts = { + ...transientOpts, + value: config.getLoginState(req, options) + }; try { - - const authParams = Object.assign( - { - nonce: transient.store('nonce', res, transientOpts), - state: transient.store('state', res, stateTransientOpts), - redirect_uri: this.getRedirectUri() - }, - options.authorizationParams - ); + const authParams = { + ...options.authorizationParams, + nonce: transient.store('nonce', res, transientOpts), + state: transient.store('state', res, stateTransientOpts) + }; const authorizationUrl = client.authorizationUrl(authParams); res.redirect(authorizationUrl); From 2e0379bdf609d9132ea48321fff2f898a57c1605 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 10 Feb 2020 13:40:13 -0800 Subject: [PATCH 7/8] Encode and decode state by default. --- EXAMPLES.md | 21 ++++++++-------- lib/context.js | 27 ++++++--------------- lib/hooks/getLoginState.js | 33 ++++++++++++++++++++++---- middleware/auth.js | 14 +++-------- test/auth.tests.js | 12 ++++------ test/callback_route_form_post.tests.js | 11 +++++---- 6 files changed, 58 insertions(+), 60 deletions(-) diff --git a/EXAMPLES.md b/EXAMPLES.md index 46c12b3a..4083b064 100644 --- a/EXAMPLES.md +++ b/EXAMPLES.md @@ -236,28 +236,27 @@ app.use(auth({ If your application needs to keep track of the request state before redirecting to log in, you can use the built-in state handling. By default, this library stores the post-callback redirect URL in a state object (along with a generated nonce) that is converted to a string, base64 encoded, and verified during callback (see [our documentation](https://auth0.com/docs/protocols/oauth2/oauth-state) for general information about this parameter). This state object can be added to and used during callback. -You can define a `getLoginState` configuration key set to a function that takes an Express `RequestHandler` and an options object and returns a URL-safe string representation: +You can define a `getLoginState` configuration key set to a function that takes an Express `RequestHandler` and an options object and returns a plain object: ```js -const crypto = require('crypto'); - app.use(auth({ getLoginState: function (req, options) { - const state = { + // This object will be stringified and base64 URL-safe encoded. + return { // Property used by the library for redirecting after logging in. returnTo: '/custom-return-path', - // Required to make sure the state parameter can't be replayed. - nonce: crypto.randomBytes(32).toString('hex'); // Additional properties as needed. customProperty: req.someProperty, }; - // This value will be sent in a URL parameter so it should be transfer-safe. - return req.openid.encodeState(state); }, handleCallback: function (req, res, next) { - const decodedState = req.openid.decodeState(eq.openidState); - // The decodedState.customProperty is now available to use. - // Call next() to redirect to decodedState.returnTo. + // The req.openidState.customProperty is now available to use. + if ( req.openidState.customProperty ) { + // Do something ... + } + + // Call next() to redirect to req.openidState.returnTo. + next(); } })); ``` diff --git a/lib/context.js b/lib/context.js index 68182131..c2a2071d 100644 --- a/lib/context.js +++ b/lib/context.js @@ -2,10 +2,10 @@ const cb = require('cb'); const url = require('url'); const urlJoin = require('url-join'); const { TokenSet } = require('openid-client'); -const { encode: base64encode, decode: base64decode } = require('base64url'); const transient = require('./transientHandler'); const { get: getClient } = require('./client'); +const { encodeState } = require('../lib/hooks/getLoginState'); class RequestContext { constructor(config, req, res, next) { @@ -30,24 +30,6 @@ class RequestContext { this.user = await this._config.getUser(this._req, this._config); } - - /** - * Prepare a state object to send. - * - * @param {object} stateObject - */ - encodeState(stateObject) { - return base64encode(JSON.stringify(stateObject)); - } - - /** - * Decode a state value. - * - * @param {string} state - */ - decodeState(state) { - return JSON.parse(base64decode(state)); - } } class ResponseContext { @@ -73,12 +55,14 @@ class ResponseContext { const config = this._config; const client = req.openid.client; + // Set default returnTo value, allow passed-in options to override. options = { returnTo: this._config.baseURL, authorizationParams: {}, ...options }; + // Ensure a redirect_uri, merge in configuration options, then passed-in options. options.authorizationParams = { redirect_uri: this.getRedirectUri(), ...config.authorizationParams, @@ -90,9 +74,12 @@ class ResponseContext { sameSite: options.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' }; + let stateValue = config.getLoginState(req, options); + stateValue.nonce = transient.createNonce(); + const stateTransientOpts = { ...transientOpts, - value: config.getLoginState(req, options) + value: encodeState(stateValue) }; try { diff --git a/lib/hooks/getLoginState.js b/lib/hooks/getLoginState.js index d5353730..5bebe129 100644 --- a/lib/hooks/getLoginState.js +++ b/lib/hooks/getLoginState.js @@ -1,18 +1,41 @@ -const { createNonce } = require('../transientHandler'); +const { encode: base64encode, decode: base64decode } = require('base64url'); module.exports.generate = generateLoginState; +module.exports.encodeState = encodeState; +module.exports.decodeState = decodeState; /** * Generate a unique state value for use during login transactions. * * @param {RequestHandler} req * @param {object} options + * + * @return {object} */ function generateLoginState(req, options) { - const state = { - returnTo: options.returnTo || req.originalUrl, - nonce: createNonce() + return { + returnTo: options.returnTo || req.originalUrl }; +} + +/** + * Prepare a state object to send. + * + * @param {object} stateObject + * + * @return {string} + */ +function encodeState(stateObject) { + return base64encode(JSON.stringify(stateObject)); +} - return req.openid.encodeState(state); +/** + * Decode a state value. + * + * @param {string} stateValue + * + * @return {object} + */ +function decodeState(stateValue) { + return JSON.parse(base64decode(stateValue)); } diff --git a/middleware/auth.js b/middleware/auth.js index 59369196..e491e88c 100644 --- a/middleware/auth.js +++ b/middleware/auth.js @@ -9,7 +9,7 @@ const requiresAuth = require('./requiresAuth'); const transient = require('../lib/transientHandler'); const { RequestContext, ResponseContext } = require('../lib/context'); const appSession = require('../lib/appSession'); -const {decode: decodeLoginState} = require('../lib/hooks/getLoginState'); +const { decodeState } = require('../lib/hooks/getLoginState'); const enforceLeadingSlash = (path) => { return '/' === path.split('')[0] ? path : '/' + path; @@ -87,7 +87,6 @@ module.exports = function (params) { req.openidState = transient.getOnce('state', req, res, transientOpts); let tokenSet; - try { const callbackParams = client.callbackParams(req); tokenSet = await client.callback(redirectUri, callbackParams, { @@ -99,6 +98,7 @@ module.exports = function (params) { throw createError.BadRequest(err.message); } + req.openidState = decodeState(req.openidState); req.openidTokens = tokenSet; if (config.appSessionSecret) { @@ -118,15 +118,7 @@ module.exports = function (params) { }, config.handleCallback, function (req, res) { - - let stateDecoded; - try { - stateDecoded = decodeLoginState(req.openidState); - } catch (err) { - stateDecoded = {}; - } - - res.redirect(stateDecoded.returnTo || config.baseURL); + res.redirect(req.openidState.returnTo || config.baseURL); } ); diff --git a/test/auth.tests.js b/test/auth.tests.js index 24da9525..c59122a4 100644 --- a/test/auth.tests.js +++ b/test/auth.tests.js @@ -5,6 +5,8 @@ const request = require('request-promise-native').defaults({ resolveWithFullResponse: true }); +const { decodeState } = require('../lib/hooks/getLoginState'); + const expressOpenid = require('..'); const server = require('./fixture/server'); @@ -240,10 +242,8 @@ describe('auth', function() { describe('custom login parameter values', () => { it('should redirect to the authorize url properly on /login', async function() { - let decodeState; const router = getRouter({routes: false}); router.get('/login', (req, res) => { - decodeState = req.openid.decodeState; res.openid.login({ returnTo: 'https://example.org/custom-redirect', authorizationParams: { @@ -281,14 +281,11 @@ describe('auth', function() { describe('custom state building', () => { it('should use a custom state builder', async function() { - let decodeState; const router = getRouter({getLoginState: (req, opts) => { - decodeState = req.openid.decodeState; - return req.openid.encodeState({ + return { returnTo: opts.returnTo + '/custom-page', - nonce: '__test_nonce__', customProp: '__test_custom_prop__', - }); + }; }}); const baseUrl = await server.create(router); @@ -300,7 +297,6 @@ describe('auth', function() { const decodedState = decodeState(parsed.query.state); assert.equal(decodedState.returnTo, 'https://example.org/custom-page'); - assert.equal(decodedState.nonce, '__test_nonce__'); assert.equal(decodedState.customProp, '__test_custom_prop__'); }); diff --git a/test/callback_route_form_post.tests.js b/test/callback_route_form_post.tests.js index 06977120..2d069ea8 100644 --- a/test/callback_route_form_post.tests.js +++ b/test/callback_route_form_post.tests.js @@ -5,11 +5,12 @@ const request = require('request-promise-native').defaults({ resolveWithFullResponse: true }); +const { encodeState } = require('../lib/hooks/getLoginState'); const expressOpenid = require('..'); const server = require('./fixture/server'); const cert = require('./fixture/cert'); const clientID = '__test_client_id__'; -const expectedDefaultState = Buffer('{returnTo:"https://example.org"}').toString('base64'); +const expectedDefaultState = encodeState({ returnTo: 'https://example.org' }); function testCase(params) { return () => { @@ -272,11 +273,11 @@ describe('callback routes response_type: id_token, response_mode: form_post', fu } }, cookies: { - _state: '__test_state__', + _state: expectedDefaultState, _nonce: '__test_nonce__' }, body: { - state: '__test_state__', + state: expectedDefaultState, id_token: makeIdToken() }, assertions() { @@ -291,11 +292,11 @@ describe('callback routes response_type: id_token, response_mode: form_post', fu identityClaimFilter: [] }, cookies: { - _state: '__test_state__', + _state: expectedDefaultState, _nonce: '__test_nonce__' }, body: { - state: '__test_state__', + state: expectedDefaultState, id_token: makeIdToken() }, assertions() { From 4edab0592c4e30641127d823a22c54905277de51 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Mon, 10 Feb 2020 14:10:14 -0800 Subject: [PATCH 8/8] Throw error if custom state is incorrect type; adjust TS defs --- index.d.ts | 2 +- lib/config.js | 2 +- lib/context.js | 5 ++++- lib/hooks/getLoginState.js | 4 ++-- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/index.d.ts b/index.d.ts index 4ca9b648..f110c872 100644 --- a/index.d.ts +++ b/index.d.ts @@ -71,7 +71,7 @@ interface ConfigParams { /** * Function that returns a URL-safe state value for `res.openid.login()`. */ - getLoginState?: (req: Request, config: object) => string; + getLoginState?: (req: Request, config: object) => object; /** * Function that returns the profile for `req.openid.user`. diff --git a/lib/config.js b/lib/config.js index d709af85..24eee79e 100644 --- a/lib/config.js +++ b/lib/config.js @@ -1,6 +1,6 @@ const Joi = require('@hapi/joi'); const clone = require('clone'); -const { generate: getLoginState } = require('./hooks/getLoginState'); +const { defaultState: getLoginState } = require('./hooks/getLoginState'); const getUser = require('./hooks/getUser'); const handleCallback = require('./hooks/handleCallback'); diff --git a/lib/context.js b/lib/context.js index c2a2071d..dbfa6749 100644 --- a/lib/context.js +++ b/lib/context.js @@ -74,7 +74,10 @@ class ResponseContext { sameSite: options.authorizationParams.response_mode === 'form_post' ? 'None' : 'Lax' }; - let stateValue = config.getLoginState(req, options); + let stateValue = await config.getLoginState(req, options); + if ( typeof stateValue !== 'object' ) { + next(new Error( 'Custom state value must be an object.' )); + } stateValue.nonce = transient.createNonce(); const stateTransientOpts = { diff --git a/lib/hooks/getLoginState.js b/lib/hooks/getLoginState.js index 5bebe129..72e569d3 100644 --- a/lib/hooks/getLoginState.js +++ b/lib/hooks/getLoginState.js @@ -1,6 +1,6 @@ const { encode: base64encode, decode: base64decode } = require('base64url'); -module.exports.generate = generateLoginState; +module.exports.defaultState = defaultState; module.exports.encodeState = encodeState; module.exports.decodeState = decodeState; @@ -12,7 +12,7 @@ module.exports.decodeState = decodeState; * * @return {object} */ -function generateLoginState(req, options) { +function defaultState(req, options) { return { returnTo: options.returnTo || req.originalUrl };