From acac5c67c35ea1d532edb77f6ba0d6fd22aa2fef Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Thu, 13 Oct 2022 15:32:26 +0200 Subject: [PATCH 1/4] 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. --- lib/safemode/parser.rb | 51 +++++++++++++++++++++--------------- test/test_safemode_eval.rb | 10 +++++++ test/test_safemode_parser.rb | 22 ++++++++++++++-- 3 files changed, 60 insertions(+), 23 deletions(-) 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/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..f049230 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 = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)" + jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)" + 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 = "@article&.method_with_kwargs('positional', a_keyword: true, **kwargs)" + jailed = "@article&.to_jail&.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)" assert_jailed jailed, unsafe end From d672fa42865481b1c8f1a81cf3c3408ee643c3c0 Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Wed, 31 Aug 2022 13:43:45 +0200 Subject: [PATCH 2/4] Deal with kwargs Ruby 3 has started to separate args and kwargs. --- lib/safemode/jail.rb | 4 ++-- lib/safemode/scope.rb | 16 +++++++++++----- test/test_helper.rb | 10 +++++++--- test/test_jail.rb | 10 ++++++++++ 4 files changed, 30 insertions(+), 10 deletions(-) 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/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/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| From cfd41854c35a68f9384142fd1b78cff9b25563b4 Mon Sep 17 00:00:00 2001 From: Oleh Fedorenko Date: Mon, 11 Dec 2023 12:52:43 +0000 Subject: [PATCH 3/4] Add context for kwargs in tests --- test/test_safemode_parser.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_safemode_parser.rb b/test/test_safemode_parser.rb index f049230..22e21fa 100644 --- a/test/test_safemode_parser.rb +++ b/test/test_safemode_parser.rb @@ -38,8 +38,8 @@ def test_call_with_shorthand end def test_call_with_complex_args - unsafe = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)" - jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)" + 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 @@ -48,8 +48,8 @@ def test_safe_call_simple end def test_safe_call_with_complex_args - unsafe = "@article&.method_with_kwargs('positional', a_keyword: true, **kwargs)" - jailed = "@article&.to_jail&.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)" + 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 From 6f8ba8f750907641a0a30453024a1aa3ea4e64b4 Mon Sep 17 00:00:00 2001 From: Oleh Fedorenko Date: Mon, 11 Dec 2023 12:58:05 +0000 Subject: [PATCH 4/4] Run tests on Ruby 3.0 --- .github/workflows/ci.yml | 1 + safemode.gemspec | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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/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"