Drop Ruby < 2.7 and deal with kwargs (#44)

* Copy process_call implementation from ruby2ruby

This brings it closer to the original, making it easier to see what's
going on. Looking at the git log of ruby2ruby there were no
modifications in this area. The only difference in process_call is that
process_call_receiver is jailed. This implementation actually supports
kwargs and kwsplat.

It also adds tests for kwargs and kwsplat.

* Deal with kwargs

Ruby 3 has started to separate args and kwargs.

* Add context for kwargs in tests

* Run tests on Ruby 3.0

---------

Co-authored-by: Oleh Fedorenko <[email protected]>

.github/workflows/ci.yml

@@ -18,6 +18,7 @@ jobs:
       matrix:
         ruby:
           - "2.7"
+          - "3.0"
     steps:
       - uses: actions/checkout@v3
       - uses: ruby/setup-ruby@v1

lib/safemode/jail.rb

@@ -12,7 +12,7 @@ def to_s
       @source.to_s
     end
 
-    def method_missing(method, *args, &block)
+    def method_missing(method, *args, **kwargs, &block)
       if @source.is_a?(Class)
         unless self.class.allowed_class_method?(method)
           raise Safemode::NoMethodError.new(".#{method}", self.class.name, @source.name)
@@ -28,7 +28,7 @@ def method_missing(method, *args, &block)
       # don't need to jail objects returned from a jail. Doing so would provide
       # "double" protection, but it also would break using a return value in an if
       # statement, passing them to a Rails helper etc.
-      @source.send(method, *args, &block)
+      @source.send(method, *args, **kwargs, &block)
     end
 
     def respond_to_missing?(method_name, include_private = false)

lib/safemode/parser.rb

@@ -36,12 +36,12 @@ def jail(str, parentheses = false, safe_call: false)
 
     # split up #process_call. see below ...
     def process_call(exp, safe_call = false)
-      exp.shift # remove ":call" symbol
-      receiver = jail(process_call_receiver(exp), safe_call: safe_call)
-      name = exp.shift
-      args = process_call_args(exp)
+      _, recv, name, *args = exp
 
-      process_call_code(receiver, name, args, safe_call)
+      receiver = jail(process_call_receiver(recv), safe_call: safe_call)
+      arguments = process_call_args(name, args)
+
+      process_call_code(receiver, name, arguments, safe_call)
     end
 
     def process_fcall(exp)
@@ -143,26 +143,35 @@ def raise_security_error(type, info)
     # split up Ruby2Ruby#process_call monster method so we can hook into it
     # in a more readable manner
 
-    def process_call_receiver(exp)
-      receiver_node_type = exp.first.nil? ? nil : exp.first.first
-      receiver = process exp.shift
-      receiver = "(#{receiver})" if
-        Ruby2Ruby::ASSIGN_NODES.include? receiver_node_type
+    def process_call_receiver(recv)
+      receiver_node_type = recv && recv.sexp_type
+      receiver = process recv
+      receiver = "(#{receiver})" if ASSIGN_NODES.include? receiver_node_type
       receiver
     end
 
-    def process_call_args(exp)
-      args = []
-      while not exp.empty? do
-        args_exp = exp.shift
-        if args_exp && args_exp.first == :array # FIX
-          processed = "#{process(args_exp)[1..-2]}"
-        else
-          processed = process args_exp
-        end
-        args << processed unless (processed.nil? or processed.empty?)
+    def process_call_args(name, args)
+      in_context :arglist do
+        max = args.size - 1
+        args = args.map.with_index { |arg, i|
+          arg_type = arg.sexp_type
+          is_empty_hash = arg == s(:hash)
+          arg = process arg
+
+          next if arg.empty?
+
+          strip_hash = (arg_type == :hash and
+                        not BINARY.include? name and
+                        not is_empty_hash and
+                        (i == max or args[i + 1].sexp_type == :splat))
+          wrap_arg = Ruby2Ruby::ASSIGN_NODES.include? arg_type
+
+          arg = arg[2..-3] if strip_hash
+          arg = "(#{arg})" if wrap_arg
+
+          arg
+        }.compact
       end
-      args
     end
 
     def process_call_code(receiver, name, args, safe_call)

lib/safemode/scope.rb

@@ -30,11 +30,11 @@ def output
       @_safemode_output
     end
 
-    def method_missing(method, *args, &block)
+    def method_missing(method, *args, **kwargs, &block)
       if @locals.has_key?(method)
         @locals[method]
       elsif @delegate_methods.include?(method)
-        @delegate.send method, *unjail_args(args), &block
+        @delegate.send method, *unjail_args(args), **unjail_kwargs(kwargs), &block
       else
         raise Safemode::SecurityError.new(method, "#<Safemode::ScopeObject>")
       end
@@ -49,10 +49,16 @@ def symbolize_keys(hash)
         end
       end
 
+      def unjail(arg)
+        arg.class.name.end_with?('::Jail') ? arg.instance_variable_get(:@source) : arg
+      end
+
       def unjail_args(args)
-        args.collect do |arg|
-          arg.class.name =~ /::Jail$/ ? arg.instance_variable_get(:@source) : arg
-        end
+        args.collect { |arg| unjail(arg) }
+      end
+
+      def unjail_kwargs(kwargs)
+        kwargs.map { |key, value| [unjail(key), unjail(value)] }.to_h
       end
   end
 end

safemode.gemspec

@@ -52,7 +52,7 @@ Gem::Specification.new do |s|
     "test/test_safemode_parser.rb"
   ]
 
-  s.required_ruby_version = ">= 2.7", "< 3"
+  s.required_ruby_version = ">= 2.7", "< 3.1"
 
   s.add_runtime_dependency "ruby2ruby", ">= 2.4.0"
   s.add_runtime_dependency "ruby_parser", ">= 3.10.1"

test/test_helper.rb

@@ -110,8 +110,12 @@ def comment_class
     Comment
   end
 
-  def method_missing(method, *args, &block)
-    super(method, *args, &block)
+  def method_with_kwargs(a_keyword: false)
+    a_keyword
+  end
+
+  def method_missing(method, *args, **kwargs, &block)
+    super
   end
 end
 
@@ -144,7 +148,7 @@ def self.destroy_all
 end
 
 class Article::Jail < Safemode::Jail
-  allow :title, :comments, :is_article?, :comment_class
+  allow :title, :comments, :is_article?, :comment_class, :method_with_kwargs
 
   def author_name
     "this article's author name"

test/test_jail.rb

@@ -24,6 +24,16 @@ def test_sending_to_jail_to_an_object_should_return_a_jail
     assert_equal "Article::Jail", @article.class.name
   end
 
+  def test_sending_of_kwargs_works
+    assert @article.method_with_kwargs(a_keyword: true)
+  end
+
+  def test_sending_to_method_missing
+    assert_raise_with_message(Safemode::NoMethodError, /#no_such_method/) do
+      @article.no_such_method('arg', key: 'value')
+    end
+  end
+
   def test_jail_instances_should_have_limited_methods
     expected = ["class", "method_missing", "methods", "respond_to?", "to_jail", "to_s", "instance_variable_get"]
     objects.each do |object|

test/test_safemode_eval.rb

@@ -88,6 +88,16 @@ def test_should_not_allow_access_to_bind
     assert_raise_security "self.bind('an arg')"
   end
 
+  def test_sending_of_kwargs_works
+    assert @box.eval("@article.method_with_kwargs(a_keyword: true)", @assigns)
+  end
+
+  def test_sending_to_method_missing
+    assert_raise_with_message(Safemode::NoMethodError, /#no_such_method/) do
+      @box.eval("@article.no_such_method('arg', key: 'value')", @assigns)
+    end
+  end
+
   TestHelper.no_method_error_raising_calls.each do |call|
     call.gsub!('"', '\\\\"')
     class_eval %Q(

test/test_safemode_parser.rb

@@ -25,13 +25,31 @@ def test_ternary_should_be_usable_for_erb
     assert_jailed "if true then\n 1\n else\n2\nend", "true ? 1 : 2"
   end
 
+  def test_call_with_shorthand
+    unsafe = <<~UNSAFE
+      a_keyword = true
+      @article.method_with_kwargs(a_keyword:)
+    UNSAFE
+    jailed = <<~JAILED
+      a_keyword = true
+      @article.to_jail.method_with_kwargs(a_keyword:)
+    JAILED
+    assert_jailed jailed, unsafe
+  end
+
+  def test_call_with_complex_args
+    unsafe = "kwargs = { b_keyword: false }; @article.method_with_kwargs('positional', a_keyword: true, **kwargs)"
+    jailed = "kwargs = { :b_keyword => false }\n@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **kwargs)\n"
+    assert_jailed jailed, unsafe
+  end
+
   def test_safe_call_simple
     assert_jailed '@article&.to_jail&.method', '@article&.method'
   end
 
   def test_safe_call_with_complex_args
-    unsafe = "@article&.method_with_kwargs('positional')"
-    jailed = "@article&.to_jail&.method_with_kwargs(\"positional\")"
+    unsafe = "kwargs = { b_keyword: false }; @article&.method_with_kwargs('positional', a_keyword: true, **kwargs)"
+    jailed = "kwargs = { :b_keyword => false }\n@article&.to_jail&.method_with_kwargs(\"positional\", :a_keyword => true, **kwargs)\n"
     assert_jailed jailed, unsafe
   end