From ab16fa47d7d9d96c13a72882964bcbfb530f1922 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike.dalessio@gmail.com>
Date: Wed, 18 Aug 2021 15:58:06 -0400
Subject: [PATCH] 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("<div></div><a></a><tr></tr>")
+    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("<div></div><!-- ohai --><tr></tr>")
+    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("<div></div><?a content><tr></tr>")
+    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