Merge pull request #408 from gjtorikian/why-double

Remove superfluous sanitization

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:
 
@@ -224,7 +224,7 @@ For more examples of customizing the sanitization process to include the tags yo
 
 `NodeFilters`s can operate either on HTML elements or text nodes using CSS selectors. Each `NodeFilter` must define a method named `selector` which provides an instance of `Selma::Selector`. If elements are being manipulated, `handle_element` must be defined, taking one argument, `element`; if text nodes are being manipulated, `handle_text_chunk` must be defined, taking one argument, `text_chunk`. `@config`, and `@result` are available to use, and any changes made to these ivars are passed on to the next filter.
 
-`NodeFilter` also has an optional method, `after_initialize`, which is run after the filter initializes. This can be useful in setting up a custom state for `result` to take advantage of.
+`NodeFilter` also has an optional method, `after_initialize`, which is run after the filter initializes. This can be useful in setting up a fresh custom state for `result` to start from each time the pipeline is called.
 
 Here's an example `NodeFilter` that adds a base url to images that are root relative:
 

html-pipeline.gemspec

@@ -25,7 +25,7 @@ Gem::Specification.new do |gem|
     "rubygems_mfa_required" => "true",
   }
 
-  gem.add_dependency("selma", "~> 0.1")
+  gem.add_dependency("selma", "~> 0.4")
   gem.add_dependency("zeitwerk", "~> 2.5")
 
   gem.post_install_message = <<~MSG

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!)
 

lib/html_pipeline/convert_filter.rb

@@ -5,7 +5,7 @@ class ConvertFilter < Filter
     attr_reader :text, :html
 
     def initialize(context: {}, result: {})
-      super(context: context, result: result)
+      super
     end
 
     class << self

lib/html_pipeline/convert_filter/markdown_filter.rb

@@ -12,7 +12,7 @@ class ConvertFilter < Filter
     #   :markdown[:extensions] => Commonmarker extensions options
     class MarkdownFilter < ConvertFilter
       def initialize(context: {}, result: {})
-        super(context: context, result: result)
+        super
       end
 
       # Convert Commonmark to HTML using the best available implementation.

lib/html_pipeline/node_filter/syntax_highlight_filter.rb

@@ -15,7 +15,7 @@ class NodeFilter
     # This filter does not write any additional information to the context hash.
     class SyntaxHighlightFilter < NodeFilter
       def initialize(context: {}, result: {})
-        super(context: context, result: result)
+        super
         # TODO: test the optionality of this
         @formatter = context[:formatter] || Rouge::Formatters::HTML.new
       end

lib/html_pipeline/text_filter.rb

@@ -5,7 +5,7 @@ class TextFilter < Filter
     attr_reader :text
 
     def initialize(context: {}, result: {})
-      super(context: context, result: result)
+      super
     end
 
     class << self

lib/html_pipeline/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 class HTMLPipeline
-  VERSION = "3.2.0"
+  VERSION = "3.2.1"
 end

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