Rubocop: Security/YAMLLoad shouldn't auto correct

[NobodysNightmare]

Expected behavior

When auto correcting my code, rubocop only performs auto-corrections that can be expected to not break the semantics of my code.

Thus I expect no auto-correction from YAML.load to YAML.safe_load, since that will break yaml files using aliases, e.g.

default: &default
  foo: "bar"
baz:
  <<: *default

safe_load will raise Unknown alias: default (Psych::BadAlias) for this yaml.

Actual behavior

Rubocop performs an auto correction that will break for yaml files using aliases.

Mitigations

• do not auto-correct YAML.load to YAML.safe_load
• auto-correct YAML.load(x) to YAML.safe_load(x, [], [], true), which would allow using aliases

Note: Since safe_load also has other implications aside from aliases, my preference would really be not to auto-correct it at all.

Steps to reproduce the problem

For this file:

YAML.load(something)

run:

rubocop -a

RuboCop version

$ rubocop -V
0.47.1 (using Parser 2.3.3.1, running on ruby 2.3.3 x86_64-linux)

[jfelchner]

I've had issues with this as well. It's not an equivalent change, which I believe is what autocorrect should be reserved for.

[bbatsov]

This goes back to an old discussion about implementing safety levels for auto-corrections and some config to pick a preferred safety level when running auto-correct. For me that's the only good solution to this problem as some people are fine with potentially dangerous corrections, while some are not.

[mvz]

@bbatsov can you provide a pointer to that discussion? I'm interested in what the arguments for allowing dangerous corrections are.

[mikegee]

I think some folks autocorrect assuming safety, and other only autocorrect when they know what Rubocop will do. I'm in that second camp. I review the offenses and autocorrect one cop at a time. When I'm deciding which cops get to autocorrect and I trust my unit tests, I'm willing to let Rubocop make changes that might be considered "dangerous" by others.

[mvz]

Thanks for the explanation, @mikegee. I used to autocorrect one cop at a time but I don't have the patience anymore.

Is there any other autocorrection that changes semantics? This is the only one I've come across.

(If someone can point me to that earlier discussion, I'm happy to avoid rehashing old arguments 😄 )

[bbatsov]

See https://github.com/bbatsov/rubocop/pull/3398

[gayarra-mdsol]

+1 to the fact that this correction can break stuff.

It just broke my app. It was easy to find and revert, so no big deal. Just want to second the potential for this to cause problems.

[mvz]

As a follow-up to my comment above (https://github.com/bbatsov/rubocop/issues/3998#issuecomment-277501414), I have now automated applying autocorrect per cop and committing the result. This works great because it allows git bisect to find any corrections that broke things.

[Darhazer]

The auto-correction will be marked unsafe in #5978 so I'm closing this issue