Skip to content

Conversation

@satchmorun
Copy link
Contributor

Fixes #2996.

An easy fix, we were simply missing a call to this.escape_html. To be honest, I'm surprised this hasn't been a reported issue yet.

As a bonus, there are even tests for red-greening the fix!

Many thanks to @cade for his assistance with figuring out was was going on in the Ancient Technology™ test (read: Prototype JS).

@satchmorun
Copy link
Contributor Author

@adunkman
Copy link
Contributor

adunkman commented Jun 7, 2018

I’m 👍🤘 — I assume you’ve double-checked for double-escaping, but that would be my only concern.

bon-jovi-livin-on-a-prayer.gif

Arun Srinivasan added 3 commits June 7, 2018 16:21
Quick note: this uses the idiom `div.innerHTML = tmpl` instead of what we've
been using in our other tests, which is `div.update(tmpl)` because of some fun
stuff that Prototype does when one calls `update`:

docs: http://api.prototypejs.org/dom/Element/prototype/update/

Relevant quote for funsies:

> If newContent is a string and contains one or more inline <script>
> tags, the scripts are scheduled to be evaluated after a very brief pause
> (using Function#defer) to allow the browser to finish updating the DOM.

Yeah.
@satchmorun satchmorun force-pushed the fix-group-label-xss-vulnerability branch from 0fec73f to 5ae77b1 Compare June 7, 2018 23:23
Copy link
Contributor

@tjschuck tjschuck left a comment

Choose a reason for hiding this comment

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

👍

@tjschuck tjschuck merged commit 77fd031 into master Jun 8, 2018
@tjschuck tjschuck deleted the fix-group-label-xss-vulnerability branch June 8, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XSS Bug when using the include_group_label_in_selected option

4 participants