Skip to content

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented May 19, 2021

Ruby master ships with Psych 4.0.0 which makes YAML.load default to safe mode (ruby/psych#487)

So a bunch of places where we parse YAML configuration and schema caches are broken.

Ultimately I think we can consider that configuration files are trustworthy, so we can parse them with unsafe_load.

A slightly trickier case is YAML serialized payloads in database columns. It could be a good idea to be stricter here, but for backward compatibility reasons there no alternatives to using unsafe_load.

@casperisfine
Copy link
Contributor Author

Ok, master CI is now fully green.

@byroot byroot requested a review from tenderlove May 19, 2021 11:43
Ruby master ships with Psych 4.0.0 which makes `YAML.load`
defaults to safe mode (ruby/psych#487).

However since these YAML files are trustworthy sources
we can parse them with `unsafe_load`.
@casperisfine
Copy link
Contributor Author

I added CHANGELOG entries for Active Record and Railties as the two main documented APIs that are impacted are YAMLColumn and Rails.application.config_for.

I'll also backport this to the 6-1-stable branch.

@marcelolx
Copy link
Contributor

@casperisfine Any chance this to be backported to 6-0-stable?

@byroot
Copy link
Member

byroot commented Aug 2, 2021

Sure.

@byroot
Copy link
Member

byroot commented Aug 2, 2021

Hum, actually did you try? As far as I can tell it was backported and released as part of 6.1.4 https://github.com/rails/rails/blob/6-1-stable/activerecord/CHANGELOG.md

@marcelolx
Copy link
Contributor

marcelolx commented Aug 2, 2021

Yeah, it was backported to 6-1-stable branch but I mean 6-0-stable, we're running on 6.0.4 and sadly stuck in some dependencies that don't allow us to upgrade to 6.1.

We can fix this in our side by adding explicit gem "psych", "~> 3.0" if it isn't worth backporting to 6-0-stable branch.

@byroot
Copy link
Member

byroot commented Aug 2, 2021

Ah sorry 🤦 I misread 6-0-stable as 6-1-stable, my bad. Let me ask around because I think 6.0 only receive security fixes now, so I don't think I'm supposed to do that.

@marcelolx
Copy link
Contributor

marcelolx commented Aug 2, 2021

@byroot Yeah, I was looking for that info too and 6.0.x only receives security fixes https://guides.rubyonrails.org/maintenance_policy.html#bug-fixes.

We'll handle it on our side, thanks for the quick response!

anarcat added a commit to anarcat/pwstore that referenced this pull request Jan 25, 2023
Since I am not sure when, pwstore just completely fails to start in
Debian bookworm:

    anarcat@curie:tor-passwords$ ~/src/pwstore/pws update-keyring
    Unknown alias: weasel

It turns out this is a failure of the YAML module to load our
configuration file because of aliases:

    irb(main):004:0> YAML::load_file("/home/anarcat/.pws.yaml")
    /usr/lib/ruby/3.1.0/psych/visitors/to_ruby.rb:430:in `visit_Psych_Nodes_Alias': Unknown alias: weasel (Psych::BadAlias)

The fix seems to be to enable aliases, which is what this project has
done:

sidekiq/sidekiq#5140

Rails seems to have switched to unsafe load, which seems like a bad
idea:

rails/rails#42257
anarcat added a commit to anarcat/pwstore that referenced this pull request Jan 25, 2023
Since I am not sure when, pwstore just completely fails to start in
Debian bookworm:

    anarcat@curie:tor-passwords$ ~/src/pwstore/pws update-keyring
    Unknown alias: weasel

It turns out this is a failure of the YAML module to load our
configuration file because of aliases:

    irb(main):004:0> YAML::load_file("/home/anarcat/.pws.yaml")
    /usr/lib/ruby/3.1.0/psych/visitors/to_ruby.rb:430:in `visit_Psych_Nodes_Alias': Unknown alias: weasel (Psych::BadAlias)

The fix seems to be to enable aliases, which is what this project has
done:

sidekiq/sidekiq#5140

Rails seems to have switched to unsafe load, which seems like a bad
idea:

rails/rails#42257
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.

5 participants