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

Fixes #21519 - Prevent stored XSS on fact charts #4967

Merged
merged 1 commit into from Nov 5, 2017

Conversation

Projects
None yet
4 participants
@tbrisker
Member

tbrisker commented Oct 31, 2017

No description provided.

@tbrisker tbrisker added the 1.16.0 label Oct 31, 2017

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot
Member

theforeman-bot commented Oct 31, 2017

Issues: #21519

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Oct 31, 2017

Member

Once accepted this should be cherry-picked into 1.16-stable and possibly also 1.15-stable if we do another release.

Member

tbrisker commented Oct 31, 2017

Once accepted this should be cherry-picked into 1.16-stable and possibly also 1.15-stable if we do another release.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Nov 1, 2017

Member

@tbrisker - thanks! does it make sense to add tests ?

Member

ohadlevy commented Nov 1, 2017

@tbrisker - thanks! does it make sense to add tests ?

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Nov 1, 2017

Member

@ohadlevy not sure if there is a good way to do that, the new charts already sanitize the text correctly, the old js code should go all away once remaining charts are migrated to react. I added the escaping on the backend as well as a backup measure in case flot.js somewhere doesn't escape correctly or in case i missed something in the awful mess that is there right now.

Member

tbrisker commented Nov 1, 2017

@ohadlevy not sure if there is a good way to do that, the new charts already sanitize the text correctly, the old js code should go all away once remaining charts are migrated to react. I added the escaping on the backend as well as a backup measure in case flot.js somewhere doesn't escape correctly or in case i missed something in the awful mess that is there right now.

@dLobatog

dLobatog approved these changes Nov 2, 2017 edited

Looks good to me - I'm not sure it's worth adding tests for checking each of the escapes here. Maybe checking that an HTML tag as part of a fact name is escaped in some page as an integration test would be good, but I'm not too adamant, your call @ohadlevy @tbrisker

@ohadlevy ohadlevy merged commit 81e40e3 into theforeman:develop Nov 5, 2017

7 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished.
Details
hound No violations found. Woof!
katello Build finished.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished.
Details
@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy
Member

ohadlevy commented Nov 5, 2017

thanks @tbrisker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment