allow time tag and lang attr, remove XPATHS_TO_REMOVE, add test coverage, get JRuby green
#156
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
Reasons: - the "datetime" attr is allowed, but is only valid in a "time" tag. - the "xml:lang" attr is allowed, but is only valid if "lang" attr is also present. The "time" tag and "lang" attribute should be considered safe and are allowed by Loofah. Also backfill tests around the default safe lists.
This has always been an implementation detail, and is not necessary with the current default sanitizer behavior.
Xerces doesn't allow node renaming (but libxml2 does)
The use of assert_dom_equal obfuscates what we're actually testing, which in many cases is "not much". These elements already have adequate coverage in Loofah, and the allowed_tags feature is tested adequately elsewhere in this suite.
The use of both was making `assert_sanitized` very ambiguous, as you can see from the tests that have been updated. The more explicit tests allow us to be sensitive to behavioral changes upstream, so that we fully understand what we're emitting.
because Nokogiri 1.14 switched from cyberneko to nekohtml-unit and there are some parsing differences.
flavorjones
force-pushed
the
flavorjones-add-scrubber-test-coverage
branch
from
4008cbd
to
2070471
Compare
flavorjones
changed the title
time tag and lang attr, and remove XPATHS_TO_REMOVEtime tag and lang attr, remove XPATHS_TO_REMOVE, get JRuby green
flavorjones
changed the title
time tag and lang attr, remove XPATHS_TO_REMOVE, get JRuby greentime tag and lang attr, remove XPATHS_TO_REMOVE, add test coverage, get JRuby green
flavorjones
force-pushed
the
flavorjones-add-scrubber-test-coverage
branch
from
27b09b1
to
2070471
Compare
|
Actions isn't working well, but I kicked off a manual run of CI on this PR at https://github.com/rails/rails-html-sanitizer/actions/runs/4948930151 which is green. |
rafaelfranca
approved these changes
May 11, 2023
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.
Preparing for a larger refactor related to HTML5 parsing support, this PR started as backfilling test coverage for the default safelist.
public API changes
timetag is now allowed by default (note that thedatetimeattribute was already allowed, and that attribute is only valid in atimetag)langattribute is now allowed by default (note that thexml:langattribute was already allowed, and that attribute is only valid if there is also a matchinglangattribute)Rails::Html::XPATHS_TO_REMOVEconstant is removedNote that
timetag andlangattribute are safe and are already allowed by Loofah, DOMPurify, and other common sanitizers.The
XPATHS_TO_REMOVEconstant was public, but probably should have been a private constant (an implementation detail) all along. It's possible that removing it might break somebody who's monkeypatching the sanitizer, but really they should be usingSafeListSanitizer'sallowed_tagsandallowed_attributesattrs instead of changing the value of this constant.test suite changes
Within the test suite, I've also made the following changes:
assert_dom_equalwhich was obfuscating what is being tested