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

Support safe call #46

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Support safe call #46

merged 2 commits into from
Dec 11, 2023

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Nov 22, 2022

This copies the process_call_code method from ruby2ruby. That also implements process_safe_call which calls process_call with a parameter.

Fixes #40

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

Choose a reason for hiding this comment

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

Looking at

@@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?

This copies the process_call_code method from ruby2ruby. That also
implements process_safe_call which calls process_call with a parameter.
@ekohl ekohl force-pushed the support-safe-call branch from 3bfea4d to 21fc4eb Compare October 25, 2023 11:10
@ekohl
Copy link
Member Author

ekohl commented Oct 25, 2023

Trivial rebase.

Copy link
Member

@ofedoren ofedoren left a 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:

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

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)
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

@@ -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?

case name
when :<=>, :==, "!=".to_sym, :<, :>, :<=, :>=, :-, :+, :*, :/, :%, :<<, :>>, :** then
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
Member

@ofedoren ofedoren left a 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.

@ofedoren ofedoren merged commit 7897457 into theforeman:master Dec 11, 2023
2 checks passed
@ekohl ekohl deleted the support-safe-call branch December 18, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Safe navigation operator is not supported
3 participants