Merge pull request #196 from rails/flavorjones-fix-frozen-allowed-tags

fix: PermitScrubber accepts frozen tags

Author: flavorjones

CHANGELOG.md

@@ -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.

lib/rails/html/scrubbers.rb

@@ -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)

test/sanitizer_test.rb

@@ -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
@@ -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
@@ -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))