Add img as allowed tag to sanitizer #5983
Conversation
jorg-vr
added
the
enhancement
WalkthroughThe pull request modifies the Changes
Possibly related PRs
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: 1
📜 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)
74-77: LGTM! Good security test coverage.
The test appropriately covers SVG-specific XSS attack vectors:
- Nested script tags within SVG
- The
usetag with javascript: protocol in xlink:href
Also applies to: 84-84
108-117: 🛠️ Refactor suggestion
Enhance security test coverage for img tags.
While the basic functionality is tested, consider adding tests for the following security scenarios:
- Filtering of dangerous event handlers (e.g., onerror, onload)
- SVG images that might contain embedded scripts
- URL scheme restrictions (e.g., javascript: URLs)
Let's check if there are any existing tests covering these scenarios:
Add these test cases:
test 'sanitize helper should allow images' do
# test link image and base64 image
dirty_html = <<~HTML
<img src="https://example.com/image.jpg" alt="Image">
<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==" alt="Red dot">
+ <img src="javascript:alert(1)" onerror="alert(2)" onload="alert(3)">
+ <img src="data:image/svg+xml;base64,PHN2ZyBvbmxvYWQ9ImFsZXJ0KDEpIj48L3N2Zz4=">
HTML
clean_html = sanitize dirty_html
- assert_equal dirty_html, clean_html
+ assert_match(/https:\/\/example\.com/, clean_html)
+ assert_match(/data:image\/png;base64,/, clean_html)
+ assert_no_match(/javascript:/, clean_html)
+ assert_no_match(/onerror|onload/, clean_html)
+ assert_no_match(/data:image\/svg\+xml/, clean_html)
end| @@ -134,7 +134,7 @@ def markdown_unsafe(source) | |||
|
|
|||
| def sanitize(html) | |||
| @tags ||= Rails::Html::SafeListSanitizer.allowed_tags.to_a + | |||
| %w[table thead tbody tr td th colgroup col style summary details] + | |||
| %w[table thead tbody tr td th colgroup col style summary details img] + | |||
There was a problem hiding this comment.
Add security controls for img tag attributes and URLs.
While adding the img tag, consider implementing these security controls:
- Explicitly allow only safe img attributes (src, alt, width, height, title)
- Implement URL scheme restrictions for the src attribute
- Consider using a URL allowlist for external images
Here's a suggested implementation:
@tags ||= Rails::Html::SafeListSanitizer.allowed_tags.to_a +
- %w[table thead tbody tr td th colgroup col style summary details img] +
+ %w[table thead tbody tr td th colgroup col style summary details] +
+ %w[img] +
%w[svg g style circle line rect path polygon polyline text defs]
@attributes ||= Rails::Html::SafeListSanitizer.allowed_attributes.to_a +
%w[style target data-bs-toggle data-parent data-tab data-line data-element id] +
- %w[viewBox width height version style class transform id x y rx ry x1 y1 x2 y2 d points fill stroke stroke-width stroke-dasharray cx cy r font-size font-family font-weight font-variant textLength writing-mode glyph-orientation-vertical text-orientation color]
+ %w[viewBox width height version style class transform id x y rx ry x1 y1 x2 y2 d points fill stroke stroke-width stroke-dasharray cx cy r font-size font-family font-weight font-variant textLength writing-mode glyph-orientation-vertical text-orientation color] +
+ %w[src alt title]
+# Add URL scheme restrictions
+def sanitize(html)
+ # ... existing code ...
+
+ # Additional URL scheme filtering for img src
+ sanitized.gsub(/(<img[^>]*\ssrc=["'])(.*?)(["'][^>]*>)/) do |match|
+ prefix, url, suffix = Regexp.last_match[1..3]
+ if url.start_with?('data:image/png;base64,', 'data:image/jpeg;base64,', 'data:image/gif;base64,', 'https://')
+ match
+ else
+ prefix + '#' + suffix # or remove the img tag entirely
+ end
+ end.html_safe
+endConsider also implementing a configuration option for URL allowlist:
# config/initializers/sanitizer.rb
Rails.application.config.allowed_image_domains = ['example.com', 'your-cdn.com']
There was a problem hiding this comment.
@jorg-vr: could you also support SVG-images; the URL is then formatted as
<img src="data:image/svg+xml;base64,..."/>There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This pull request adds
imgas allowed tag in sanitized html.This to support more complex SVG's without allowing scripts.