Drop Ruby < 2.7 and deal with kwargs #44

ekohl · 2022-08-31T11:51:26Z

Ruby 3 has started to separate args and kwargs.

ekohl · 2022-08-31T11:55:19Z

This fails on Ruby < 2.7. We can either use some complicated method to support both, or move forward and drop it. Today Foreman is the main consumer and that only cares about Ruby 2.7+. 2.6 and older are EOL anyway.

lib/ruby_parser_string_io_patch.diff

ares

I've tested this with Foreman and it shows there's some change in the behavior. The kickstart template no longer works, complaining about kickstart_ifcfg_get_identifier_names snippet calling inheriting_mac on nil. This method is called on @interface which is supposed to be a variable holding the interface object. Reading the code, it looks like we can no longer pass variables through the snippet macro.

It seems like the Safemode::Box#eval no longer supports second argument of allowed variables (assigns). While this PR does not touch the code, I guess the kwargs somehow impacts that. However this code seems to work fine

require 'safemode'
require 'erb'

erb_code = %q{<%= @a %>}
box = Safemode::Box.new

puts "Doing the ERB code in safe mode\n-----"
puts box.eval(ERB.new(erb_code).src, { :a => 'b' })

I couldn't find the actual cause, but it's quite simple to reproduce with Foreman. Just create a snippet with <%= @a %> and then call it from the template <%= snippet('name', variables: { :a => 'b' }) %>

ares · 2022-10-04T09:09:34Z

lib/safemode/scope.rb

      if @locals.has_key?(method)
        @locals[method]
      elsif @delegate_methods.include?(method)
+        # TODO: deal with kwargs


is this still a TODO? should we keep it or resolve it before the merge?

You're right, this is precisely why it fails. I struggled to find ways to properly test this. Is it me or is there no test coverage for Safemode::Scope?

ares · 2022-10-04T09:12:57Z

And I can confirm, it's the last commit that caused the change in the behavior.

ekohl · 2022-10-04T11:21:36Z

I've tested this with Foreman and it shows there's some change in the behavior.

Since there is a difference in Ruby versions, did you test it with Ruby 2.7 or 3.0?

And I can confirm, it's the last commit that caused the change in the behavior.

Any thoughts on already merging the first 3 commits?

ares · 2022-10-05T11:50:11Z

It's 2.7.6 available on CentOS Stream 8.

Happy to merge the first three commits if you open them as a separate PR

ekohl · 2022-10-07T13:42:37Z

I've pushed a fix that locally made Foreman's rake test TEST=test/controllers/api/v2/report_templates_controller_test.rb:229 pass. However, I'm still running more tests and I also see undefined method '#name' for NilClass::Jail.

ekohl · 2022-10-07T14:42:51Z

test/test_safemode_parser.rb

+    # TODO: this is wrong for Ruby 3 and raises a deprecation warning in 2.7
+    assert_jailed "@article.to_jail.method_with_kwargs({ :a_keyword => true })", "@article.method_with_kwargs(a_keyword: true)"


I tracked it down to this bit. Reading seattlerb/ruby2ruby#55 (comment) I'm afraid we may have to dive into ruby2ruby to properly deal with this.

I should note that ruby2ruby 2.5.0 does add support for kwargs, so we can at least do it correctly.

ekohl · 2022-10-13T14:03:41Z

test/test_safemode_parser.rb

+
+  def test_call_with_complex_args
+    unsafe = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)"
+    jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)"


I got further by copying the implementation from ruby2ruby. However, I don't think **to_jail.kwargs is correct.

@adamruzicka any thoughts?

That indeed looks awkward..

I guess I found why is that, or at least how to make it be less awkward. It seems like parser needs more context, so if we slightly changes this example to:

unsafe = "kwargs = {a: 1, b: 2} ; @article&.method_with_kwargs('positional', a_keyword: true, **kwargs)"

then the results will change as well:

"kwargs = { :a => 1, :b => 2 }\n@article&.to_jail&.method_with_kwargs(\"positional\", :a_keyword => true, **kwargs)\n"

ekohl · 2022-10-13T14:07:02Z

I made some progress by copying ruby2ruby code. While I was going through it I also noticed safe mode was rather trivial so this now also fixes #40.

ofedoren

Hmm... As of now it looks sane. I've locally run Foreman tests with this PR and they are green (Ruby 2.7.6).

Although, I'd prefer to extract safe call operator from this PR (I know there is one already) and merge that first.

I'm going to run the tests on Ruby 3 and I'll try to do some testing by hands. If that passes as well, I'd be OK with merging even some tests look awkward :)

I'd highly appreciate some @adamruzicka's eyes on this as well.

ofedoren · 2023-11-24T17:09:34Z

test/test_safemode_parser.rb

+
+  def test_call_with_complex_args
+    unsafe = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)"
+    jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)"


That indeed looks awkward..

ofedoren · 2023-11-27T13:55:34Z

Update: I've run the set of Foreman's tests on Ruby 3.0.4 and with theforeman/foreman#9871 it was green. At least on my machine. I've tried to run GHA with that (ofedoren/foreman#9), but I forgot that this lib is still pinned to Ruby < 3.0.

@ekohl, since we're trying to make this lib compatible with Ruby 3, could we update the pinned version? It'll help with automated tests. Which reminds me, maybe we could update ci.yml here to run the tests on Ruby 3.0?

Looking into manual testing now.

ofedoren

@ares, I've re-checked your examples, both work with the latest version of the PR. Anything else comes to your mind worth checking?

Otherwise ACK from me. Still not sure what to do with https://github.com/theforeman/safemode/pull/44/files#diff-5ef4e011cefc0a9aee550ce6870c2739cdb54393cbf42028389a276b4a035aaaR42 though...

This brings it closer to the original, making it easier to see what's going on. Looking at the git log of ruby2ruby there were no modifications in this area. The only difference in process_call is that process_call_receiver is jailed. This implementation actually supports kwargs and kwsplat. It also adds tests for kwargs and kwsplat.

Ruby 3 has started to separate args and kwargs.

ofedoren

Thanks, all, this seems to be ready. I've also included Ruby 3.0 CI and the tests are green. Let's get this in.

ekohl mentioned this pull request Aug 31, 2022

Rails 7 & Ruby 3.1 theforeman/foreman#9328

Closed

ekohl force-pushed the ruby-3 branch from 5b065c8 to de581f5 Compare August 31, 2022 12:04

ekohl changed the title ~~Deal with kwargs~~ Drop Ruby < 2.7 and deal with kwargs Aug 31, 2022

ekohl commented Sep 12, 2022

View reviewed changes

lib/ruby_parser_string_io_patch.diff Outdated Show resolved Hide resolved

ekohl force-pushed the ruby-3 branch from de581f5 to 8db7155 Compare September 12, 2022 11:05

ares requested changes Oct 4, 2022

View reviewed changes

ares reviewed Oct 4, 2022

View reviewed changes

ekohl force-pushed the ruby-3 branch from 8db7155 to 66ac2c0 Compare October 7, 2022 13:35

ekohl marked this pull request as draft October 7, 2022 13:42

ekohl commented Oct 7, 2022

View reviewed changes

ekohl force-pushed the ruby-3 branch 2 times, most recently from 6c46db6 to adbc3c4 Compare October 13, 2022 14:03

ekohl commented Oct 13, 2022

View reviewed changes

ekohl force-pushed the ruby-3 branch from adbc3c4 to fa062f4 Compare October 13, 2022 14:06

ekohl force-pushed the ruby-3 branch from fa062f4 to 59970b6 Compare October 25, 2023 11:11

ekohl mentioned this pull request Nov 23, 2023

Fixes #36849 - Run GHA on Ruby 3.0 theforeman/foreman#9871

Closed

ofedoren reviewed Nov 24, 2023

View reviewed changes

ofedoren reviewed Nov 27, 2023

View reviewed changes

ekohl added 2 commits December 11, 2023 11:50

Deal with kwargs

d672fa4

Ruby 3 has started to separate args and kwargs.

ofedoren force-pushed the ruby-3 branch from 59970b6 to d672fa4 Compare December 11, 2023 11:51

ofedoren added 2 commits December 11, 2023 12:52

Add context for kwargs in tests

cfd4185

Run tests on Ruby 3.0

6f8ba8f

ofedoren marked this pull request as ready for review December 11, 2023 12:58

ofedoren approved these changes Dec 11, 2023

View reviewed changes

ofedoren merged commit eb143d8 into theforeman:master Dec 11, 2023
3 checks passed

ekohl deleted the ruby-3 branch December 18, 2023 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Ruby < 2.7 and deal with kwargs #44

Drop Ruby < 2.7 and deal with kwargs #44

ekohl commented Aug 31, 2022

ekohl commented Aug 31, 2022

ares left a comment

ares Oct 4, 2022

ekohl Oct 7, 2022

ares commented Oct 4, 2022

ekohl commented Oct 4, 2022

ares commented Oct 5, 2022

ekohl commented Oct 7, 2022

ekohl Oct 7, 2022

ekohl Oct 10, 2022

ekohl Oct 13, 2022

ofedoren Nov 24, 2023

ofedoren Nov 27, 2023

ekohl commented Oct 13, 2022

ofedoren left a comment

ofedoren Nov 24, 2023

ofedoren commented Nov 27, 2023

ofedoren left a comment

ofedoren left a comment

		# TODO: this is wrong for Ruby 3 and raises a deprecation warning in 2.7
		assert_jailed "@article.to_jail.method_with_kwargs({ :a_keyword => true })", "@article.method_with_kwargs(a_keyword: true)"

Drop Ruby < 2.7 and deal with kwargs #44

Drop Ruby < 2.7 and deal with kwargs #44

Conversation

ekohl commented Aug 31, 2022

ekohl commented Aug 31, 2022

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Oct 4, 2022

ekohl commented Oct 4, 2022

ares commented Oct 5, 2022

ekohl commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 13, 2022

ofedoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren commented Nov 27, 2023

ofedoren left a comment

Choose a reason for hiding this comment

ofedoren left a comment

Choose a reason for hiding this comment