Skip to content

Commit

Permalink
fix: disallow 'noscript' from safe lists
Browse files Browse the repository at this point in the history
  • Loading branch information
flavorjones committed Dec 1, 2024
1 parent 5104ca9 commit 1625173
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 0 deletions.
6 changes: 6 additions & 0 deletions lib/rails/html/scrubbers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,12 @@ def validate!(var, name)
if var && !var.is_a?(Enumerable)
raise ArgumentError, "You should pass :#{name} as an Enumerable"
end

if var && name == :tags && var.include?("noscript")
warn("WARNING: 'noscript' tags cannot be allowed by the PermitScrubber and will be scrubbed")
var.delete("noscript")
end

var
end

Expand Down
35 changes: 35 additions & 0 deletions test/sanitizer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,24 @@ def test_should_sanitize_across_newlines
assert_equal "", sanitize_css(raw)
end

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"]
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))
end

acceptable_results = [
# libxml2
"<div><p id=\"&lt;/noscript&gt;&lt;script&gt;alert(1)&lt;/script&gt;\"></p></div>",
# libgumbo
"<div><p id=\"</noscript><script>alert(1)</script>\"></p></div>",
]

assert_includes(acceptable_results, actual)
end

protected
def safe_list_sanitize(input, options = {})
module_under_test::SafeListSanitizer.new.sanitize(input, options)
Expand Down Expand Up @@ -1075,5 +1093,22 @@ class HTML4SafeListSanitizerTest < Minitest::Test
class HTML5SafeListSanitizerTest < Minitest::Test
@module_under_test = Rails::HTML5
include SafeListSanitizerTest

def test_should_not_be_vulnerable_to_noscript_attacks
# https://hackerone.com/reports/2509647
skip("browser assertion requires parse_noscript_content_as_text") unless Nokogiri::VERSION >= "1.17"

input = '<noscript><p id="</noscript><script>alert(1)</script>"></noscript>'

result = nil
assert_output(nil, /WARNING/) do
result = Rails::HTML5::SafeListSanitizer.new.sanitize(input, tags: %w(p div noscript), attributes: %w(id class style))
end

browser = Nokogiri::HTML5::Document.parse(result, parse_noscript_content_as_text: true)
xss = browser.at_xpath("//script")

assert_nil(xss)
end
end if loofah_html5_support?
end
8 changes: 8 additions & 0 deletions test/scrubbers_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ def test_leaves_only_supplied_tags_and_attributes
assert_scrubbed html, '<tag></tag><tag cooler=""></tag>'
end

def test_does_not_allow_safelisted_noscript
# https://hackerone.com/reports/2509647
assert_output(nil, /WARNING: 'noscript' tags cannot be allowed by the PermitScrubber/) do
@scrubber.tags = ["div", "noscript", "span"]
end
assert_equal(["div", "span"], @scrubber.tags)
end

def test_leaves_text
assert_scrubbed("some text")
end
Expand Down

0 comments on commit 1625173

Please sign in to comment.