Security/JSONLoad autocorrect potential runtime error

[savef]

The autocorrect on Security/JSONLoad will replace JSON.load(request) with JSON.parse(request). One difference between these two methods is that JSON.load works with any object that responds to the #read method, such as an OpenURI HTTP request or a file handler. Here's an example:

request = open("http://localhost:3000/output.json")
result = JSON.load(request)

This will crash if replaced with JSON.parse.

---

The above is the exact issue I experienced, however I think there are other potential issues with this autocorrect. For example, consider the following difference between JSON.load and JSON.parse:

irb(main):001:0> JSON.load('false')
=> false
irb(main):002:0> JSON.parse('false')
JSON::ParserError: 784: unexpected token at 'false'
irb(main):003:0> JSON.parse('false', quirks_mode: true)
=> false

JSON.load allows primitives to be parsed, without requiring them to be in an actual JSON object. JSON.parse requires the :quirks_mode option to be true for this to work. So this type of use could also cause runtime errors in applications.

---

I think that the solution is to remove autocorrect for this cop.

---

RuboCop version

$ rubocop -V
0.43.0 (using Parser 2.3.1.3, running on ruby 2.3.1 x86_64-linux)

[bbatsov]

I think that the solution is to remove autocorrect for this cop.

Or just disable it by default. After all the auto-correct is not reliable, because the cop is not reliable. I'd also extend the cop's description a bit.

[savef]

Or just disable it by default. After all the auto-correct is not reliable, because the cop is not reliable.

I disagree. The cop's position that JSON.load(x) is potentially dangerous seems a fair one.

Forcing the author to change their code to either JSON.parse(stream.read) or JSON.parse(value, quirks_mode: true) when necessary both lead to more explicit code, so readability is improved in these edge case scenarios. And the improved security provided by JSON.parse that the cop originally intended to provide still applies when passing in a full JSON string.

[bbatsov]

Fair enough. Let's agree that the solution is to extend the cop's description and disable its auto-correct by default.

[savef]

disable its auto-correct by default

Just to clarify this, you're talking about removing the Security::JSONLoad#autocorrect method, right? If not I'm not sure I understand the distinction. I don't know what it means for the autocorrect to be disabled by default, without the whole cop being disabled. I don't know what it'd mean to be enabled by default either since the autocorrect code (AFAIK) always requires the --auto-correct switch.

I'll try to put in a PR for this within the next week though.

[bbatsov]

All cops have an autocorrect attribute that can be set to false. :-) This enables/disable autocorrection for the particular cop. Some people might be perfectly fine with the autocorrect shortcomings.

[savef]

Right, think I've got it, the AutoCorrect cop config. Thanks.