diff --git a/CHANGELOG.md b/CHANGELOG.md index 342d228..7d9cea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 4.6 + +-[#53](https://github.com/okta/okta-oidc-middleware/pull/53) Fix: prevents open redirects + # 4.5.1 ### Bug Fixes diff --git a/src/oidcUtil.js b/src/oidcUtil.js index 0b60538..1a8597d 100644 --- a/src/oidcUtil.js +++ b/src/oidcUtil.js @@ -142,7 +142,8 @@ oidcUtil.ensureAuthenticated = (context, options = {}) => { if (negotiator.mediaType() === 'text/html') { if (!isAuthenticated) { if (req.session) { - req.session.returnTo = req.originalUrl || req.url; + // collapse any leading slashes to a single slash to prevent open redirects (OKTA-499372) + req.session.returnTo = (req.originalUrl || req.url).replace(/^\/+/, '/'); } let url = options.redirectTo; if (!url) { diff --git a/test/e2e/harness/server.js b/test/e2e/harness/server.js index b858baa..4ecba92 100644 --- a/test/e2e/harness/server.js +++ b/test/e2e/harness/server.js @@ -53,6 +53,10 @@ module.exports = class DemoServer { res.send(JSON.stringify(req.userContext)); }); + app.get('/*', oidc.ensureAuthenticated(), (req, res) => { + res.send(JSON.stringify(req.userContext)); + }); + return new Promise((resolve, reject) => { oidc.on('error', err => { console.log('Unable to start the server', err); diff --git a/test/e2e/page-objects/OktaSignInPage.js b/test/e2e/page-objects/OktaSignInPage.js index 38bc260..30807cb 100644 --- a/test/e2e/page-objects/OktaSignInPage.js +++ b/test/e2e/page-objects/OktaSignInPage.js @@ -24,6 +24,8 @@ module.exports = class OktaSignInPage { } async signIn({username, password}) { + await this.username.clear(); + await this.password.clear(); await this.username.sendKeys(username); await this.password.sendKeys(password); await this.submit.click(); diff --git a/test/e2e/page-objects/ProtectedPage.js b/test/e2e/page-objects/ProtectedPage.js index 92df6dc..17e1927 100644 --- a/test/e2e/page-objects/ProtectedPage.js +++ b/test/e2e/page-objects/ProtectedPage.js @@ -14,16 +14,17 @@ const constants = require('../util/constants'); const EC = protractor.ExpectedConditions; module.exports = class ProtectedPage { - constructor() { + constructor(path) { this.body = $('body'); + this.path = constants.BASE_URI + (path || '/protected'); } async load() { - await browser.get('/protected'); + await browser.get(this.path); } - async waitUntilVisible() { - await browser.wait(EC.urlIs(constants.BASE_URI + '/protected'), 10000, 'wait for protected url'); + async waitUntilVisible(path=this.path) { + await browser.wait(EC.urlIs(path), 10000, 'wait for protected url (' + path + ')'); } async getBodyText() { diff --git a/test/e2e/specs/basic.js b/test/e2e/specs/basic.js index e2f99e3..33e1165 100644 --- a/test/e2e/specs/basic.js +++ b/test/e2e/specs/basic.js @@ -16,6 +16,7 @@ const OktaSignInPage = require('../page-objects/OktaSignInPage'); const ProtectedPage = require('../page-objects/ProtectedPage'); const HomePage = require('../page-objects/HomePage'); + browser.waitForAngularEnabled(false); describe('Basic login redirect', () => { @@ -77,4 +78,42 @@ describe('Basic login redirect', () => { await privatePage.load(); await signInPage.waitUntilVisible(); }); + + it('should handle open redirect attempt gracefully', async () => { + // attempt to instigate an open redirect to okta.com + const path = '//okta.com' + const privatePage = new ProtectedPage(path); + await privatePage.load(); + + // we're not logged in, so we should redirect + const signInPage = new OktaSignInPage(); + await signInPage.waitUntilVisible(); + await signInPage.signIn({ + username: constants.USERNAME, + password: constants.PASSWORD + }); + + // wait for protected page to appear with contents + // NOTE: may see failure here if open redirect occurs (see OKTA-499372) + await privatePage.waitUntilVisible(constants.BASE_URI + path.slice(1)); // leading '/' will be stripped off + + expect(privatePage.getBodyText()).toContain('sub'); + + // Default response_type of library should contain an accessToken and idToken + expect(privatePage.getBodyText()).toContain('access_token'); + expect(privatePage.getBodyText()).toContain('id_token'); + + // navigate to home page + const homePage = new HomePage(); + await homePage.load(); + await homePage.waitUntilVisible(); + + expect(homePage.getBodyText()).toContain('Welcome home'); + + // navigate to Okta logout and follow redirects + await homePage.performLogout(); + await homePage.waitUntilVisible(); // after all redirects + + expect(browser.getPageSource()).not.toContain('Welcome home'); + }); }); diff --git a/test/unit/callback.spec.js b/test/unit/callback.spec.js index 30a1bf8..c3ef6e8 100644 --- a/test/unit/callback.spec.js +++ b/test/unit/callback.spec.js @@ -266,7 +266,5 @@ describe('callback', () => { resolve() }); }); - }) - - + }); }); \ No newline at end of file diff --git a/test/unit/oidcUtil.spec.js b/test/unit/oidcUtil.spec.js index 4070bd3..c584421 100644 --- a/test/unit/oidcUtil.spec.js +++ b/test/unit/oidcUtil.spec.js @@ -144,5 +144,22 @@ describe('oidcUtil', function () { requestHandler(req, res, () => {}); expect(res.redirect).toBeCalledWith('http://localhost:56789/foo'); }); + + it('strips leading slashes to prevent open redirects', () => { + // see OKTA-499372 + const requestHandler = oidcUtil.ensureAuthenticated({}, { + redirectTo: '/login' + }); + const req = { + session: {}, + url: '//okta.com' + }; + const res = { + redirect: jest.fn() + }; + requestHandler(req, res, () => {}); + expect(res.redirect).toHaveBeenCalledWith('/login'); + expect(req.session.returnTo).toBe('/okta.com'); + }); }); }) diff --git a/yarn.lock b/yarn.lock index f4e96a5..c770a25 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4238,7 +4238,7 @@ stack-utils@^2.0.3: resolved "https://registry.yarnpkg.com/statuses/-/statuses-1.5.0.tgz#161c7dac177659fd9811f43771fa99381478628c" integrity sha1-Fhx9rBd2Wf2YEfQ3cfqZOBR4Yow= -string-length@^4.0.1, string-length@^4.0.2: +string-length@^4.0.1: version "4.0.2" resolved "https://registry.yarnpkg.com/string-length/-/string-length-4.0.2.tgz#a8a8dc7bd5c1a82b9b3c8b87e125f66871b6e57a" integrity sha512-+l6rNN5fYHNhZZy41RXsYptCjA2Igmq4EG7kZAYFQI1E1VTXarr6ZPXBg6eq7Y6eK4FEhY6AJlyuFIb/v/S0VQ==