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
Comments
FYI got the same with
|
@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. |
I created a script that can help to figure what actually needs to be whitelisted. |
See this issue for some discussion and (personally untested) examples of migrating from YAML to JSON |
Pretty much same issue when upgrading to Rails 7.0.3.1:
|
@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 |
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. |
@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. |
@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: |
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 |
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. |
[deleted my issue] Operator error, carry on, nothing to see with my particular issue. |
lets move to json @crawler is this list all complete?
|
@laptopmutia for two my apps where i used this list. |
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.
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>
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:
It happens when using YAML to deserialize an audit record with a column of
ActiveSupport::TimeWithZone
type: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:
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?
The text was updated successfully, but these errors were encountered: