From 1d1e3908195fe5a26fbaa778d0646e6b502ba197 Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Tue, 28 Jun 2022 16:43:20 +0100 Subject: [PATCH] discovery errors should be handled in express middleware --- lib/context.js | 112 ++++++++++++++++++++++--------------------- test/login.tests.js | 29 +++++++++++ test/logout.tests.js | 30 ++++++++++++ test/setup.js | 8 ++-- 4 files changed, 120 insertions(+), 59 deletions(-) diff --git a/lib/context.js b/lib/context.js index 0949773b..77ee5f3a 100644 --- a/lib/context.js +++ b/lib/context.js @@ -179,50 +179,52 @@ class ResponseContext { async login(options = {}) { let { config, req, res, next, transient } = weakRef(this); next = cb(next).once(); - const client = await getClient(config); - - // Set default returnTo value, allow passed-in options to override or use originalUrl on GET - let returnTo = config.baseURL; - if (options.returnTo) { - returnTo = options.returnTo; - debug('req.oidc.login() called with returnTo: %s', returnTo); - } else if (req.method === 'GET' && req.originalUrl) { - // Collapse any leading slashes to a single slash to prevent Open Redirects - returnTo = req.originalUrl.replace(/^\/+/, '/'); - debug('req.oidc.login() without returnTo, using: %s', returnTo); - } + try { + const client = await getClient(config); + + // Set default returnTo value, allow passed-in options to override or use originalUrl on GET + let returnTo = config.baseURL; + if (options.returnTo) { + returnTo = options.returnTo; + debug('req.oidc.login() called with returnTo: %s', returnTo); + } else if (req.method === 'GET' && req.originalUrl) { + // Collapse any leading slashes to a single slash to prevent Open Redirects + returnTo = req.originalUrl.replace(/^\/+/, '/'); + debug('req.oidc.login() without returnTo, using: %s', returnTo); + } - options = { - authorizationParams: {}, - returnTo, - ...options, - }; + options = { + authorizationParams: {}, + returnTo, + ...options, + }; - // Ensure a redirect_uri, merge in configuration options, then passed-in options. - options.authorizationParams = { - redirect_uri: this.getRedirectUri(), - ...config.authorizationParams, - ...options.authorizationParams, - }; + // Ensure a redirect_uri, merge in configuration options, then passed-in options. + options.authorizationParams = { + redirect_uri: this.getRedirectUri(), + ...config.authorizationParams, + ...options.authorizationParams, + }; - const stateValue = await config.getLoginState(req, options); - if (typeof stateValue !== 'object') { - next(new Error('Custom state value must be an object.')); - } - stateValue.nonce = transient.generateNonce(); - if (options.silent) { - stateValue.attemptingSilentLogin = true; - } + const stateValue = await config.getLoginState(req, options); + if (typeof stateValue !== 'object') { + next(new Error('Custom state value must be an object.')); + } + stateValue.nonce = transient.generateNonce(); + if (options.silent) { + stateValue.attemptingSilentLogin = true; + } - const usePKCE = options.authorizationParams.response_type.includes('code'); - if (usePKCE) { - debug( - 'response_type includes code, the authorization request will use PKCE' + const usePKCE = options.authorizationParams.response_type.includes( + 'code' ); - stateValue.code_verifier = transient.generateCodeVerifier(); - } + if (usePKCE) { + debug( + 'response_type includes code, the authorization request will use PKCE' + ); + stateValue.code_verifier = transient.generateCodeVerifier(); + } - try { const validResponseTypes = ['id_token', 'code id_token', 'code']; assert( validResponseTypes.includes(options.authorizationParams.response_type), @@ -275,31 +277,31 @@ class ResponseContext { async logout(params = {}) { let { config, req, res, next } = weakRef(this); next = cb(next).once(); - const client = await getClient(config); - let returnURL = params.returnTo || config.routes.postLogoutRedirect; debug('req.oidc.logout() with return url: %s', returnURL); - if (url.parse(returnURL).host === null) { - returnURL = urlJoin(config.baseURL, returnURL); - } + try { + const client = await getClient(config); - cancelSilentLogin(req, res); + if (url.parse(returnURL).host === null) { + returnURL = urlJoin(config.baseURL, returnURL); + } - if (!req.oidc.isAuthenticated()) { - debug('end-user already logged out, redirecting to %s', returnURL); - return res.redirect(returnURL); - } + cancelSilentLogin(req, res); - const { idToken: id_token_hint } = req.oidc; - req[config.session.name] = undefined; + if (!req.oidc.isAuthenticated()) { + debug('end-user already logged out, redirecting to %s', returnURL); + return res.redirect(returnURL); + } - if (!config.idpLogout) { - debug('performing a local only logout, redirecting to %s', returnURL); - return res.redirect(returnURL); - } + const { idToken: id_token_hint } = req.oidc; + req[config.session.name] = undefined; + + if (!config.idpLogout) { + debug('performing a local only logout, redirecting to %s', returnURL); + return res.redirect(returnURL); + } - try { returnURL = client.endSessionUrl({ ...config.logoutParams, ...params.logoutParams, diff --git a/test/login.tests.js b/test/login.tests.js index f28cc787..8daf31f3 100644 --- a/test/login.tests.js +++ b/test/login.tests.js @@ -1,6 +1,7 @@ const assert = require('chai').assert; const url = require('url'); const querystring = require('querystring'); +const nock = require('nock'); const request = require('request-promise-native').defaults({ simple: false, resolveWithFullResponse: true, @@ -419,4 +420,32 @@ describe('auth', () => { assert.include(fetchAuthCookie(res), 'SameSite=None'); }); + + it('should pass discovery errors to the express mw', async () => { + nock('https://example.com') + .get('/.well-known/openid-configuration') + .reply(500); + nock('https://example.com') + .get('/.well-known/oauth-authorization-server') + .reply(500); + + server = await createServer( + auth({ + ...defaultConfig, + issuerBaseURL: 'https://example.com', + }) + ); + const res = await request.get('/login', { + baseUrl, + followRedirect: false, + json: true, + }); + assert.equal(res.statusCode, 500); + console.log(res.body.err.message); + assert.match( + res.body.err.message, + /^Issuer.discover\(\) failed/, + 'Should get error json from server error middleware' + ); + }); }); diff --git a/test/logout.tests.js b/test/logout.tests.js index 163b65a5..f7d7b87a 100644 --- a/test/logout.tests.js +++ b/test/logout.tests.js @@ -1,3 +1,4 @@ +const nock = require('nock'); const { assert } = require('chai'); const { URL } = require('url'); const { create: createServer } = require('./fixture/server'); @@ -320,4 +321,33 @@ describe('logout route', async () => { assert.isFalse(url.searchParams.has('baz')); assert.equal(url.searchParams.get('qux'), ''); }); + + it('should pass discovery errors to the express mw', async () => { + nock('https://example.com') + .get('/.well-known/openid-configuration') + .reply(500); + nock('https://example.com') + .get('/.well-known/oauth-authorization-server') + .reply(500); + + server = await createServer( + auth({ + ...defaultConfig, + issuerBaseURL: 'https://example.com', + }) + ); + const res = await request.get({ + uri: '/logout', + baseUrl: 'http://localhost:3000', + followRedirect: false, + json: true, + }); + assert.equal(res.statusCode, 500); + console.log(res.body.err.message); + assert.match( + res.body.err.message, + /^Issuer.discover\(\) failed/, + 'Should get error json from server error middleware' + ); + }); }); diff --git a/test/setup.js b/test/setup.js index dc182014..8e88cd05 100644 --- a/test/setup.js +++ b/test/setup.js @@ -7,22 +7,22 @@ let warn; beforeEach(function () { warn = sinon.stub(global.console, 'warn'); - nock('https://op.example.com', { allowUnmocked: true }) + nock('https://op.example.com') .persist() .get('/.well-known/openid-configuration') .reply(200, wellKnown); - nock('https://op.example.com', { allowUnmocked: true }) + nock('https://op.example.com') .persist() .get('/.well-known/jwks.json') .reply(200, certs.jwks); - nock('https://test.eu.auth0.com', { allowUnmocked: true }) + nock('https://test.eu.auth0.com') .persist() .get('/.well-known/openid-configuration') .reply(200, { ...wellKnown, end_session_endpoint: undefined }); - nock('https://test.eu.auth0.com', { allowUnmocked: true }) + nock('https://test.eu.auth0.com') .persist() .get('/.well-known/jwks.json') .reply(200, certs.jwks);