Use jQuery's text() instead of html() where safer #3779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javierm
changed the title
Doing so will cause the `<span>` tag to be rendered in the document, instead of being rendered as a data attribute.
Using html() makes it possible to insert <script> tags in the DOM, and in this case we aren't supposed to be inserting any HTML. I haven't found a way to focus on a field with Capybara, then add a character, and focus on another field. So I've manually triggered the change event in the test.
The jQuery html() function does not filter <script> tags, so if somehow an attacker introduced a <script> in the translation, we would be vulnerable to a XSS attack. Note using $.parseHTML wouldn't solve the problem, since it doesn't filter attributes in image tags. Since changing the text of the part which doesn't have the count wasn't very clean, I've added another <span> tag for the part with the description, and so we can use jQuery's text() function to replace it.
smarques
pushed a commit
to venetochevogliamo/consul
that referenced
this pull request
Apr 29, 2020
Use jQuery's text() instead of html() where safer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Using
html()makes it possible to insert <script> tags in the DOM, so it shouldn't be done unless we're 100% sure the content is safe. While the content is safe in most cases, since it's already escaped by Rails, there are a couple of places where there's a remote chance of the content not being safe.Objectives
Reduce the (small) chances of XSS attacks because of the way we manipulate the DOM.
Notes
The same happens with many jQuery methods, like
appendorinsertBefore. After reviewing the code, I haven't identified a place where these methods might be potentially dangerous.