Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix XSS in data-target #23679

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Fix XSS in data-target #23679

merged 2 commits into from
Aug 25, 2017

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Aug 25, 2017

Fix XSS in our data-target (CVE-2016-10735).

Closes #20184
/CC @meeque

@meeque
Copy link
Contributor

meeque commented Aug 25, 2017

What about js/dist/util.js will CI generate these?

Asking because I have started from writing failing unit tests, and they still fail unless I also fix it over there.

Fyi, you can find these tests here:
https://github.com/meeque/bootstrap/tree/topic/20184-xss-in-data-target-attribute
I've already merged your fix into that.

@Johann-S
Copy link
Member Author

Yes Travis build our code and in this case update js/dist/util.js and only added a Visual test because when we fail to get a target they aren't any error so that's a bit difficult to test that with Travis and we don't have unit tests for util.js

Is it a correct fix for you ?

@meeque
Copy link
Contributor

meeque commented Aug 25, 2017

Yes, the fix looks fine.

I'll have a look around, and check if other features might be affected similarly. I'd open new issues in that case...

The qunit test that I wrote seem to work rather fine either way. Of course, for the positive case there is not much to check, so they just wait for a (very short) timeout, and assert that nothing has happened.

@Johann-S
Copy link
Member Author

Maybe you can create a PR which target my branch v4-xss-data-attribute with your unit test ? ☺️

@Johann-S
Copy link
Member Author

Thanks @meeque 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants