From 6b3bf3b295febc4a6b47bb284bbcae68d648ec72 Mon Sep 17 00:00:00 2001 From: Jonathan Ginsburg Date: Tue, 8 Feb 2022 20:39:01 -0600 Subject: [PATCH] fix(security): mitigate the "Open Redirect Vulnerability" --- client/karma.js | 11 ++++++++++- docs/config/01-configuration-file.md | 9 +++++++++ lib/config.js | 9 +++++---- static/karma.js | 11 ++++++++++- test/client/karma.spec.js | 29 +++++++++++++++++++++++++--- 5 files changed, 60 insertions(+), 9 deletions(-) diff --git a/client/karma.js b/client/karma.js index 50651037b..9444e0c72 100644 --- 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 index ce83ec28c..904a96cd5 100644 --- 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 index cd9510bfd..bd90ef2c9 100644 --- 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 index f0b2548af..7268287ed 100644 --- 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 index 3e7af73d8..adb44f66e 100644 --- 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