stop running sanitization twice

Author: gjtorikian

README.md

@@ -171,9 +171,9 @@ The `ConvertFilter` takes text and turns it into HTML. `@text`, `@config`, and `
 
 ### Sanitization
 
-Because the web can be a scary place, HTML is automatically sanitized after the `ConvertFilter` runs and before the `NodeFilter`s are processed. This is to prevent malicious or unexpected input from entering the pipeline.
+Because the web can be a scary place, **HTML is automatically sanitized** after the `ConvertFilter` runs and before the `NodeFilter`s are processed. This is to prevent malicious or unexpected input from entering the pipeline.
 
-The sanitization process takes a hash configuration of settings. See the [Selma](https://www.github.com/gjtorikian/selma) documentation for more information on how to configure these settings.
+The sanitization process takes a hash configuration of settings. See the [Selma](https://www.github.com/gjtorikian/selma) documentation for more information on how to configure these settings. Note that users must correctly configure the sanitization configuration if they expect to use it correctly in conjunction with handlers which manipulate HTML.
 
 A default sanitization config is provided by this library (`HTMLPipeline::SanitizationFilter::DEFAULT_CONFIG`). A sample custom sanitization allowlist might look like this:
 

lib/html_pipeline.rb

@@ -175,11 +175,20 @@ def call(text, context: {}, result: {})
       end
     end
 
-    unless @node_filters.empty?
+    rewriter_options = {
+      memory: {
+        max_allowed_memory_usage: 5242880, # arbitrary limit of 5MB
+      },
+    }
+
+    if @node_filters.empty?
+      instrument("sanitization.html_pipeline", payload) do
+        result[:output] = Selma::Rewriter.new(sanitizer: @sanitization_config, handlers: @node_filters, options: rewriter_options).rewrite(html)
+      end unless @convert_filter.nil? # no html, so no sanitization
+    else
       instrument("call_node_filters.html_pipeline", payload) do
         @node_filters.each { |filter| filter.context = (filter.context || {}).merge(context) }
-        result[:output] = Selma::Rewriter.new(sanitizer: @sanitization_config, handlers: @node_filters).rewrite(html)
-        html = result[:output]
+        result[:output] = Selma::Rewriter.new(sanitizer: @sanitization_config, handlers: @node_filters, options: rewriter_options).rewrite(html)
         payload = default_payload({
           node_filters: @node_filters.map { |f| f.class.name },
           context: context,
@@ -188,10 +197,6 @@ def call(text, context: {}, result: {})
       end
     end
 
-    instrument("sanitization.html_pipeline", payload) do
-      result[:output] = Selma::Rewriter.new(sanitizer: @sanitization_config, handlers: @node_filters).rewrite(html)
-    end
-
     result = result.merge(@node_filters.collect(&:result).reduce({}, :merge))
     @node_filters.each(&:reset!)
 

test/html_pipeline_test.rb

@@ -131,7 +131,7 @@ def test_context_is_carried_over_in_call
     # - yeH is bolded
     # - strikethroughs are rendered
     # - mentions are not linked
-    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"/gjtorikian\">@gjtorikian</a> is <del>great</del>!</p>", result)
+    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"/gjtorikian\" class=\"user-mention\">@gjtorikian</a> is <del>great</del>!</p>", result)
 
     context = {
       bolded: false,
@@ -144,7 +144,7 @@ def test_context_is_carried_over_in_call
     # - yeH is not bolded
     # - strikethroughs are not rendered
     # - mentions are linked
-    assert_equal("<p>yeH! I <em>think</em> <a href=\"http://your-domain.com/gjtorikian\">@gjtorikian</a> is ~great~!</p>", result_with_context)
+    assert_equal("<p>yeH! I <em>think</em> <a href=\"http://your-domain.com/gjtorikian\" class=\"user-mention\">@gjtorikian</a> is ~great~!</p>", result_with_context)
   end
 
   def test_text_filter_instance_context_is_carried_over_in_call
@@ -160,7 +160,7 @@ def test_text_filter_instance_context_is_carried_over_in_call
 
     # note:
     # - yeH is not bolded due to previous context
-    assert_equal("<p>yeH! I <em>think</em> <a href=\"/gjtorikian\">@gjtorikian</a> is <del>great</del>!</p>", result)
+    assert_equal("<p>yeH! I <em>think</em> <a href=\"/gjtorikian\" class=\"user-mention\">@gjtorikian</a> is <del>great</del>!</p>", result)
   end
 
   def test_convert_filter_instance_context_is_carried_over_in_call
@@ -176,7 +176,7 @@ def test_convert_filter_instance_context_is_carried_over_in_call
 
     # note:
     # - strikethroughs are not rendered due to previous context
-    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"/gjtorikian\">@gjtorikian</a> is <del>great</del>!</p>", result)
+    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"/gjtorikian\" class=\"user-mention\">@gjtorikian</a> is <del>great</del>!</p>", result)
   end
 
   def test_node_filter_instance_context_is_carried_over_in_call
@@ -192,6 +192,6 @@ def test_node_filter_instance_context_is_carried_over_in_call
 
     # note:
     # - mentions are linked
-    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"http://your-domain.com/gjtorikian\">@gjtorikian</a> is <del>great</del>!</p>", result)
+    assert_equal("<p><strong>yeH</strong>! I <em>think</em> <a href=\"http://your-domain.com/gjtorikian\" class=\"user-mention\">@gjtorikian</a> is <del>great</del>!</p>", result)
   end
 end

test/sanitization_filter_test.rb

@@ -232,7 +232,7 @@ def test_sanitization_pipeline_can_be_configured
       CODE
 
       expected = <<~HTML
-        <p>This is great, <a>@balevine</a>:</p>
+        <p>This is great, <a href="/balevine" class="user-mention">@balevine</a>:</p>
         <pre><code>some_code(:first)
         </code></pre>
       HTML