diff --git a/History.md b/History.md index cf6382eb..cf93d993 100644 --- a/History.md +++ b/History.md @@ -29,6 +29,8 @@ * `strict2_parse` is renamed to `parse_markup` * The `warnings` system has been removed. * `safe_parse_expression` has been moved to `Parser.expression_node` +* `parse_expression` methods have been moved to `Parser#unsafe_parse_expression` + * Use `Parser#expression_node`, `Parser#string`, etc. instead ### Migrating from `^5.11.0` - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index 9d55c42b..bd9b775d 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -48,8 +48,9 @@ module Liquid @@operators end - def self.parse_expression(parse_context, markup, safe: false) - @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe) + def self.parse_expression(parser) + markup = parser.expression + @@method_literals[markup] || parser.unsafe_parse_expression(markup) end attr_reader :attachment, :child_condition diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index e6279d76..c8cc35e3 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -49,17 +49,6 @@ module Liquid ) end - def parse_expression(markup, safe: false) - # markup MUST come from a string returned by the parser - # (e.g., parser.expression). We're not calling the parser here to - # prevent redundant parser overhead. The `safe` opt-in - # exists to ensure it is not accidentally still called with - # the result of a regex. - raise Liquid::InternalError, "unsafe parse_expression cannot be used" unless safe - - Expression.parse(markup, @string_scanner, @expression_cache) - end - def partial=(value) @partial = value @options = value ? partial_options : @template_options diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 86de2eab..60664407 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -72,9 +72,9 @@ module Liquid end end + def expression_node - expr = expression - Expression.parse(expr, @ss, @cache) + parse_expression(expression) end def argument @@ -104,5 +104,17 @@ module Liquid end str end + + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + + private + + def parse_expression(markup) + Expression.parse(markup, @ss, @cache) + end end end diff --git a/lib/liquid/parser.rb.orig b/lib/liquid/parser.rb.orig new file mode 100644 index 00000000..3e177e8e --- /dev/null +++ b/lib/liquid/parser.rb.orig @@ -0,0 +1,165 @@ +# frozen_string_literal: true + +module Liquid + class Parser + def initialize(input, expression_cache = nil) + @ss = input.is_a?(StringScanner) ? input : StringScanner.new(input) + @cache = expression_cache + @tokens = Lexer.tokenize(@ss) + @p = 0 # pointer to current location + end + + def jump(point) + @p = point + end + + def consume(type = nil) + token = @tokens[@p] + if type && token[0] != type + raise SyntaxError, "Expected #{type} but found #{@tokens[@p].first}" + end + @p += 1 + token[1] + end + + # Only consumes the token if it matches the type + # Returns the token's contents if it was consumed + # or false otherwise. + def consume?(type) + token = @tokens[@p] + return false unless token && token[0] == type + @p += 1 + token[1] + end + + # Like consume? Except for an :id token of a certain name + def id?(str) + token = @tokens[@p] + return false unless token && token[0] == :id + return false unless token[1] == str + @p += 1 + token[1] + end + + def look(type, ahead = 0) + tok = @tokens[@p + ahead] + return false unless tok + tok[0] == type + end + + def expression + token = @tokens[@p] + case token[0] + when :id + str = consume + str << variable_lookups + when :open_square + str = consume.dup + str << expression + str << consume(:close_square) + str << variable_lookups + when :string, :number + consume + when :open_round + consume + first = expression + consume(:dotdot) + last = expression + consume(:close_round) + "(#{first}..#{last})" + else + raise SyntaxError, "#{token} is not a valid expression" + end + end + + def string + parse_expression(consume(:string)) + end + + def expression_node + parse_expression(expression) + end + +<<<<<<< HEAD + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + + def argument +======= + def string + consume(:string)[1..-2] + end + + def argument_string +>>>>>>> 68476f39 (Move unsafe_parse_expression to the end) + str = +"" + # might be a keyword argument (identifier: expression) + if look(:id) && look(:colon, 1) + str << consume << consume << ' ' + end + + str << expression + str + end + + def variable_lookups + str = +"" + loop do + if look(:open_square) + str << consume + str << expression + str << consume(:close_square) + elsif look(:dot) + str << consume + str << consume(:id) + else + break + end + end + str + end + +<<<<<<< HEAD +======= + def variable_lookup + name = consume(:id) + lookups, command_flags = variable_lookups + if Expression::LITERALS.key?(name) && lookups.empty? + Expression::LITERALS[name] + else + VariableLookup.new(name, lookups, command_flags) + end + end + + def unnamed_variable_lookup + name = indexed_lookup + lookups, command_flags = variable_lookups + VariableLookup.new(name, lookups, command_flags) + end + + def range_lookup + consume(:open_round) + first = expression + consume(:dotdot) + last = expression + consume(:close_round) + RangeLookup.create(first, last) + end + + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + +>>>>>>> 68476f39 (Move unsafe_parse_expression to the end) + private + + def parse_expression(markup) + Expression.parse(markup, @ss, @cache) + end + end +end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 501eec61..216a80bd 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -65,11 +65,5 @@ module Liquid def blank? false end - - private - - def parse_expression(markup, safe: false) - parse_context.parse_expression(markup, safe: safe) - end end end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index cac9d239..ae507c25 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -76,7 +76,7 @@ module Liquid raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') collection_name = p.expression - @collection_name = parse_expression(collection_name, safe: true) + @collection_name = p.unsafe_parse_expression(collection_name) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') @@ -87,7 +87,7 @@ module Liquid raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute") end p.consume(:colon) - set_attribute(attribute, p.expression, safe: true) + set_attribute(attribute, p) end p.consume(:end_of_string) end @@ -157,16 +157,17 @@ module Liquid output end - def set_attribute(key, expr, safe: false) + def set_attribute(key, p) + expr = p.expression case key when 'offset' @from = if expr == 'continue' :continue else - parse_expression(expr, safe: safe) + p.unsafe_parse_expression(expr) end when 'limit' - @limit = parse_expression(expr, safe: safe) + @limit = p.unsafe_parse_expression(expr) end end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 06c752da..2081c788 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -73,8 +73,8 @@ module Liquid block.attach(new_body) end - def parse_expression(markup, safe: false) - Condition.parse_expression(parse_context, markup, safe: safe) + def parse_expression(parser) + Condition.parse_expression(parser) end def parse_markup(markup) @@ -96,9 +96,9 @@ module Liquid end def parse_comparison(p) - a = parse_expression(p.expression, safe: true) + a = parse_expression(p) if (op = p.consume?(:comparison)) - b = parse_expression(p.expression, safe: true) + b = parse_expression(p) Condition.new(a, op, b) else Condition.new(a) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 5a2ef067..fab99487 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -87,7 +87,7 @@ module Liquid def parse_markup(markup) p = @parse_context.new_parser(markup) - @template_name_expr = parse_expression(template_name(p), safe: true) + @template_name_expr = template_name(p) with_or_for = p.id?("for") || p.id?("with") @variable_name_expr = p.expression_node if with_or_for @alias_name = p.consume(:id) if p.id?("as") @@ -107,7 +107,7 @@ module Liquid end def template_name(p) - p.consume(:string) + p.string end class ParseTreeVisitor < Liquid::ParseTreeVisitor diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 194cbf0e..e9add4c8 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -166,25 +166,24 @@ class ConditionUnitTest < Minitest::Test assert_includes(err.lines.map(&:strip), expected) end - def test_parse_expression_with_safe_true + def test_parse_expression environment = Environment.build parse_context = ParseContext.new(environment: environment) - result = Condition.parse_expression(parse_context, 'product.title', safe: true) + parser = parse_context.new_parser('product.title') + result = Condition.parse_expression(parser) assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_raises_internal_error_if_not_safe + def test_parse_expression_returns_method_literal_for_blank_and_empty environment = Environment.build parse_context = ParseContext.new(environment: environment) + parser = parse_context.new_parser('blank') + result = Condition.parse_expression(parser) - error = assert_raises(Liquid::InternalError) do - Condition.parse_expression(parse_context, 'product.title') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) + assert_instance_of(Condition::MethodLiteral, result) end # Tests for blank? comparison without ActiveSupport diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index 89923a75..80b8ffc6 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/test/unit/parse_context_unit_test.rb @@ -5,7 +5,7 @@ require 'test_helper' class ParseContextUnitTest < Minitest::Test include Liquid - def test_safe_parse_expression_with_variable_lookup + def test_parser_expression_node_with_variable_lookup parser = parse_context.new_parser('product.title') result = parser.expression_node @@ -14,7 +14,7 @@ class ParseContextUnitTest < Minitest::Test assert_equal(['title'], result.lookups) end - def test_safe_parse_expression_raises_syntax_error_for_invalid_expression + def test_parser_expression_node_raises_syntax_error_for_invalid_expression parser = parse_context.new_parser('') error = assert_raises(Liquid::SyntaxError) do @@ -25,35 +25,14 @@ class ParseContextUnitTest < Minitest::Test end def test_parse_expression_with_variable_lookup - error = assert_raises(Liquid::InternalError) do - parse_context.parse_expression('product.title') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) - end - - def test_parse_expression_with_safe_true - result = parse_context.parse_expression('product.title', safe: true) + result = parse_context.new_parser('product.title').expression_node assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_with_empty_string - error = assert_raises(Liquid::InternalError) do - parse_context.parse_expression('') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) - end - - def test_parse_expression_with_empty_string_and_safe_true - result = parse_context.parse_expression('', safe: true) - assert_nil(result) - end - - def test_safe_parse_expression_advances_parser_pointer + def test_parser_expression_node_advances_parser_pointer parser = parse_context.new_parser('foo, bar') # parser.expression_node consumes "foo" @@ -71,11 +50,6 @@ class ParseContextUnitTest < Minitest::Test parser.consume(:end_of_string) end - def test_parse_expression_with_whitespace - result = parse_context.parse_expression(' ', safe: true) - assert_nil(result) - end - private def parse_context