From 4c9638008fdcfb4c71b1ed660f6ab6d120b7f02d Mon Sep 17 00:00:00 2001 From: baizg1107 Date: Thu, 14 Jul 2022 15:23:03 +0800 Subject: [PATCH] fix test failures --- lib/rails/html/sanitizer.rb | 19 +++++- test/sanitizer_test.rb | 122 +++++++++++++++++++++++++++++++----- 2 files changed, 124 insertions(+), 17 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index 5633ca1..13fb963 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -141,8 +141,25 @@ module Rails private + def loofah_using_html5? + # future-proofing, see https://github.com/flavorjones/loofah/pull/239 + Loofah.respond_to?(:html5_mode?) && Loofah.html5_mode? + end + + def remove_safelist_tag_combinations(tags) + if !loofah_using_html5? && tags.include?("select") && tags.include?("style") + warn("WARNING: #{self.class}: removing 'style' from safelist, should not be combined with 'select'") + tags.delete("style") + end + tags + end + def allowed_tags(options) - options[:tags] || self.class.allowed_tags + if options[:tags] + remove_safelist_tag_combinations(options[:tags]) + else + self.class.allowed_tags + end end def allowed_attributes(options) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 7938433..c6800a2 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -2,6 +2,8 @@ require "minitest/autorun" require "rails-html-sanitizer" require "rails/dom/testing/assertions/dom_assertions" +puts Nokogiri::VERSION_INFO + class SanitizersTest < Minitest::Test include Rails::Dom::Testing::Assertions::DomAssertions @@ -12,13 +14,11 @@ class SanitizersTest < Minitest::Test end def test_sanitize_nested_script - sanitizer = Rails::Html::SafeListSanitizer.new - assert_equal '<script>alert("XSS");</script>', sanitizer.sanitize('alert("XSS");/', tags: %w(em)) + assert_equal '<script>alert("XSS");</script>', safe_list_sanitize('alert("XSS");/', tags: %w(em)) end def test_sanitize_nested_script_in_style - sanitizer = Rails::Html::SafeListSanitizer.new - assert_equal '<script>alert("XSS");</script>', sanitizer.sanitize('alert("XSS");/', tags: %w(em)) + assert_equal '<script>alert("XSS");</script>', safe_list_sanitize('alert("XSS");/', tags: %w(em)) end class XpathRemovalTestSanitizer < Rails::Html::Sanitizer @@ -54,7 +54,8 @@ class SanitizersTest < Minitest::Test def test_strip_tags_with_quote input = '<" hi' - assert_equal ' hi', full_sanitize(input) + expected = libxml_2_9_14_recovery? ? %{<" hi} : %{ hi} + assert_equal(expected, full_sanitize(input)) end def test_strip_invalid_html @@ -75,15 +76,21 @@ class SanitizersTest < Minitest::Test end def test_remove_unclosed_tags - assert_equal "This is ", full_sanitize("This is <-- not\n a comment here.") + input = "This is <-- not\n a comment here." + expected = libxml_2_9_14_recovery? ? %{This is <-- not\n a comment here.} : %{This is } + assert_equal(expected, full_sanitize(input)) end def test_strip_cdata - assert_equal "This has a ]]> here.", full_sanitize("This has a ]]> here.") + input = "This has a ]]> here." + expected = libxml_2_9_14_recovery? ? %{This has a <![CDATA[]]> here.} : %{This has a ]]> here.} + assert_equal(expected, full_sanitize(input)) end def test_strip_unclosed_cdata - assert_equal "This has an unclosed ]] here...", full_sanitize("This has an unclosed ]] here...") + input = "This has an unclosed ]] here..." + expected = libxml_2_9_14_recovery? ? %{This has an unclosed <![CDATA[]] here...} : %{This has an unclosed ]] here...} + assert_equal(expected, full_sanitize(input)) end def test_strip_blank_string @@ -414,8 +421,25 @@ class SanitizersTest < Minitest::Test end def test_should_sanitize_div_background_image_unicode_encoded - raw = %(background-image:\u0075\u0072\u006C\u0028\u0027\u006a\u0061\u0076\u0061\u0073\u0063\u0072\u0069\u0070\u0074\u003a\u0061\u006c\u0065\u0072\u0074\u0028\u0031\u0032\u0033\u0034\u0029\u0027\u0029) - assert_equal '', sanitize_css(raw) + [ + convert_to_css_hex("url(javascript:alert(1))", false), + convert_to_css_hex("url(javascript:alert(1))", true), + convert_to_css_hex("url(https://example.com)", false), + convert_to_css_hex("url(https://example.com)", true), + ].each do |propval| + raw = "background-image:" + propval + assert_empty(sanitize_css(raw)) + end + end + + def test_should_allow_div_background_image_unicode_encoded_safe_functions + [ + convert_to_css_hex("rgb(255,0,0)", false), + convert_to_css_hex("rgb(255,0,0)", true), + ].each do |propval| + raw = "background-image:" + propval + assert_includes(sanitize_css(raw), "background-image") + end end def test_should_sanitize_div_style_expression @@ -433,11 +457,15 @@ class SanitizersTest < Minitest::Test end def test_should_sanitize_cdata_section - assert_sanitized "section]]>", "section]]>" + input = "section]]>" + expected = libxml_2_9_14_recovery? ? %{<![CDATA[section]]>} : %{section]]>} + assert_sanitized(input, expected) end def test_should_sanitize_unterminated_cdata_section - assert_sanitized "neverending...", "neverending..." + input = "neverending..." + expected = libxml_2_9_14_recovery? ? %{<![CDATA[neverending...} : %{neverending...} + assert_sanitized(input, expected) end def test_should_not_mangle_urls_with_ampersand @@ -488,7 +516,13 @@ class SanitizersTest < Minitest::Test text = safe_list_sanitize(html) - assert_equal %{test}, text + acceptable_results = [ + # nokogiri w/vendored+patched libxml2 + %{test}, + # nokogiri w/ system libxml2 + %{test}, + ] + assert_includes(acceptable_results, text) end def test_uri_escaping_of_src_attr_in_a_tag_in_safe_list_sanitizer @@ -498,7 +532,13 @@ class SanitizersTest < Minitest::Test text = safe_list_sanitize(html) - assert_equal %{test}, text + acceptable_results = [ + # nokogiri w/vendored+patched libxml2 + %{test}, + # nokogiri w/system libxml2 + %{test}, + ] + assert_includes(acceptable_results, text) end def test_uri_escaping_of_name_attr_in_a_tag_in_safe_list_sanitizer @@ -508,7 +548,13 @@ class SanitizersTest < Minitest::Test text = safe_list_sanitize(html) - assert_equal %{test}, text + acceptable_results = [ + # nokogiri w/vendored+patched libxml2 + %{test}, + # nokogiri w/system libxml2 + %{test}, + ] + assert_includes(acceptable_results, text) end def test_uri_escaping_of_name_action_in_a_tag_in_safe_list_sanitizer @@ -518,7 +564,13 @@ class SanitizersTest < Minitest::Test text = safe_list_sanitize(html, attributes: ['action']) - assert_equal %{test}, text + acceptable_results = [ + # nokogiri w/vendored+patched libxml2 + %{test}, + # nokogiri w/system libxml2 + %{test}, + ] + assert_includes(acceptable_results, text) end def test_exclude_node_type_processing_instructions @@ -529,6 +581,25 @@ class SanitizersTest < Minitest::Test assert_equal("
text
text", safe_list_sanitize("
text
text")) end + def test_disallow_the_dangerous_safelist_combination_of_select_and_style + input = "" + tags = ["select", "style"] + warning = /WARNING: Rails::Html::SafeListSanitizer: removing 'style' from safelist/ + sanitized = nil + invocation = Proc.new { sanitized = safe_list_sanitize(input, tags: tags) } + + if html5_mode? + # if Loofah is using an HTML5 parser, + # then "style" should be removed by the parser as an invalid child of "select" + assert_silent(&invocation) + else + # if Loofah is using an HTML4 parser, + # then SafeListSanitizer should remove "style" from the safelist + assert_output(nil, warning, &invocation) + end + refute_includes(sanitized, "style") + end + protected def xpath_sanitize(input, options = {}) @@ -567,6 +638,25 @@ protected Rails::Html::SafeListSanitizer.allowed_tags = old_tags end + # note that this is used for testing CSS hex encoding: \\[0-9a-f]{1,6} + def convert_to_css_hex(string, escape_parens=false) + string.chars.map do |c| + if !escape_parens && (c == "(" || c == ")") + c + else + format('\00%02X', c.ord) + end + end.join + end + + def libxml_2_9_14_recovery? + Nokogiri.method(:uses_libxml?).arity == -1 && Nokogiri.uses_libxml?(">= 2.9.14") + end + + def html5_mode? + ::Loofah.respond_to?(:html5_mode?) && ::Loofah.html5_mode? + end + def scope_allowed_attributes(attributes) old_attributes = Rails::Html::SafeListSanitizer.allowed_attributes Rails::Html::SafeListSanitizer.allowed_attributes = attributes -- 2.27.0