fix(security): mitigate the "Open Redirect Vulnerability"

Jonathan Ginsburg · Jonathan Ginsburg · commit ff7edbb2ffbc · 2022-02-10T12:25:22.000-06:00
diff --git a/client/karma.js b/client/karma.js
@@ -239,7 +239,15 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       self.updater.updateTestStatus('complete')
     }
     if (returnUrl) {
-      if (!/^https?:\/\//.test(returnUrl)) {
+      var isReturnUrlAllowed = false
+      for (var i = 0; i < this.config.allowedReturnUrlPatterns.length; i++) {
+        var allowedReturnUrlPattern = new RegExp(this.config.allowedReturnUrlPatterns[i])
+        if (allowedReturnUrlPattern.test(returnUrl)) {
+          isReturnUrlAllowed = true
+          break
+        }
+      }
+      if (!isReturnUrlAllowed) {
         throw new Error(
           'Security: Navigation to '.concat(
             returnUrl,
diff --git a/docs/config/01-configuration-file.md b/docs/config/01-configuration-file.md
@@ -277,6 +277,15 @@ upon the completion of running the tests. Setting this to false is useful when e
 
 If true, Karma does not display the banner and browser list. Useful when using karma on component tests with screenshots.
 
+## client.allowedReturnUrlPatterns
+**Type:** Array
+
+**Default:** `['^https?://']`
+
+**Description:** Define the string representations of the regular expressions that will be allowed for the `return_url` query parameter.
+
+If the value of the `return_url` query parameter does not match any regular expression derived from the string representation of each of the elements of this array, navigation to it will be blocked.
+
 ## colors
 **Type:** Boolean
 
diff --git a/lib/config.js b/lib/config.js
@@ -17,19 +17,19 @@ let TYPE_SCRIPT_AVAILABLE = false
 try {
   require('coffeescript').register()
   COFFEE_SCRIPT_AVAILABLE = true
-} catch (e) {}
+} catch {}
 
 // LiveScript is required here to enable config files written in LiveScript.
 // It's not directly used in this file.
 try {
   require('LiveScript')
   LIVE_SCRIPT_AVAILABLE = true
-} catch (e) {}
+} catch {}
 
 try {
   require('ts-node')
   TYPE_SCRIPT_AVAILABLE = true
-} catch (e) {}
+} catch {}
 
 class Pattern {
   constructor (pattern, served, included, watched, nocache, type, isBinary) {
@@ -324,7 +324,8 @@ class Config {
       useIframe: true,
       runInParent: false,
       captureConsole: true,
-      clearContext: true
+      clearContext: true,
+      allowedReturnUrlPatterns: ['^https?://']
     }
     this.browserDisconnectTimeout = 2000
     this.browserDisconnectTolerance = 0
diff --git a/static/karma.js b/static/karma.js
@@ -249,7 +249,15 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       self.updater.updateTestStatus('complete')
     }
     if (returnUrl) {
-      if (!/^https?:\/\//.test(returnUrl)) {
+      var isReturnUrlAllowed = false
+      for (var i = 0; i < this.config.allowedReturnUrlPatterns.length; i++) {
+        var allowedReturnUrlPattern = new RegExp(this.config.allowedReturnUrlPatterns[i])
+        if (allowedReturnUrlPattern.test(returnUrl)) {
+          isReturnUrlAllowed = true
+          break
+        }
+      }
+      if (!isReturnUrlAllowed) {
         throw new Error(
           'Security: Navigation to '.concat(
             returnUrl,
diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js
@@ -442,15 +442,18 @@ describe('Karma', function () {
       assert(spyResult.called)
     })
 
-    it('should navigate the client to return_url if specified', function (done) {
+    it('should navigate the client to return_url if specified and allowed', function (done) {
+      var config = {
+        // The default value.
+        allowedReturnUrlPatterns: ['^https?://']
+      }
       windowLocation.search = '?id=567&return_url=http://return.com'
       socket = new MockSocket()
       k = new ClientKarma(updater, socket, iframe, windowStub, windowNavigator, windowLocation)
       clientWindow = { karma: k }
       ck = new ContextKarma(ContextKarma.getDirectCallParentKarmaMethod(clientWindow))
-      ck.config = {}
+      socket.emit('execute', config)
 
-      sinon.spy(socket, 'disconnect')
       clock.tick(500)
 
       ck.complete()
@@ -462,6 +465,26 @@ describe('Karma', function () {
       clock.tick(10)
     })
 
+    it('should not navigate the client to return_url if not allowed', function () {
+      var config = {
+        allowedReturnUrlPatterns: []
+      }
+
+      windowLocation.search = '?id=567&return_url=javascript:alert(document.domain)'
+      socket = new MockSocket()
+      k = new ClientKarma(updater, socket, iframe, windowStub, windowNavigator, windowLocation)
+      clientWindow = { karma: k }
+      ck = new ContextKarma(ContextKarma.getDirectCallParentKarmaMethod(clientWindow))
+      socket.emit('execute', config)
+
+      try {
+        ck.complete()
+        throw new Error('An error should have been caught.')
+      } catch (error) {
+        assert(/Error: Security: Navigation to .* was blocked to prevent malicious exploits./.test(error))
+      }
+    })
+
     it('should clear context window upon complete when clearContext config is true', function () {
       var config = ck.config = {
         clearContext: true
