Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix/xss issues on data attributes #27047

Merged
merged 5 commits into from
Aug 13, 2018
Merged

Fix/xss issues on data attributes #27047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b4597a
fix(collapse): xss CVE-2018-14040
don-spyker Aug 10, 2018
7cfc6a9
fix(tooltip): xss CVE-2018-14042
don-spyker Aug 10, 2018
5c71ecf
fix(tooltip): XSS on data-viewport attribute
don-spyker Aug 10, 2018
ae29b6a
fix(affix): XSS on target config
don-spyker Aug 10, 2018
3f01fe2
add unit tests for plugins
Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion js/affix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
var Affix = function (element, options) {
this.options = $.extend({}, Affix.DEFAULTS, options)

this.$target = $(this.options.target)
var target = this.options.target === Affix.DEFAULTS.target ? $(this.options.target) : $(document).find(this.options.target)
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for your ternary here: $(document).find(this.options.target) is enough

Copy link
Author

Choose a reason for hiding this comment

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

I tried that first, but that broke the functionality as the default is "window" and $(document).find('window') didn't work while $(window) evaluates correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I understand 👍


this.$target = target
.on('scroll.bs.affix.data-api', $.proxy(this.checkPosition, this))
.on('click.bs.affix.data-api', $.proxy(this.checkPositionWithEventLoop, this))

Expand Down
2 changes: 1 addition & 1 deletion js/collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@
}

Collapse.prototype.getParent = function () {
return $(this.options.parent)
return $(document).find(this.options.parent)
.find('[data-toggle="collapse"][data-parent="' + this.options.parent + '"]')
.each($.proxy(function (i, element) {
var $element = $(element)
Expand Down
15 changes: 15 additions & 0 deletions js/tests/unit/affix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,19 @@ $(function () {
}, 250)
}, 250)
})

QUnit.test('should raise exception to avoid xss on target', function (assert) {
assert.expect(1)
assert.throws(function () {

var templateHTML = '<div id="affixTarget"></div>'
$(templateHTML).appendTo(document.body)

$('#affixTarget').bootstrapAffix({
target: '<img src=1 onerror=\'alert(0)\'>'
})

}, new Error('Syntax error, unrecognized expression: <img src=1 onerror=\'alert(0)\'>'))
})

})
10 changes: 10 additions & 0 deletions js/tests/unit/collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,4 +440,14 @@ $(function () {
.bootstrapCollapse('show')
})

QUnit.test('should raise exception to avoid xss on data-parent', function (assert) {
assert.expect(1)
assert.throws(function () {
$('<a role="button" data-toggle="collapse" data-parent="<img src=1 onerror=\'alert(0)\'>" href="#collapseThree">')
.appendTo('#qunit-fixture')
.bootstrapCollapse('show')
.trigger('click');
}, new Error('Syntax error, unrecognized expression: <img src=1 onerror=\'alert(0)\'>'))
})

})
18 changes: 18 additions & 0 deletions js/tests/unit/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1322,4 +1322,22 @@ $(function () {
})
})

QUnit.test('should raise exception to avoid xss on data-container', function (assert) {
assert.expect(1)
assert.throws(function () {
$('<button data-toggle="tooltip" data-container="<img src=1 onerror=\'alert(0)\'>" title="Tooltip on right">Tooltip on right</button>')
.appendTo('#qunit-fixture')
.bootstrapTooltip('show')
}, new Error('Syntax error, unrecognized expression: <img src=1 onerror=\'alert(0)\'>'))
})

QUnit.test('should raise exception to avoid xss on data-viewport', function (assert) {
assert.expect(1)
assert.throws(function () {
$('<button data-toggle="tooltip" data-viewport="<img src=1 onerror=\'alert(0)\'>" title="Tooltip on right">Tooltip on right</button>')
.appendTo('#qunit-fixture')
.bootstrapTooltip('show')
}, new Error('Syntax error, unrecognized expression: <img src=1 onerror=\'alert(0)\'>'))
})

})
4 changes: 2 additions & 2 deletions js/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
this.type = type
this.$element = $(element)
this.options = this.getOptions(options)
this.$viewport = this.options.viewport && $($.isFunction(this.options.viewport) ? this.options.viewport.call(this, this.$element) : (this.options.viewport.selector || this.options.viewport))
this.$viewport = this.options.viewport && $(document).find($.isFunction(this.options.viewport) ? this.options.viewport.call(this, this.$element) : (this.options.viewport.selector || this.options.viewport))
this.inState = { click: false, hover: false, focus: false }

if (this.$element[0] instanceof document.constructor && !this.options.selector) {
Expand Down Expand Up @@ -204,7 +204,7 @@
.addClass(placement)
.data('bs.' + this.type, this)

this.options.container ? $tip.appendTo(this.options.container) : $tip.insertAfter(this.$element)
this.options.container ? $tip.appendTo($(document).find(this.options.container)) : $tip.insertAfter(this.$element)
this.$element.trigger('inserted.bs.' + this.type)

var pos = this.getPosition()
Expand Down