From 95e9fa5010ac53f35853e8cf06d9b077583febd9 Mon Sep 17 00:00:00 2001 From: Watson Date: Sun, 26 Sep 2021 01:51:56 +0900 Subject: [PATCH 1/7] Use `String#=~` and `Regexp.last_match` instead to retrieve the markup content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the first value is only used obtained with String#scan, it will increase the performance if replace with `String#=~` and `Regexp.last_match`. ### Environment - MacBook Air (M1, 2020) - macOS 12.0 beta 7 - Apple M1 - Ruby 3.0.2 ### Test code ```ruby require 'benchmark/ips' WhitespaceControl = '-' VariableStart = /\{\{/ VariableEnd = /\}\}/ ContentOfVariable = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om token = "{{item.product.featured_image | product_img_url: 'thumb' }}" Benchmark.ips do |x| x.report("String#scan") { token.scan(ContentOfVariable) {|content| break } } x.report("String#match") { m = token.match(ContentOfVariable); m[1] } x.report("String#=~") { token =~ ContentOfVariable; Regexp.last_match(1) } x.compare! end ``` ### Result ``` Warming up -------------------------------------- String#scan 135.724k i/100ms String#match 117.397k i/100ms String#=~ 151.637k i/100ms Calculating ------------------------------------- String#scan 1.351M (± 0.8%) i/s - 6.786M in 5.021955s String#match 1.169M (± 1.3%) i/s - 5.870M in 5.020429s String#=~ 1.520M (± 0.9%) i/s - 7.733M in 5.087427s Comparison: String#=~: 1520250.9 i/s String#scan: 1351399.0 i/s - 1.12x (± 0.00) slower String#match: 1169384.1 i/s - 1.30x (± 0.00) slower ``` --- lib/liquid/block_body.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 76ab0a85..2921ce88 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -231,8 +231,8 @@ module Liquid end def create_variable(token, parse_context) - token.scan(ContentOfVariable) do |content| - markup = content.first + if token =~ ContentOfVariable + markup = Regexp.last_match(1) return Variable.new(markup, parse_context) end BlockBody.raise_missing_variable_terminator(token, parse_context) From ebdfdb80e5040d76118d91022a97b189f2be9a4b Mon Sep 17 00:00:00 2001 From: Watson Date: Sun, 26 Sep 2021 03:03:13 +0900 Subject: [PATCH 2/7] Detect quoted string using String#{start_with?, end_with?} to reduce Regexp#=== calling --- lib/liquid/expression.rb | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index c2c27909..fcdd4d8a 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -10,21 +10,23 @@ module Liquid 'empty' => '' }.freeze - SINGLE_QUOTED_STRING = /\A\s*'(.*)'\s*\z/m - DOUBLE_QUOTED_STRING = /\A\s*"(.*)"\s*\z/m - INTEGERS_REGEX = /\A\s*(-?\d+)\s*\z/ - FLOATS_REGEX = /\A\s*(-?\d[\d\.]+)\s*\z/ + INTEGERS_REGEX = /\A(-?\d+)\z/ + FLOATS_REGEX = /\A(-?\d[\d\.]+)\z/ # Use an atomic group (?>...) to avoid pathological backtracing from # malicious input as described in https://github.com/Shopify/liquid/issues/1357 - RANGES_REGEX = /\A\s*\(\s*(?>(\S+)\s*\.\.)\s*(\S+)\s*\)\s*\z/ + RANGES_REGEX = /\A\(\s*(?>(\S+)\s*\.\.)\s*(\S+)\s*\)\z/ def self.parse(markup) + return nil unless markup + + markup = markup.strip + if (markup.start_with?('"') && markup.end_with?('"')) || + (markup.start_with?("'") && markup.end_with?("'")) + return markup[1..-2] + end + case markup - when nil - nil - when SINGLE_QUOTED_STRING, DOUBLE_QUOTED_STRING - Regexp.last_match(1) when INTEGERS_REGEX Regexp.last_match(1).to_i when RANGES_REGEX @@ -32,7 +34,6 @@ module Liquid when FLOATS_REGEX Regexp.last_match(1).to_f else - markup = markup.strip if LITERALS.key?(markup) LITERALS[markup] else From d8ef698539dd880bd82a8d4ea26c9081798bc3cc Mon Sep 17 00:00:00 2001 From: Shaina Raskas Date: Thu, 20 Jan 2022 09:27:00 -0500 Subject: [PATCH 3/7] fix Shopify documentation link --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6802a717..ec259230 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ * [Contributing guidelines](CONTRIBUTING.md) * [Version history](History.md) -* [Liquid documentation from Shopify](http://docs.shopify.com/themes/liquid-basics) +* [Liquid documentation from Shopify](https://shopify.dev/api/liquid) * [Liquid Wiki at GitHub](https://github.com/Shopify/liquid/wiki) * [Website](http://liquidmarkup.org/) @@ -56,7 +56,7 @@ For standard use you can just pass it the content of a file and call render with Setting the error mode of Liquid lets you specify how strictly you want your templates to be interpreted. 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. +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 when templates are invalid. You can enable this new parser like this: From 7acea2a9c9706664717decbc8d0e1e463b2e880a Mon Sep 17 00:00:00 2001 From: Shaina Raskas Date: Thu, 20 Jan 2022 09:37:26 -0500 Subject: [PATCH 4/7] Revert "fix Shopify documentation link" This reverts commit d8ef698539dd880bd82a8d4ea26c9081798bc3cc. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index ec259230..6802a717 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ * [Contributing guidelines](CONTRIBUTING.md) * [Version history](History.md) -* [Liquid documentation from Shopify](https://shopify.dev/api/liquid) +* [Liquid documentation from Shopify](http://docs.shopify.com/themes/liquid-basics) * [Liquid Wiki at GitHub](https://github.com/Shopify/liquid/wiki) * [Website](http://liquidmarkup.org/) @@ -56,7 +56,7 @@ For standard use you can just pass it the content of a file and call render with Setting the error mode of Liquid lets you specify how strictly you want your templates to be interpreted. 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. +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 when templates are invalid. You can enable this new parser like this: From 03522caaf8d864f60cd89eee004a8f509f33f7b1 Mon Sep 17 00:00:00 2001 From: Shaina Raskas Date: Thu, 20 Jan 2022 09:40:48 -0500 Subject: [PATCH 5/7] fix Shopify documentation link --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6802a717..ec259230 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ * [Contributing guidelines](CONTRIBUTING.md) * [Version history](History.md) -* [Liquid documentation from Shopify](http://docs.shopify.com/themes/liquid-basics) +* [Liquid documentation from Shopify](https://shopify.dev/api/liquid) * [Liquid Wiki at GitHub](https://github.com/Shopify/liquid/wiki) * [Website](http://liquidmarkup.org/) @@ -56,7 +56,7 @@ For standard use you can just pass it the content of a file and call render with Setting the error mode of Liquid lets you specify how strictly you want your templates to be interpreted. 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. +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 when templates are invalid. You can enable this new parser like this: From 1310c4978d15023982b2edbf25a7f8ed86cdcd3c Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 11 Feb 2022 11:37:06 -0500 Subject: [PATCH 6/7] Fix kwarg parsing inconsistency with Liquid::C Liquid::C parses liquid filter arguments with dashes in them, Liquid does not. For tags that accept kwargs and dumps them on the HTML tag, this is an important feature. e.g. {{ ... | image_tag: loading: 'lazy', data-something: 'value!' }} Without this change, Liquid would incorrectly parse the `data-something` kwarg as a single argument and would skip over the invalid characters. See https://github.com/Shopify/theme-check/issues/539 for more context --- lib/liquid.rb | 2 +- test/integration/filter_kwarg_test.rb | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 test/integration/filter_kwarg_test.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index 28337943..7772bb5f 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -36,7 +36,7 @@ module Liquid VariableIncompleteEnd = /\}\}?/ QuotedString = /"[^"]*"|'[^']*'/ QuotedFragment = /#{QuotedString}|(?:[^\s,\|'"]|#{QuotedString})+/o - TagAttributes = /(\w+)\s*\:\s*(#{QuotedFragment})/o + TagAttributes = /(\w[\w-]*)\s*\:\s*(#{QuotedFragment})/o AnyStartingTag = /#{TagStart}|#{VariableStart}/o PartialTemplateParser = /#{TagStart}.*?#{TagEnd}|#{VariableStart}.*?#{VariableIncompleteEnd}/om TemplateParser = /(#{PartialTemplateParser}|#{AnyStartingTag})/om diff --git a/test/integration/filter_kwarg_test.rb b/test/integration/filter_kwarg_test.rb new file mode 100644 index 00000000..2bd3cbdf --- /dev/null +++ b/test/integration/filter_kwarg_test.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'test_helper' + +class FilterKwargTest < Minitest::Test + module KwargFilter + def html_tag(_tag, attributes) + attributes + .map { |key, value| "#{key}='#{value}'" } + .join(' ') + end + end + + include Liquid + + def test_can_parse_data_kwargs + with_global_filter(KwargFilter) do + assert_equal( + "data-src='src' data-widths='100, 200'", + Template.parse("{{ 'img' | html_tag: data-src: 'src', data-widths: '100, 200' }}").render(nil, nil) + ) + end + end +end From 0d5e01ae9825f8ae237fbfcf88e53ab05799ab32 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Thu, 24 Feb 2022 09:17:37 -0500 Subject: [PATCH 7/7] Fix some internal errors in filters from invalid input. (#1476) These fixes came from improving the corresponding test, so these might not actually be causing problems in practice. --- History.md | 6 +++ lib/liquid/standardfilters.rb | 48 ++++++++++++++++-------- test/integration/standard_filter_test.rb | 23 +++++------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/History.md b/History.md index a3a62052..935151bc 100644 --- a/History.md +++ b/History.md @@ -1,5 +1,11 @@ # Liquid Change Log +## 5.1.1 (unreleased) + +### Fixes + +* Fix some internal errors in filters from invalid input [Dylan Thacker-Smith] + ## 5.1.0 / 2021-09-09 ### Features diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb index e7cb5c38..98e38f5f 100644 --- a/lib/liquid/standardfilters.rb +++ b/lib/liquid/standardfilters.rb @@ -213,17 +213,23 @@ module Liquid if ary.empty? [] - elsif ary.first.respond_to?(:[]) && target_value.nil? - begin - ary.select { |item| item[property] } + elsif target_value.nil? + ary.select do |item| + item[property] rescue TypeError raise_property_error(property) + rescue NoMethodError + return nil unless item.respond_to?(:[]) + raise end - elsif ary.first.respond_to?(:[]) - begin - ary.select { |item| item[property] == target_value } + else + ary.select do |item| + item[property] == target_value rescue TypeError raise_property_error(property) + rescue NoMethodError + return nil unless item.respond_to?(:[]) + raise end end end @@ -237,11 +243,14 @@ module Liquid ary.uniq elsif ary.empty? # The next two cases assume a non-empty array. [] - elsif ary.first.respond_to?(:[]) - begin - ary.uniq { |a| a[property] } + else + ary.uniq do |item| + item[property] rescue TypeError raise_property_error(property) + rescue NoMethodError + return nil unless item.respond_to?(:[]) + raise end end end @@ -277,11 +286,14 @@ module Liquid ary.compact elsif ary.empty? # The next two cases assume a non-empty array. [] - elsif ary.first.respond_to?(:[]) - begin - ary.reject { |a| a[property].nil? } + else + ary.reject do |item| + item[property].nil? rescue TypeError raise_property_error(property) + rescue NoMethodError + return nil unless item.respond_to?(:[]) + raise end end end @@ -486,10 +498,16 @@ module Liquid end def nil_safe_compare(a, b) - if !a.nil? && !b.nil? - a <=> b + result = a <=> b + + if result + result + elsif a.nil? + 1 + elsif b.nil? + -1 else - a.nil? ? 1 : -1 + raise Liquid::ArgumentError, "cannot sort values of incompatible types" end end diff --git a/test/integration/standard_filter_test.rb b/test/integration/standard_filter_test.rb index f80d7ad0..e7b34c16 100644 --- a/test/integration/standard_filter_test.rb +++ b/test/integration/standard_filter_test.rb @@ -259,8 +259,8 @@ class StandardFiltersTest < Minitest::Test { "price" => 1, "handle" => "gamma" }, { "price" => 2, "handle" => "epsilon" }, { "price" => 4, "handle" => "alpha" }, - { "handle" => "delta" }, { "handle" => "beta" }, + { "handle" => "delta" }, ] assert_equal(expectation, @filters.sort(input, "price")) end @@ -852,19 +852,14 @@ class StandardFiltersTest < Minitest::Test { 1 => "bar" }, ["foo", 123, nil, true, false, Drop, ["foo"], { foo: "bar" }], ] - test_types.each do |first| - test_types.each do |other| - (@filters.methods - Object.methods).each do |method| - arg_count = @filters.method(method).arity - arg_count *= -1 if arg_count < 0 - inputs = [first] - inputs << ([other] * (arg_count - 1)) if arg_count > 1 - begin - @filters.send(method, *inputs) - rescue Liquid::ArgumentError, Liquid::ZeroDivisionError - nil - end - end + StandardFilters.public_instance_methods(false).each do |method| + arg_count = @filters.method(method).arity + arg_count *= -1 if arg_count < 0 + + test_types.repeated_permutation(arg_count) do |args| + @filters.send(method, *args) + rescue Liquid::Error + nil end end end