Skip to content

Commit

Permalink
fix(security): mitigate the "Open Redirect Vulnerability"
Browse files Browse the repository at this point in the history
  • Loading branch information
Jonathan Ginsburg committed Feb 10, 2022
1 parent c1befa0 commit ff7edbb
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 9 deletions.
10 changes: 9 additions & 1 deletion client/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
9 changes: 9 additions & 0 deletions docs/config/01-configuration-file.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 5 additions & 4 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -324,7 +324,8 @@ class Config {
useIframe: true,
runInParent: false,
captureConsole: true,
clearContext: true
clearContext: true,
allowedReturnUrlPatterns: ['^https?://']
}
this.browserDisconnectTimeout = 2000
this.browserDisconnectTolerance = 0
Expand Down
10 changes: 9 additions & 1 deletion static/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
29 changes: 26 additions & 3 deletions test/client/karma.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
Expand Down

1 comment on commit ff7edbb

@Lauznis89
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Es esmu cilvēks

Please # to comment.