Rebase #6094 "Provide a more useful definition of json_escape" and update docs.

[Empact]

See #6094 for discussion.

[sorentwo]

👍 Thanks for rebasing this!

[jcaudle]

👍 on the rebase. @steveklabnik is this something that could work?

[steveklabnik]

I like to get a 👍 from @NZKoz before merging anything even vaguely security related, and I think this counts.

[NZKoz]

👎 we should do this differently.

You're changing the escaping methods here, and changing the tests, and changing the intention of what this method is used for. At the very least this would not be suitable for backporting to the 4.0 series and would have to live in master until 4.1 ships after a long painful beta series :)

The whole rationale for and method of using json_escape is kind of broken. When you're escaping values you need to work at the components, not the document as a whole. So with JSON this means escaping has to happen to the contents of the strings in JSON, not the document as a whole. It's the same reason you need to escape every fragment of html you generate and components of an SQL query, you can't just do:

  response.body = h(response.body)
  sql = connection.escape_some_how(entire_sql)

The reason it strips your " chars is that it can't possibly tell the difference between a " in user provided data and one in the json that you want to keep. Clearly the helper isn't fit for purpose, and can't be made so.

So going back to what people want to do, ideally it's this:

<script>
  var someThing = <%= @some_thing.to_json %>;
</script>

Which they can do if they remember to set escape_html_entities_in_json which is weirdly implemented and poorly documented. However they'll still have to call raw or html_safe to have it output correctly. There's really no reason for turning this option off, the strings are still perfectly valid JSON, it's just encoded differently.

So if we're going to go to the trouble of changing all this escaping, and we should, I'd suggest:

1. always escape the html entities in json, remove that option
2. make the encoded json be html_safe as we know the values are safe for inclusion in the body of the document at that point.

The only issue that would raise from my POV would be someone doing something crazy like:

<div id='something' json-in-attribute-for-some-reason="<%= @something.to_json %>">

I'm not sure if there'd be an attack vector there, it'd be worth investigating? Would that solve the issues people asked about in #6094 ?

[Empact]

@NZKoz, @steveklabnik for the record I'm 😐 about this PR. Just want to get the the PRs moving. I'll leave it to @jfirebaugh and #6094 to defend.

[Empact]

Closing due to my own indifference to pursuing this.