-
Notifications
You must be signed in to change notification settings - Fork 20
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
Support safe call #46
Conversation
case name | ||
when :<=>, :==, "!=".to_sym, :<, :>, :<=, :>=, :-, :+, :*, :/, :%, :<<, :>>, :** then |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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( % & * ** + +@ - -@ / < << <= <=> != == === > >= >> ^ | ~ |
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?
This copies the process_call_code method from ruby2ruby. That also implements process_safe_call which calls process_call with a parameter.
3bfea4d
to
21fc4eb
Compare
Trivial rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ekohl, some comments inline:
lib/safemode/parser.rb
Outdated
def jail(str, parentheses = false, safe_call: false) | ||
str = if str | ||
dot = safe_call ? "&." : "." | ||
parentheses ? "(#{str})&#{dot}" : "#{str}#{dot}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could lead to double &
if safe_call == true && parentheses == true
, which will raise an error.
"#{str}to_jail" | ||
end | ||
|
||
# split up #process_call. see below ... | ||
def process_call(exp) | ||
def process_call(exp, safe_call = false) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
- ruby_parser generates a :safe_call sexp when it encounters
&.
https://github.com/seattlerb/ruby_parser/blob/8f419fdbe7e58bf039cd948b426b18c2f9158548/lib/ruby_parser_extras.rb#L882 - sexp processor does some magic to generate various
process_*
methods, these methods get called as it walks the tree of sexps (somewhere around here) - ruby2ruby has
process_safe_call(exp)
, which just callsprocess_call(exp, :safe)
@@ -159,25 +162,39 @@ def process_call_args(exp) | |||
end | |||
args << processed unless (processed.nil? or processed.empty?) | |||
end | |||
args.empty? ? nil : args.join(", ") |
There was a problem hiding this comment.
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?
case name | ||
when :<=>, :==, "!=".to_sym, :<, :>, :<=, :>=, :-, :+, :*, :/, :%, :<<, :>>, :** then |
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ekohl and @adamruzicka, I think this is ready.
This copies the process_call_code method from ruby2ruby. That also implements process_safe_call which calls process_call with a parameter.
Fixes #40