Fix YAML.load to YAML.unsafe_load if possible with rails 6.1 tests

[atatb]

YAML.load uses safe mode by default from Psych version 4.0.0 (and plan to be used in ruby 3.1 ?), so that unsafe yaml descriptions such as alias and disallowed classes and so on are failed in loading.

In this PR, I fixed to use YAML.unsafe_load instead of YAML.load if unsafe_load is available.
(unsafe_load is implemented in Psych 3.3.2 and YAML.load is still unsafe then.)

For the testing, psych >= 4 needs to be added to the dependency explicitly (unless other gems depend on this specific version of psych) and I don't bother the existing tests, so I added a new rails 6.1 to the test matrix with psych >= 4 dependency.

ref:

• Use Psych.safe_load by default ruby/psych#487
• Fix ruby-master test suite (Psych 4.0.0) rails/rails#42257
• Fix a build error for Ruby 3.1.0-dev rubocop/rubocop#9806

[cjlarose]

@atatb I'm curious on your thoughts on instead forcing users to opt into unsafe YAML parsing.

At first glance, using :unsafe_load whenever it's available seems appropriate because we expect that the YAML configuration files passed into config to be trusted sources. However, my guess is that for most folks, they're not using any of the "unsafe" features for their settings: they're just using Symbols, Strings, numerics, booleans, arrays and hashes.

Did you run into this problem because you have a YAML file that you use with the config gem that legitimately does not parse when using YAML.safe_load? If so, I'd like to know more about your use case.

[atatb]

@cjlarose Thank you for your comment!

Did you run into this problem because you have a YAML file that you use with the config gem that legitimately does not parse when using YAML.safe_load? If so, I'd like to know more about your use case.

Exactly I came across this problem due to my unsafe yaml file using regex and anchors with aliases like the below for example.
(I'm surprised that this alias feature of YAML is unsafe though it is quite common...)

puma:
  threads_count_max: &threads_count_max 16
db:
  pool: *threads_count_max
cors:
  origin: !ruby/regexp /^(.*\\.|)example\\.com$/

setting_a: &boolean_items
  items
    - - 'ON'
      - 'true'
    - - 'OFF'
      - 'false'
setting_b: *boolean_items

Also unfortunately one of a gem (reet) used in my rails app started to depend on psych 4.0.1 after bundle update, so it results in failing to load my unsafe yaml file by this config gem and I couldn't even run my app.

I could solve this issue by adding old version's psych gem in my Gemfile explicitly but it should be just a workaround and the root cause to solve is to specify which YAML.load (safe or unsafe) to use in code, I guessed.

In case of config gem, because a psych which is generally used (like by ruby built-in) uses unsafe mode for YAML.load method by default and most people who use this config gem depend on it for now, so the YAML.load should be specified to YAML.unsafe_load I thought. Otherwise all those who use unsafe features of YAML gets in trouble with loading their yaml config file and even can't run the app like me.

That's my main motivation to make this PR as well but I still have some considerations so let me write them down in the list below.

• YAML.safe_load can pass an option such as aliases and permitted_classes to enable each unsafe features but it looks complicated to consider psych versions because arguments format is changed by versions.
• For those who case about "unsafe", I can add a new configuration option like config.yaml_safe_load to config.rb, which forces users to use YAML.safe_load (no additional options like aliases and permitted_classes).

[cjlarose]

Exactly I came across this problem due to my unsafe yaml file using regex and anchors with aliases

Awesome. Thanks so much for providing the context!

• YAML.safe_load can pass an option such as aliases and permitted_classes to enable each unsafe features but it looks complicated to consider psych versions because arguments format is changed by versions.

This makes sense. I guess if people really need to use YAML.safe_load options, they can just parse the YAML themselves and pass the resulting Hash into Settings.add_source! since that supports adding a Hash directly.

• For those who case about "unsafe", I can add a new configuration option like config.yaml_safe_load to config.rb, which forces users to use YAML.safe_load (no additional options like aliases and permitted_classes).

I actually think the PR as written does the right thing for now: we're just preserving the existing unsafe-by-default parsing, regardless of which version of psych the user has. Longer term, I think using safe-by-default parsing is probably better, but that would mean a breaking change and a major version bump. Of course, if we did that, we could introduce a config.allow_unsafe_yaml_features for people that need it. For now though, this change will be the least surprising for people upgrading to newer psych versions.

[atatb]

@cjlarose Thanks!

[cjlarose]

Published in version 3.1.1