Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop Ruby < 2.7 and deal with kwargs #44

Merged
merged 4 commits into from
Dec 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
matrix:
ruby:
- "2.7"
- "3.0"
steps:
- uses: actions/checkout@v3
- uses: ruby/setup-ruby@v1
Expand Down
4 changes: 2 additions & 2 deletions lib/safemode/jail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
51 changes: 30 additions & 21 deletions lib/safemode/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
16 changes: 11 additions & 5 deletions lib/safemode/scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
2 changes: 1 addition & 1 deletion safemode.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
10 changes: 7 additions & 3 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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"
Expand Down
10 changes: 10 additions & 0 deletions test/test_jail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
10 changes: 10 additions & 0 deletions test/test_safemode_eval.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
22 changes: 20 additions & 2 deletions test/test_safemode_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }\[email protected]_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

Expand Down