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

Autoload Rails Models unless called from safe_load #19701

Merged

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Jan 8, 2020

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Psych::ClassLoader::Restricted is the class_loader if you use safe_load and was
added in psych 2.0.0:
ruby/psych@2c644e1

Note, ruby 2.4.0 shipped with psych 2.2.2+. This class_loader would not work with ruby 2.3 and older.

From the BZ:

Steps to Reproduce:

  1. bundle exec rails c
  2. YAML.load("--- !ruby/object:PidFile {}\n")
  3. exit console and do step 1 again
  4. YAML.safe_load("--- !ruby/object:PidFile {}\n")

Actual results:

$ bundle exec rails c
...
irb(main):001:0> defined?(PidFile)
=> nil
irb(main):002:0> YAML.load("--- !ruby/object:PidFile {}\n")
=> #<PidFile:0x00007fd988e9b718>
$ bundle exec rails c
...
irb(main):001:0> defined?(PidFile)
=> nil
irb(main):002:0> YAML.safe_load("--- !ruby/object:PidFile {}\n")
=> #<PidFile:0x00007f9ba73d5a28>

This should have failed ^^^

Expected results:

$ bundle exec rails c
...
irb(main):001:0> defined?(PidFile)
=> nil
irb(main):002:0> YAML.load("--- !ruby/object:PidFile {}\n")
=> #<PidFile:0x00007f9d55bdd8b0>
$ bundle exec rails c
...
irb(main):001:0> defined?(PidFile)
=> nil
irb(main):002:0> YAML.safe_load("--- !ruby/object:PidFile {}\n")
Traceback (most recent call last):
       16: from railties (5.1.7) lib/rails/commands/console/console_command.rb:17:in `start'
        ...
        1: from /Users/joerafaniello/.rubies/ruby-2.6.5/lib/ruby/2.6.0/psych/class_loader.rb:97:in `find'
Psych::DisallowedClass (Tried to load unspecified class: PidFile)

This is what should be happening ^^^

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Psych::ClassLoader::Restricted is the class_loader if you use safe_load and was
added in psych 2.0.0:
ruby/psych@2c644e1

Note, ruby 2.4.0 shipped with psych 2.2.2+.  This class_loader would not work with ruby 2.3 and older.
@jrafanie jrafanie force-pushed the do_not_autoload_models_in_safe_load branch from 60a6372 to 85adac0 Compare January 8, 2020 22:37
@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2020

Checked commits jrafanie/manageiq@85adac0~...11a69b5 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 3 offenses detected

spec/initializers/yaml_autoloader_spec.rb

spec/lib/rbac/filterer_spec.rb

@Fryguy Fryguy merged commit b91909c into ManageIQ:master Jan 9, 2020
@Fryguy Fryguy added this to the Sprint 128 Ending Jan 20, 2020 milestone Jan 9, 2020
@Fryguy Fryguy self-assigned this Jan 9, 2020
@jrafanie jrafanie deleted the do_not_autoload_models_in_safe_load branch January 9, 2020 15:17
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

At the very least, we know MiqExpression objects are in the YAML but other
custom classes could be in the YAML so we need to use YAML.load to return to the
prior behavior.
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

At the very least, we know MiqExpression objects are in the YAML but other
custom classes could be in the YAML so we need to use YAML.load to return to the
prior behavior.
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

We're trying to load MiqAeMethodService::MiqAeServiceService and
ActiveSupport::HashWithIndifferentAccess in these places.  The prior behavior in
core was to treat YAML.safe_load like YAML.load so let's change these to .load
for now.  If we want to use safe_load, we'll need to enumerate all of the
additional classes we want to allow to be loaded beyond the ruby basic types.
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

The prior behavior in core was to treat YAML.safe_load like YAML.load so let's change
some of these to .load for now.  We'll enumerate the list of classes where we
can.
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

Seen when run locally or in travis output:

```
472.50s$ bundle exec rake
** ManageIQ master, codename: Jansa
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.
** ManageIQ master, codename: Jansa
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
Randomized with seed 8807
....
```
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

The prior behavior in core was to treat YAML.safe_load like YAML.load so let's change
some of these to .load for now.  We'll enumerate the list of classes where we
can.
jrafanie added a commit to jrafanie/manageiq-automation_engine that referenced this pull request Jan 9, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

Seen when run locally or in travis output:

```
472.50s$ bundle exec rake
** ManageIQ master, codename: Jansa
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.
** ManageIQ master, codename: Jansa
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
Randomized with seed 8807
....
```
pkomanek pushed a commit to pkomanek/manageiq-automation_engine that referenced this pull request Jan 22, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

The prior behavior in core was to treat YAML.safe_load like YAML.load so let's change
some of these to .load for now.  We'll enumerate the list of classes where we
can.
pkomanek pushed a commit to pkomanek/manageiq-automation_engine that referenced this pull request Jan 22, 2020
https://bugzilla.redhat.com/show_bug.cgi?id=1789153

Followup to ManageIQ/manageiq#19701

Seen when run locally or in travis output:

```
472.50s$ bundle exec rake
** ManageIQ master, codename: Jansa
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.
** ManageIQ master, codename: Jansa
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
/home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does
Randomized with seed 8807
....
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants