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