Skip to content

Commit

Permalink
fix: PermitScrubber accepts frozen tags
Browse files Browse the repository at this point in the history
Previously, if an invalid/unsafe tag was present, the scrubber
attempted to modify the tags array. Now it properly copies the tags
when they are assigned.

Fixes #195

(cherry picked from commit 9a1e376)
  • Loading branch information
flavorjones committed Dec 12, 2024
1 parent 5e96b19 commit 5843d4d
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 5 deletions.
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

0 comments on commit 5843d4d

Please sign in to comment.