Merge pull request #1755 from Shopify/non-parsing-comment

Don't parse nodes inside a comment tag

lib/liquid/block_body.rb

@@ -6,6 +6,7 @@ module Liquid
   class BlockBody
     LiquidTagToken      = /\A\s*(#{TagName})\s*(.*?)\z/o
     FullToken           = /\A#{TagStart}#{WhitespaceControl}?(\s*)(#{TagName})(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om
+    FullTokenPossiblyInvalid = /\A(.*)#{TagStart}#{WhitespaceControl}?\s*(\w+)\s*(.*)?#{WhitespaceControl}?#{TagEnd}\z/om
     ContentOfVariable   = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om
     WhitespaceOrNothing = /\A\s*\z/
     TAGSTART            = "{%"

lib/liquid/tags/comment.rb

@@ -15,6 +15,8 @@ module Liquid
   #   {% endcomment %}
   # @liquid_syntax_keyword content The content of the comment.
   class Comment < Block
+    TAG_DELIMITER = /\A(.*)#{TagStart}#{WhitespaceControl}?\s*(endcomment)\s*(.*)?#{WhitespaceControl}?#{TagEnd}\z/om
+
     def render_to_output_buffer(_context, output)
       output
     end
@@ -25,6 +27,71 @@ module Liquid
     def blank?
       true
     end
+
+    private
+
+    def parse_body(body, tokenizer)
+      if parse_context.depth >= MAX_DEPTH
+        raise StackLevelError, "Nesting too deep"
+      end
+
+      parse_context.depth += 1
+      comment_tag_depth = 1
+
+      begin
+        # Consume tokens without creating child nodes.
+        # The children tag doesn't require to be a valid Liquid except the comment and raw tag.
+        # The child comment and raw tag must be closed.
+        while (token = tokenizer.send(:shift))
+          tag_name = if tokenizer.for_liquid_tag
+            next if token.empty? || token.match?(BlockBody::WhitespaceOrNothing)
+
+            tag_name_match = BlockBody::LiquidTagToken.match(token)
+
+            next if tag_name_match.nil?
+
+            tag_name_match[1]
+          elsif TAG_DELIMITER.match?(token)
+            # aggressively match comment delimiter
+            "endcomment"
+          elsif token =~ BlockBody::FullToken && Regexp.last_match(2) == "comment"
+            # aggressively match comment tag
+            "comment"
+          else
+            tag_name_match = BlockBody::FullTokenPossiblyInvalid.match(token)
+
+            next if tag_name_match.nil?
+
+            tag_name_match[2]
+          end
+
+          case tag_name
+          when "raw"
+            parse_raw_tag_body(tokenizer)
+          when "comment"
+            comment_tag_depth += 1
+          when "endcomment"
+            comment_tag_depth -= 1
+
+            return false if comment_tag_depth.zero?
+          end
+        end
+
+        raise_tag_never_closed(block_name)
+      ensure
+        parse_context.depth -= 1
+      end
+
+      false
+    end
+
+    def parse_raw_tag_body(tokenizer)
+      while (token = tokenizer.send(:shift))
+        return if token =~ BlockBody::FullTokenPossiblyInvalid && "endraw" == Regexp.last_match(2)
+      end
+
+      raise_tag_never_closed("raw")
+    end
   end
 
   Template.register_tag('comment', Comment)

lib/liquid/tags/raw.rb

@@ -14,7 +14,6 @@ module Liquid
   # @liquid_syntax_keyword expression The expression to be output without being rendered.
   class Raw < Block
     Syntax = /\A\s*\z/
-    FullTokenPossiblyInvalid = /\A(.*)#{TagStart}#{WhitespaceControl}?\s*(\w+)\s*(.*)?#{WhitespaceControl}?#{TagEnd}\z/om
 
     def initialize(tag_name, markup, parse_context)
       super
@@ -25,7 +24,7 @@ module Liquid
     def parse(tokens)
       @body = +''
       while (token = tokens.shift)
-        if token =~ FullTokenPossiblyInvalid && block_delimiter == Regexp.last_match(2)
+        if token =~ BlockBody::FullTokenPossiblyInvalid && block_delimiter == Regexp.last_match(2)
           parse_context.trim_whitespace = (token[-3] == WhitespaceControl)
           @body << Regexp.last_match(1) if Regexp.last_match(1) != ""
           return

test/unit/tags/comment_tag_unit_test.rb

@@ -0,0 +1,138 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class CommentTagUnitTest < Minitest::Test
+  def test_comment_inside_liquid_tag
+    assert_template_result("", <<~LIQUID.chomp)
+      {% liquid
+        if 1 != 1
+        comment
+        else
+          echo 123
+        endcomment
+        endif
+      %}
+    LIQUID
+  end
+
+  def test_does_not_parse_nodes_inside_a_comment
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+        {% if true %}
+        {% if ... %}
+        {%- for ? -%}
+        {% while true %}
+        {%
+          unless if
+        %}
+        {% endcase %}
+      {% endcomment %}
+    LIQUID
+  end
+
+  def test_allows_incomplete_tags_inside_a_comment
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+        {% assign foo = "1"
+      {% endcomment %}
+    LIQUID
+
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+        {% comment %}
+          {% invalid
+        {% endcomment %}
+      {% endcomment %}
+    LIQUID
+
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+      {% {{ {%- endcomment %}
+    LIQUID
+  end
+
+  def test_child_comment_tags_need_to_be_closed
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+        {% comment %}
+          {% comment %}{%    endcomment     %}
+        {% endcomment %}
+      {% endcomment %}
+    LIQUID
+
+    assert_raises(Liquid::SyntaxError) do
+      assert_template_result("", <<~LIQUID.chomp)
+        {% comment %}
+          {% comment %}
+            {% comment %}
+          {% endcomment %}
+        {% endcomment %}
+      LIQUID
+    end
+  end
+
+  def test_child_raw_tags_need_to_be_closed
+    assert_template_result("", <<~LIQUID.chomp)
+      {% comment %}
+        {% raw %}
+          {% endcomment %}
+        {% endraw %}
+      {% endcomment %}
+    LIQUID
+
+    assert_raises(Liquid::SyntaxError) do
+      Liquid::Template.parse(<<~LIQUID.chomp)
+        {% comment %}
+          {% raw %}
+          {% endcomment %}
+        {% endcomment %}
+      LIQUID
+    end
+  end
+
+  def test_error_line_number_is_correct
+    template = Liquid::Template.parse(<<~LIQUID.chomp, line_numbers: true)
+      {% comment %}
+        {% if true %}
+      {% endcomment %}
+      {{ errors.standard_error }}
+    LIQUID
+
+    output = template.render('errors' => ErrorDrop.new)
+    expected = <<~TEXT.chomp
+
+      Liquid error (line 4): standard error
+    TEXT
+
+    assert_equal(expected, output)
+  end
+
+  def test_comment_tag_delimiter_with_extra_strings
+    assert_template_result(
+      '',
+      <<~LIQUID.chomp,
+        {% comment %}
+          {% comment %}
+          {% endcomment
+          {% if true %}
+          {% endif %}
+        {% endcomment %}
+      LIQUID
+    )
+  end
+
+  def test_nested_comment_tag_with_extra_strings
+    assert_template_result(
+      '',
+      <<~LIQUID.chomp,
+        {% comment %}
+          {% comment
+            {% assign foo = 1 %}
+          {% endcomment
+          {% assign foo = 1 %}
+        {% endcomment %}
+      LIQUID
+    )
+  end
+end