Fix issues loading serialized logs #4376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
waiting-for-dev
referenced
this pull request
May 17, 2022
The new Psych version required by Ruby 3.1 no longer allows the loading of arbitrary classes into YAML by default. We switch to `#safe_load` and enable the class we're currently using. See: ruby/psych@cb50aa8 ruby/psych@1764942 [skip ci]
waiting-for-dev
force-pushed
the
waiting-for-dev/psych_allowed
branch
from
9169af9 to
5d86c11
Compare
waiting-for-dev
changed the title
aldesantis
approved these changes
May 18, 2022
waiting-for-dev
added
the
release:major
|
We are putting it on hold, as we'll rescue from Psych exception about unknown classes and print a message redirecting to the new preference. |
waiting-for-dev
force-pushed
the
waiting-for-dev/psych_allowed
branch
from
5d86c11 to
f3a5a62
Compare
waiting-for-dev
removed
the
release:major
Done. @albertoalmagro, @aldesantis, do you want to take a final look? |
aldesantis
reviewed
May 19, 2022
There was a problem hiding this comment.
@waiting-for-dev the implementation looks good, but I've left a couple of suggestions. Let me know your thoughts!
`Spree::LogEntry` loads details from a YAML string that can potentially contain any serialized Ruby object if users have overridden some part of the codebase. New Psych (YAML parser) version packed with Ruby 3.1 requires that, by default, users are explicit about which type of classes are allowed to be loaded (see ruby/psych@cb50aa8 and ruby/psych@1764942). On 008168c, we updated our code to allow instances of `ActiveMerchant::Billing::Response`. However, as [noted in a comment](solidusio@008168c#r73483078), it seems we're missing, at least, instances of `ActiveSupport::TimeWithZone`. We need to add all classes used internally and leave a door open for users to add their own classes (via a new `log_entry_permitted_classes` preference).
waiting-for-dev
force-pushed
the
waiting-for-dev/psych_allowed
branch
from
f3a5a62 to
c8e274f
Compare
By default, `YAML.safe_load` won't allow YAML aliases to be parsed. However, we need them at least for older versions of serialized `ActiveSupport::TimeWithZone`. We add a configuration option so that users can still disable it. Depending on the source of the YAML, parsing aliases could open gates to entity expansion attacks (a DoS created by nesting self-references).
waiting-for-dev
force-pushed
the
waiting-for-dev/psych_allowed
branch
from
c8e274f to
256bf7c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spree::LogEntryloads details from a YAML string that can potentiallycontain any serialized Ruby object if users have overridden some part of
the codebase.
New Psych (YAML parser) version packed with Ruby 3.1 requires that, by
default, users are explicit about which type of classes are allowed to
be loaded (see ruby/psych@cb50aa8 and ruby/psych@1764942).
On 008168c, we updated our code to allow instances of
ActiveMerchant::Billing::Response. However, as noted in acomment,
it seems we're missing, at least, instances of
ActiveSupport::TimeWithZone.We need to add all classes used internally and leave a door open for
users to add their own classes (via a new
log_entry_permitted_classespreference).
On top of that, by default,
YAML.safe_loadwon't allow YAML aliases to be parsed.However, we need them at least for older versions of serialized
ActiveSupport::TimeWithZone.We add a configuration option so that users can still disable it.
Depending on the source of the YAML, parsing aliases could open gates to
entity expansion attacks (a DoS created by nesting self-references).
Checklist: