Allow using use tag in sanitized html
#5984
Conversation
|
Ok this just drove me nearly insane. I think the best solution is to allow The below code comes from the As you can see it is more complex then expected. def scrub_attribute(node, attr_node)
attr_name = if attr_node.namespace
"#{attr_node.namespace.prefix}:#{attr_node.node_name}"
else
attr_node.node_name
end
return if Loofah::HTML5::SafeList::ATTR_VAL_IS_URI.include?(attr_name) && Loofah::HTML5::Scrub.scrub_uri_attribute(attr_node)
if Loofah::HTML5::SafeList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name)
Loofah::HTML5::Scrub.scrub_attribute_that_allows_local_ref(attr_node)
end
if Loofah::HTML5::SafeList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == "xlink:href" && attr_node.value =~ /^\s*[^#\s].*/m
attr_node.remove
end
node.remove_attribute(attr_node.name) if attr_name == "src" && attr_node.value !~ /[^[:space:]]/
Loofah::HTML5::Scrub.force_correct_attribute_escaping! node
endSo I stick to my point that we don't want to be implementing this ourselves. So I wouldn't overwrite this code to allow SVG's inside |
|
@jorg-vr my use case for using the |
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/helpers/application_helper_test.rb (1)
87-87: Consider adding positive test cases for safeusetags.While the current tests verify that dangerous attributes are filtered, it would be beneficial to add assertions that confirm safe
usetags are preserved. This ensures the sanitizer isn't overly aggressive.assert_no_match(/xlink:href/, clean_html) +# Verify safe use tags are preserved +assert_match(/<use href="#diamond"/, clean_html) +assert_match(/<use x="50" y="50"/, clean_html)app/helpers/application_helper.rb (1)
Add
hrefandxlink:hrefattributes to the safelist for SVG use tag functionalityBased on the test file, specifically the "sanitize helper should allow a selection of svg tags" test, both
hrefandxlink:hrefattributes are required for proper SVGusetag functionality. The test expects these attributes to be preserved when they reference valid SVG elements (e.g.,#diamond), while the security test confirms they should be stripped when containing JavaScript URLs.
- Add
hrefandxlink:hrefto the allowed attributes list to support proper SVGusetag functionality🔗 Analysis chain
Line range hint
138-142: Verify attribute requirements for SVG use tag.While the tag is now allowed, please verify if any additional SVG-specific attributes are required for the
usetag to function properly (e.g.,href,xlink:href).
Based on the test file output, I can see that both
hrefandxlink:hrefattributes are being used with the SVGusetag, but these attributes are not included in the current safelist. Let me verify the test expectations to ensure these attributes are actually required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for use tag attributes in the codebase to ensure we haven't missed any required attributes # Search for use tag usage patterns rg -A 2 "<use\s+[^>]+>" # Search for SVG-related tests or documentation fd -e md -e txt . | xargs rg -l "svg|use tag"Length of output: 904
Script:
#!/bin/bash # Check the test file to understand the test expectations cat test/helpers/application_helper_test.rbLength of output: 6510
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/helpers/application_helper.rb(1 hunks)test/helpers/application_helper_test.rb(2 hunks)
🔇 Additional comments (2)
test/helpers/application_helper_test.rb (2)
77-79: LGTM! Good security test coverage.
The test cases appropriately verify that both xlink:href and href attributes with JavaScript URLs are filtered out, while allowing safe URLs.
136-137: LGTM! Comprehensive test coverage for SVG use cases.
The test cases properly verify that use tags work with both xlink:href and href attributes when referencing local IDs, which is the intended use case.
|
@pdawyndt your usecase would be supported by this pr |
bmesuere
changed the title
use tag in sanitized html
|
Confirmation that everything works on my side. Thanks for taking this up. |
This pull request adds the possibility to use
usetags in html that will pass through our sanitizer.Looking at the code in the underlying library I deem it safe to use and I have added testcases that should warn us if support in that library changes.