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

karma-runner · Feb 9, 2022 · 6b3bf3b · 6b3bf3b
1 parent c1befa0
commit 6b3bf3b
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 9 deletions.
diff --git a/client/karma.js b/client/karma.js
@@ -239,7 +239,16 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       self.updater.updateTestStatus('complete')
     }
     if (returnUrl) {
-      if (!/^https?:\/\//.test(returnUrl)) {
+      // TODO(https://github.com/karma-runner/karma/issues/3503): replace the for-loop below with the `Array.prototype.includes()` method, when dropping support for IE.
+      var isReturnUrlInAllowlist = false
+      for (var i = 0; i < this.config.allowedReturnUrls.length; i++) {
+        var allowedReturnUrl = this.config.allowedReturnUrls[i]
+        if (allowedReturnUrl === returnUrl) {
+          isReturnUrlInAllowlist = true
+        }
+      }
+      var isReturnUrlsSchemeBenign = /^https?:\/\//.test(returnUrl)
+      if (!isReturnUrlInAllowlist || !isReturnUrlsSchemeBenign) {
         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.allowedReturnUrls
+**Type:** Array
+
+**Default:** `[]`
+
+**Description:** Define the allowed values for the `return_url` query parameter.
+
+If the value of the `return_url` query parameter is not in this list, navigation to it will be blocked. This mitigates the "Open Redirect Vulnerability".
+
 ## 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,
+      allowedReturnUrls: []
     }
     this.browserDisconnectTimeout = 2000
     this.browserDisconnectTolerance = 0

diff --git a/static/karma.js b/static/karma.js
@@ -249,7 +249,16 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       self.updater.updateTestStatus('complete')
     }
     if (returnUrl) {
-      if (!/^https?:\/\//.test(returnUrl)) {
+      // TODO(https://github.com/karma-runner/karma/issues/3503): replace the for-loop below with the `Array.prototype.includes()` method, when dropping support for IE.
+      var isReturnUrlInAllowlist = false
+      for (var i = 0; i < this.config.allowedReturnUrls.length; i++) {
+        var allowedReturnUrl = this.config.allowedReturnUrls[i]
+        if (allowedReturnUrl === returnUrl) {
+          isReturnUrlInAllowlist = true
+        }
+      }
+      var isReturnUrlsSchemeBenign = /^https?:\/\//.test(returnUrl)
+      if (!isReturnUrlInAllowlist || !isReturnUrlsSchemeBenign) {
         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, benign and allowed', function (done) {
+      var config = {
+        allowedReturnUrls: ['http://return.com']
+      }
+
       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.only('should not navigate the client to return_url if not benign', function () {
+      var config = {
+        allowedReturnUrls: ['javascript:alert(document.domain)']
+      }
+
+      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