From 4ccc7742d8cbd95e620887d60b802a1d9a83d424 Mon Sep 17 00:00:00 2001 From: Nicolas Leger Date: Tue, 27 Mar 2018 12:43:51 +0200 Subject: [PATCH 01/52] [CI] Test against Ruby 2.5 --- .travis.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index d514f0f..7c5da59 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,9 +6,10 @@ rvm: - 1.9.3 - 2.0.0 - 2.1.10 - - 2.2.6 - - 2.3.3 - - 2.4.0 + - 2.2.9 + - 2.3.6 + - 2.4.3 + - 2.5.0 - ruby-head - jruby matrix: From e4e6cabeafa2a032e24f06e251d127d10d2cf9fa Mon Sep 17 00:00:00 2001 From: Nicolas Leger Date: Tue, 27 Mar 2018 13:02:35 +0200 Subject: [PATCH 02/52] [CI] Allow failure with ruby head --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 7c5da59..be7c3d2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -15,6 +15,7 @@ rvm: matrix: fast_finish: true allow_failures: + - rvm: ruby-head - rvm: jruby notifications: email: false From d4d823c617fdd0064956047f7fbf23fff305a69b Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Mon, 5 Nov 2018 17:56:54 +0100 Subject: [PATCH 03/52] [ci skip] Please don't send more PRs trying to bump Loofah. --- rails-html-sanitizer.gemspec | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rails-html-sanitizer.gemspec b/rails-html-sanitizer.gemspec index e774244..fe993ca 100644 --- a/rails-html-sanitizer.gemspec +++ b/rails-html-sanitizer.gemspec @@ -17,6 +17,8 @@ Gem::Specification.new do |spec| spec.test_files = Dir["test/**/*"] spec.require_paths = ["lib"] + # NOTE: There's no need to update this dependency for Loofah CVEs + # in minor releases when users can simply run `bundle update loofah`. spec.add_dependency "loofah", "~> 2.2", ">= 2.2.2" spec.add_development_dependency "bundler", "~> 1.3" From cba410ff71d2d0bdb03b59a6bca1d6222b3d0b4d Mon Sep 17 00:00:00 2001 From: Tebs Date: Tue, 26 Feb 2019 08:53:20 +0000 Subject: [PATCH 04/52] Fix Nokogiri link in documentation --- README.md | 2 +- lib/rails/html/scrubbers.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 5a6ffc2..ed01f05 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,7 @@ Loofah is what underlies the sanitizers and scrubbers of rails-html-sanitizer. - [Loofah and Loofah Scrubbers](https://github.com/flavorjones/loofah) The `node` argument passed to some methods in a custom scrubber is an instance of `Nokogiri::XML::Node`. -- [`Nokogiri::XML::Node`](http://nokogiri.org/Nokogiri/XML/Node.html) +- [`Nokogiri::XML::Node`](https://nokogiri.org/rdoc/Nokogiri/XML/Node.html) - [Nokogiri](http://nokogiri.org) ## Contributing to Rails Html Sanitizers diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 42436f4..a92c8a0 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -43,7 +43,7 @@ module Html # end # # See the documentation for Nokogiri::XML::Node to understand what's possible - # with nodes: http://nokogiri.org/Nokogiri/XML/Node.html + # with nodes: https://nokogiri.org/rdoc/Nokogiri/XML/Node.html class PermitScrubber < Loofah::Scrubber attr_reader :tags, :attributes From 89ae177f05f9336fd7af61b01e657404d4d1f60e Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Thu, 21 Mar 2019 10:16:05 +0900 Subject: [PATCH 05/52] Use a inclusive Bundler version Bundler 2.0 became the default on CI. This commit enables this gem with wider range of users. --- rails-html-sanitizer.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rails-html-sanitizer.gemspec b/rails-html-sanitizer.gemspec index fe993ca..4edbaa7 100644 --- a/rails-html-sanitizer.gemspec +++ b/rails-html-sanitizer.gemspec @@ -21,7 +21,7 @@ Gem::Specification.new do |spec| # in minor releases when users can simply run `bundle update loofah`. spec.add_dependency "loofah", "~> 2.2", ">= 2.2.2" - spec.add_development_dependency "bundler", "~> 1.3" + spec.add_development_dependency "bundler", ">= 1.3" spec.add_development_dependency "rake" spec.add_development_dependency "minitest" spec.add_development_dependency "rails-dom-testing" From 630d2f7806067037c16366106662e69ca7b25ade Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Thu, 21 Mar 2019 10:20:30 +0900 Subject: [PATCH 06/52] Update Ruby version matrix on CI https://github.com/postmodern/ruby-versions/blob/3fc3154ae59668e518d271db58c095332478795c/ruby/stable.txt --- .travis.yml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index be7c3d2..491a556 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,13 +3,11 @@ sudo: false cache: bundler before_install: gem update bundler --no-document rvm: - - 1.9.3 - - 2.0.0 - - 2.1.10 - - 2.2.9 - - 2.3.6 - - 2.4.3 - - 2.5.0 + - 2.2 + - 2.3 + - 2.4 + - 2.5 + - 2.6 - ruby-head - jruby matrix: From 3ca8a870d78e4e0adde8c5e27a695ca6670f14eb Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Tue, 2 Apr 2019 13:55:44 +0900 Subject: [PATCH 07/52] Update test behavior for Nokogiri > 1.9.1. Skip some tests for Ruby 2.2 because Nokogiri behavior changed. --- test/sanitizer_test.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 7bcab6f..340565e 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -485,35 +485,43 @@ def test_allow_data_attribute_if_requested end def test_uri_escaping_of_href_attr_in_a_tag_in_white_list_sanitizer + skip if RUBY_VERSION < "2.3" + html = %{test} text = white_list_sanitize(html) - assert_equal %{test}, text + assert_equal %{test}, text end def test_uri_escaping_of_src_attr_in_a_tag_in_white_list_sanitizer + skip if RUBY_VERSION < "2.3" + html = %{test} text = white_list_sanitize(html) - assert_equal %{test}, text + assert_equal %{test}, text end def test_uri_escaping_of_name_attr_in_a_tag_in_white_list_sanitizer + skip if RUBY_VERSION < "2.3" + html = %{test} text = white_list_sanitize(html) - assert_equal %{test}, text + assert_equal %{test}, text end def test_uri_escaping_of_name_action_in_a_tag_in_white_list_sanitizer + skip if RUBY_VERSION < "2.3" + html = %{test} text = white_list_sanitize(html, attributes: ['action']) - assert_equal %{test}, text + assert_equal %{test}, text end protected From 41b0b4972673b7f186bf1fd4426153a6f7f66f51 Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Thu, 21 Mar 2019 10:05:14 +0900 Subject: [PATCH 08/52] Migrate to SafeListSanitizer SafeList is easier to understand --- README.md | 20 +++++------ lib/rails-html-sanitizer.rb | 12 +++++-- lib/rails/html/sanitizer.rb | 39 +++++++++++--------- test/sanitizer_test.rb | 72 ++++++++++++++++++------------------- 4 files changed, 77 insertions(+), 66 deletions(-) diff --git a/README.md b/README.md index ed01f05..b3e9fb0 100644 --- a/README.md +++ b/README.md @@ -41,22 +41,22 @@ link_sanitizer.sanitize('Only the link text will be kept.< # => Only the link text will be kept. ``` -#### WhiteListSanitizer +#### SafeListSanitizer ```ruby -white_list_sanitizer = Rails::Html::WhiteListSanitizer.new +safe_list_sanitizer = Rails::Html::SafeListSanitizer.new -# sanitize via an extensive white list of allowed elements -white_list_sanitizer.sanitize(@article.body) +# sanitize via an extensive safe list of allowed elements +safe_list_sanitizer.sanitize(@article.body) -# white list only the supplied tags and attributes -white_list_sanitizer.sanitize(@article.body, tags: %w(table tr td), attributes: %w(id class style)) +# safe list only the supplied tags and attributes +safe_list_sanitizer.sanitize(@article.body, tags: %w(table tr td), attributes: %w(id class style)) -# white list via a custom scrubber -white_list_sanitizer.sanitize(@article.body, scrubber: ArticleScrubber.new) +# safe list via a custom scrubber +safe_list_sanitizer.sanitize(@article.body, scrubber: ArticleScrubber.new) -# white list sanitizer can also sanitize css -white_list_sanitizer.sanitize_css('background-color: #000;') +# safe list sanitizer can also sanitize css +safe_list_sanitizer.sanitize_css('background-color: #000;') ``` ### Scrubbers diff --git a/lib/rails-html-sanitizer.rb b/lib/rails-html-sanitizer.rb index 494a56f..f8d1736 100644 --- a/lib/rails-html-sanitizer.rb +++ b/lib/rails-html-sanitizer.rb @@ -15,8 +15,14 @@ def link_sanitizer Html::LinkSanitizer end + def safe_list_sanitizer + Html::SafeListSanitizer + end + def white_list_sanitizer - Html::WhiteListSanitizer + ActiveSupport::Deprecation.warn "warning: white_list_sanitizer is" \ + "deprecated, please use safe_list_sanitizer instead." + safe_list_sanitizer end end end @@ -34,7 +40,7 @@ module ClassMethods # end # def sanitized_allowed_tags=(tags) - sanitizer_vendor.white_list_sanitizer.allowed_tags = tags + sanitizer_vendor.safe_list_sanitizer.allowed_tags = tags end # Replaces the allowed HTML attributes for the +sanitize+ helper. @@ -44,7 +50,7 @@ def sanitized_allowed_tags=(tags) # end # def sanitized_allowed_attributes=(attributes) - sanitizer_vendor.white_list_sanitizer.allowed_attributes = attributes + sanitizer_vendor.safe_list_sanitizer.allowed_attributes = attributes end [:protocol_separator, diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index 7fc6994..f9bef9f 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -57,8 +57,8 @@ def sanitize(html, options = {}) end end - # === Rails::Html::WhiteListSanitizer - # Sanitizes html and css from an extensive white list (see link further down). + # === Rails::Html::SafeListSanitizer + # Sanitizes html and css from an extensive safe list (see link further down). # # === Whitespace # We can't make any guarantees about whitespace being kept or stripped. @@ -72,34 +72,34 @@ def sanitize(html, options = {}) # so automatically. # # === Options - # Sanitizes both html and css via the white lists found here: + # Sanitizes both html and css via the safe lists found here: # https://github.com/flavorjones/loofah/blob/master/lib/loofah/html5/whitelist.rb # - # WhiteListSanitizer also accepts options to configure - # the white list used when sanitizing html. + # SafeListSanitizer also accepts options to configure + # the safe list used when sanitizing html. # There's a class level option: - # Rails::Html::WhiteListSanitizer.allowed_tags = %w(table tr td) - # Rails::Html::WhiteListSanitizer.allowed_attributes = %w(id class style) + # Rails::Html::SafeListSanitizer.allowed_tags = %w(table tr td) + # Rails::Html::SafeListSanitizer.allowed_attributes = %w(id class style) # # Tags and attributes can also be passed to +sanitize+. # Passed options take precedence over the class level options. # # === Examples - # white_list_sanitizer = Rails::Html::WhiteListSanitizer.new + # safe_list_sanitizer = Rails::Html::SafeListSanitizer.new # # Sanitize css doesn't take options - # white_list_sanitizer.sanitize_css('background-color: #000;') + # safe_list_sanitizer.sanitize_css('background-color: #000;') # - # Default: sanitize via a extensive white list of allowed elements - # white_list_sanitizer.sanitize(@article.body) + # Default: sanitize via a extensive safe list of allowed elements + # safe_list_sanitizer.sanitize(@article.body) # - # White list via the supplied tags and attributes - # white_list_sanitizer.sanitize(@article.body, tags: %w(table tr td), + # Safe list via the supplied tags and attributes + # safe_list_sanitizer.sanitize(@article.body, tags: %w(table tr td), # attributes: %w(id class style)) # - # White list via a custom scrubber - # white_list_sanitizer.sanitize(@article.body, scrubber: ArticleScrubber.new) - class WhiteListSanitizer < Sanitizer + # Safe list via a custom scrubber + # safe_list_sanitizer.sanitize(@article.body, scrubber: ArticleScrubber.new) + class SafeListSanitizer < Sanitizer class << self attr_accessor :allowed_tags attr_accessor :allowed_attributes @@ -146,7 +146,12 @@ def allowed_tags(options) def allowed_attributes(options) options[:attributes] || self.class.allowed_attributes - end + end + end + + WhiteListSanitizer = SafeListSanitizer + if Object.respond_to?(:deprecate_constant) + deprecate_constant :WhiteListSanitizer end end end diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 340565e..6aa0509 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -12,12 +12,12 @@ def test_sanitizer_sanitize_raises_not_implemented_error end def test_sanitize_nested_script - sanitizer = Rails::Html::WhiteListSanitizer.new + sanitizer = Rails::Html::SafeListSanitizer.new assert_equal '<script>alert("XSS");</script>', sanitizer.sanitize('alert("XSS");/', tags: %w(em)) end def test_sanitize_nested_script_in_style - sanitizer = Rails::Html::WhiteListSanitizer.new + sanitizer = Rails::Html::SafeListSanitizer.new assert_equal '<script>alert("XSS");</script>', sanitizer.sanitize('alert("XSS");/', tags: %w(em)) end @@ -255,38 +255,38 @@ def test_custom_attributes_overrides_allowed_attributes def test_should_allow_custom_tags text = "foo" - assert_equal text, white_list_sanitize(text, tags: %w(u)) + assert_equal text, safe_list_sanitize(text, tags: %w(u)) end def test_should_allow_only_custom_tags text = "foo with bar" - assert_equal "foo with bar", white_list_sanitize(text, tags: %w(u)) + assert_equal "foo with bar", safe_list_sanitize(text, tags: %w(u)) end def test_should_allow_custom_tags_with_attributes text = %(
foo
) - assert_equal text, white_list_sanitize(text) + assert_equal text, safe_list_sanitize(text) end def test_should_allow_custom_tags_with_custom_attributes text = %(
Lorem ipsum
) - assert_equal text, white_list_sanitize(text, attributes: ['foo']) + assert_equal text, safe_list_sanitize(text, attributes: ['foo']) end def test_scrub_style_if_style_attribute_option_is_passed input = '

' - assert_equal '

', white_list_sanitize(input, attributes: %w(style)) + assert_equal '

', safe_list_sanitize(input, attributes: %w(style)) end def test_should_raise_argument_error_if_tags_is_not_enumerable assert_raises ArgumentError do - white_list_sanitize('some html', tags: 'foo') + safe_list_sanitize('some html', tags: 'foo') end end def test_should_raise_argument_error_if_attributes_is_not_enumerable assert_raises ArgumentError do - white_list_sanitize('some html', attributes: 'foo') + safe_list_sanitize('some html', attributes: 'foo') end end @@ -295,7 +295,7 @@ def test_should_not_accept_non_loofah_inheriting_scrubber def scrubber.scrub(node); node.name = 'h1'; end assert_raises Loofah::ScrubberNotFound do - white_list_sanitize('some html', scrubber: scrubber) + safe_list_sanitize('some html', scrubber: scrubber) end end @@ -304,19 +304,19 @@ def test_should_accept_loofah_inheriting_scrubber def scrubber.scrub(node); node.name = 'h1'; end html = "" - assert_equal "

hello!

", white_list_sanitize(html, scrubber: scrubber) + assert_equal "

hello!

", safe_list_sanitize(html, scrubber: scrubber) end def test_should_accept_loofah_scrubber_that_wraps_a_block scrubber = Loofah::Scrubber.new { |node| node.name = 'h1' } html = "" - assert_equal "

hello!

", white_list_sanitize(html, scrubber: scrubber) + assert_equal "

hello!

", safe_list_sanitize(html, scrubber: scrubber) end def test_custom_scrubber_takes_precedence_over_other_options scrubber = Loofah::Scrubber.new { |node| node.name = 'h1' } html = "" - assert_equal "

hello!

", white_list_sanitize(html, scrubber: scrubber, tags: ['foo']) + assert_equal "

hello!

", safe_list_sanitize(html, scrubber: scrubber, tags: ['foo']) end [%w(img src), %w(a href)].each do |(tag, attr)| @@ -468,7 +468,7 @@ def test_x03a_legitimate end def test_sanitize_ascii_8bit_string - white_list_sanitize('hello'.encode('ASCII-8BIT')).tap do |sanitized| + safe_list_sanitize('hello'.encode('ASCII-8BIT')).tap do |sanitized| assert_equal 'hello', sanitized assert_equal Encoding::UTF_8, sanitized.encoding end @@ -481,45 +481,45 @@ def test_sanitize_data_attributes def test_allow_data_attribute_if_requested text = %(foo) - assert_equal %(foo), white_list_sanitize(text, attributes: ['data-foo']) + assert_equal %(foo), safe_list_sanitize(text, attributes: ['data-foo']) end - def test_uri_escaping_of_href_attr_in_a_tag_in_white_list_sanitizer + def test_uri_escaping_of_href_attr_in_a_tag_in_safe_list_sanitizer skip if RUBY_VERSION < "2.3" html = %{test} - text = white_list_sanitize(html) + text = safe_list_sanitize(html) assert_equal %{test}, text end - def test_uri_escaping_of_src_attr_in_a_tag_in_white_list_sanitizer + def test_uri_escaping_of_src_attr_in_a_tag_in_safe_list_sanitizer skip if RUBY_VERSION < "2.3" html = %{test} - text = white_list_sanitize(html) + text = safe_list_sanitize(html) assert_equal %{test}, text end - def test_uri_escaping_of_name_attr_in_a_tag_in_white_list_sanitizer + def test_uri_escaping_of_name_attr_in_a_tag_in_safe_list_sanitizer skip if RUBY_VERSION < "2.3" html = %{test} - text = white_list_sanitize(html) + text = safe_list_sanitize(html) assert_equal %{test}, text end - def test_uri_escaping_of_name_action_in_a_tag_in_white_list_sanitizer + def test_uri_escaping_of_name_action_in_a_tag_in_safe_list_sanitizer skip if RUBY_VERSION < "2.3" html = %{test} - text = white_list_sanitize(html, attributes: ['action']) + text = safe_list_sanitize(html, attributes: ['action']) assert_equal %{test}, text end @@ -538,35 +538,35 @@ def link_sanitize(input, options = {}) Rails::Html::LinkSanitizer.new.sanitize(input, options) end - def white_list_sanitize(input, options = {}) - Rails::Html::WhiteListSanitizer.new.sanitize(input, options) + def safe_list_sanitize(input, options = {}) + Rails::Html::SafeListSanitizer.new.sanitize(input, options) end def assert_sanitized(input, expected = nil) if input - assert_dom_equal expected || input, white_list_sanitize(input) + assert_dom_equal expected || input, safe_list_sanitize(input) else - assert_nil white_list_sanitize(input) + assert_nil safe_list_sanitize(input) end end def sanitize_css(input) - Rails::Html::WhiteListSanitizer.new.sanitize_css(input) + Rails::Html::SafeListSanitizer.new.sanitize_css(input) end def scope_allowed_tags(tags) - old_tags = Rails::Html::WhiteListSanitizer.allowed_tags - Rails::Html::WhiteListSanitizer.allowed_tags = tags - yield Rails::Html::WhiteListSanitizer.new + old_tags = Rails::Html::SafeListSanitizer.allowed_tags + Rails::Html::SafeListSanitizer.allowed_tags = tags + yield Rails::Html::SafeListSanitizer.new ensure - Rails::Html::WhiteListSanitizer.allowed_tags = old_tags + Rails::Html::SafeListSanitizer.allowed_tags = old_tags end def scope_allowed_attributes(attributes) - old_attributes = Rails::Html::WhiteListSanitizer.allowed_attributes - Rails::Html::WhiteListSanitizer.allowed_attributes = attributes - yield Rails::Html::WhiteListSanitizer.new + old_attributes = Rails::Html::SafeListSanitizer.allowed_attributes + Rails::Html::SafeListSanitizer.allowed_attributes = attributes + yield Rails::Html::SafeListSanitizer.new ensure - Rails::Html::WhiteListSanitizer.allowed_attributes = old_attributes + Rails::Html::SafeListSanitizer.allowed_attributes = old_attributes end end From b3c4f7d4951c7727c750c0cc5ec1ef98806a9bcf Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Fri, 10 May 2019 13:40:01 +0900 Subject: [PATCH 09/52] Improve Scrubber documentations - Fix code snippet of PermitScrubber - Wraps code references --- lib/rails/html/scrubbers.rb | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index a92c8a0..6050235 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -2,9 +2,9 @@ module Rails module Html # === Rails::Html::PermitScrubber # - # Rails::Html::PermitScrubber allows you to permit only your own tags and/or attributes. + # +Rails::Html::PermitScrubber+ allows you to permit only your own tags and/or attributes. # - # Rails::Html::PermitScrubber can be subclassed to determine: + # +Rails::Html::PermitScrubber+ can be subclassed to determine: # - When a node should be skipped via +skip_node?+. # - When a node is allowed via +allowed_node?+. # - When an attribute should be scrubbed via +scrub_attribute?+. @@ -27,22 +27,22 @@ module Html # If set, attributes excluded will be removed. # If not, attributes are removed based on Loofahs +HTML5::Scrub.scrub_attributes+. # - # class CommentScrubber < Html::PermitScrubber - # def initialize - # super - # self.tags = %w(form script comment blockquote) - # end + # class CommentScrubber < Html::PermitScrubber + # def initialize + # super + # self.tags = %w(form script comment blockquote) + # end # - # def skip_node?(node) - # node.text? - # end + # def skip_node?(node) + # node.text? + # end # - # def scrub_attribute?(name) - # name == "style" - # end - # end + # def scrub_attribute?(name) + # name == "style" + # end + # end # - # See the documentation for Nokogiri::XML::Node to understand what's possible + # See the documentation for +Nokogiri::XML::Node+ to understand what's possible # with nodes: https://nokogiri.org/rdoc/Nokogiri/XML/Node.html class PermitScrubber < Loofah::Scrubber attr_reader :tags, :attributes @@ -160,8 +160,8 @@ def scrub_attribute(node, attr_node) # === Rails::Html::TargetScrubber # - # Where Rails::Html::PermitScrubber picks out tags and attributes to permit in - # sanitization, Rails::Html::TargetScrubber targets them for removal. + # Where +Rails::Html::PermitScrubber+ picks out tags and attributes to permit in + # sanitization, +Rails::Html::TargetScrubber+ targets them for removal. # # +tags=+ # If set, elements included will be stripped. @@ -180,7 +180,7 @@ def scrub_attribute?(name) # === Rails::Html::TextOnlyScrubber # - # Rails::Html::TextOnlyScrubber allows you to permit text nodes. + # +Rails::Html::TextOnlyScrubber+ allows you to permit text nodes. # # Unallowed elements will be stripped, i.e. element is removed but its subtree kept. class TextOnlyScrubber < Loofah::Scrubber From 2523282e1e498fc39f395433877c94e09f106338 Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Fri, 10 May 2019 15:15:08 +0900 Subject: [PATCH 10/52] href is not a HTML element https://developer.mozilla.org/en-US/docs/Web/HTML/Element --- lib/rails/html/sanitizer.rb | 4 ++-- test/sanitizer_test.rb | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index f9bef9f..e73bb8c 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -48,7 +48,7 @@ def sanitize(html, options = {}) class LinkSanitizer < Sanitizer def initialize @link_scrubber = TargetScrubber.new - @link_scrubber.tags = %w(a href) + @link_scrubber.tags = %w(a) @link_scrubber.attributes = %w(href) end @@ -146,7 +146,7 @@ def allowed_tags(options) def allowed_attributes(options) options[:attributes] || self.class.allowed_attributes - end + end end WhiteListSanitizer = SafeListSanitizer diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 6aa0509..8c579af 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -154,10 +154,6 @@ def test_strip_links_with_linkception assert_equal "Magic", link_sanitize("Magic") end - def test_strip_links_with_a_tag_in_href - assert_equal "FrrFox", link_sanitize("FrrFox") - end - def test_sanitize_form assert_sanitized "
", '' end From 5d735a7693d8bf3d016464e127299a5da549824f Mon Sep 17 00:00:00 2001 From: Juanito Fatas Date: Fri, 10 May 2019 15:17:04 +0900 Subject: [PATCH 11/52] Improve LinkSanitizer's documentation --- lib/rails/html/sanitizer.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index e73bb8c..a539102 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -40,11 +40,12 @@ def sanitize(html, options = {}) end # === Rails::Html::LinkSanitizer - # Removes a tags and href attributes leaving only the link text + # Removes +a+ tags and +href+ attributes leaving only the link text. # - # link_sanitizer = Rails::Html::LinkSanitizer.new - # link_sanitizer.sanitize('Only the link text will be kept.') - # # => Only the link text will be kept. + # link_sanitizer = Rails::Html::LinkSanitizer.new + # link_sanitizer.sanitize('Only the link text will be kept.') + # + # => 'Only the link text will be kept.' class LinkSanitizer < Sanitizer def initialize @link_scrubber = TargetScrubber.new From df0c946aa0c1913e9b8e94be96da59fb57ec9d67 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Mon, 5 Aug 2019 03:07:55 +0200 Subject: [PATCH 12/52] Prepare version 1.1.0 --- CHANGELOG.md | 17 +++++++++++++++++ lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 547d8bb..1ca2f38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,20 @@ +## 1.1.0 + +* Add `safe_list_sanitizer` and deprecate `white_list_sanitizer` to be removed + in 1.2.0. https://github.com/rails/rails-html-sanitizer/pull/87 + + *Juanito Fatas* + +* Remove `href` from LinkScrubber's `tags` as it's not an element. + https://github.com/rails/rails-html-sanitizer/pull/92 + + *Juanito Fatas* + +* Explain that we don't need to bump Loofah here if there's CVEs. + https://github.com/rails/rails-html-sanitizer/commit/d4d823c617fdd0064956047f7fbf23fff305a69b + + *Kasper Timm Hansen* + ## 1.0.1 * Added support for Rails 4.2.0.beta2 and above diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index 9a815d6..052479d 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.0.4" + VERSION = "1.1.0" end end end From 21da0389eae482818d6991eb1bba025c8b116927 Mon Sep 17 00:00:00 2001 From: rwojnarowski Date: Mon, 5 Aug 2019 13:34:11 +0200 Subject: [PATCH 13/52] Deprecated warning text, missing space --- lib/rails-html-sanitizer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rails-html-sanitizer.rb b/lib/rails-html-sanitizer.rb index f8d1736..39ac71d 100644 --- a/lib/rails-html-sanitizer.rb +++ b/lib/rails-html-sanitizer.rb @@ -21,7 +21,7 @@ def safe_list_sanitizer def white_list_sanitizer ActiveSupport::Deprecation.warn "warning: white_list_sanitizer is" \ - "deprecated, please use safe_list_sanitizer instead." + " deprecated, please use safe_list_sanitizer instead." safe_list_sanitizer end end From 31cf584cd67419723ecb16d8505bcebde1b3c444 Mon Sep 17 00:00:00 2001 From: Olle Jonsson Date: Mon, 5 Aug 2019 17:25:55 +0200 Subject: [PATCH 14/52] CI: Drop unused sudo: false Travis directive See https://blog.travis-ci.com/2018-11-19-required-linux-infrastructure-migration for more details --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 491a556..3e0b723 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,4 @@ language: ruby -sudo: false cache: bundler before_install: gem update bundler --no-document rvm: From 55818711cce9e7d7185a116abe7b0cada7414785 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Thu, 8 Aug 2019 23:56:28 +0200 Subject: [PATCH 15/52] Remove needless white list sanitizer deprecations Keeping support for these methods, without deprecations, makes Rails 5.2 just work. Then Rails 6 will depend on 1.2.0 which uses safe_list_sanitizer etc. Sorry for all the needless ruckus and confusion around 1.1.0! --- lib/rails-html-sanitizer.rb | 2 -- lib/rails/html/sanitizer.rb | 3 --- 2 files changed, 5 deletions(-) diff --git a/lib/rails-html-sanitizer.rb b/lib/rails-html-sanitizer.rb index 39ac71d..59ed70d 100644 --- a/lib/rails-html-sanitizer.rb +++ b/lib/rails-html-sanitizer.rb @@ -20,8 +20,6 @@ def safe_list_sanitizer end def white_list_sanitizer - ActiveSupport::Deprecation.warn "warning: white_list_sanitizer is" \ - " deprecated, please use safe_list_sanitizer instead." safe_list_sanitizer end end diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index a539102..b35010a 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -151,8 +151,5 @@ def allowed_attributes(options) end WhiteListSanitizer = SafeListSanitizer - if Object.respond_to?(:deprecate_constant) - deprecate_constant :WhiteListSanitizer - end end end From b8ea80d5f840a834a808a2171df3ada524b2a010 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Fri, 9 Aug 2019 00:02:02 +0200 Subject: [PATCH 16/52] Prepare 1.2.0 --- CHANGELOG.md | 14 ++++++++++++++ lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ca2f38..45d9c69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## 1.2.0 + +* Remove needless `white_list_sanitizer` deprecation. + + By deprecating this, we were forcing Rails 5.2 to be updated or spew + deprecations that users could do nothing about. + + That's pointless and I'm sorry for adding that! + + Now there's no deprecation warning and Rails 5.2 works out of the box, while + Rails 6 can use the updated naming. + + *Kasper Timm Hansen* + ## 1.1.0 * Add `safe_list_sanitizer` and deprecate `white_list_sanitizer` to be removed diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index 052479d..3482107 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.1.0" + VERSION = "1.2.0" end end end From 43a87f538af42dd11cd72fddacbba2519d1e5c78 Mon Sep 17 00:00:00 2001 From: Josh Goodall Date: Sun, 29 Sep 2019 20:20:30 +1000 Subject: [PATCH 17/52] Match Loofah's API changes. Short term, reduces log noise due to deprecated constants. Long term, necessary just to keep up. --- lib/rails/html/sanitizer.rb | 2 +- lib/rails/html/scrubbers.rb | 8 ++++---- rails-html-sanitizer.gemspec | 2 +- test/sanitizer_test.rb | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index b35010a..5633ca1 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -74,7 +74,7 @@ def sanitize(html, options = {}) # # === Options # Sanitizes both html and css via the safe lists found here: - # https://github.com/flavorjones/loofah/blob/master/lib/loofah/html5/whitelist.rb + # https://github.com/flavorjones/loofah/blob/master/lib/loofah/html5/safelist.rb # # SafeListSanitizer also accepts options to configure # the safe list used when sanitizing html. diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 6050235..c44d0ee 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -138,17 +138,17 @@ def scrub_attribute(node, attr_node) attr_node.node_name end - if Loofah::HTML5::WhiteList::ATTR_VAL_IS_URI.include?(attr_name) + if Loofah::HTML5::SafeList::ATTR_VAL_IS_URI.include?(attr_name) # this block lifted nearly verbatim from HTML5 sanitization val_unescaped = CGI.unescapeHTML(attr_node.value).gsub(Loofah::HTML5::Scrub::CONTROL_CHARACTERS,'').downcase - if val_unescaped =~ /^[a-z0-9][-+.a-z0-9]*:/ && ! Loofah::HTML5::WhiteList::ALLOWED_PROTOCOLS.include?(val_unescaped.split(Loofah::HTML5::WhiteList::PROTOCOL_SEPARATOR)[0]) + if val_unescaped =~ /^[a-z0-9][-+.a-z0-9]*:/ && ! Loofah::HTML5::SafeList::ALLOWED_PROTOCOLS.include?(val_unescaped.split(Loofah::HTML5::SafeList::PROTOCOL_SEPARATOR)[0]) attr_node.remove end end - if Loofah::HTML5::WhiteList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name) + if Loofah::HTML5::SafeList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name) attr_node.value = attr_node.value.gsub(/url\s*\(\s*[^#\s][^)]+?\)/m, ' ') if attr_node.value end - if Loofah::HTML5::WhiteList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == 'xlink:href' && attr_node.value =~ /^\s*[^#\s].*/m + if Loofah::HTML5::SafeList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == 'xlink:href' && attr_node.value =~ /^\s*[^#\s].*/m attr_node.remove end diff --git a/rails-html-sanitizer.gemspec b/rails-html-sanitizer.gemspec index 4edbaa7..317f38b 100644 --- a/rails-html-sanitizer.gemspec +++ b/rails-html-sanitizer.gemspec @@ -19,7 +19,7 @@ Gem::Specification.new do |spec| # NOTE: There's no need to update this dependency for Loofah CVEs # in minor releases when users can simply run `bundle update loofah`. - spec.add_dependency "loofah", "~> 2.2", ">= 2.2.2" + spec.add_dependency "loofah", "~> 2.3" spec.add_development_dependency "bundler", ">= 1.3" spec.add_development_dependency "rake" diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 8c579af..6d44008 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -181,7 +181,7 @@ def test_sanitize_image_src assert_sanitized raw, %{src="javascript:bang" foo, bar} end - tags = Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS - %w(script form) + tags = Loofah::HTML5::SafeList::ALLOWED_ELEMENTS - %w(script form) tags.each do |tag_name| define_method "test_should_allow_#{tag_name}_tag" do scope_allowed_tags(tags) do From 845da04cad37885aadc4495c8e7e476de54aa803 Mon Sep 17 00:00:00 2001 From: Orien Madgwick <_@orien.io> Date: Sat, 5 Oct 2019 12:14:39 +1000 Subject: [PATCH 18/52] Add project metadata to the gemspec As per https://guides.rubygems.org/specification-reference/#metadata, add metadata to the gemspec file. This'll allow people to more easily access the source code, raise issues and read the changelog. These `bug_tracker_uri`, `changelog_uri`, `documentation_uri`, and `source_code_uri` links will appear on the rubygems page at https://rubygems.org/gems/rails-html-sanitizer and be available via the rubygems API after the next release. --- rails-html-sanitizer.gemspec | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rails-html-sanitizer.gemspec b/rails-html-sanitizer.gemspec index 317f38b..c9637b7 100644 --- a/rails-html-sanitizer.gemspec +++ b/rails-html-sanitizer.gemspec @@ -13,6 +13,13 @@ Gem::Specification.new do |spec| spec.homepage = "https://github.com/rails/rails-html-sanitizer" spec.license = "MIT" + spec.metadata = { + "bug_tracker_uri" => "https://github.com/rails/rails-html-sanitizer/issues", + "changelog_uri" => "https://github.com/rails/rails-html-sanitizer/blob/v#{spec.version}/CHANGELOG.md", + "documentation_uri" => "https://www.rubydoc.info/gems/rails-html-sanitizer/#{spec.version}", + "source_code_uri" => "https://github.com/rails/rails-html-sanitizer/tree/v#{spec.version}", + } + spec.files = Dir["lib/**/*", "README.md", "MIT-LICENSE", "CHANGELOG.md"] spec.test_files = Dir["test/**/*"] spec.require_paths = ["lib"] From 51dc564c6509201070f72456bb2c13f87bb373d6 Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Sun, 6 Oct 2019 11:12:06 -0400 Subject: [PATCH 19/52] v1.3.0 --- CHANGELOG.md | 6 ++++++ lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45d9c69..9af8242 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.3.0 + +* Address deprecations in Loofah 2.3.0. + + *Josh Goodall* + ## 1.2.0 * Remove needless `white_list_sanitizer` deprecation. diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index 3482107..f0c553d 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.2.0" + VERSION = "1.3.0" end end end From 1e64885d30668165c63cccc4e371d1fc2755324a Mon Sep 17 00:00:00 2001 From: Paul Mesnilgrente Date: Tue, 9 Feb 2021 18:03:27 +0100 Subject: [PATCH 20/52] Add a note for whitelisted tags by default in the TargetScrubber (#110) * add a note for whitelisted tags by default in the TargetScrubber * reword the whitelisted to permitted Co-authored-by: Kasper Timm Hansen * added the permitted tag list to the README Co-authored-by: Kasper Timm Hansen --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b3e9fb0..7b160b5 100644 --- a/README.md +++ b/README.md @@ -81,8 +81,10 @@ html_fragment.to_s # => "" #### `Rails::Html::TargetScrubber` Where `PermitScrubber` picks out tags and attributes to permit in sanitization, -`Rails::Html::TargetScrubber` targets them for removal. +`Rails::Html::TargetScrubber` targets them for removal. See https://github.com/flavorjones/loofah/blob/main/lib/loofah/html5/safelist.rb for the tag list. +**Note:** by default, it will scrub anything that is not part of the permitted tags from +loofah `HTML5::Scrub.allowed_element?`. ```ruby scrubber = Rails::Html::TargetScrubber.new From becbad07e734cfbc01cda98ef6d1c87a205df6d2 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 8 Apr 2021 11:39:11 -0400 Subject: [PATCH 21/52] test: handle variations in loofah whitespace which changed in Loofah v2.9.0 Related to #111 --- test/sanitizer_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 6d44008..2a45d98 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -271,7 +271,8 @@ def test_should_allow_custom_tags_with_custom_attributes def test_scrub_style_if_style_attribute_option_is_passed input = '

' - assert_equal '

', safe_list_sanitize(input, attributes: %w(style)) + actual = safe_list_sanitize(input, attributes: %w(style)) + assert_includes(['

', '

'], actual) end def test_should_raise_argument_error_if_tags_is_not_enumerable From c190b3247afe43329e696120a578cee8ac9c3fd4 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 8 Apr 2021 11:39:43 -0400 Subject: [PATCH 22/52] test: fix encoding in the unicode XSS test See https://github.com/flavorjones/loofah/pull/205 for a short history of this test string. Related to #111 --- test/sanitizer_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 2a45d98..d81ab2a 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -414,7 +414,7 @@ def test_should_sanitize_img_dynsrc_lowsrc end def test_should_sanitize_div_background_image_unicode_encoded - raw = %(background-image:\0075\0072\006C\0028'\006a\0061\0076\0061\0073\0063\0072\0069\0070\0074\003a\0061\006c\0065\0072\0074\0028.1027\0058.1053\0053\0027\0029'\0029) + raw = %(background-image:\u0075\u0072\u006C\u0028'\u006a\u0061\u0076\u0061\u0073\u0063\u0072\u0069\u0070\u0074\u003a\u0061\u006c\u0065\u0072\u0074\u0028.1027\u0058.1053\u0053\u0027\u0029'\u0029) assert_equal '', sanitize_css(raw) end From f59ecbcfc8300ce040efead7147af03283453b73 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 8 Apr 2021 11:53:34 -0400 Subject: [PATCH 23/52] test: fix typographical errors in the XSS encoding test See https://github.com/flavorjones/loofah/pull/205 for a short history of this test string. Related to #111 --- test/sanitizer_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index d81ab2a..c8a02c6 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -414,7 +414,7 @@ def test_should_sanitize_img_dynsrc_lowsrc end def test_should_sanitize_div_background_image_unicode_encoded - raw = %(background-image:\u0075\u0072\u006C\u0028'\u006a\u0061\u0076\u0061\u0073\u0063\u0072\u0069\u0070\u0074\u003a\u0061\u006c\u0065\u0072\u0074\u0028.1027\u0058.1053\u0053\u0027\u0029'\u0029) + 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) end From e1f3af01549e4ff743a7f64ba03f8521d973b447 Mon Sep 17 00:00:00 2001 From: John Bampton Date: Wed, 14 Apr 2021 05:32:24 +1000 Subject: [PATCH 24/52] chore: fix spelling --- test/sanitizer_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 6d44008..fb75ccb 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -93,7 +93,7 @@ def test_strip_blank_string end def test_strip_tags_with_plaintext - assert_equal "Dont touch me", full_sanitize("Dont touch me") + assert_equal "Don't touch me", full_sanitize("Don't touch me") end def test_strip_tags_with_tags @@ -135,7 +135,7 @@ def test_strip_links_with_unclosed_tags end def test_strip_links_with_plaintext - assert_equal "Dont touch me", link_sanitize("Dont touch me") + assert_equal "Don't touch me", link_sanitize("Don't touch me") end def test_strip_links_with_line_feed_and_uppercase_tag From c06d465f577818b438c1a475a0b5e1dbcbe5408a Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 18 Jul 2021 23:15:14 -0400 Subject: [PATCH 25/52] PermitScrubber does not permit Processing Instructions Fixes #115 --- CHANGELOG.md | 12 ++++++++++++ lib/rails/html/scrubbers.rb | 2 +- test/sanitizer_test.rb | 8 ++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9af8242..7fa6422 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +## next / unreleased + +* Processing Instructions are no longer allowed by Rails::Html::PermitScrubber + + Previously, a PI with a name (or "target") matching an allowed tag name was not scrubbed. There + are no known security issues associated with these PIs, but similar to comments it's preferred to + omit these nodes when possible from sanitized output. + + Fixes #115. + + *Mike Dalessio* + ## 1.3.0 * Address deprecations in Loofah 2.3.0. diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index c44d0ee..ad27971 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -68,7 +68,7 @@ def scrub(node) end return CONTINUE if skip_node?(node) - unless keep_node?(node) + unless node.element? && keep_node?(node) return STOP if scrub_node(node) == STOP end diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index f3624e7..7938433 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -521,6 +521,14 @@ def test_uri_escaping_of_name_action_in_a_tag_in_safe_list_sanitizer assert_equal %{test}, text end + def test_exclude_node_type_processing_instructions + assert_equal("
text
text", safe_list_sanitize("
text
text")) + end + + def test_exclude_node_type_comment + assert_equal("
text
text", safe_list_sanitize("
text
text")) + end + protected def xpath_sanitize(input, options = {}) From 2a7d3f208601fa069f804c54744b8f33df901773 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 18 Jul 2021 23:19:14 -0400 Subject: [PATCH 26/52] CI: add modern rubies to the matrix --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index 3e0b723..9b992ac 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,6 +7,8 @@ rvm: - 2.4 - 2.5 - 2.6 + - 2.7 + - 3.0 - ruby-head - jruby matrix: From 2e9ec19859c03c15c912732e5528ea0e8a7326da Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Aug 2021 13:10:27 -0400 Subject: [PATCH 27/52] version bump to v1.4.0 --- CHANGELOG.md | 2 +- lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa6422..a2398cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## next / unreleased +## 1.4.0 / 2021-08-18 * Processing Instructions are no longer allowed by Rails::Html::PermitScrubber diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index f0c553d..fc59d97 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.3.0" + VERSION = "1.4.0" end end end From ab16fa47d7d9d96c13a72882964bcbfb530f1922 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Aug 2021 15:58:06 -0400 Subject: [PATCH 28/52] fix: pass comment nodes to the scrubber Some scrubbers want to allow comments through, but in v1.4.0 didn't get the chance because only elements were passed through to `keep_node?`. This change allows comments and elements through, but still omits other non-elements like processing instructions (see #115). --- CHANGELOG.md | 12 ++++++++++ lib/rails/html/scrubbers.rb | 2 +- test/scrubbers_test.rb | 44 +++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2398cb..1bdebeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +## next / unreleased + +* Fix regression in v1.4.0 that did not pass comment nodes to the scrubber. + + Some scrubbers will want to override the default behavior and allow comments, but v1.4.0 only + passed through elements to the scrubber's `keep_node?` method. + + This change once again allows the scrubber to make the decision on comment nodes, but still skips + other non-elements like processing instructions (see #115). + + *Mike Dalessio* + ## 1.4.0 / 2021-08-18 * Processing Instructions are no longer allowed by Rails::Html::PermitScrubber diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index ad27971..385d357 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -68,7 +68,7 @@ def scrub(node) end return CONTINUE if skip_node?(node) - unless node.element? && keep_node?(node) + unless (node.comment? || node.element?) && keep_node?(node) return STOP if scrub_node(node) == STOP end diff --git a/test/scrubbers_test.rb b/test/scrubbers_test.rb index 4b84263..e6612e9 100644 --- a/test/scrubbers_test.rb +++ b/test/scrubbers_test.rb @@ -112,6 +112,50 @@ def test_attributes_accessor_validation end end +class PermitScrubberSubclassTest < ScrubberTest + def setup + @scrubber = Class.new(::Rails::Html::PermitScrubber) do + attr :nodes_seen + + def initialize + super() + @nodes_seen = [] + end + + def keep_node?(node) + @nodes_seen << node.name + super(node) + end + end.new + end + + def test_elements_are_checked + html = %Q("
") + Loofah.scrub_fragment(html, @scrubber) + assert_includes(@scrubber.nodes_seen, "div") + assert_includes(@scrubber.nodes_seen, "a") + assert_includes(@scrubber.nodes_seen, "tr") + end + + def test_comments_are_checked + # this passes in v1.3.0 but fails in v1.4.0 + html = %Q("
") + Loofah.scrub_fragment(html, @scrubber) + assert_includes(@scrubber.nodes_seen, "div") + assert_includes(@scrubber.nodes_seen, "comment") + assert_includes(@scrubber.nodes_seen, "tr") + end + + def test_craftily_named_processing_instructions_are_not_checked + # this fails in v1.3.0 but passes in v1.4.0 + html = %Q("
") + Loofah.scrub_fragment(html, @scrubber) + assert_includes(@scrubber.nodes_seen, "div") + refute_includes(@scrubber.nodes_seen, "a") + assert_includes(@scrubber.nodes_seen, "tr") + end +end + class TargetScrubberTest < ScrubberTest def setup @scrubber = Rails::Html::TargetScrubber.new From b41bc7a9d04190d4237aa263c9a2ff70afbcc5bf Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Aug 2021 16:51:54 -0400 Subject: [PATCH 29/52] version bump to v1.4.1 --- CHANGELOG.md | 2 +- lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bdebeb..58ae8ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## next / unreleased +## 1.4.1 / 2021-08-18 * Fix regression in v1.4.0 that did not pass comment nodes to the scrubber. diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index fc59d97..103c532 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.4.0" + VERSION = "1.4.1" end end end From 8d3db9211266fb87ab0a1d0015d91476b925df4f Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 20 Aug 2021 15:00:03 -0400 Subject: [PATCH 30/52] ci: add github actions workflow --- .github/workflows/ci.yml | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..dac037a --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,46 @@ +name: ci +concurrency: + group: "${{github.workflow}}-${{github.ref}}" + cancel-in-progress: true +on: + workflow_dispatch: + push: + branches: + - main + - v*.*.x + tags: + - v*.*.* + pull_request: + types: [opened, synchronize] + branches: + - '*' + +jobs: + cruby: + strategy: + fail-fast: false + matrix: + ruby: ["2.2", "2.3", "2.4", "2.5", "2.6", "2.7", "3.0", "ruby-head"] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{matrix.ruby}} + bundler-cache: true + - run: bundle exec rake + + jruby: + continue-on-error: true # nokogiri on jruby has different behavior + strategy: + fail-fast: false + matrix: + ruby: ["jruby-9.2", "jruby-head"] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{matrix.ruby}} + bundler-cache: true + - run: bundle exec rake From e73f3e940f267a9d399e48e7c4cfd6a536b01c1c Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 20 Aug 2021 15:46:32 -0400 Subject: [PATCH 31/52] ci: remove travis and update the CI badge --- .travis.yml | 30 ------------------------------ CONTRIBUTING.md | 4 ++-- 2 files changed, 2 insertions(+), 32 deletions(-) delete mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 9b992ac..0000000 --- a/.travis.yml +++ /dev/null @@ -1,30 +0,0 @@ -language: ruby -cache: bundler -before_install: gem update bundler --no-document -rvm: - - 2.2 - - 2.3 - - 2.4 - - 2.5 - - 2.6 - - 2.7 - - 3.0 - - ruby-head - - jruby -matrix: - fast_finish: true - allow_failures: - - rvm: ruby-head - - rvm: jruby -notifications: - email: false - irc: - on_success: change - on_failure: always - channels: - - "irc.freenode.org#rails-contrib" - campfire: - on_success: change - on_failure: always - rooms: - - secure: "VZlPFPKZPJtzHnGpoV/BV2IGbJjplemHgqDciE2jRtwepjZMekqLgaUaH9CWUggQdaVCMQI0XLl0qrU54SMBNjE2tdx3WiAzN83oWVCBg4XYogY1U0N0/92egld6UcVAK7HKYzSGYj6hqxmkCn8c8reih7eUFCFUsn/5D/FPlI4=" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 40f4dd1..8885691 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,7 +1,7 @@ Contributing to Rails Html Sanitizers ===================== -[![Build Status](https://travis-ci.org/rails/rails-html-sanitizer.svg?branch=master)](https://travis-ci.org/rails/rails-html-sanitizer) +[![Build Status](https://github.com/rails/rails-html-sanitizer/actions/workflows/ci.yml/badge.svg)](https://github.com/rails/rails-html-sanitizer/actions/workflows/ci.yml) Rails Html Sanitizers is work of [many contributors](https://github.com/rails/rails-html-sanitizer/graphs/contributors). You're encouraged to submit [pull requests](https://github.com/rails/rails-html-sanitizer/pulls), [propose features and discuss issues](https://github.com/rails/rails-html-sanitizer/issues). @@ -88,7 +88,7 @@ git push origin my-feature-branch -f #### Check on Your Pull Request -Go back to your pull request after a few minutes and see whether it passed muster with Travis-CI. Everything should look green, otherwise fix issues and amend your commit as described above. +Go back to your pull request after a few minutes and see whether it passed muster with CI. Everything should look green, otherwise fix issues and amend your commit as described above. #### Be Patient From db9ccec4d4f6756488c59c89afd3a414777fdea5 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 20 Aug 2021 15:52:13 -0400 Subject: [PATCH 32/52] ci: update default git branch --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dac037a..7dc0557 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,7 +6,7 @@ on: workflow_dispatch: push: branches: - - main + - master - v*.*.x tags: - v*.*.* From ab21d78f75edb9fb8f91a668cd99875439522eb5 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 19 Aug 2021 23:12:57 -0400 Subject: [PATCH 33/52] test: rewrite test coverage for comments and PIs to test behavior and not implementation. --- test/scrubbers_test.rb | 60 +++++++++++------------------------------- 1 file changed, 16 insertions(+), 44 deletions(-) diff --git a/test/scrubbers_test.rb b/test/scrubbers_test.rb index e6612e9..a825404 100644 --- a/test/scrubbers_test.rb +++ b/test/scrubbers_test.rb @@ -41,6 +41,16 @@ def test_default_scrub_behavior assert_scrubbed 'hello', 'hello' end + def test_default_scrub_removes_comments + assert_scrubbed('
one
three', + '
one
three') + end + + def test_default_scrub_removes_processing_instructions + assert_scrubbed('
one
three', + '
one
three') + end + def test_default_attributes_removal_behavior assert_scrubbed '

hello

', '

hello

' end @@ -56,6 +66,12 @@ def test_leaves_only_supplied_tags assert_scrubbed html, 'leave me now' end + def test_leaves_comments_when_supplied_as_tag + @scrubber.tags = %w(div comment) + assert_scrubbed('
one
three', + '
one
three') + end + def test_leaves_only_supplied_tags_nested html = 'leave me now' @scrubber.tags = %w(tag) @@ -112,50 +128,6 @@ def test_attributes_accessor_validation end end -class PermitScrubberSubclassTest < ScrubberTest - def setup - @scrubber = Class.new(::Rails::Html::PermitScrubber) do - attr :nodes_seen - - def initialize - super() - @nodes_seen = [] - end - - def keep_node?(node) - @nodes_seen << node.name - super(node) - end - end.new - end - - def test_elements_are_checked - html = %Q("
") - Loofah.scrub_fragment(html, @scrubber) - assert_includes(@scrubber.nodes_seen, "div") - assert_includes(@scrubber.nodes_seen, "a") - assert_includes(@scrubber.nodes_seen, "tr") - end - - def test_comments_are_checked - # this passes in v1.3.0 but fails in v1.4.0 - html = %Q("
") - Loofah.scrub_fragment(html, @scrubber) - assert_includes(@scrubber.nodes_seen, "div") - assert_includes(@scrubber.nodes_seen, "comment") - assert_includes(@scrubber.nodes_seen, "tr") - end - - def test_craftily_named_processing_instructions_are_not_checked - # this fails in v1.3.0 but passes in v1.4.0 - html = %Q("
") - Loofah.scrub_fragment(html, @scrubber) - assert_includes(@scrubber.nodes_seen, "div") - refute_includes(@scrubber.nodes_seen, "a") - assert_includes(@scrubber.nodes_seen, "tr") - end -end - class TargetScrubberTest < ScrubberTest def setup @scrubber = Rails::Html::TargetScrubber.new From b1fe437c9e0c59d367b8c49e09e4d42ef8628ace Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 19 Aug 2021 23:14:51 -0400 Subject: [PATCH 34/52] perf: PermitScrubber#scrub checks node.element? before node.comment? Assuming elements are more common than comments, this is going to be one less method call per node. --- CHANGELOG.md | 8 ++++++++ lib/rails/html/scrubbers.rb | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ae8ff..ca593b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +## next / unreleased + +* Slightly improve performance. + + Assuming elements are more common than comments, make one less method call per node. + + *Mike Dalessio* + ## 1.4.1 / 2021-08-18 * Fix regression in v1.4.0 that did not pass comment nodes to the scrubber. diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 385d357..09cfe95 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -68,7 +68,7 @@ def scrub(node) end return CONTINUE if skip_node?(node) - unless (node.comment? || node.element?) && keep_node?(node) + unless (node.element? || node.comment?) && keep_node?(node) return STOP if scrub_node(node) == STOP end From c86fed1dedb5380a4e46df5b4e8ee2904eac369d Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 23 Aug 2021 20:15:05 -0400 Subject: [PATCH 35/52] version bump to v1.4.2 --- CHANGELOG.md | 2 +- lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca593b9..d6399fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## next / unreleased +## 1.4.2 / 2021-08-23 * Slightly improve performance. diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index 103c532..446c687 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.4.1" + VERSION = "1.4.2" end end end From 18f2f2c17e86d149bbf0f6d0aa5000fcbf1e9105 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 29 Oct 2021 00:38:54 -0400 Subject: [PATCH 36/52] test: finally use the CSS hex encoding originally intended This was mis-fixed in c190b32 which encoded the Ruby strings as unicode to fix the previous bad encoding which dated back to the original Instiki that should have single-quoted the CSS unicode strings. --- test/sanitizer_test.rb | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 7938433..241564c 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -414,8 +414,25 @@ def test_should_sanitize_img_dynsrc_lowsrc 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 @@ -574,4 +591,15 @@ def scope_allowed_attributes(attributes) ensure Rails::Html::SafeListSanitizer.allowed_attributes = old_attributes 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 end From 984b82e07b81a427e1c6473d40fe3c81faeab5bc Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 17 Jan 2022 12:45:36 -0500 Subject: [PATCH 37/52] ci: include coverage of ruby 3.1 and jruby 9.3 --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7dc0557..e049171 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ jobs: strategy: fail-fast: false matrix: - ruby: ["2.2", "2.3", "2.4", "2.5", "2.6", "2.7", "3.0", "ruby-head"] + ruby: ["2.2", "2.3", "2.4", "2.5", "2.6", "2.7", "3.0", "3.1", "ruby-head"] runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 @@ -35,7 +35,7 @@ jobs: strategy: fail-fast: false matrix: - ruby: ["jruby-9.2", "jruby-head"] + ruby: ["jruby-9.2", "jruby-9.3", "jruby-head"] runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 From 9c421f0c932f6dd97f59ed96f57eef21193736c4 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 23 Jan 2022 14:55:20 -0500 Subject: [PATCH 38/52] ci: add coverage for system libxml2 --- .github/workflows/ci.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e049171..de6f26f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,6 +30,22 @@ jobs: bundler-cache: true - run: bundle exec rake + cruby-nokogiri-system-libraries: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.1" + - name: Install nokogiri with system libraries + run: | + sudo apt install pkg-config libxml2-dev libxslt-dev + bundle config set force_ruby_platform true + bundle config build.nokogiri --enable-system-libraries + bundle install + bundle exec nokogiri -v + - run: bundle exec rake + jruby: continue-on-error: true # nokogiri on jruby has different behavior strategy: From 9778c471211af9c9bdd6185c71b4594711ab49c9 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 23 Jan 2022 15:13:56 -0500 Subject: [PATCH 39/52] test: ensure tests pass when nokogiri uses system libxml2 Specifically the patch that affects this behavior is nokogiri/patches/libxml2/0002-Update-entities-to-remove-handling-of-ssi.patch which was introduced to avoid server-side-include vulnerabilities, see https://github.com/sparklemotion/nokogiri/commit/4852e43 --- test/sanitizer_test.rb | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 241564c..1de5a99 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -505,7 +505,13 @@ def test_uri_escaping_of_href_attr_in_a_tag_in_safe_list_sanitizer 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 @@ -515,7 +521,13 @@ def test_uri_escaping_of_src_attr_in_a_tag_in_safe_list_sanitizer 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 @@ -525,7 +537,13 @@ def test_uri_escaping_of_name_attr_in_a_tag_in_safe_list_sanitizer 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 @@ -535,7 +553,13 @@ def test_uri_escaping_of_name_action_in_a_tag_in_safe_list_sanitizer 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 From fe109c9fd4bfc5fbe954edb9e39410ae416b8f4f Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 4 May 2022 16:35:31 -0400 Subject: [PATCH 40/52] test: ensure we pass with libxml 2.9.14 see release notes for Nokogiri v1.13.5 --- test/sanitizer_test.rb | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 1de5a99..8b0b7ab 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -2,6 +2,8 @@ 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 @@ -54,7 +56,8 @@ def test_remove_xpaths_called_with_enumerable_xpaths 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 +78,21 @@ def test_strip_tags_multiline 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 @@ -450,11 +459,15 @@ def test_should_sanitize_img_vbscript 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 @@ -626,4 +639,8 @@ def convert_to_css_hex(string, escape_parens=false) end end.join end + + def libxml_2_9_14_recovery? + Nokogiri.method(:uses_libxml?).arity == -1 && Nokogiri.uses_libxml?(">= 2.9.14") + end end From 045774aec722d2f6bae99e8b3143b3e893e5eb29 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 27 May 2022 17:56:04 -0400 Subject: [PATCH 41/52] test: clean up tests by using the helpers --- test/sanitizer_test.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 8b0b7ab..5bf188e 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -14,13 +14,11 @@ def test_sanitizer_sanitize_raises_not_implemented_error 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 From 45a5c10fed3d9aa141594c80afa06d748fa0967d Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 9 Jun 2022 11:13:33 -0400 Subject: [PATCH 42/52] fix: modify safelist option if it contains both `select` and `style` Addresses CVE-2022-32209 --- lib/rails/html/sanitizer.rb | 19 ++++++++++++++++++- test/sanitizer_test.rb | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) 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 @@ def sanitize_css(style_string) 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 5bf188e..592ed7e 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -581,6 +581,25 @@ def test_exclude_node_type_comment 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 = {}) @@ -641,4 +660,8 @@ def convert_to_css_hex(string, escape_parens=false) 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 end From 924e3ab05ca56e53ebcb994e4a63977e56f06d2f Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 9 Jun 2022 18:21:38 -0400 Subject: [PATCH 43/52] update CHANGELOG for v1.4.3 --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6399fb..5e6ebd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +## 1.4.3 / 2022-06-09 + +* Address a possible XSS vulnerability with certain configurations of Rails::Html::Sanitizer. + + Prevent the combination of `select` and `style` as allowed tags in SafeListSanitizer. + + Fixes CVE-2022-32209 + + *Mike Dalessio* + + ## 1.4.2 / 2021-08-23 * Slightly improve performance. From f83f08c81a3a33ce0fb1c379933c416ae80672fa Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 9 Jun 2022 18:23:09 -0400 Subject: [PATCH 44/52] version bump to v1.4.3 --- lib/rails/html/sanitizer/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index 446c687..af67a0e 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.4.2" + VERSION = "1.4.3" end end end From 11752a6427283e8a836608cd1d3ba27b6fb97f78 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 17 Aug 2022 10:54:37 -0400 Subject: [PATCH 45/52] tests: handle libxml 2.10.0 incorrectly-opened comment parsing Related, see: - https://github.com/sparklemotion/nokogiri/pull/2625 - https://gitlab.gnome.org/GNOME/libxml2/-/issues/380 --- test/sanitizer_test.rb | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 592ed7e..9f84a4c 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -54,7 +54,7 @@ def test_remove_xpaths_called_with_enumerable_xpaths def test_strip_tags_with_quote input = '<" hi' - expected = libxml_2_9_14_recovery? ? %{<" hi} : %{ hi} + expected = libxml_2_9_14_recovery_lt? ? %{<" hi} : %{ hi} assert_equal(expected, full_sanitize(input)) end @@ -77,19 +77,19 @@ def test_strip_tags_multiline def test_remove_unclosed_tags input = "This is <-- not\n a comment here." - expected = libxml_2_9_14_recovery? ? %{This is <-- not\n a comment here.} : %{This is } + expected = libxml_2_9_14_recovery_lt? ? %{This is <-- not\n a comment here.} : %{This is } assert_equal(expected, full_sanitize(input)) end def test_strip_cdata input = "This has a ]]> here." - expected = libxml_2_9_14_recovery? ? %{This has a <![CDATA[]]> here.} : %{This has a ]]> here.} + expected = libxml_2_9_14_recovery_lt_bang? ? %{This has a <![CDATA[]]> here.} : %{This has a ]]> here.} assert_equal(expected, full_sanitize(input)) end def test_strip_unclosed_cdata input = "This has an unclosed ]] here..." - expected = libxml_2_9_14_recovery? ? %{This has an unclosed <![CDATA[]] here...} : %{This has an unclosed ]] here...} + expected = libxml_2_9_14_recovery_lt_bang? ? %{This has an unclosed <![CDATA[]] here...} : %{This has an unclosed ]] here...} assert_equal(expected, full_sanitize(input)) end @@ -458,13 +458,13 @@ def test_should_sanitize_img_vbscript def test_should_sanitize_cdata_section input = "section]]>" - expected = libxml_2_9_14_recovery? ? %{<![CDATA[section]]>} : %{section]]>} + expected = libxml_2_9_14_recovery_lt_bang? ? %{<![CDATA[section]]>} : %{section]]>} assert_sanitized(input, expected) end def test_should_sanitize_unterminated_cdata_section input = "neverending..." - expected = libxml_2_9_14_recovery? ? %{<![CDATA[neverending...} : %{neverending...} + expected = libxml_2_9_14_recovery_lt_bang? ? %{<![CDATA[neverending...} : %{neverending...} assert_sanitized(input, expected) end @@ -657,10 +657,17 @@ def convert_to_css_hex(string, escape_parens=false) end.join end - def libxml_2_9_14_recovery? + def libxml_2_9_14_recovery_lt? + # changed in 2.9.14, see https://github.com/sparklemotion/nokogiri/releases/tag/v1.13.5 Nokogiri.method(:uses_libxml?).arity == -1 && Nokogiri.uses_libxml?(">= 2.9.14") end + def libxml_2_9_14_recovery_lt_bang? + # changed in 2.9.14, see https://github.com/sparklemotion/nokogiri/releases/tag/v1.13.5 + # then reverted in 2.10.0, see https://gitlab.gnome.org/GNOME/libxml2/-/issues/380 + 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 From df03f2f600cafff3e349b1ec48b730c43b381b65 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 11 Dec 2022 14:06:46 -0500 Subject: [PATCH 46/52] ci: pin system lib test to 20.04 because the 22.04 has a version with 4fd69f3 but not e986d09 from 2.9.14 and that's causing leading `<` to be parsed differently. i'd fix it better than this, but I think only 2.9.13 has this behavior. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index de6f26f..0b72d60 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,7 @@ jobs: - run: bundle exec rake cruby-nokogiri-system-libraries: - runs-on: ubuntu-latest + runs-on: ubuntu-20.04 steps: - uses: actions/checkout@v2 - uses: ruby/setup-ruby@v1 From f0e33477a0557dbdbefc3e470c7df3a64efb002a Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 17 Nov 2022 22:51:58 -0500 Subject: [PATCH 47/52] fix: replace slow regex attribute check with Loofah method which uses the Crass parser --- lib/rails/html/scrubbers.rb | 4 +++- test/sanitizer_test.rb | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 09cfe95..18bac0a 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -145,9 +145,11 @@ def scrub_attribute(node, attr_node) attr_node.remove end end + if Loofah::HTML5::SafeList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name) - attr_node.value = attr_node.value.gsub(/url\s*\(\s*[^#\s][^)]+?\)/m, ' ') if attr_node.value + Loofah::HTML5::Scrub.scrub_attribute_that_allows_local_ref(attr_node) end + if Loofah::HTML5::SafeList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == 'xlink:href' && attr_node.value =~ /^\s*[^#\s].*/m attr_node.remove end diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 9f84a4c..8a1d5ac 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -600,6 +600,16 @@ def test_disallow_the_dangerous_safelist_combination_of_select_and_style refute_includes(sanitized, "style") end + def test_scrubbing_svg_attr_values_that_allow_ref + input = %Q(
hey
) + expected = %Q(
hey
) + actual = scope_allowed_attributes %w(fill) do + safe_list_sanitize(input) + end + + assert_equal(expected, actual) + end + protected def xpath_sanitize(input, options = {}) From d1223a29cb3e4151cdcb6ba6c8431708d8ce40a6 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 18 Nov 2022 00:10:30 -0500 Subject: [PATCH 48/52] fix: use Loofah's scrub_uri_attribute method which correctly sanitizes data URL mediatypes --- lib/rails/html/scrubbers.rb | 6 +---- test/sanitizer_test.rb | 50 +++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 18bac0a..f9e14cd 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -139,11 +139,7 @@ def scrub_attribute(node, attr_node) end if Loofah::HTML5::SafeList::ATTR_VAL_IS_URI.include?(attr_name) - # this block lifted nearly verbatim from HTML5 sanitization - val_unescaped = CGI.unescapeHTML(attr_node.value).gsub(Loofah::HTML5::Scrub::CONTROL_CHARACTERS,'').downcase - if val_unescaped =~ /^[a-z0-9][-+.a-z0-9]*:/ && ! Loofah::HTML5::SafeList::ALLOWED_PROTOCOLS.include?(val_unescaped.split(Loofah::HTML5::SafeList::PROTOCOL_SEPARATOR)[0]) - attr_node.remove - end + return if Loofah::HTML5::Scrub.scrub_uri_attribute(attr_node) end if Loofah::HTML5::SafeList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 8a1d5ac..7a60956 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -600,6 +600,56 @@ def test_disallow_the_dangerous_safelist_combination_of_select_and_style refute_includes(sanitized, "style") end + %w[text/plain text/css image/png image/gif image/jpeg].each do |mediatype| + define_method "test_mediatype_#{mediatype}_allowed" do + input = %Q() + expected = input + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + + input = %Q() + expected = input + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + end + end + + def test_mediatype_text_html_disallowed + input = %q() + expected = %q() + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + + input = %q() + expected = %q() + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + end + + def test_mediatype_image_svg_xml_disallowed + input = %q() + expected = %q() + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + + input = %q() + expected = %q() + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + end + + def test_mediatype_other_disallowed + input = %q(foo) + expected = %q(foo) + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + + input = %q(foo) + expected = %q(foo) + actual = safe_list_sanitize(input) + assert_equal(expected, actual) + end + def test_scrubbing_svg_attr_values_that_allow_ref input = %Q(
hey
) expected = %Q(
hey
) From e6d52d3b6db99d07399498b1287997302d444a8d Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 24 Aug 2022 16:10:39 -0400 Subject: [PATCH 49/52] revert 45a5c10 to prepare for a better fix --- lib/rails/html/sanitizer.rb | 19 +------------------ test/sanitizer_test.rb | 23 ----------------------- 2 files changed, 1 insertion(+), 41 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index 13fb963..5633ca1 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -141,25 +141,8 @@ def sanitize_css(style_string) 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) - if options[:tags] - remove_safelist_tag_combinations(options[:tags]) - else - self.class.allowed_tags - end + options[:tags] || self.class.allowed_tags end def allowed_attributes(options) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 7a60956..99221db 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -581,25 +581,6 @@ def test_exclude_node_type_comment 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 - %w[text/plain text/css image/png image/gif image/jpeg].each do |mediatype| define_method "test_mediatype_#{mediatype}_allowed" do input = %Q() @@ -727,8 +708,4 @@ def libxml_2_9_14_recovery_lt_bang? # then reverted in 2.10.0, see https://gitlab.gnome.org/GNOME/libxml2/-/issues/380 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 end From 0713caf2ee23801cfb85e37065cf406368b20082 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 25 Aug 2022 15:03:39 -0400 Subject: [PATCH 50/52] fix: escape CDATA nodes using Loofah's escaping methods Also, notably, document the decisions behind this approach in a decision record. --- lib/rails/html/scrubbers.rb | 6 ++-- test/sanitizer_test.rb | 60 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index f9e14cd..674d1c4 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -61,9 +61,9 @@ def attributes=(attributes) end def scrub(node) - if node.cdata? - text = node.document.create_text_node node.text - node.replace text + if Loofah::HTML5::Scrub.cdata_needs_escaping?(node) + replacement = Loofah::HTML5::Scrub.cdata_escape(node) + node.replace(replacement) return CONTINUE end return CONTINUE if skip_node?(node) diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 99221db..cd0b046 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -641,6 +641,66 @@ def test_scrubbing_svg_attr_values_that_allow_ref assert_equal(expected, actual) end + def test_style_with_css_payload + input, tags = "", ["style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + + def test_combination_of_select_and_style_with_css_payload + input, tags = "", ["select", "style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + + def test_combination_of_select_and_style_with_script_payload + input, tags = "", ["select", "style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + + def test_combination_of_svg_and_style_with_script_payload + input, tags = "", ["svg", "style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + + def test_combination_of_math_and_style_with_img_payload + input, tags = "", ["math", "style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + + input, tags = "", ["math", "style", "img"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + + def test_combination_of_svg_and_style_with_img_payload + input, tags = "", ["svg", "style"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + + input, tags = "", ["svg", "style", "img"] + expected = "" + actual = safe_list_sanitize(input, tags: tags) + + assert_equal(expected, actual) + end + protected def xpath_sanitize(input, options = {}) From 48ae90acfce9cacbd7cec9963498f6a7b5bc3d5c Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 12 Dec 2022 17:29:01 -0500 Subject: [PATCH 51/52] dep: bump dependency on loofah v2.19.1 has the new methods we're using: - Loofah::HTML5::Scrub.cdata_needs_escaping? - Loofah::HTML5::Scrub.cdata_escape - Loofah::HTML5::Scrub.scrub_uri_attribute - Loofah::HTML5::Scrub.scrub_attribute_that_allows_local_ref avoiding code duplication in this gem. --- rails-html-sanitizer.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rails-html-sanitizer.gemspec b/rails-html-sanitizer.gemspec index c9637b7..653084c 100644 --- a/rails-html-sanitizer.gemspec +++ b/rails-html-sanitizer.gemspec @@ -26,7 +26,7 @@ Gem::Specification.new do |spec| # NOTE: There's no need to update this dependency for Loofah CVEs # in minor releases when users can simply run `bundle update loofah`. - spec.add_dependency "loofah", "~> 2.3" + spec.add_dependency "loofah", "~> 2.19", ">= 2.19.1" spec.add_development_dependency "bundler", ">= 1.3" spec.add_development_dependency "rake" From fd63deaeb22e601237d4d4d12014e7ebd410ea9b Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 12 Dec 2022 17:43:11 -0500 Subject: [PATCH 52/52] version bump to v1.4.4 --- CHANGELOG.md | 35 +++++++++++++++++++++++++++++ lib/rails/html/sanitizer/version.rb | 2 +- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e6ebd5..e18051c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,38 @@ +## 1.4.4 / 2022-12-13 + +* Address inefficient regular expression complexity with certain configurations of Rails::Html::Sanitizer. + + Fixes CVE-2022-23517. See + [GHSA-5x79-w82f-gw8w](https://github.com/rails/rails-html-sanitizer/security/advisories/GHSA-5x79-w82f-gw8w) + for more information. + + *Mike Dalessio* + +* Address improper sanitization of data URIs. + + Fixes CVE-2022-23518 and #135. See + [GHSA-mcvf-2q2m-x72m](https://github.com/rails/rails-html-sanitizer/security/advisories/GHSA-mcvf-2q2m-x72m) + for more information. + + *Mike Dalessio* + +* Address possible XSS vulnerability with certain configurations of Rails::Html::Sanitizer. + + Fixes CVE-2022-23520. See + [GHSA-rrfc-7g8p-99q8](https://github.com/rails/rails-html-sanitizer/security/advisories/GHSA-rrfc-7g8p-99q8) + for more information. + + *Mike Dalessio* + +* Address possible XSS vulnerability with certain configurations of Rails::Html::Sanitizer. + + Fixes CVE-2022-23519. See + [GHSA-9h9g-93gc-623h](https://github.com/rails/rails-html-sanitizer/security/advisories/GHSA-9h9g-93gc-623h) + for more information. + + *Mike Dalessio* + + ## 1.4.3 / 2022-06-09 * Address a possible XSS vulnerability with certain configurations of Rails::Html::Sanitizer. diff --git a/lib/rails/html/sanitizer/version.rb b/lib/rails/html/sanitizer/version.rb index af67a0e..3ceb4c8 100644 --- a/lib/rails/html/sanitizer/version.rb +++ b/lib/rails/html/sanitizer/version.rb @@ -1,7 +1,7 @@ module Rails module Html class Sanitizer - VERSION = "1.4.3" + VERSION = "1.4.4" end end end