Clarify behavior of json_escape

[jenseng]

The behavior of json_escape was fixed in 2f1c578, but the doc
changes and example in that commit incorrectly indicated that the
return value would be html-safe. Since quotation marks are
preserved, the raw value is not safe to use in other contexts
(specifically HTML attributes).

[chancancode]

Given the intense use case, this seems to make sense, but I'll delegate to other s on the sec team to decide. 

cc @NZKoz also
—
Sent from Mailbox for iPhone

On Wed, Jan 8, 2014 at 8:48 PM, Jon Jensen notifications@github.com
wrote:

The escaping behavior of json_escape was fixed in #13073, but you still have to raw the return value in order to avoid the dreaded "s blowing up your javascript. That conflicts with an explicit doc change in the very same commit, and was most likely an oversight.
This commit essentially reverts 0b02284, and also always flags the result as html-safe. Per the docs, you should not put the raw result of json_escape in an HTML attribute, as double-quotes are left as-is and can break your layout.
/cc @chancancode @jeremy @tenderlove
You can merge this Pull Request by running:
git pull https://github.com/jenseng/rails json_escape
Or you can view, comment on it, or merge it online at:
#13646
-- Commit Summary --

• Make the result of json_escape always html-safe
  -- File Changes --
  M actionview/CHANGELOG.md (4)
  M actionview/test/template/erb_util_test.rb (7)
  M activesupport/lib/active_support/core_ext/string/output_safety.rb (3)
  -- Patch Links --
  https://github.com/rails/rails/pull/13646.patch
  https://github.com/rails/rails/pull/13646.diff

  Reply to this email directly or view it on GitHub:
  Clarify behavior of json_escape #13646

[chancancode]

intended*—
Sent from Mailbox for iPhone

On Wed, Jan 8, 2014 at 8:48 PM, Jon Jensen notifications@github.com
wrote:

The escaping behavior of json_escape was fixed in #13073, but you still have to raw the return value in order to avoid the dreaded "s blowing up your javascript. That conflicts with an explicit doc change in the very same commit, and was most likely an oversight.
This commit essentially reverts 0b02284, and also always flags the result as html-safe. Per the docs, you should not put the raw result of json_escape in an HTML attribute, as double-quotes are left as-is and can break your layout.
/cc @chancancode @jeremy @tenderlove
You can merge this Pull Request by running:
git pull https://github.com/jenseng/rails json_escape
Or you can view, comment on it, or merge it online at:
#13646
-- Commit Summary --

• Make the result of json_escape always html-safe
  -- File Changes --
  M actionview/CHANGELOG.md (4)
  M actionview/test/template/erb_util_test.rb (7)
  M activesupport/lib/active_support/core_ext/string/output_safety.rb (3)
  -- Patch Links --
  https://github.com/rails/rails/pull/13646.patch
  https://github.com/rails/rails/pull/13646.diff

  Reply to this email directly or view it on GitHub:
  Clarify behavior of json_escape #13646

[NZKoz]

I dunno, my first instinct is that if the value isn't actually safe for insertion in attributes, it should not be returned html safe. It's not a huge problem for users to call raw, but it is a huge problem if someone accidentally gets themselves pwnt.

[rafaelfranca]

I agree with @NZKoz. I'm seeing in the horizon a security report because of this change.

[chancancode]

👍 I can see the argument for leaning towards the safer side. Let's update the docs then? @jenseng can you do that?

[jenseng]

Sure thing, I'll amend the PR.

As an aside, it would be nice if there were a concept of the current html context in ERB (attr vs <script> vs other) and corresponding levels/types of html-safety, but I could see that being pretty gnarly to implement :-/

[jenseng]

On a related note, should we go ahead and make the return value never be html-safe? It seems weird to preserve the html-safety from the input string. Like, what exactly would an html-safe JSON input to this be? And if your JSON string is already legitimately html-safe, why would you pass it into json_escape?

I guess I'm having a hard time understanding the rationale behind this old commit.

It lets you do stuff like <%= json_escape(user.to_json.html_safe) %>, but that seems a little wonky.

[jenseng]

Do I need to worry about the build failures? Looks database/travis related, which seems unlikely to be due to this simple doc change.

[chancancode]

Thanks! For the future, please add [ci skip] to doc changes :)

[jenseng]

Awesome, will do. Thanks again!