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.

Sign up for GitHub

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

Jump to bottom

Support safe call #46

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Support safe call #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21fc4eb
Support safe call
ekohl Oct 13, 2022
23633f0
Remove extra &
ofedoren Dec 11, 2023
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
47 changes: 32 additions & 15 deletions lib/safemode/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,22 @@ def parser=(parser)
end
end

def jail(str, parentheses = false)
str = parentheses ? "(#{str})." : "#{str}." if str
def jail(str, parentheses = false, safe_call: false)
str = if str
dot = safe_call ? "&." : "."
parentheses ? "(#{str})#{dot}" : "#{str}#{dot}"
end
"#{str}to_jail"
end

# split up #process_call. see below ...
def process_call(exp)
def process_call(exp, safe_call = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're now more familiar with the lib, when does safe_call change to true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading things right then

exp.shift # remove ":call" symbol
receiver = jail process_call_receiver(exp)
receiver = jail(process_call_receiver(exp), safe_call: safe_call)
name = exp.shift
args = process_call_args(exp)

process_call_code(receiver, name, args)
process_call_code(receiver, name, args, safe_call)
end

def process_fcall(exp)
Expand Down Expand Up @@ -159,25 +162,39 @@ def process_call_args(exp)
end
args << processed unless (processed.nil? or processed.empty?)
end
args.empty? ? nil : args.join(", ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite familiar with what's that for, but why is this change?

args
end

def process_call_code(receiver, name, args)
def process_call_code(receiver, name, args, safe_call)
case name
when :<=>, :==, "!=".to_sym, :<, :>, :<=, :>=, :-, :+, :*, :/, :%, :<<, :>>, :** then
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at https://github.com/seattlerb/ruby2ruby/blob/ddf539eaf348b57c35acd91109fc6d368289b517/lib/ruby2ruby.rb#L39-L40 this implies we'll also support | and & operators which previously weren't supported. I'm not sure if there's a security risk involved there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of completeness, we will support ^ as well. How would someone misuse these operators?.. @adamruzicka, maybe you have an idea? :)

Copy link
Contributor

@adamruzicka adamruzicka Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at

safemode/lib/safemode/core_jails.rb

Line 30 in 01af658

@@default_methods = %w( % & * ** + +@ - -@ / < << <= <=> != == === > >= >> ^ | ~
I'd say we did support them even before, but in a non-operator kind of way?

Before this change when I try to render <%= 5 ^ 3 %>

> receiver
=> "5.to_jail"
> name
=> :^
> args
=> "3"
> "#{receiver}.#{name}#{args ? "(#{args})" : args}"
=> "5.to_jail.^(3)"

With this change the generated code will be (5.to_jail ^ 3), but semantically it should be the same thing so we don't really need to worry about it?

"(#{receiver} #{name} #{args})"
when *BINARY then
if safe_call
"#{receiver}&.#{name}(#{args.join(", ")})"
elsif args.length > 1
"#{receiver}.#{name}(#{args.join(", ")})"
else
"(#{receiver} #{name} #{args.join(", ")})"
end
when :[] then
"#{receiver}[#{args}]"
receiver ||= "self"
"#{receiver}[#{args.join(", ")}]"
when :[]= then
receiver ||= "self"
rhs = args.pop
"#{receiver}[#{args.join(", ")}] = #{rhs}"
when :"!" then
"(not #{receiver})"
when :"-@" then
"-#{receiver}"
when :"+@" then
"+#{receiver}"
else
unless receiver.nil? then
"#{receiver}.#{name}#{args ? "(#{args})" : args}"
else
"#{name}#{args ? "(#{args})" : args}"
end
args = nil if args.empty?
args = "(#{args.join(", ")})" if args
receiver = "#{receiver}." if receiver and not safe_call
receiver = "#{receiver}&." if receiver and safe_call

"#{receiver}#{name}#{args}"
end
end

Expand Down
4 changes: 4 additions & 0 deletions test/test_safemode_eval.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def test_some_stuff_that_should_work
end
end

def test_safe_navigation_operator
assert_equal "1", @box.eval('x = 1; x&.to_s')
end

def test_unary_operators_on_instances_of_boolean_vars
assert @box.eval('not false')
assert @box.eval('!false')
Expand Down
10 changes: 10 additions & 0 deletions test/test_safemode_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ 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_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\")"
assert_jailed jailed, unsafe
end

def test_output_buffer_should_be_assignable
assert_nothing_raised do
jail('@output_buffer = ""')
Expand Down