From eb143d8cbf019d3bbd5c6e6fa5143145a763996d Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Mon, 11 Dec 2023 15:44:05 +0100 Subject: [PATCH] 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 --- .github/workflows/ci.yml | 1 + lib/safemode/jail.rb | 4 +-- lib/safemode/parser.rb | 51 +++++++++++++++++++++--------------- lib/safemode/scope.rb | 16 +++++++---- safemode.gemspec | 2 +- test/test_helper.rb | 10 ++++--- test/test_jail.rb | 10 +++++++ test/test_safemode_eval.rb | 10 +++++++ test/test_safemode_parser.rb | 22 ++++++++++++++-- 9 files changed, 92 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 253af17..ecf69d0 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/lib/safemode/jail.rb b/lib/safemode/jail.rb index de88a75..bd47add 100644 --- a/lib/safemode/jail.rb +++ b/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) diff --git a/lib/safemode/parser.rb b/lib/safemode/parser.rb index 0d298d6..c731bda 100644 --- a/lib/safemode/parser.rb +++ b/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) diff --git a/lib/safemode/scope.rb b/lib/safemode/scope.rb index e6174fd..9d6525b 100644 --- a/lib/safemode/scope.rb +++ b/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, "#") 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 diff --git a/safemode.gemspec b/safemode.gemspec index f5826e3..d6f8c48 100644 --- a/safemode.gemspec +++ b/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" diff --git a/test/test_helper.rb b/test/test_helper.rb index 82e1247..52416da 100644 --- a/test/test_helper.rb +++ b/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" diff --git a/test/test_jail.rb b/test/test_jail.rb index 3a42176..3629cd9 100644 --- a/test/test_jail.rb +++ b/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| diff --git a/test/test_safemode_eval.rb b/test/test_safemode_eval.rb index c9a23d9..832fdb3 100644 --- a/test/test_safemode_eval.rb +++ b/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( diff --git a/test/test_safemode_parser.rb b/test/test_safemode_parser.rb index 03697d9..22e21fa 100644 --- a/test/test_safemode_parser.rb +++ b/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