refactor: Reduce maximum block nesting in Liquid::BlockBody#parse

Avoid hash with default values due to inconsistent marshaling

.rubocop.yml

@@ -7,8 +7,6 @@ AllCops:
 
 Metrics/BlockNesting:
   Max: 3
-  Exclude:
-    - 'lib/liquid/block_body.rb'
 
 Metrics/ModuleLength:
   Enabled: false

Gemfile

@@ -1,14 +1,14 @@
 source 'https://rubygems.org'
 
 gemspec
-gem 'stackprof', platforms: :mri_21
+
+gem 'stackprof', platforms: :mri
 
 group :benchmark, :test do
   gem 'benchmark-ips'
 end
 
 group :test do
-  gem 'spy', '0.4.1'
   gem 'rubocop', '0.34.2'
 
   platform :mri do

lib/liquid/block.rb

@@ -1,5 +1,7 @@
 module Liquid
   class Block < Tag
+    MAX_DEPTH = 100
+
     def initialize(tag_name, markup, options)
       super
       @blank = true
@@ -48,17 +50,25 @@ module Liquid
     protected
 
     def parse_body(body, tokens)
-      body.parse(tokens, parse_context) do |end_tag_name, end_tag_params|
+      if parse_context.depth >= MAX_DEPTH
-        @blank &&= body.blank?
+        raise StackLevelError, "Nesting too deep".freeze
+      end
+      parse_context.depth += 1
+      begin
+        body.parse(tokens, parse_context) do |end_tag_name, end_tag_params|
+          @blank &&= body.blank?
 
-        return false if end_tag_name == block_delimiter
+          return false if end_tag_name == block_delimiter
-        unless end_tag_name
+          unless end_tag_name
-          raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name))
+            raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name))
+          end
+
+          # this tag is not registered with the system
+          # pass it to the current block for special handling or error reporting
+          unknown_tag(end_tag_name, end_tag_params, tokens)
         end
-
+      ensure
-        # this tag is not registered with the system
+        parse_context.depth -= 1
-        # pass it to the current block for special handling or error reporting
-        unknown_tag(end_tag_name, end_tag_params, tokens)
       end
 
       true

lib/liquid/block_body.rb

@@ -15,38 +15,35 @@ module Liquid
     def parse(tokenizer, parse_context)
       parse_context.line_number = tokenizer.line_number
       while token = tokenizer.shift
-        unless token.empty?
+        next if token.empty?
-          case
+        case
-          when token.start_with?(TAGSTART)
+        when token.start_with?(TAGSTART)
-            whitespace_handler(token, parse_context)
+          whitespace_handler(token, parse_context)
-            if token =~ FullToken
+          unless token =~ FullToken
-              tag_name = $1
+            raise_missing_tag_terminator(token, parse_context)
-              markup = $2
-              # fetch the tag from registered blocks
-              if tag = registered_tags[tag_name]
-                new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
-                @blank &&= new_tag.blank?
-                @nodelist << new_tag
-              else
-                # end parsing if we reach an unknown tag and let the caller decide
-                # determine how to proceed
-                return yield tag_name, markup
-              end
-            else
-              raise_missing_tag_terminator(token, parse_context)
-            end
-          when token.start_with?(VARSTART)
-            whitespace_handler(token, parse_context)
-            @nodelist << create_variable(token, parse_context)
-            @blank = false
-          else
-            if parse_context.trim_whitespace
-              token.lstrip!
-            end
-            parse_context.trim_whitespace = false
-            @nodelist << token
-            @blank &&= !!(token =~ /\A\s*\z/)
           end
+          tag_name = $1
+          markup = $2
+          # fetch the tag from registered blocks
+          unless tag = registered_tags[tag_name]
+            # end parsing if we reach an unknown tag and let the caller decide
+            # determine how to proceed
+            return yield tag_name, markup
+          end
+          new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
+          @blank &&= new_tag.blank?
+          @nodelist << new_tag
+        when token.start_with?(VARSTART)
+          whitespace_handler(token, parse_context)
+          @nodelist << create_variable(token, parse_context)
+          @blank = false
+        else
+          if parse_context.trim_whitespace
+            token.lstrip!
+          end
+          parse_context.trim_whitespace = false
+          @nodelist << token
+          @blank &&= !!(token =~ /\A\s*\z/)
         end
         parse_context.line_number = tokenizer.line_number
       end

lib/liquid/condition.rb

@@ -7,7 +7,6 @@ module Liquid
   #   c.evaluate #=> true
   #
   class Condition #:nodoc:
-    @@depth = 0
     @@operators = {
       '=='.freeze => ->(cond, left, right) {  cond.send(:equal_variables, left, right) },
       '!='.freeze => ->(cond, left, right) { !cond.send(:equal_variables, left, right) },
@@ -42,21 +41,22 @@ module Liquid
     end
 
     def evaluate(context = Context.new)
-      result = interpret_condition(left, right, operator, context)
+      condition = self
+      result = nil
+      loop do
+        result = interpret_condition(condition.left, condition.right, condition.operator, context)
 
-      case @child_relation
+        case condition.child_relation
-      when :or
+        when :or
-        result || @child_condition.evaluate(context)
+          break if result
-      when :and
+        when :and
-        @@depth += 1
+          break unless result
-        if @@depth >= 500
+        else
-          @@depth = 0
+          break
-          raise StackLevelError, "Nesting too deep".freeze
         end
-        result && @child_condition.evaluate(context)
+        condition = condition.child_condition
-      else
-        result
       end
+      result
     end
 
     def or(condition)
@@ -81,6 +81,10 @@ module Liquid
       "#<Condition #{[@left, @operator, @right].compact.join(' '.freeze)}>"
     end
 
+    protected
+
+    attr_reader :child_relation, :child_condition
+
     private
 
     def equal_variables(left, right)

lib/liquid/context.rb

@@ -89,7 +89,7 @@ module Liquid
     # Push new local scope on the stack. use <tt>Context#stack</tt> instead
     def push(new_scope = {})
       @scopes.unshift(new_scope)
-      raise StackLevelError, "Nesting too deep".freeze if @scopes.length > 100
+      raise StackLevelError, "Nesting too deep".freeze if @scopes.length > Block::MAX_DEPTH
     end
 
     # Merge a hash of variables in the current local scope
@@ -171,7 +171,9 @@ module Liquid
       if scope.nil?
         @environments.each do |e|
           variable = lookup_and_evaluate(e, key, raise_on_not_found: raise_on_not_found)
-          unless variable.nil?
+          # When lookup returned a value OR there is no value but the lookup also did not raise
+          # then it is the value we are looking for.
+          if !variable.nil? || @strict_variables && raise_on_not_found
             scope = e
             break
           end

lib/liquid/extensions.rb

@@ -7,6 +7,12 @@ class String # :nodoc:
   end
 end
 
+class Symbol # :nodoc:
+  def to_liquid
+    to_s
+  end
+end
+
 class Array # :nodoc:
   def to_liquid
     self

lib/liquid/parse_context.rb

@@ -1,12 +1,13 @@
 module Liquid
   class ParseContext
-    attr_accessor :locale, :line_number, :trim_whitespace
+    attr_accessor :locale, :line_number, :trim_whitespace, :depth
     attr_reader :partial, :warnings, :error_mode
 
     def initialize(options = {})
       @template_options = options ? options.dup : {}
       @locale = @template_options[:locale] ||= I18n.new
       @warnings = []
+      self.depth = 0
       self.partial = false
     end
 

lib/liquid/standardfilters.rb

@@ -33,7 +33,7 @@ module Liquid
     end
 
     def escape(input)
-      CGI.escapeHTML(input).untaint unless input.nil?
+      CGI.escapeHTML(input.to_s).untaint unless input.nil?
     end
     alias_method :h, :escape
 
@@ -42,11 +42,11 @@ module Liquid
     end
 
     def url_encode(input)
-      CGI.escape(input) unless input.nil?
+      CGI.escape(input.to_s) unless input.nil?
     end
 
     def url_decode(input)
-      CGI.unescape(input) unless input.nil?
+      CGI.unescape(input.to_s) unless input.nil?
     end
 
     def slice(input, offset, length = nil)
@@ -201,12 +201,14 @@ module Liquid
 
     # Replace occurrences of a string with another
     def replace(input, string, replacement = ''.freeze)
-      input.to_s.gsub(string.to_s, replacement.to_s)
+      replacement = replacement.to_s
+      input.to_s.gsub(string.to_s) { replacement }
     end
 
     # Replace the first occurrences of a string with another
     def replace_first(input, string, replacement = ''.freeze)
-      input.to_s.sub(string.to_s, replacement.to_s)
+      replacement = replacement.to_s
+      input.to_s.sub(string.to_s) { replacement }
     end
 
     # remove a substring

lib/liquid/tags/cycle.rb

@@ -30,11 +30,11 @@ module Liquid
     end
 
     def render(context)
-      context.registers[:cycle] ||= Hash.new(0)
+      context.registers[:cycle] ||= {}
 
       context.stack do
         key = context.evaluate(@name)
-        iteration = context.registers[:cycle][key]
+        iteration = context.registers[:cycle][key].to_i
         result = context.evaluate(@variables[iteration])
         iteration += 1
         iteration  = 0 if iteration >= @variables.size

lib/liquid/tags/for.rb

@@ -46,6 +46,9 @@ module Liquid
   class For < Block
     Syntax = /\A(#{VariableSegment}+)\s+in\s+(#{QuotedFragment}+)\s*(reversed)?/o
 
+    attr_reader :collection_name
+    attr_reader :variable_name
+
     def initialize(tag_name, markup, options)
       super
       @from = @limit = nil
@@ -117,7 +120,7 @@ module Liquid
     private
 
     def collection_segment(context)
-      offsets = context.registers[:for] ||= Hash.new(0)
+      offsets = context.registers[:for] ||= {}
 
       from = if @from == :continue
         offsets[@name].to_i

lib/liquid/tags/if.rb

@@ -83,17 +83,20 @@ module Liquid
 
     def strict_parse(markup)
       p = Parser.new(markup)
-      condition = parse_binary_comparison(p)
+      condition = parse_binary_comparisons(p)
       p.consume(:end_of_string)
       condition
     end
 
-    def parse_binary_comparison(p)
+    def parse_binary_comparisons(p)
       condition = parse_comparison(p)
-      if op = (p.id?('and'.freeze) || p.id?('or'.freeze))
+      first_condition = condition
-        condition.send(op, parse_binary_comparison(p))
+      while op = (p.id?('and'.freeze) || p.id?('or'.freeze))
+        child_condition = parse_comparison(p)
+        condition.send(op, child_condition)
+        condition = child_condition
       end
-      condition
+      first_condition
     end
 
     def parse_comparison(p)

test/integration/security_test.rb

@@ -63,4 +63,18 @@ class SecurityTest < Minitest::Test
 
     assert_equal [], (Symbol.all_symbols - current_symbols)
   end
+
+  def test_max_depth_nested_blocks_does_not_raise_exception
+    depth = Liquid::Block::MAX_DEPTH
+    code = "{% if true %}" * depth + "rendered" + "{% endif %}" * depth
+    assert_equal "rendered", Template.parse(code).render!
+  end
+
+  def test_more_than_max_depth_nested_blocks_raises_exception
+    depth = Liquid::Block::MAX_DEPTH + 1
+    code = "{% if true %}" * depth + "rendered" + "{% endif %}" * depth
+    assert_raises(Liquid::StackLevelError) do
+      Template.parse(code).render!
+    end
+  end
 end # SecurityTest

test/integration/standard_filter_test.rb

@@ -128,8 +128,16 @@ class StandardFiltersTest < Minitest::Test
 
   def test_escape
     assert_equal '&lt;strong&gt;', @filters.escape('<strong>')
-    assert_equal nil, @filters.escape(nil)
+    assert_equal '1', @filters.escape(1)
+    assert_equal '2001-02-03', @filters.escape(Date.new(2001, 2, 3))
+    assert_nil @filters.escape(nil)
+  end
+
+  def test_h
     assert_equal '&lt;strong&gt;', @filters.h('<strong>')
+    assert_equal '1', @filters.h(1)
+    assert_equal '2001-02-03', @filters.h(Date.new(2001, 2, 3))
+    assert_nil @filters.h(nil)
   end
 
   def test_escape_once
@@ -138,14 +146,18 @@ class StandardFiltersTest < Minitest::Test
 
   def test_url_encode
     assert_equal 'foo%2B1%40example.com', @filters.url_encode('foo+1@example.com')
-    assert_equal nil, @filters.url_encode(nil)
+    assert_equal '1', @filters.url_encode(1)
+    assert_equal '2001-02-03', @filters.url_encode(Date.new(2001, 2, 3))
+    assert_nil @filters.url_encode(nil)
   end
 
   def test_url_decode
     assert_equal 'foo bar', @filters.url_decode('foo+bar')
     assert_equal 'foo bar', @filters.url_decode('foo%20bar')
     assert_equal 'foo+1@example.com', @filters.url_decode('foo%2B1%40example.com')
-    assert_equal nil, @filters.url_decode(nil)
+    assert_equal '1', @filters.url_decode(1)
+    assert_equal '2001-02-03', @filters.url_decode(Date.new(2001, 2, 3))
+    assert_nil @filters.url_decode(nil)
   end
 
   def test_truncatewords
@@ -330,7 +342,7 @@ class StandardFiltersTest < Minitest::Test
     assert_equal "#{Date.today.year}", @filters.date('today', '%Y')
     assert_equal "#{Date.today.year}", @filters.date('Today', '%Y')
 
-    assert_equal nil, @filters.date(nil, "%B")
+    assert_nil @filters.date(nil, "%B")
 
     assert_equal '', @filters.date('', "%B")
 
@@ -343,15 +355,17 @@ class StandardFiltersTest < Minitest::Test
   def test_first_last
     assert_equal 1, @filters.first([1, 2, 3])
     assert_equal 3, @filters.last([1, 2, 3])
-    assert_equal nil, @filters.first([])
+    assert_nil @filters.first([])
-    assert_equal nil, @filters.last([])
+    assert_nil @filters.last([])
   end
 
   def test_replace
     assert_equal '2 2 2 2', @filters.replace('1 1 1 1', '1', 2)
     assert_equal '2 2 2 2', @filters.replace('1 1 1 1', 1, 2)
+    assert_equal "\\& \\& \\& \\&", @filters.replace('1 1 1 1', '1', "\\&")
     assert_equal '2 1 1 1', @filters.replace_first('1 1 1 1', '1', 2)
     assert_equal '2 1 1 1', @filters.replace_first('1 1 1 1', 1, 2)
+    assert_equal '\\& 1 1 1', @filters.replace_first("1 1 1 1", '1', "\\&")
     assert_template_result '2 1 1 1', "{{ '1 1 1 1' | replace_first: '1', 2 }}"
   end
 

test/integration/tags/include_tag_test.rb

@@ -137,7 +137,7 @@ class IncludeTagTest < Minitest::Test
 
     Liquid::Template.file_system = infinite_file_system.new
 
-    assert_raises(Liquid::StackLevelError, SystemStackError) do
+    assert_raises(Liquid::StackLevelError) do
       Template.parse("{% include 'loop' %}").render!
     end
   end

test/integration/template_test.rb

@@ -261,6 +261,15 @@ class TemplateTest < Minitest::Test
     assert_equal 'Liquid error: undefined variable d', t.errors[2].message
   end
 
+  def test_nil_value_does_not_raise
+    Liquid::Template.error_mode = :strict
+    t = Template.parse("some{{x}}thing")
+    result = t.render!({ 'x' => nil }, strict_variables: true)
+
+    assert_equal 0, t.errors.count
+    assert_equal 'something', result
+  end
+
   def test_undefined_variables_raise
     t = Template.parse("{{x}} {{y}} {{z.a}} {{z.b}} {{z.c.d}}")
 

test/integration/variable_test.rb

@@ -89,4 +89,8 @@ class VariableTest < Minitest::Test
   def test_multiline_variable
     assert_equal 'worked', Template.parse("{{\ntest\n}}").render!('test' => 'worked')
   end
+
+  def test_render_symbol
+    assert_template_result 'bar', '{{ foo }}', 'foo' => :bar
+  end
 end

test/test_helper.rb

@@ -2,7 +2,6 @@
 
 ENV["MT_NO_EXPECTATIONS"] = "1"
 require 'minitest/autorun'
-require 'spy/integration'
 
 $LOAD_PATH.unshift(File.join(File.expand_path(__dir__), '..', 'lib'))
 require 'liquid.rb'

test/unit/condition_unit_test.rb

@@ -65,8 +65,8 @@ class ConditionUnitTest < Minitest::Test
   end
 
   def test_hash_compare_backwards_compatibility
-    assert_equal nil, Condition.new({}, '>', 2).evaluate
+    assert_nil Condition.new({}, '>', 2).evaluate
-    assert_equal nil, Condition.new(2, '>', {}).evaluate
+    assert_nil Condition.new(2, '>', {}).evaluate
     assert_equal false, Condition.new({}, '==', 2).evaluate
     assert_equal true, Condition.new({ 'a' => 1 }, '==', { 'a' => 1 }).evaluate
     assert_equal true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate
@@ -130,17 +130,6 @@ class ConditionUnitTest < Minitest::Test
     assert_equal false, condition.evaluate
   end
 
-  def test_maximum_recursion_depth
-    condition = Condition.new(1, '==', 1)
-
-    assert_raises(Liquid::StackLevelError) do
-      (1..510).each do
-        condition.evaluate
-        condition.and Condition.new(2, '==', 2)
-      end
-    end
-  end
-
   def test_should_allow_custom_proc_operator
     Condition.operators['starts_with'] = proc { |cond, left, right| left =~ %r{^#{right}} }
 

test/unit/context_unit_test.rb

@@ -70,10 +70,6 @@ class ContextUnitTest < Minitest::Test
     @context = Liquid::Context.new
   end
 
-  def teardown
-    Spy.teardown
-  end
-
   def test_variables
     @context['string'] = 'string'
     assert_equal 'string', @context['string']
@@ -98,12 +94,12 @@ class ContextUnitTest < Minitest::Test
     assert_equal false, @context['bool']
 
     @context['nil'] = nil
-    assert_equal nil, @context['nil']
+    assert_nil @context['nil']
-    assert_equal nil, @context['nil']
+    assert_nil @context['nil']
   end
 
   def test_variables_not_existing
-    assert_equal nil, @context['does_not_exist']
+    assert_nil @context['does_not_exist']
   end
 
   def test_scoping
@@ -185,7 +181,7 @@ class ContextUnitTest < Minitest::Test
     @context['test'] = 'test'
     assert_equal 'test', @context['test']
     @context.pop
-    assert_equal nil, @context['test']
+    assert_nil @context['test']
   end
 
   def test_hierachical_data
@@ -300,7 +296,7 @@ class ContextUnitTest < Minitest::Test
     @context['hash'] = { 'first' => 'Hello' }
 
     assert_equal 1, @context['array.first']
-    assert_equal nil, @context['array["first"]']
+    assert_nil @context['array["first"]']
     assert_equal 'Hello', @context['hash["first"]']
   end
 
@@ -450,14 +446,10 @@ class ContextUnitTest < Minitest::Test
     assert_equal @context, @context['category'].context
   end
 
-  def test_use_empty_instead_of_any_in_interrupt_handling_to_avoid_lots_of_unnecessary_object_allocations
+  def test_interrupt_avoids_object_allocations
-    mock_any = Spy.on_instance_method(Array, :any?)
+    assert_no_object_allocations do
-    mock_empty = Spy.on_instance_method(Array, :empty?)
+      @context.interrupt?
-
+    end
-    @context.interrupt?
-
-    refute mock_any.has_been_called?
-    assert mock_empty.has_been_called?
   end
 
   def test_context_initialization_with_a_proc_in_environment
@@ -480,4 +472,18 @@ class ContextUnitTest < Minitest::Test
     context = Context.new
     assert_equal 'hi', context.apply_global_filter('hi')
   end
+
+  private
+
+  def assert_no_object_allocations
+    unless RUBY_ENGINE == 'ruby'
+      skip "stackprof needed to count object allocations"
+    end
+    require 'stackprof'
+
+    profile = StackProf.run(mode: :object) do
+      yield
+    end
+    assert_equal 0, profile[:samples]
+  end
 end # ContextTest