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

Error after upgrading to Rails 6.0.5.1: Psych::DisallowedClass: Tried to load unspecified class: ActiveSupport::TimeWithZone #631

Open
KevinBongart opened this issue Jul 13, 2022 · 14 comments

Comments

@KevinBongart
Copy link

KevinBongart commented Jul 13, 2022

Hello, I upgraded a Rails app from 6.0.5 to 6.0.5.1 which was released yesterday, and saw this error when saving an audited record:

Psych::DisallowedClass: Tried to load unspecified class: ActiveSupport::TimeZone

It happens when using YAML to deserialize an audit record with a column of ActiveSupport::TimeWithZone type:

From: /Users/kevin/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/audited-5.0.2/lib/audited/audit.rb:23 Audited::YAMLIfTextColumnType.load:

    20: def load(obj)
    21:   if text_column?
    22:     binding.pry
 => 23:     ActiveRecord::Coders::YAMLColumn.new(Object).load(obj)
    24:   else
    25:     obj
    26:   end
    27: end
pry(Audited::YAMLIfTextColumnType)> puts obj
---
first_name: Test
last_name: Test
email: test@example.org
phone: "+15555555555"
[]
confirmation_sent_at: !ruby/object:ActiveSupport::TimeWithZone
  utc: &1 2022-07-13 07:42:50.642612000 Z
  zone: !ruby/object:ActiveSupport::TimeZone
    name: Etc/UTC
  time: *1
[]

After investigating, it's a direct result from the security patch introduced in Rails 7.0.3.1, 6.1.6.1, 6.0.5.1 and 5.2.8.1 and I was able to configure the application to permit some classes. In my case:

# in config/application.rb

# List of classes deemed safe to load by YAML, and required by the Audited
# gem when deserialized audit records.
# As of Rails 6.0.5.1, YAML safe-loading method does not allow all classes
# to be deserialized by default: https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017
config.active_record.yaml_column_permitted_classes = [
  ActiveSupport::HashWithIndifferentAccess,
  ActiveSupport::TimeWithZone,
  ActiveSupport::TimeZone,
  Date,
  Time
]

Hope this is useful to other Audited users! Perhaps Audited could come with this config out of the box, or including some guidance in the README?

@grosser
Copy link
Contributor

grosser commented Jul 13, 2022

FYI got the same with

Psych::DisallowedClass: Tried to load unspecified class: ActiveSupport::HashWithIndifferentAccess

@KevinBongart
Copy link
Author

@grosser Ah yes, I saw the same in my app after opening the issue above. Updating the issue to make it easier for other Audited users to copy/paste a solution.

@crawler
Copy link

crawler commented Jul 13, 2022

I created a script that can help to figure what actually needs to be whitelisted.

what_i_serialize.rb

@barrywoolgar
Copy link

See this issue for some discussion and (personally untested) examples of migrating from YAML to JSON

@vrinek
Copy link

vrinek commented Jul 15, 2022

Pretty much same issue when upgrading to Rails 7.0.3.1:

Psych::DisallowedClass: Tried to load unspecified class: ActiveSupport::TimeWithZone

@crawler
Copy link

crawler commented Jul 15, 2022

@vrinek this is already mentioned in the initial message of this thread.

The fast, unsafe and wrong way:

config.active_record.use_yaml_unsafe_load = ture

The correct way is to eighter somehow migrate to the JSON store or whitelist ruby that being serialized classes:

My case:

config.active_record.yaml_column_permitted_classes =
  %w[String Integer NilClass Float Time Date FalseClass Hash Array DateTime TrueClass BigDecimal
      ActiveSupport::TimeWithZone ActiveSupport::TimeZone ActiveSupport::HashWithIndifferentAccess]

To find what needs to be whitelisted in your case you can try to use this script:

https://gist.github.com/crawler/47a1e66ee2c2ea37f56f9c0c2aac071a

@barrywoolgar
Copy link

I do worry that we can't necessarily know what classes may end up in the audit log later on, so no matter how thorough @crawler's script is, it'll only need a new edge case to come along and potentially cause an exception.

It's a good thing this gem defaults to JSON now, so it's only existing users who will need to figure out a safe migration path.

@crawler
Copy link

crawler commented Jul 15, 2022

@barrywoolgar sure, this is a better way.

I think it is possible to stay with the YAML because you should know what types are present in the schema of the models that are under audit. But it looks like a large surface for the making mistake out of nothing.

@vrinek
Copy link

vrinek commented Jul 15, 2022

@crawler apologies for not reading the first post thoroughly, and thanks for pointing it out as a possible course of action.

@barrywoolgar "will need to figure out a safe migration path" 😅. Yeah, that's gonna take a while to be prioritized. In the meanwhile if anyone has gone through this already, I would love to learn from their experiences.

For now, my migration references include only this:

@alejandrovelez7
Copy link

alejandrovelez7 commented Jul 18, 2022

Should we consider adding a config option similar to active record's but scoped only to audited? In our codebase the only library preventing the upgrade is audited. We don't need to specify the permitted classes array for anything else, so it seems a little overkill to do it for the whole app.

@MSchmidt
Copy link

I'd also prefer a migration to JSON. However there doesn't seem to be a clear path yet besides removing the whole audited history.

@walterdavis
Copy link

[deleted my issue] Operator error, carry on, nothing to see with my particular issue.

@laptopmutia
Copy link

laptopmutia commented Aug 13, 2022

lets move to json

@crawler is this list all complete?

config.active_record.yaml_column_permitted_classes =
  %w[String Integer NilClass Float Time Date FalseClass Hash Array DateTime TrueClass BigDecimal
      ActiveSupport::TimeWithZone ActiveSupport::TimeZone ActiveSupport::HashWithIndifferentAccess]

@crawler
Copy link

crawler commented Aug 19, 2022

@laptopmutia for two my apps where i used this list.

jazairi added a commit to MITLibraries/thing that referenced this issue Mar 13, 2023
Why these changes are being introduced:

The versions table, used by paper_trail, has two columns that use
YAML. PaperTrail prefers that these tables use JSON or JSONB.
In order to update paper_trail to the latest version, we need
either to update our config to use YAML.safe_load or migrate
the data to JSON.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-626
* https://mitlibraries.atlassian.net/browse/ETD-615

How this addresses that need:

This converts the columns to JSON as described here:
https://github.com/paper-trail-gem/paper_trail/blob/v14.0.0/README.md#6b-custom-serializer

Our migration uses `PaperTrail::Serializers::YAML.load`. This is
effectively the same as `YAML.safe_load`. `YAML.load`,
awhich is uggested in the readme, will no longer work due to a
long-running bug in Psych:
collectiveidea/audited#631

We are using JSON instead of JSONB for two reasons:

1. SQLite does not support JSONB, so our local environments will
differ from Heroku; and
2. We are not actively leveraging JSONB's performance advantages in
the codebase, so there is likely no functional difference.

Long-term, it's worth considering whether we should use postgres
locally, as this would support better dev/prod parity and allow us
to use psql features like JSONB with more confidence. We discussed
this briefly in a team meeting on 3/7 and decided that such a
change could bring value but is not necessary at this time.

Side effects of this change:

* One test has been updated to accommodate a value that change from
a date to a string.
* We are adding some `yaml_column_permitted_classes` to our ActiveRecord
config. This is required to run the db migration because of the
aforementioned bug in Psych.
* The migration is relatively inefficient. However, we tested it
against a clone of our prod db and found that it completed in 83.12s,
which we find acceptable.
jazairi added a commit to MITLibraries/thing that referenced this issue Apr 18, 2023
Why these changes are being introduced:

The versions table, used by paper_trail, has two columns that use
YAML. PaperTrail prefers that these tables use JSON or JSONB.
In order to update paper_trail to the latest version, we need
either to update our config to use YAML.safe_load or migrate
the data to JSON.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-626
* https://mitlibraries.atlassian.net/browse/ETD-615

How this addresses that need:

This converts the columns to JSON as described here:
https://github.com/paper-trail-gem/paper_trail/blob/v14.0.0/README.md#6b-custom-serializer

Our migration uses `PaperTrail::Serializers::YAML.load`. This is
effectively the same as `YAML.safe_load`. `YAML.load`,
awhich is uggested in the readme, will no longer work due to a
long-running bug in Psych:
collectiveidea/audited#631

We are using JSON instead of JSONB for two reasons:

1. SQLite does not support JSONB, so our local environments will
differ from Heroku; and
2. We are not actively leveraging JSONB's performance advantages in
the codebase, so there is likely no functional difference.

Long-term, it's worth considering whether we should use postgres
locally, as this would support better dev/prod parity and allow us
to use psql features like JSONB with more confidence. We discussed
this briefly in a team meeting on 3/7/23 and decided that such a
change could bring value, but is not necessary at this time.

Side effects of this change:

* One test has been updated to accommodate a value that change from
a date to a string.
* We are adding some `yaml_column_permitted_classes` to our ActiveRecord
config. This is required to run the db migration because of the
aforementioned bug in Psych. I've added a comment to remove it once
this commit is deployed.
* The migration is relatively inefficient. However, we tested it
against a clone of our prod db and found that it completed in 83.12s,
which we find acceptable.
* Running db migrations sometimes incorrectly parses the
`null: false` param into its own column. This bug occurs despite
PaperTrail's claiming to have fixed it, and it's inconsistent
(it occurs when Adam runs the migrations, but not when Jeremy does).
There's not much we can do about this except to keep an eye on it
in the future.

co-authored-by: Jeremy Prevost <jprevost@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants