Prevent open redirects

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

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) {

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);

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();

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() {

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');
+  });
 });

test/unit/callback.spec.js

@@ -266,7 +266,5 @@ describe('callback', () => {
           resolve()
         });
     });  
-  })
-
-
+  });
 });

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');
+    });
   });
 })

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==