diff --git a/History.md b/History.md index 3d979dc14..763b80533 100644 --- a/History.md +++ b/History.md @@ -1,5 +1,8 @@ # Liquid Change Log +## 5.9.0 +* Introduce `:rigid` error mode for stricter, safer parsing of all tags [CP Clermont, Guilherme Carreiro] + ## 5.8.7 * Expose body content in the `Doc` tag [James Meng] diff --git a/README.md b/README.md index 3734a290f..34884464d 100644 --- a/README.md +++ b/README.md @@ -99,14 +99,14 @@ Setting the error mode of Liquid lets you specify how strictly you want your tem Normally the parser is very lax and will accept almost anything without error. Unfortunately this can make it very hard to debug and can lead to unexpected behaviour. -Liquid also comes with a stricter parser that can be used when editing templates to give better error messages +Liquid also comes with different parsers that can be used when editing templates to give better error messages when templates are invalid. You can enable this new parser like this: ```ruby -Liquid::Environment.default.error_mode = :strict -Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used -Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal -Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. +Liquid::Environment.default.error_mode = :rigid # Raises a SyntaxError when invalid syntax is used in all tags +Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used in some tags +Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal +Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. ``` If you want to set the error mode only on specific templates you can pass `:error_mode` as an option to `parse`: diff --git a/Rakefile b/Rakefile index 6ccd2c866..878536bf5 100755 --- a/Rakefile +++ b/Rakefile @@ -33,7 +33,7 @@ task :rubocop do end end -desc('runs test suite with both strict and lax parsers') +desc('runs test suite with lax, strict, and rigid parsers') task :test do ENV['LIQUID_PARSER_MODE'] = 'lax' Rake::Task['base_test'].invoke @@ -42,6 +42,10 @@ task :test do Rake::Task['base_test'].reenable Rake::Task['base_test'].invoke + ENV['LIQUID_PARSER_MODE'] = 'rigid' + Rake::Task['base_test'].reenable + Rake::Task['base_test'].invoke + if RUBY_ENGINE == 'ruby' || RUBY_ENGINE == 'truffleruby' ENV['LIQUID_PARSER_MODE'] = 'lax' Rake::Task['integration_test'].reenable @@ -50,6 +54,10 @@ task :test do ENV['LIQUID_PARSER_MODE'] = 'strict' Rake::Task['integration_test'].reenable Rake::Task['integration_test'].invoke + + ENV['LIQUID_PARSER_MODE'] = 'rigid' + Rake::Task['integration_test'].reenable + Rake::Task['integration_test'].invoke end end @@ -80,8 +88,13 @@ namespace :benchmark do ruby "./performance/benchmark.rb strict" end - desc "Run the liquid benchmark with both lax and strict parsing" - task run: [:lax, :strict] + desc "Run the liquid benchmark with rigid parsing" + task :rigid do + ruby "./performance/benchmark.rb rigid" + end + + desc "Run the liquid benchmark with lax, strict, and rigid parsing" + task run: [:lax, :strict, :rigid] desc "Run unit benchmarks" namespace :unit do diff --git a/bin/render b/bin/render new file mode 100755 index 000000000..26d9b53d8 --- /dev/null +++ b/bin/render @@ -0,0 +1,46 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require 'bundler/setup' +require 'liquid' + +class VirtualFileSystem + def initialize + snippet_1 = <<~LIQUID +

+ {{- greating | default: 'Hello' }}, {{ name | default: 'world' -}}! +

+ LIQUID + snippet_2 = <<~LIQUID + {%- for i in (1..5) -%} + > {{ i }} + {%- endfor -%} + LIQUID + + @templates = { + 'snippet-1' => snippet_1, + 'snippet-2' => snippet_2, + } + end + + def read_template_file(key) + @templates[key] || raise(Liquid::FileSystemError, "No such template '#{key}'") + end +end + +def source + File.read(ARGV[0]) +rescue StandardError + 'Usage: bin/render example/server/templates/index.liquid' +end + +def assigns + { + 'date' => Time.now, + } +end + +puts Liquid::Template + .parse(source, error_mode: :rigid) + .tap { |t| t.registers[:file_system] = VirtualFileSystem.new } + .render(assigns) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index e5c321dca..9ab350f07 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -48,8 +48,8 @@ def self.operators @@operators end - def self.parse_expression(parse_context, markup) - @@method_literals[markup] || parse_context.parse_expression(markup) + def self.parse_expression(parse_context, markup, safe: false) + @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe) end attr_reader :attachment, :child_condition diff --git a/lib/liquid/environment.rb b/lib/liquid/environment.rb index 31b17b234..59719c4ad 100644 --- a/lib/liquid/environment.rb +++ b/lib/liquid/environment.rb @@ -34,7 +34,7 @@ class << self # @param file_system The default file system that is used # to load templates from. # @param error_mode [Symbol] The default error mode for all templates - # (either :strict, :warn, or :lax). + # (either :rigid, :strict, :warn, or :lax). # @param exception_renderer [Proc] The exception renderer that is used to # render exceptions. # @yieldparam environment [Environment] The environment instance that is being built. diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index adf340f1f..2605c5576 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -28,6 +28,10 @@ class Expression FLOAT_REGEX = /\A(-?\d+)\.\d+\z/ class << self + def safe_parse(parser, ss = StringScanner.new(""), cache = nil) + parse(parser.expression, ss, cache) + end + def parse(markup, ss = StringScanner.new(""), cache = nil) return unless markup diff --git a/lib/liquid/locales/en.yml b/lib/liquid/locales/en.yml index cd1607b3c..b99d490c8 100644 --- a/lib/liquid/locales/en.yml +++ b/lib/liquid/locales/en.yml @@ -20,6 +20,7 @@ invalid_template_encoding: "Invalid template encoding" render: "Syntax error in tag 'render' - Template name must be a quoted string" table_row: "Syntax Error in 'table_row loop' - Valid syntax: table_row [item] in [collection] cols=3" + table_row_invalid_attribute: "Invalid attribute '%{attribute}' in tablerow loop. Valid attributes are cols, limit, offset, and range" tag_never_closed: "'%{block_name}' tag was never closed" tag_termination: "Tag '%{token}' was not properly terminated with regexp: %{tag_end}" unexpected_else: "%{block_name} tag does not expect 'else' tag" diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 60cdf9e41..476123393 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -50,7 +50,22 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) ) end - def parse_expression(markup) + def safe_parse_expression(parser) + Expression.safe_parse(parser, @string_scanner, @expression_cache) + end + + def parse_expression(markup, safe: false) + if !safe && @error_mode == :rigid + # parse_expression is a widely used API. To maintain backward + # compatibility while raising awareness about rigid parser standards, + # the safe flag supports API users make a deliberate decision. + # + # In rigid mode, 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. + raise Liquid::InternalError, "unsafe parse_expression cannot be used in rigid mode" + end + Expression.parse(markup, @string_scanner, @expression_cache) end diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 78afd58a9..7c662e928 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -2,10 +2,22 @@ module Liquid module ParserSwitching + # Do not use this. + # + # It's basically doing the same thing the {#parse_with_selected_parser}, + # except this will try the strict parser regardless of the error mode, + # and fall back to the lax parser if the error mode is lax or warn, + # except when in rigid mode where it uses the rigid parser. + # + # @deprecated Use {#parse_with_selected_parser} instead. def strict_parse_with_error_mode_fallback(markup) + return rigid_parse_with_error_context(markup) if rigid_mode? + strict_parse_with_error_context(markup) rescue SyntaxError => e case parse_context.error_mode + when :rigid + raise when :strict raise when :warn @@ -16,11 +28,12 @@ def strict_parse_with_error_mode_fallback(markup) def parse_with_selected_parser(markup) case parse_context.error_mode + when :rigid then rigid_parse_with_error_context(markup) when :strict then strict_parse_with_error_context(markup) when :lax then lax_parse(markup) when :warn begin - strict_parse_with_error_context(markup) + rigid_parse_with_error_context(markup) rescue SyntaxError => e parse_context.warnings << e lax_parse(markup) @@ -28,8 +41,20 @@ def parse_with_selected_parser(markup) end end + def rigid_mode? + parse_context.error_mode == :rigid + end + private + def rigid_parse_with_error_context(markup) + rigid_parse(markup) + rescue SyntaxError => e + e.line_number = line_number + e.markup_context = markup_context(markup) + raise e + end + def strict_parse_with_error_context(markup) strict_parse(markup) rescue SyntaxError => e diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 9ca97d0f3..374ee511e 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -68,8 +68,12 @@ def blank? private - def parse_expression(markup) - parse_context.parse_expression(markup) + def safe_parse_expression(parser) + parse_context.safe_parse_expression(parser) + end + + def parse_expression(markup, safe: false) + parse_context.parse_expression(markup, safe: safe) end end end diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 6b67601fe..926e4107a 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -31,12 +31,7 @@ class Case < Block def initialize(tag_name, markup, options) super @blocks = [] - - if markup =~ Syntax - @left = parse_expression(Regexp.last_match(1)) - else - raise SyntaxError, options[:locale].t("errors.syntax.case") - end + parse_with_selected_parser(markup) end def parse(tokens) @@ -91,9 +86,50 @@ def render_to_output_buffer(context, output) private + def rigid_parse(markup) + parser = @parse_context.new_parser(markup) + @left = safe_parse_expression(parser) + parser.consume(:end_of_string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + if markup =~ Syntax + @left = parse_expression(Regexp.last_match(1)) + else + raise SyntaxError, options[:locale].t("errors.syntax.case") + end + end + def record_when_condition(markup) body = new_body + if rigid_mode? + parse_rigid_when(markup, body) + else + parse_lax_when(markup, body) + end + end + + def parse_rigid_when(markup, body) + parser = @parse_context.new_parser(markup) + + loop do + expr = safe_parse_expression(parser) + block = Condition.new(@left, '==', expr) + block.attach(body) + @blocks << block + + break unless parser.id?('or') || parser.consume?(:comma) + end + + parser.consume(:end_of_string) + end + + def parse_lax_when(markup, body) while markup unless markup =~ WhenSyntax raise SyntaxError, options[:locale].t("errors.syntax.case_invalid_when") diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index c2d94d5f4..7e17a9662 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -17,23 +17,13 @@ module Liquid class Cycle < Tag SimpleSyntax = /\A#{QuotedFragment}+/o NamedSyntax = /\A(#{QuotedFragment})\s*\:\s*(.*)/om + UNNAMED_CYCLE_PATTERN = /\w+:0x\h{8}/ attr_reader :variables def initialize(tag_name, markup, options) super - case markup - when NamedSyntax - @variables = variables_from_string(Regexp.last_match(2)) - @name = parse_expression(Regexp.last_match(1)) - @is_named = true - when SimpleSyntax - @variables = variables_from_string(markup) - @name = @variables.to_s - @is_named = !@name.match?(/\w+:0x\h{8}/) - else - raise SyntaxError, options[:locale].t("errors.syntax.cycle") - end + parse_with_selected_parser(markup) end def named? @@ -65,19 +55,82 @@ def render_to_output_buffer(context, output) private + # cycle [name:] expression(, expression)* + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + @variables = [] + + raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) + + first_expression = safe_parse_expression(p) + if p.look(:colon) + # cycle name: expr1, expr2, ... + @name = first_expression + @is_named = true + p.consume(:colon) + # After the colon, parse the first variable (required for named cycles) + @variables << maybe_dup_lookup(safe_parse_expression(p)) + else + # cycle expr1, expr2, ... + @variables << maybe_dup_lookup(first_expression) + end + + # Parse remaining comma-separated expressions + while p.consume?(:comma) + break if p.look(:end_of_string) + + @variables << maybe_dup_lookup(safe_parse_expression(p)) + end + + p.consume(:end_of_string) + + unless @is_named + @name = @variables.to_s + @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN) + end + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + case markup + when NamedSyntax + @variables = variables_from_string(Regexp.last_match(2)) + @name = parse_expression(Regexp.last_match(1)) + @is_named = true + when SimpleSyntax + @variables = variables_from_string(markup) + @name = @variables.to_s + @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN) + else + raise SyntaxError, options[:locale].t("errors.syntax.cycle") + end + end + def variables_from_string(markup) markup.split(',').collect do |var| var =~ /\s*(#{QuotedFragment})\s*/o next unless Regexp.last_match(1) - # Expression Parser returns cached objects, and we need to dup them to - # start the cycle over for each new cycle call. - # Liquid-C does not have a cache, so we don't need to dup the object. var = parse_expression(Regexp.last_match(1)) - var.is_a?(VariableLookup) ? var.dup : var + maybe_dup_lookup(var) end.compact end + # For backwards compatibility, whenever a lookup is used in an unnamed cycle, + # we make it so that the @variables.to_s produces different strings for cycles + # called with the same arguments (since @variables.to_s is used as the cycle counter key) + # This makes it so {% cycle a, b %} and {% cycle a, b %} have independent counters even if a and b share value. + # This is not true for literal values, {% cycle "a", "b" %} and {% cycle "a", "b" %} share the same counter. + # I was really scratching my head about this one, but migrating away from this would be more headache + # than it's worth. So we're keeping this quirk for now. + def maybe_dup_lookup(var) + var.is_a?(VariableLookup) ? var.dup : var + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children Array(@node.variables) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 6aa308f1c..da06d64ad 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -93,7 +93,7 @@ def strict_parse(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') collection_name = p.expression - @collection_name = parse_expression(collection_name) + @collection_name = parse_expression(collection_name, safe: true) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') @@ -104,13 +104,17 @@ def strict_parse(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute") end p.consume(:colon) - set_attribute(attribute, p.expression) + set_attribute(attribute, p.expression, safe: true) end p.consume(:end_of_string) end private + def rigid_parse(markup) + strict_parse(markup) + end + def collection_segment(context) offsets = context.registers[:for] ||= {} @@ -174,16 +178,16 @@ def render_segment(context, output, segment) output end - def set_attribute(key, expr) + def set_attribute(key, expr, safe: false) case key when 'offset' @from = if expr == 'continue' :continue else - parse_expression(expr) + parse_expression(expr, safe: safe) end when 'limit' - @limit = parse_expression(expr) + @limit = parse_expression(expr, safe: safe) end end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 040fecb84..e25d62505 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -66,6 +66,10 @@ def render_to_output_buffer(context, output) private + def rigid_parse(markup) + strict_parse(markup) + end + def push_block(tag, markup) block = if tag == 'else' ElseCondition.new @@ -77,8 +81,8 @@ def push_block(tag, markup) block.attach(new_body) end - def parse_expression(markup) - Condition.parse_expression(parse_context, markup) + def parse_expression(markup, safe: false) + Condition.parse_expression(parse_context, markup, safe: safe) end def lax_parse(markup) @@ -120,9 +124,9 @@ def parse_binary_comparisons(p) end def parse_comparison(p) - a = parse_expression(p.expression) + a = parse_expression(p.expression, safe: true) if (op = p.consume?(:comparison)) - b = parse_expression(p.expression) + b = parse_expression(p.expression, safe: true) Condition.new(a, op, b) else Condition.new(a) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 1fefa16f4..5b2aaa432 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -27,24 +27,7 @@ class Include < Tag def initialize(tag_name, markup, options) super - - if markup =~ SYNTAX - - template_name = Regexp.last_match(1) - variable_name = Regexp.last_match(3) - - @alias_name = Regexp.last_match(5) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @attributes = {} - - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end - - else - raise SyntaxError, options[:locale].t("errors.syntax.include") - end + parse_with_selected_parser(markup) end def parse(_tokens) @@ -101,6 +84,49 @@ def render_to_output_buffer(context, output) alias_method :parse_context, :options private :parse_context + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + @template_name_expr = safe_parse_expression(p) + @variable_name_expr = safe_parse_expression(p) if p.id?("for") || p.id?("with") + @alias_name = p.consume(:id) if p.id?("as") + + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + p.consume(:colon) + @attributes[key] = safe_parse_expression(p) + p.consume?(:comma) + end + + p.consume(:end_of_string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + if markup =~ SYNTAX + template_name = Regexp.last_match(1) + variable_name = Regexp.last_match(3) + + @alias_name = Regexp.last_match(5) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @template_name_expr = parse_expression(template_name) + @attributes = {} + + markup.scan(TagAttributes) do |key, value| + @attributes[key] = parse_expression(value) + end + + else + raise SyntaxError, options[:locale].t("errors.syntax.include") + end + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 26004d647..4d29e420e 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -35,22 +35,7 @@ class Render < Tag def initialize(tag_name, markup, options) super - - raise SyntaxError, options[:locale].t("errors.syntax.render") unless markup =~ SYNTAX - - template_name = Regexp.last_match(1) - with_or_for = Regexp.last_match(3) - variable_name = Regexp.last_match(4) - - @alias_name = Regexp.last_match(6) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @is_for_loop = (with_or_for == FOR) - - @attributes = {} - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end + parse_with_selected_parser(markup) end def for_loop? @@ -99,6 +84,55 @@ def render_tag(context, output) output end + # render (string) (with|for expression)? (as id)? (key: value)* + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + @template_name_expr = parse_expression(rigid_template_name(p), safe: true) + with_or_for = p.id?("for") || p.id?("with") + @variable_name_expr = safe_parse_expression(p) if with_or_for + @alias_name = p.consume(:id) if p.id?("as") + @is_for_loop = (with_or_for == FOR) + + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + p.consume(:colon) + @attributes[key] = safe_parse_expression(p) + p.consume?(:comma) + end + + p.consume(:end_of_string) + end + + def rigid_template_name(p) + p.consume(:string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + raise SyntaxError, options[:locale].t("errors.syntax.render") unless markup =~ SYNTAX + + template_name = Regexp.last_match(1) + with_or_for = Regexp.last_match(3) + variable_name = Regexp.last_match(4) + + @alias_name = Regexp.last_match(6) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @template_name_expr = parse_expression(template_name) + @is_for_loop = (with_or_for == FOR) + + @attributes = {} + markup.scan(TagAttributes) do |key, value| + @attributes[key] = parse_expression(value) + end + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 6767e4fb5..7348b5d91 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -25,11 +25,48 @@ module Liquid # @liquid_optional_param range [untyped] A custom numeric range to iterate over. class TableRow < Block Syntax = /(\w+)\s+in\s+(#{QuotedFragment}+)/o + ALLOWED_ATTRIBUTES = ['cols', 'limit', 'offset', 'range'].freeze attr_reader :variable_name, :collection_name, :attributes def initialize(tag_name, markup, options) super + parse_with_selected_parser(markup) + end + + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + @variable_name = p.consume(:id) + + unless p.id?("in") + raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") + end + + @collection_name = safe_parse_expression(p) + + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + unless ALLOWED_ATTRIBUTES.include?(key) + raise SyntaxError, options[:locale].t("errors.syntax.table_row_invalid_attribute", attribute: key) + end + + p.consume(:colon) + @attributes[key] = safe_parse_expression(p) + p.consume?(:comma) + end + + p.consume(:end_of_string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) if markup =~ Syntax @variable_name = Regexp.last_match(1) @collection_name = parse_expression(Regexp.last_match(2)) diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index a6d80e0ae..5d349abd6 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -24,7 +24,8 @@ class << self # Sets how strict the parser should be. # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases. # :warn is the default and will give deprecation warnings when invalid syntax is used. - # :strict will enforce correct syntax. + # :strict enforces correct syntax for most tags + # :rigid enforces correct syntax for all tags def error_mode=(mode) Deprecations.warn("Template.error_mode=", "Environment#error_mode=") Environment.default.error_mode = mode diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 209570654..3fd947a7d 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -54,7 +54,7 @@ def lax_parse(markup) next unless f =~ /\w+/ filtername = Regexp.last_match(0) filterargs = f.scan(FilterArgsRegex).flatten - @filters << parse_filter_expressions(filtername, filterargs) + @filters << lax_parse_filter_expressions(filtername, filterargs) end end end @@ -65,15 +65,26 @@ def strict_parse(markup) return if p.look(:end_of_string) - @name = parse_context.parse_expression(p.expression) + @name = parse_context.safe_parse_expression(p) while p.consume?(:pipe) filtername = p.consume(:id) filterargs = p.consume?(:colon) ? parse_filterargs(p) : Const::EMPTY_ARRAY - @filters << parse_filter_expressions(filtername, filterargs) + @filters << lax_parse_filter_expressions(filtername, filterargs) end p.consume(:end_of_string) end + def rigid_parse(markup) + @filters = [] + p = @parse_context.new_parser(markup) + + return if p.look(:end_of_string) + + @name = parse_context.safe_parse_expression(p) + @filters << rigid_parse_filter_expressions(p) while p.consume?(:pipe) + p.consume(:end_of_string) + end + def parse_filterargs(p) # first argument filterargs = [p.argument] @@ -122,7 +133,7 @@ def disabled_tags private - def parse_filter_expressions(filter_name, unparsed_args) + def lax_parse_filter_expressions(filter_name, unparsed_args) filter_args = [] keyword_args = nil unparsed_args.each do |a| @@ -138,6 +149,46 @@ def parse_filter_expressions(filter_name, unparsed_args) result end + # Surprisingly, positional and keyword arguments can be mixed. + # + # filter = filtername [":" filterargs?] + # filterargs = argument ("," argument)* + # argument = (positional_argument | keyword_argument) + # positional_argument = expression + # keyword_argument = id ":" expression + def rigid_parse_filter_expressions(p) + filtername = p.consume(:id) + filter_args = [] + keyword_args = {} + + if p.consume?(:colon) + # Parse first argument (no leading comma) + argument(p, filter_args, keyword_args) unless end_of_arguments?(p) + + # Parse remaining arguments (with leading commas) and optional trailing comma + argument(p, filter_args, keyword_args) while p.consume?(:comma) && !end_of_arguments?(p) + end + + result = [filtername, filter_args] + result << keyword_args unless keyword_args.empty? + result + end + + def argument(p, positional_arguments, keyword_arguments) + if p.look(:id) && p.look(:colon, 1) + key = p.consume(:id) + p.consume(:colon) + value = parse_context.safe_parse_expression(p) + keyword_arguments[key] = value + else + positional_arguments << parse_context.safe_parse_expression(p) + end + end + + def end_of_arguments?(p) + p.look(:pipe) || p.look(:end_of_string) + end + def evaluate_filter_expressions(context, filter_args, filter_kwargs) parsed_args = filter_args.map { |expr| context.evaluate(expr) } if filter_kwargs diff --git a/test/integration/context_test.rb b/test/integration/context_test.rb index f6afaca52..f59147e29 100644 --- a/test/integration/context_test.rb +++ b/test/integration/context_test.rb @@ -632,7 +632,7 @@ def notice(output) end def test_has_key_will_not_add_an_error_for_missing_keys - with_error_mode(:strict) do + with_error_modes(:strict) do context = Context.new context.key?('unknown') assert_empty(context.errors) diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 26b0e5f95..0fda83ca5 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -67,7 +67,7 @@ def test_missing_endtag_parse_time_error end def test_unrecognized_operator - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') end diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 29e456321..ae84fa36d 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -26,8 +26,12 @@ def test_int def test_float assert_template_result("-17.42", "{{ -17.42 }}") assert_template_result("2.5", "{{ 2.5 }}") - assert_expression_result(0.0, "0.....5") - assert_expression_result(0.0, "-0..1") + + with_error_modes(:lax) do + assert_expression_result(0.0, "0.....5") + assert_expression_result(0.0, "-0..1") + end + assert_expression_result(1.5, "1.5") # this is a unfortunate quirky behavior of Liquid @@ -61,6 +65,7 @@ def test_quirky_negative_sign_expression_markup assert_template_result( "", "{{ - 'theme.css' - }}", + error_mode: :lax, ) end @@ -147,6 +152,35 @@ def test_disable_expression_cache assert(parse_context.instance_variable_get(:@expression_cache).nil?) end + def test_safe_parse_with_variable_lookup + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('product.title') + result = Liquid::Expression.safe_parse(parser) + + assert_instance_of(Liquid::VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_safe_parse_with_number + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('42') + result = Liquid::Expression.safe_parse(parser) + + assert_equal(42, result) + end + + def test_safe_parse_raises_syntax_error_for_invalid_expression + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('') + + error = assert_raises(Liquid::SyntaxError) do + Liquid::Expression.safe_parse(parser) + end + + assert_match(/is not a valid expression/, error.message) + end + private def assert_expression_result(expect, markup, **assigns) diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index f5b483fcc..b82ce86c5 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -31,18 +31,18 @@ def test_raise_on_label_and_no_close_bracets_percent def test_error_on_empty_filter assert(Template.parse("{{test}}")) - with_error_mode(:lax) do + with_error_modes(:lax) do assert(Template.parse("{{|test}}")) end - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) { Template.parse("{{|test}}") } assert_raises(SyntaxError) { Template.parse("{{test |a|b|}}") } end end def test_meaningless_parens_error - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -51,7 +51,7 @@ def test_meaningless_parens_error end def test_unexpected_characters_syntax_error - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do markup = "true && false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -70,7 +70,7 @@ def test_no_error_on_lax_empty_filter end def test_meaningless_parens_lax - with_error_mode(:lax) do + with_error_modes(:lax) do assigns = { 'b' => 'bar', 'c' => 'baz' } markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}", assigns) @@ -78,7 +78,7 @@ def test_meaningless_parens_lax end def test_unexpected_characters_silently_eat_logic_lax - with_error_mode(:lax) do + with_error_modes(:lax) do markup = "true && false" assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}") markup = "false || true" @@ -93,7 +93,7 @@ def test_raise_on_invalid_tag_delimiter end def test_unanchored_filter_arguments - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('hi', "{{ 'hi there' | split$$$:' ' | first }}") assert_template_result('x', "{{ 'X' | downcase) }}") @@ -106,14 +106,14 @@ def test_unanchored_filter_arguments end def test_invalid_variables_work - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('bar', "{% assign 123foo = 'bar' %}{{ 123foo }}") assert_template_result('123', "{% assign 123 = 'bar' %}{{ 123 }}") end end def test_extra_dots_in_ranges - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('12345', "{% for i in (1...5) %}{{ i }}{% endfor %}") end end @@ -133,7 +133,7 @@ def test_contains_in_id end def test_incomplete_expression - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result("false", "{{ false - }}") assert_template_result("false", "{{ false > }}") assert_template_result("false", "{{ false < }}") diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index a034db17e..f3f865f08 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -3,20 +3,10 @@ require 'test_helper' class CycleTagTest < Minitest::Test - def test_simple_cycle - template = <<~LIQUID - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - LIQUID - - assert_template_result("123", template) - end - def test_simple_cycle_inside_for_loop template = <<~LIQUID {%- for i in (1..3) -%} - {% cycle '1', '2', '3' %} + {%- cycle '1', '2', '3' -%} {%- endfor -%} LIQUID @@ -36,13 +26,157 @@ def test_cycle_with_variables_inside_for_loop assert_template_result("123", template) end - def test_cycle_tag_always_resets_cycle + def test_cycle_named_groups_string + template = <<~LIQUID + {%- for i in (1..3) -%} + {%- cycle 'placeholder1': 1, 2, 3 -%} + {%- cycle 'placeholder2': 1, 2, 3 -%} + {%- endfor -%} + LIQUID + + assert_template_result("112233", template) + end + + def test_cycle_named_groups_vlookup + template = <<~LIQUID + {%- assign placeholder1 = 'placeholder1' -%} + {%- assign placeholder2 = 'placeholder2' -%} + {%- for i in (1..3) -%} + {%- cycle placeholder1: 1, 2, 3 -%} + {%- cycle placeholder2: 1, 2, 3 -%} + {%- endfor -%} + LIQUID + + assert_template_result("112233", template) + end + + def test_unnamed_cycle_have_independent_counters_when_used_with_lookups template = <<~LIQUID {%- assign a = "1" -%} - {%- cycle a, "2" -%} - {%- cycle a, "2" -%} + {%- for i in (1..3) -%} + {%- cycle a, "2" -%} + {%- cycle a, "2" -%} + {%- endfor -%} + LIQUID + + assert_template_result("112211", template) + end + + def test_unnamed_cycle_dependent_counter_when_used_with_literal_values + template = <<~LIQUID + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} + LIQUID + + assert_template_result("121", template) + end + + def test_optional_trailing_comma + template = <<~LIQUID + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", -%} + LIQUID + + assert_template_result("1211", template) + end + + def test_cycle_tag_without_arguments + error = assert_raises(Liquid::SyntaxError) do + Template.parse("{% cycle %}") + end + + assert_match(/Syntax Error in 'cycle' - Valid syntax: cycle \[name :\] var/, error.message) + end + + def test_cycle_tag_with_error_mode + # QuotedFragment is more permissive than what Parser#expression allows. + template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" + template2 = "{% cycle .5: 'a', 'b' %}" + + with_error_modes(:lax, :strict) do + assert_template_result("b", template1) + assert_template_result("a", template2) + end + + with_error_modes(:rigid) do + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } + + expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/ + + assert_match(expected_error, error1.message) + assert_match(expected_error, error2.message) + end + end + + def test_cycle_with_trailing_elements + assignments = "{% assign a = 'A' %}{% assign n = 'N' %}" + + template1 = "#{assignments}{% cycle 'a' 'b', 'c' %}" + template2 = "#{assignments}{% cycle name: 'a' 'b', 'c' %}" + template3 = "#{assignments}{% cycle name: 'a', 'b' 'c' %}" + template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" + template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" + + with_error_modes(:lax, :strict) do + assert_template_result("a", template1) + assert_template_result("a", template2) + assert_template_result("a", template3) + assert_template_result("N", template4) + assert_template_result("N", template5) + end + + with_error_modes(:rigid) do + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } + error3 = assert_raises(Liquid::SyntaxError) { Template.parse(template3) } + error4 = assert_raises(Liquid::SyntaxError) { Template.parse(template4) } + error5 = assert_raises(Liquid::SyntaxError) { Template.parse(template5) } + + expected_error = /Expected end_of_string but found/ + + assert_match(expected_error, error1.message) + assert_match(expected_error, error2.message) + assert_match(expected_error, error3.message) + assert_match(expected_error, error4.message) + assert_match(expected_error, error5.message) + end + end + + def test_cycle_name_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar: "a", "b" %} + {% endfor %} LIQUID - assert_template_result("11", template) + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_cycle_variable_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar, "a", "b" %} + {% endfor %} + LIQUID + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end end end diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index 6e1649663..3ccd17794 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -204,6 +204,32 @@ def test_dynamically_choosen_template ) end + def test_rigid_parsing_errors + with_error_modes(:lax, :strict) do + assert_template_result( + 'hello value1 value2', + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) + end + + with_error_modes(:rigid) do + assert_syntax_error( + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + ) + assert_syntax_error( + '{% include "snippet" | filter %}', + ) + end + end + + def test_optional_commas + partials = { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' } + assert_template_result('hello value1 value2', '{% include "snippet", arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% include "snippet" arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% include "snippet" arg1: "value1" arg2: "value2" %}', partials: partials) + end + def test_include_tag_caches_second_read_of_same_partial file_system = CountingFileSystem.new environment = Liquid::Environment.build(file_system: file_system) @@ -277,13 +303,13 @@ def test_passing_options_to_included_templates assert_raises(Liquid::SyntaxError) do Template.parse("{% include template %}", error_mode: :strict, environment: env).render!("template" => '{{ "X" || downcase }}') end - with_error_mode(:lax) do + with_error_modes(:lax) do assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: true, environment: env).render!("template" => '{{ "X" || downcase }}')) end assert_raises(Liquid::SyntaxError) do Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') end - with_error_mode(:lax) do + with_error_modes(:lax) do assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:error_mode], environment: env).render!("template" => '{{ "X" || downcase }}')) end end @@ -374,4 +400,43 @@ def test_render_tag_renders_error_with_template_name_from_template_factory render_errors: true, ) end + + def test_include_template_with_invalid_expression + template = "{% include foo=>bar %}" + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_include_with_invalid_expression + template = '{% include "snippet" with foo=>bar %}' + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_include_attribute_with_invalid_expression + template = '{% include "snippet", key: foo=>bar %}' + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end # IncludeTagTest diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index eda80a040..d6453fb51 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -105,7 +105,33 @@ def test_dynamically_choosen_templates_are_not_allowed assert_syntax_error("{% assign name = 'snippet' %}{% render name %}") end - def test_include_tag_caches_second_read_of_same_partial + def test_rigid_parsing_errors + with_error_modes(:lax, :strict) do + assert_template_result( + 'hello value1 value2', + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) + end + + with_error_modes(:rigid) do + assert_syntax_error( + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + ) + assert_syntax_error( + '{% render "snippet" | filter %}', + ) + end + end + + def test_optional_commas + partials = { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' } + assert_template_result('hello value1 value2', '{% render "snippet", arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% render "snippet" arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% render "snippet" arg1: "value1" arg2: "value2" %}', partials: partials) + end + + def test_render_tag_caches_second_read_of_same_partial file_system = StubFileSystem.new('snippet' => 'echo') assert_equal( 'echoecho', @@ -288,4 +314,30 @@ def test_render_tag_renders_error_with_template_name_from_template_factory render_errors: true, ) end + + def test_render_with_invalid_expression + template = '{% render "snippet" with foo=>bar %}' + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_render_attribute_with_invalid_expression + template = '{% render "snippet", key: foo=>bar %}' + + with_error_modes(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index ddc0877df..44cdcdd99 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -138,7 +138,7 @@ def test_nil_offset_is_treated_as_zero def test_tablerow_loop_drop_attributes template = <<~LIQUID.chomp - {% tablerow i in (1...2) %} + {% tablerow i in (1..2) %} col: {{ tablerowloop.col }} col0: {{ tablerowloop.col0 }} col_first: {{ tablerowloop.col_first }} @@ -192,12 +192,14 @@ def test_table_row_renders_correct_error_message_for_invalid_parameters assert_template_result( "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) limit:true %} {{n}} {% endtablerow %}', + error_mode: :warn, render_errors: true, ) assert_template_result( "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) offset:true %} {{n}} {% endtablerow %}', + error_mode: :warn, render_errors: true, ) @@ -205,18 +207,19 @@ def test_table_row_renders_correct_error_message_for_invalid_parameters "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) cols:true %} {{n}} {% endtablerow %}', render_errors: true, + error_mode: :warn, ) end def test_table_row_handles_interrupts assert_template_result( "\n 1 \n", - '{% tablerow n in (1...3) cols:2 %} {{n}} {% break %} {{n}} {% endtablerow %}', + '{% tablerow n in (1..3) cols:2 %} {{n}} {% break %} {{n}} {% endtablerow %}', ) assert_template_result( "\n 1 2 \n 3 \n", - '{% tablerow n in (1...3) cols:2 %} {{n}} {% continue %} {{n}} {% endtablerow %}', + '{% tablerow n in (1..3) cols:2 %} {{n}} {% continue %} {{n}} {% endtablerow %}', ) end @@ -255,4 +258,211 @@ def test_table_row_does_not_leak_interrupts template, ) end + + def test_tablerow_with_cols_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..6) cols: 3 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + 456 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_with_limit_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..10) limit: 3 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_with_offset_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..5) offset: 2 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 345 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_with_range_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..3) range: (1..10) %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_with_multiple_attributes_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..10) cols: 2, limit: 4, offset: 1 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 23 + 45 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_with_variable_collection_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in numbers cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 12 + 34 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template, { 'numbers' => [1, 2, 3, 4] }) + end + end + + def test_tablerow_with_dotted_access_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in obj.numbers cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 12 + 34 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template, { 'obj' => { 'numbers' => [1, 2, 3, 4] } }) + end + end + + def test_tablerow_with_bracketed_access_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in obj["numbers"] cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 1020 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template, { 'obj' => { 'numbers' => [10, 20] } }) + end + end + + def test_tablerow_without_attributes_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..3) %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template) + end + end + + def test_tablerow_without_in_keyword_in_rigid_mode + template = '{% tablerow i (1..10) %}{{ i }}{% endtablerow %}' + + with_error_modes(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) + end + end + + def test_tablerow_with_multiple_invalid_attributes_reports_first_in_rigid_mode + template = '{% tablerow i in (1..10) invalid1: 5, invalid2: 10 %}{{ i }}{% endtablerow %}' + + with_error_modes(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) + end + end + + def test_tablerow_with_empty_collection_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in empty_array cols: 2 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + + OUTPUT + + with_error_modes(:rigid) do + assert_template_result(expected, template, { 'empty_array' => [] }) + end + end + + def test_tablerow_with_invalid_attribute_strict_vs_rigid + template = '{% tablerow i in (1..5) invalid_attr: 10 %}{{ i }}{% endtablerow %}' + + expected = <<~OUTPUT + + 12345 + OUTPUT + + with_error_modes(:lax, :strict) do + assert_template_result(expected, template) + end + + with_error_modes(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Invalid attribute 'invalid_attr'/, error.message) + end + end + + def test_tablerow_with_invalid_expression_strict_vs_rigid + template = '{% tablerow i in (1..5) limit: foo=>bar %}{{ i }}{% endtablerow %}' + + with_error_modes(:lax, :strict) do + expected = <<~OUTPUT + + + OUTPUT + assert_template_result(expected, template) + end + + with_error_modes(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index 19922c191..f0f2bc409 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -209,4 +209,69 @@ def test_variable_lookup_should_not_hang_with_invalid_syntax end end end + + def test_filter_with_single_trailing_comma + template = '{{ "hello" | append: "world", }}' + + with_error_modes(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_modes(:rigid) do + assert_template_result('helloworld', template) + end + end + + def test_multiple_filters_with_trailing_commas + template = '{{ "hello" | append: "1", | append: "2", }}' + + with_error_modes(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_modes(:rigid) do + assert_template_result('hello12', template) + end + end + + def test_filter_with_colon_but_no_arguments + template = '{{ "test" | upcase: }}' + + with_error_modes(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_modes(:rigid) do + assert_template_result('TEST', template) + end + end + + def test_filter_chain_with_colon_no_args + template = '{{ "test" | append: "x" | upcase: }}' + + with_error_modes(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_modes(:rigid) do + assert_template_result('TESTX', template) + end + end + + def test_combining_trailing_comma_and_empty_args + template = '{{ "test" | append: "x", | upcase: }}' + + with_error_modes(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_modes(:rigid) do + assert_template_result('TESTX', template) + end + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 4f4447384..293ea4766 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -34,7 +34,7 @@ module Assertions def assert_template_result( expected, template, assigns = {}, - message: nil, partials: nil, error_mode: nil, render_errors: false, + message: nil, partials: nil, error_mode: Liquid::Environment.default.error_mode, render_errors: false, template_factory: nil ) file_system = StubFileSystem.new(partials || {}) @@ -82,10 +82,12 @@ def with_global_filter(*globals, &blk) Environment.dangerously_override(environment, &blk) end - def with_error_mode(mode) + def with_error_modes(*modes) old_mode = Liquid::Environment.default.error_mode - Liquid::Environment.default.error_mode = mode - yield + modes.each do |mode| + Liquid::Environment.default.error_mode = mode + yield + end ensure Liquid::Environment.default.error_mode = old_mode end diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index acc71dc4e..f5206ff53 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -166,6 +166,37 @@ def test_default_context_is_deprecated assert_includes(err.lines.map(&:strip), expected) end + def test_parse_expression_in_strict_mode + environment = Environment.build(error_mode: :strict) + parse_context = ParseContext.new(environment: environment) + result = Condition.parse_expression(parse_context, 'product.title') + + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_parse_expression_in_rigid_mode_raises_internal_error + environment = Environment.build(error_mode: :rigid) + parse_context = ParseContext.new(environment: environment) + + error = assert_raises(Liquid::InternalError) do + Condition.parse_expression(parse_context, 'product.title') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_safe_true_in_rigid_mode + environment = Environment.build(error_mode: :rigid) + parse_context = ParseContext.new(environment: environment) + result = Condition.parse_expression(parse_context, 'product.title', safe: true) + + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + private def assert_evaluates_true(left, op, right) diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb new file mode 100644 index 000000000..b1c8fe470 --- /dev/null +++ b/test/unit/parse_context_unit_test.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ParseContextUnitTest < Minitest::Test + include Liquid + + def test_safe_parse_expression_with_variable_lookup + parser_strict = strict_parse_context.new_parser('product.title') + result_strict = strict_parse_context.safe_parse_expression(parser_strict) + + parser_rigid = rigid_parse_context.new_parser('product.title') + result_rigid = rigid_parse_context.safe_parse_expression(parser_rigid) + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + assert_instance_of(VariableLookup, result_rigid) + assert_equal('product', result_rigid.name) + assert_equal(['title'], result_rigid.lookups) + end + + def test_safe_parse_expression_raises_syntax_error_for_invalid_expression + parser_strict = strict_parse_context.new_parser('') + parser_rigid = rigid_parse_context.new_parser('') + + error_strict = assert_raises(Liquid::SyntaxError) do + strict_parse_context.safe_parse_expression(parser_strict) + end + assert_match(/is not a valid expression/, error_strict.message) + + error_rigid = assert_raises(Liquid::SyntaxError) do + rigid_parse_context.safe_parse_expression(parser_rigid) + end + + assert_match(/is not a valid expression/, error_rigid.message) + end + + def test_parse_expression_with_variable_lookup + result_strict = strict_parse_context.parse_expression('product.title') + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + error = assert_raises(Liquid::InternalError) do + rigid_parse_context.parse_expression('product.title') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_safe_true + result_strict = strict_parse_context.parse_expression('product.title', safe: true) + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + result_rigid = rigid_parse_context.parse_expression('product.title', safe: true) + + assert_instance_of(VariableLookup, result_rigid) + assert_equal('product', result_rigid.name) + assert_equal(['title'], result_rigid.lookups) + end + + def test_parse_expression_with_empty_string + result_strict = strict_parse_context.parse_expression('') + assert_nil(result_strict) + + error = assert_raises(Liquid::InternalError) do + rigid_parse_context.parse_expression('') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_empty_string_and_safe_true + result_strict = strict_parse_context.parse_expression('', safe: true) + assert_nil(result_strict) + + result_rigid = rigid_parse_context.parse_expression('', safe: true) + assert_nil(result_rigid) + end + + def test_safe_parse_expression_advances_parser_pointer + parser = rigid_parse_context.new_parser('foo, bar') + + # safe_parse_expression consumes "foo" + first_result = rigid_parse_context.safe_parse_expression(parser) + assert_instance_of(VariableLookup, first_result) + assert_equal('foo', first_result.name) + + parser.consume(:comma) + + # safe_parse_expression consumes "bar" + second_result = rigid_parse_context.safe_parse_expression(parser) + assert_instance_of(VariableLookup, second_result) + assert_equal('bar', second_result.name) + + parser.consume(:end_of_string) + end + + def test_parse_expression_with_whitespace_in_rigid_mode + result = rigid_parse_context.parse_expression(' ', safe: true) + assert_nil(result) + end + + private + + def strict_parse_context + @strict_parse_context ||= ParseContext.new( + environment: Environment.build(error_mode: :strict), + ) + end + + def rigid_parse_context + @rigid_parse_context ||= ParseContext.new( + environment: Environment.build(error_mode: :rigid), + ) + end +end diff --git a/test/unit/partial_cache_unit_test.rb b/test/unit/partial_cache_unit_test.rb index b081afe52..31790b0d8 100644 --- a/test/unit/partial_cache_unit_test.rb +++ b/test/unit/partial_cache_unit_test.rb @@ -184,7 +184,7 @@ def test_includes_error_mode_into_template_cache }, ) - [:lax, :warn, :strict].each do |error_mode| + [:lax, :warn, :strict, :rigid].each do |error_mode| Liquid::PartialCache.load( 'my_partial', context: context, @@ -193,7 +193,7 @@ def test_includes_error_mode_into_template_cache end assert_equal( - ["my_partial:lax", "my_partial:warn", "my_partial:strict"], + ["my_partial:lax", "my_partial:warn", "my_partial:strict", "my_partial:rigid"], context.registers[:cached_partials].keys, ) end diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index a94d167f5..5e181f3eb 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -9,4 +9,120 @@ def test_case_nodelist template = Liquid::Template.parse('{% case var %}{% when true %}WHEN{% else %}ELSE{% endcase %}') assert_equal(['WHEN', 'ELSE'], template.root.nodelist[0].nodelist.map(&:nodelist).flatten) end + + def test_case_with_trailing_element + template = <<~LIQUID + {%- case 1 bar -%} + {%- when 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + with_error_modes(:lax, :strict) do + assert_template_result("one", template) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Expected end_of_string but found/, error.message) + end + end + + def test_case_when_with_trailing_element + template = <<~LIQUID + {%- case 1 -%} + {%- when 1 bar -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + with_error_modes(:lax, :strict) do + assert_template_result("one", template) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Expected end_of_string but found/, error.message) + end + end + + def test_case_when_with_comma + template = <<~LIQUID + {%- case 1 -%} + {%- when 2, 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + with_error_modes(:lax, :strict, :rigid) do + assert_template_result("one", template) + end + end + + def test_case_when_with_or + template = <<~LIQUID + {%- case 1 -%} + {%- when 2 or 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + with_error_modes(:lax, :strict, :rigid) do + assert_template_result("one", template) + end + end + + def test_case_with_invalid_expression + template = <<~LIQUID + {%- case foo=>bar -%} + {%- when 'baz' -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + assigns = { 'foo' => { 'bar' => 'baz' } } + + with_error_modes(:lax, :strict) do + assert_template_result("one", template, assigns) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Unexpected character =/, error.message) + end + end + + def test_case_when_with_invalid_expression + template = <<~LIQUID + {%- case 'baz' -%} + {%- when foo=>bar -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + assigns = { 'foo' => { 'bar' => 'baz' } } + + with_error_modes(:lax, :strict) do + assert_template_result("one", template, assigns) + end + + with_error_modes(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Unexpected character =/, error.message) + end + end end diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 2cc42e7d3..cb3a6d8ce 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -108,7 +108,7 @@ def test_dashes assert_equal(VariableLookup.new('foo-bar'), create_variable('foo-bar').name) assert_equal(VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name) - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } assert_raises(Liquid::SyntaxError) { create_variable('-foo') } assert_raises(Liquid::SyntaxError) { create_variable('2foo') } @@ -135,16 +135,68 @@ def test_lax_filter_argument_parsing var = create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax) assert_equal(VariableLookup.new('number_of_comments'), var.name) assert_equal([['pluralize', ['comment', 'comments']]], var.filters) + + # missing does not throws error + create_variable(%(n | f1: ,), error_mode: :lax) + create_variable(%(n | f1: ,| f2), error_mode: :lax) + + # arg does not require colon, but ignores args :O, also ignores first kwarg since it splits on ':' + var = create_variable(%(n | f1 1 | f2 k1: v1), error_mode: :lax) + assert_equal([['f1', []], ['f2', [VariableLookup.new('v1')]]], var.filters) + + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2), error_mode: :lax) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title"), error_mode: :lax) + assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters) end def test_strict_filter_argument_parsing - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) end end end + def test_rigid_filter_argument_parsing + with_error_modes(:rigid) do + # optional colon + var = create_variable(%(n | f1 | f2:)) + assert_equal([['f1', []], ['f2', []]], var.filters) + + # missing argument throws error + assert_raises(SyntaxError) { create_variable(%(n | f1: ,)) } + assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2)) } + + # arg requires colon + assert_raises(SyntaxError) { create_variable(%(n | f1 1)) } + + # trailing comma doesn't throw + create_variable(%(n | f1: 1, 2, 3, | f2:)) + + # missing comma throws error + assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3)) } + + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2)) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + + # positional and kwargs mixed + var = create_variable(%(n | filter: 'a', 'b', key1: 1, key2: 2, 'c')) + assert_equal([["filter", ["a", "b", "c"], { "key1" => 1, "key2" => 2 }]], var.filters) + + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title")) + assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters) + + # string key throws + assert_raises(SyntaxError) { create_variable(%(n | pluralize: 'comment': 'comments')) } + end + end + def test_output_raw_source_of_variable var = create_variable(%( name_of_variable | upcase )) assert_equal(" name_of_variable | upcase ", var.raw)