Skip to content
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

fix: PermitScrubber accepts frozen tags #196

Merged
merged 1 commit into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
## next / unreleased

* `PermitScrubber` fully supports frozen "allowed tags".

v1.6.1 introduced safety checks that may remove unsafe tags from the allowed list, which
introduced a regression for applications passing a frozen array of allowed tags. Tags and
attributes are now properly copied when they are passed to the scrubber.

Fixes #195.

*Mike Dalessio*


## 1.6.1 / 2024-12-02

This is a performance and security release which addresses several possible XSS vulnerabilities.
Expand Down
4 changes: 2 additions & 2 deletions lib/rails/html/scrubbers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ def initialize(prune: false)
end

def tags=(tags)
@tags = validate!(tags, :tags)
@tags = validate!(tags.dup, :tags)
end

def attributes=(attributes)
@attributes = validate!(attributes, :attributes)
@attributes = validate!(attributes.dup, :attributes)
end

def scrub(node)
Expand Down
8 changes: 5 additions & 3 deletions test/sanitizer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,7 @@ def test_should_sanitize_across_newlines
def test_should_prune_mglyph
# https://hackerone.com/reports/2519936
input = "<math><mtext><table><mglyph><style><img src=: onerror=alert(1)>"
tags = %w(math mtext table mglyph style)
tags = %w(math mtext table mglyph style).freeze

actual = nil
assert_output(nil, /WARNING: 'mglyph' tags cannot be allowed by the PermitScrubber/) do
Expand All @@ -1119,7 +1119,7 @@ def test_should_prune_mglyph
def test_should_prune_malignmark
# https://hackerone.com/reports/2519936
input = "<math><mtext><table><malignmark><style><img src=: onerror=alert(1)>"
tags = %w(math mtext table malignmark style)
tags = %w(math mtext table malignmark style).freeze

actual = nil
assert_output(nil, /WARNING: 'malignmark' tags cannot be allowed by the PermitScrubber/) do
Expand All @@ -1138,7 +1138,9 @@ def test_should_prune_malignmark

def test_should_prune_noscript
# https://hackerone.com/reports/2509647
input, tags = "<div><noscript><p id='</noscript><script>alert(1)</script>'></noscript>", ["p", "div", "noscript"]
input = "<div><noscript><p id='</noscript><script>alert(1)</script>'></noscript>"
tags = ["p", "div", "noscript"].freeze

actual = nil
assert_output(nil, /WARNING: 'noscript' tags cannot be allowed by the PermitScrubber/) do
actual = safe_list_sanitize(input, tags: tags, attributes: %w(id))
Expand Down
Loading