From c3e6cde67f4af0604f2e8e14cc747eb7bcb5b844 Mon Sep 17 00:00:00 2001
From: Jason Roelofs <jasongroelofs@gmail.com>
Date: Wed, 16 Jan 2013 09:45:55 -0500
Subject: [PATCH 1/5] Add security tests to show that the symbol table doesn't
 grow

---
 test/liquid/security_test.rb | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/test/liquid/security_test.rb b/test/liquid/security_test.rb
index 413cdf6..0e9ac33 100644
--- a/test/liquid/security_test.rb
+++ b/test/liquid/security_test.rb
@@ -38,4 +38,27 @@ class SecurityTest < Test::Unit::TestCase
 
     assert_equal expected, Template.parse(text).render(@assigns, :filters => SecurityFilter)
   end
+
+  def test_does_not_add_filters_to_symbol_table
+    current_symbols = Symbol.all_symbols
+
+    test = %( {{ "some_string" | a_bad_filter }} )
+
+    template = Template.parse(test)
+    assert_equal [], (Symbol.all_symbols - current_symbols)
+
+    template.render
+    assert_equal [], (Symbol.all_symbols - current_symbols)
+  end
+
+  def test_does_not_add_drop_methods_to_symbol_table
+    current_symbols = Symbol.all_symbols
+
+    drop = Drop.new
+    drop.invoke_drop("custom_method_1")
+    drop.invoke_drop("custom_method_2")
+    drop.invoke_drop("custom_method_3")
+
+    assert_equal [], (Symbol.all_symbols - current_symbols)
+  end
 end # SecurityTest

From 7bcb565668a17cbdd08dedba60dfc972a00c1f63 Mon Sep 17 00:00:00 2001
From: Jason Roelofs <jasongroelofs@gmail.com>
Date: Tue, 15 Jan 2013 11:33:17 -0500
Subject: [PATCH 2/5] Remove #to_sym calls from Drop and Variable

Symbols are not needed here and using plain strings is nicer on Ruby
---
 lib/liquid/drop.rb           |  4 ++--
 lib/liquid/variable.rb       |  2 +-
 test/liquid/variable_test.rb | 30 +++++++++++++++---------------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/lib/liquid/drop.rb b/lib/liquid/drop.rb
index ee2a4d7..4d415c8 100644
--- a/lib/liquid/drop.rb
+++ b/lib/liquid/drop.rb
@@ -31,8 +31,8 @@ module Liquid
 
     # called by liquid to invoke a drop
     def invoke_drop(method_or_key)
-      if method_or_key && method_or_key != EMPTY_STRING && self.class.public_method_defined?(method_or_key.to_s.to_sym)
-        send(method_or_key.to_s.to_sym)
+      if method_or_key && method_or_key != EMPTY_STRING && self.class.public_method_defined?(method_or_key.to_s)
+        send(method_or_key.to_s)
       else
         before_method(method_or_key)
       end
diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb
index 0c04fe2..962a992 100644
--- a/lib/liquid/variable.rb
+++ b/lib/liquid/variable.rb
@@ -26,7 +26,7 @@ module Liquid
             if matches = f.match(/\s*(\w+)/)
               filtername = matches[1]
               filterargs = f.scan(/(?:#{FilterArgumentSeparator}|#{ArgumentSeparator})\s*(#{QuotedFragment})/o).flatten
-              @filters << [filtername.to_sym, filterargs]
+              @filters << [filtername, filterargs]
             end
           end
         end
diff --git a/test/liquid/variable_test.rb b/test/liquid/variable_test.rb
index 7bdef35..1e3335b 100644
--- a/test/liquid/variable_test.rb
+++ b/test/liquid/variable_test.rb
@@ -11,71 +11,71 @@ class VariableTest < Test::Unit::TestCase
   def test_filters
     var = Variable.new('hello | textileze')
     assert_equal 'hello', var.name
-    assert_equal [[:textileze,[]]], var.filters
+    assert_equal [["textileze",[]]], var.filters
 
     var = Variable.new('hello | textileze | paragraph')
     assert_equal 'hello', var.name
-    assert_equal [[:textileze,[]], [:paragraph,[]]], var.filters
+    assert_equal [["textileze",[]], ["paragraph",[]]], var.filters
 
     var = Variable.new(%! hello | strftime: '%Y'!)
     assert_equal 'hello', var.name
-    assert_equal [[:strftime,["'%Y'"]]], var.filters
+    assert_equal [["strftime",["'%Y'"]]], var.filters
 
     var = Variable.new(%! 'typo' | link_to: 'Typo', true !)
     assert_equal %!'typo'!, var.name
-    assert_equal [[:link_to,["'Typo'", "true"]]], var.filters
+    assert_equal [["link_to",["'Typo'", "true"]]], var.filters
 
     var = Variable.new(%! 'typo' | link_to: 'Typo', false !)
     assert_equal %!'typo'!, var.name
-    assert_equal [[:link_to,["'Typo'", "false"]]], var.filters
+    assert_equal [["link_to",["'Typo'", "false"]]], var.filters
 
     var = Variable.new(%! 'foo' | repeat: 3 !)
     assert_equal %!'foo'!, var.name
-    assert_equal [[:repeat,["3"]]], var.filters
+    assert_equal [["repeat",["3"]]], var.filters
 
     var = Variable.new(%! 'foo' | repeat: 3, 3 !)
     assert_equal %!'foo'!, var.name
-    assert_equal [[:repeat,["3","3"]]], var.filters
+    assert_equal [["repeat",["3","3"]]], var.filters
 
     var = Variable.new(%! 'foo' | repeat: 3, 3, 3 !)
     assert_equal %!'foo'!, var.name
-    assert_equal [[:repeat,["3","3","3"]]], var.filters
+    assert_equal [["repeat",["3","3","3"]]], var.filters
 
     var = Variable.new(%! hello | strftime: '%Y, okay?'!)
     assert_equal 'hello', var.name
-    assert_equal [[:strftime,["'%Y, okay?'"]]], var.filters
+    assert_equal [["strftime",["'%Y, okay?'"]]], var.filters
 
     var = Variable.new(%! hello | things: "%Y, okay?", 'the other one'!)
     assert_equal 'hello', var.name
-    assert_equal [[:things,["\"%Y, okay?\"","'the other one'"]]], var.filters
+    assert_equal [["things",["\"%Y, okay?\"","'the other one'"]]], var.filters
   end
 
   def test_filter_with_date_parameter
 
     var = Variable.new(%! '2006-06-06' | date: "%m/%d/%Y"!)
     assert_equal "'2006-06-06'", var.name
-    assert_equal [[:date,["\"%m/%d/%Y\""]]], var.filters
+    assert_equal [["date",["\"%m/%d/%Y\""]]], var.filters
 
   end
 
   def test_filters_without_whitespace
     var = Variable.new('hello | textileze | paragraph')
     assert_equal 'hello', var.name
-    assert_equal [[:textileze,[]], [:paragraph,[]]], var.filters
+    assert_equal [["textileze",[]], ["paragraph",[]]], var.filters
 
     var = Variable.new('hello|textileze|paragraph')
     assert_equal 'hello', var.name
-    assert_equal [[:textileze,[]], [:paragraph,[]]], var.filters
+    assert_equal [["textileze",[]], ["paragraph",[]]], var.filters
 
     var = Variable.new("hello|replace:'foo','bar'|textileze")
     assert_equal 'hello', var.name
-    assert_equal [[:replace, ["'foo'", "'bar'"]], [:textileze, []]], var.filters
+    assert_equal [["replace", ["'foo'", "'bar'"]], ["textileze", []]], var.filters
   end
 
   def test_symbol
     var = Variable.new("http://disney.com/logo.gif | image: 'med' ")
     assert_equal 'http://disney.com/logo.gif', var.name
-    assert_equal [[:image,["'med'"]]], var.filters
+    assert_equal [["image",["'med'"]]], var.filters
   end
 
   def test_string_single_quoted

From a48e162237ae50e72dc7f9c3ae125372eea1fbea Mon Sep 17 00:00:00 2001
From: Jason Roelofs <jasongroelofs@gmail.com>
Date: Wed, 16 Jan 2013 10:20:01 -0500
Subject: [PATCH 3/5] Change Drop method lookup to not hit respond_to?

Class.public_method_defined? ends up diving into Ruby's core looking for
a method with the given method_or_key. This process at some point turns
method_or_key into a Symbol. This change no longer takes that path and
thus doesn't grow the Symbol table.
---
 lib/liquid/drop.rb | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/liquid/drop.rb b/lib/liquid/drop.rb
index 4d415c8..3972d08 100644
--- a/lib/liquid/drop.rb
+++ b/lib/liquid/drop.rb
@@ -31,7 +31,7 @@ module Liquid
 
     # called by liquid to invoke a drop
     def invoke_drop(method_or_key)
-      if method_or_key && method_or_key != EMPTY_STRING && self.class.public_method_defined?(method_or_key.to_s)
+      if method_or_key && method_or_key != EMPTY_STRING && drop_method_defined?(method_or_key.to_s)
         send(method_or_key.to_s)
       else
         before_method(method_or_key)
@@ -47,5 +47,12 @@ module Liquid
     end
 
     alias :[] :invoke_drop
+
+    private
+
+    # Check for method existence without invoking respond_to?, which creates symbols
+    def drop_method_defined?(method_name)
+      self.class.public_instance_methods.any? {|method| method.to_s == method_name }
+    end
   end
 end

From 1300210f05ee06ffda69365711556c7f5ed4276d Mon Sep 17 00:00:00 2001
From: Jason Roelofs <jasongroelofs@gmail.com>
Date: Wed, 16 Jan 2013 11:14:01 -0500
Subject: [PATCH 4/5] Convert Strainer to white-list method protection

After moving the method existence check from Context into Strainer,
updated Strainer to only accept invokation methods that were added via
filter Modules, and done in a way that respond_to? is never called,
preventing unconstrained Symbol table growth.
---
 lib/liquid/context.rb        |  6 +---
 lib/liquid/strainer.rb       | 38 +++++++++++-----------
 test/liquid/context_test.rb  |  6 ++--
 test/liquid/strainer_test.rb | 62 ++++++++++++++++++++++++++++--------
 4 files changed, 70 insertions(+), 42 deletions(-)

diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb
index 4500aef..e334677 100644
--- a/lib/liquid/context.rb
+++ b/lib/liquid/context.rb
@@ -71,11 +71,7 @@ module Liquid
     end
 
     def invoke(method, *args)
-      if strainer.respond_to?(method)
-        strainer.__send__(method, *args)
-      else
-        args.first
-      end
+      strainer.invoke(method, *args)
     end
 
     # Push new local scope on the stack. use <tt>Context#stack</tt> instead
diff --git a/lib/liquid/strainer.rb b/lib/liquid/strainer.rb
index 445a0ae..79f9ca7 100644
--- a/lib/liquid/strainer.rb
+++ b/lib/liquid/strainer.rb
@@ -9,16 +9,11 @@ module Liquid
   end
 
   # Strainer is the parent class for the filters system.
-  # New filters are mixed into the strainer class which is then instanciated for each liquid template render run.
+  # New filters are mixed into the strainer class which is then instantiated for each liquid template render run.
   #
-  # One of the strainer's responsibilities is to keep malicious method calls out
+  # The Strainer only allows method calls defined in filters given to it via Strainer.global_filter,
+  # Context#add_filters or Template.register_filter
   class Strainer < parent_object #:nodoc:
-    INTERNAL_METHOD = /^__/
-    @@required_methods = Set.new([:__id__, :__send__, :respond_to?, :kind_of?, :extend, :methods, :singleton_methods, :class, :object_id])
-
-    # Ruby 1.9.2 introduces Object#respond_to_missing?, which is invoked by Object#respond_to?
-    @@required_methods << :respond_to_missing? if Object.respond_to? :respond_to_missing?
-
     @@filters = {}
 
     def initialize(context)
@@ -36,19 +31,22 @@ module Liquid
       strainer
     end
 
-    def respond_to?(method, include_private = false)
-      method_name = method.to_s
-      return false if method_name =~ INTERNAL_METHOD
-      return false if @@required_methods.include?(method_name)
-      super
-    end
-
-    # remove all standard methods from the bucket so circumvent security
-    # problems
-    instance_methods.each do |m|
-      unless @@required_methods.include?(m.to_sym)
-        undef_method m
+    def invoke(method, *args)
+      if has_method?(method)
+        send(method, *args)
+      else
+        args.first
       end
     end
+
+    private
+
+    def has_method?(method)
+      methods_to_check = self.methods - self.class.public_instance_methods
+      methods_to_check.any? do |instance_method|
+        instance_method.to_s == method.to_s
+      end
+    end
+
   end
 end
diff --git a/test/liquid/context_test.rb b/test/liquid/context_test.rb
index c2c316f..e50b237 100644
--- a/test/liquid/context_test.rb
+++ b/test/liquid/context_test.rb
@@ -189,10 +189,10 @@ class ContextTest < Test::Unit::TestCase
     end
 
     context = Context.new
-    methods_before = context.strainer.methods.map { |method| method.to_s }
+    assert_equal "Wookie", context.invoke("hi", "Wookie")
+
     context.add_filters(filter)
-    methods_after = context.strainer.methods.map { |method| method.to_s }
-    assert_equal (methods_before + ["hi"]).sort, methods_after.sort
+    assert_equal "Wookie hi!", context.invoke("hi", "Wookie")
   end
 
   def test_add_item_in_outer_scope
diff --git a/test/liquid/strainer_test.rb b/test/liquid/strainer_test.rb
index fc92e10..7e759f7 100644
--- a/test/liquid/strainer_test.rb
+++ b/test/liquid/strainer_test.rb
@@ -3,23 +3,57 @@ require 'test_helper'
 class StrainerTest < Test::Unit::TestCase
   include Liquid
 
-  def test_strainer
-    strainer = Strainer.create(nil)
-    assert_equal false, strainer.respond_to?('__test__')
-    assert_equal false, strainer.respond_to?('test')
-    assert_equal false, strainer.respond_to?('instance_eval')
-    assert_equal false, strainer.respond_to?('__send__')
-    assert_equal true, strainer.respond_to?('size') # from the standard lib
+  module AccessScopeFilters
+    def public_filter
+      "public"
+    end
+
+    def private_filter
+      "private"
+    end
+    private :private_filter
   end
 
-  def test_should_respond_to_two_parameters
-    strainer = Strainer.create(nil)
-    assert_equal true, strainer.respond_to?('size', false)
+  module TestingFilter
+    def test1
+      "test1"
+    end
+
+    def test2
+      "test2"
+    end
   end
 
-  # Asserts that Object#respond_to_missing? is not being undefined in Ruby versions where it has been implemented
-  # Currently this method is only present in Ruby v1.9.2, or higher
-  def test_object_respond_to_missing
-    assert_equal Object.respond_to?(:respond_to_missing?), Strainer.create(nil).respond_to?(:respond_to_missing?)
+  Strainer.global_filter(AccessScopeFilters)
+  Strainer.global_filter(TestingFilter)
+
+  def test_strainer_only_invokes_public_filter_methods
+    strainer = Strainer.create(nil)
+    assert_equal "public", strainer.invoke("public_filter")
   end
+
+  def test_strainer_returns_nil_if_no_filter_method_found
+    strainer = Strainer.create(nil)
+    assert_nil strainer.invoke("private_filter")
+    assert_nil strainer.invoke("undef_the_filter")
+  end
+
+  def test_strainer_returns_first_argument_if_no_method_and_arguments_given
+    strainer = Strainer.create(nil)
+    assert_equal "password", strainer.invoke("undef_the_method", "password")
+  end
+
+  def test_strainer_allows_multiple_filters
+    strainer = Strainer.create(nil)
+    assert_equal "test1", strainer.invoke("test1")
+    assert_equal "test2", strainer.invoke("test2")
+  end
+
+  def test_strainer_only_allows_methods_defined_in_filters
+    strainer = Strainer.create(nil)
+    assert_equal "1 + 1", strainer.invoke("instance_eval", "1 + 1")
+    assert_equal "puts",  strainer.invoke("__send__", "puts", "Hi Mom")
+    assert_equal "has_method?", strainer.invoke("invoke", "has_method?", "invoke")
+  end
+
 end # StrainerTest

From 38b4543bf184cb2fcae743528d6a9b39c20bbd32 Mon Sep 17 00:00:00 2001
From: Dylan Smith <Dylan.Smith@shopify.com>
Date: Tue, 5 Feb 2013 14:45:08 -0500
Subject: [PATCH 5/5] Use sets to check if methods are invokable without
 symbolizing.

---
 lib/liquid/context.rb        |  1 +
 lib/liquid/drop.rb           | 11 +++++++----
 lib/liquid/strainer.rb       | 33 +++++++++++++++++----------------
 test/liquid/drop_test.rb     |  7 +++++++
 test/liquid/strainer_test.rb | 29 +++++++++++------------------
 5 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb
index e334677..129b71a 100644
--- a/lib/liquid/context.rb
+++ b/lib/liquid/context.rb
@@ -39,6 +39,7 @@ module Liquid
 
       filters.each do |f|
         raise ArgumentError, "Expected module but got: #{f.class}" unless f.is_a?(Module)
+        Strainer.add_known_filter(f)
         strainer.extend(f)
       end
     end
diff --git a/lib/liquid/drop.rb b/lib/liquid/drop.rb
index 3972d08..88adb6b 100644
--- a/lib/liquid/drop.rb
+++ b/lib/liquid/drop.rb
@@ -1,3 +1,5 @@
+require 'set'
+
 module Liquid
 
   # A drop in liquid is a class which allows you to export DOM like things to liquid.
@@ -31,8 +33,8 @@ module Liquid
 
     # called by liquid to invoke a drop
     def invoke_drop(method_or_key)
-      if method_or_key && method_or_key != EMPTY_STRING && drop_method_defined?(method_or_key.to_s)
-        send(method_or_key.to_s)
+      if method_or_key && method_or_key != EMPTY_STRING && self.class.invokable?(method_or_key)
+        send(method_or_key)
       else
         before_method(method_or_key)
       end
@@ -51,8 +53,9 @@ module Liquid
     private
 
     # Check for method existence without invoking respond_to?, which creates symbols
-    def drop_method_defined?(method_name)
-      self.class.public_instance_methods.any? {|method| method.to_s == method_name }
+    def self.invokable?(method_name)
+      @invokable_methods ||= Set.new((public_instance_methods - Liquid::Drop.public_instance_methods).map(&:to_s))
+      @invokable_methods.include?(method_name.to_s)
     end
   end
 end
diff --git a/lib/liquid/strainer.rb b/lib/liquid/strainer.rb
index 79f9ca7..15aefa4 100644
--- a/lib/liquid/strainer.rb
+++ b/lib/liquid/strainer.rb
@@ -2,19 +2,15 @@ require 'set'
 
 module Liquid
 
-  parent_object = if defined? BlankObject
-    BlankObject
-  else
-    Object
-  end
-
   # Strainer is the parent class for the filters system.
   # New filters are mixed into the strainer class which is then instantiated for each liquid template render run.
   #
   # The Strainer only allows method calls defined in filters given to it via Strainer.global_filter,
   # Context#add_filters or Template.register_filter
-  class Strainer < parent_object #:nodoc:
+  class Strainer #:nodoc:
     @@filters = {}
+    @@known_filters = Set.new
+    @@known_methods = Set.new
 
     def initialize(context)
       @context = context
@@ -22,9 +18,20 @@ module Liquid
 
     def self.global_filter(filter)
       raise ArgumentError, "Passed filter is not a module" unless filter.is_a?(Module)
+      add_known_filter(filter)
       @@filters[filter.name] = filter
     end
 
+    def self.add_known_filter(filter)
+      unless @@known_filters.include?(filter)
+        @@method_blacklist ||= Set.new(Strainer.instance_methods.map(&:to_s))
+        new_methods = filter.instance_methods.map(&:to_s)
+        new_methods.reject!{ |m| @@method_blacklist.include?(m) }
+        @@known_methods.merge(new_methods)
+        @@known_filters.add(filter)
+      end
+    end
+
     def self.create(context)
       strainer = Strainer.new(context)
       @@filters.each { |k,m| strainer.extend(m) }
@@ -32,21 +39,15 @@ module Liquid
     end
 
     def invoke(method, *args)
-      if has_method?(method)
+      if invokable?(method)
         send(method, *args)
       else
         args.first
       end
     end
 
-    private
-
-    def has_method?(method)
-      methods_to_check = self.methods - self.class.public_instance_methods
-      methods_to_check.any? do |instance_method|
-        instance_method.to_s == method.to_s
-      end
+    def invokable?(method)
+      @@known_methods.include?(method.to_s) && respond_to?(method)
     end
-
   end
 end
diff --git a/test/liquid/drop_test.rb b/test/liquid/drop_test.rb
index ebac1e5..7819f64 100644
--- a/test/liquid/drop_test.rb
+++ b/test/liquid/drop_test.rb
@@ -115,6 +115,13 @@ class DropsTest < Test::Unit::TestCase
     assert_equal '  ', output
   end
 
+  def test_object_methods_not_allowed
+    [:dup, :clone, :singleton_class, :eval, :class_eval, :inspect].each do |method|
+      output = Liquid::Template.parse(" {{ product.#{method} }} ").render('product' => ProductDrop.new)
+      assert_equal '  ', output
+    end
+  end
+
   def test_scope
     assert_equal '1', Liquid::Template.parse( '{{ context.scopes }}'  ).render('context' => ContextDrop.new)
     assert_equal '2', Liquid::Template.parse( '{%for i in dummy%}{{ context.scopes }}{%endfor%}'  ).render('context' => ContextDrop.new, 'dummy' => [1])
diff --git a/test/liquid/strainer_test.rb b/test/liquid/strainer_test.rb
index 7e759f7..582ed7f 100644
--- a/test/liquid/strainer_test.rb
+++ b/test/liquid/strainer_test.rb
@@ -14,22 +14,21 @@ class StrainerTest < Test::Unit::TestCase
     private :private_filter
   end
 
-  module TestingFilter
-    def test1
-      "test1"
-    end
-
-    def test2
-      "test2"
-    end
-  end
-
   Strainer.global_filter(AccessScopeFilters)
-  Strainer.global_filter(TestingFilter)
+
+  def test_strainer
+    strainer = Strainer.create(nil)
+    assert_equal 5, strainer.invoke('size', 'input')
+    assert_equal "public", strainer.invoke("public_filter")
+  end
 
   def test_strainer_only_invokes_public_filter_methods
     strainer = Strainer.create(nil)
-    assert_equal "public", strainer.invoke("public_filter")
+    assert_equal false, strainer.invokable?('__test__')
+    assert_equal false, strainer.invokable?('test')
+    assert_equal false, strainer.invokable?('instance_eval')
+    assert_equal false, strainer.invokable?('__send__')
+    assert_equal true, strainer.invokable?('size') # from the standard lib
   end
 
   def test_strainer_returns_nil_if_no_filter_method_found
@@ -43,12 +42,6 @@ class StrainerTest < Test::Unit::TestCase
     assert_equal "password", strainer.invoke("undef_the_method", "password")
   end
 
-  def test_strainer_allows_multiple_filters
-    strainer = Strainer.create(nil)
-    assert_equal "test1", strainer.invoke("test1")
-    assert_equal "test2", strainer.invoke("test2")
-  end
-
   def test_strainer_only_allows_methods_defined_in_filters
     strainer = Strainer.create(nil)
     assert_equal "1 + 1", strainer.invoke("instance_eval", "1 + 1")