rails/rails — pull request #13073 by chancancode (2013-11-27 12:10:38+00:00)
Title: Fixed json_escape (again)

TL;DR
=====

The [original implementation](https://github.com/rails/rails/commit/0ff7a2d89fc95dcb0a32ed92aab7156b0778a7ea) of this helper had a bug that was never fixed for the past six years, let's fix it now.

Long Version
==========

So apparently we tried this multiple times before ([LH1485](https://rails.lighthouseapp.com/projects/8994/tickets/1485), #6904 and #11526). I'll try this one more time so we can fix this for real before Rails 4.1 ships :grin: (or remove it!)

As a reminder, this is what `json_escape` currently does:

1. Given a String `s`...
2. It replaces all occurrences of & to \u0026
3. It replaces all occurrences of > to \u003E
4. It replaces all occurrences of < to \u003C
5. The kicker: **it removes all occurrences of " (the double quotation mark)**

Operations 2-4 are just the standard HTML entities escaping performed by the [AS JSON encoder](https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb#L42-L44), which is harmless. Operation 5, however, destroys the JSON string completely.

I know @NZKoz said this is working as intended, but respectfully I think he might have misinterpreted the [original implementation](https://github.com/rails/rails/commit/0ff7a2d89fc95dcb0a32ed92aab7156b0778a7ea). It appears to me that @technoweenie probably just forgot to delete the `"` from the regex after copy and pasting it from `escape_html`.

We can also try to reason about this. There are a few scenarios that you would want a string "escaped" for "json" that could possibly warrant the creation of such helper.

1. To use it inside a JSON string value:

```
<%# users/show.json.erb -%>
{"id":user.id,"name":"<%= json_escape(user.name) %>"}
```

Because the original helper specifically escapes HTML entities, we can probably rule out this as the intended purpose. Escaping HTML entities only makes sense if the output of this helper goes into an HTML file.

2. To use it inside a JSON string value in a JSON string inside an HTML template:

```html
{"id":user.id,"name":"<%= json_escape(user.name) %>"}
```

This is also unlikely, because you wouldn't want to use "\uXXXX" here, because the browser would just display that. Instead, you can just use `html_escape` here.

3. To use it inside a JSON string value in a JSON string inside a `
```

This is getting closer, because we the "\uXXXX" sequences here will have identical meaning as their original values. However, this is still not quite right, because you'd have to do a lot more escaping than what this helper does (unicode control characters, slashes, etc), plus if you want to do that you would just replace " with \" like the JSON spec says, instead of removing them.

4. The string is already an encoded JSON string, and you want to use it inside an attribute:

```html

...

```

While this is a valid use case, if you wanted to do this, you can just call html_escape on it and everything would work fine, so nope.

5. The string is already an encoded JSON string, and you want to use it a JavaScript string:

```html

```

While this is a valid use case, if you wanted to do this, you would also have to escape single quotes, new lines etc (which is what the `escape_javascript` helper does, btw, although it doesn't exists back then), so nope.

6. The string is already an encoded JSON string, and you want to use it as a JavaScript variable:

```html

```

This is a very common use case, and everything else fits the bill perfectly, except the bug where it strips double quotation marks.

Since I cannot think of additional use cases where the current behaviour is useful, the copy-and-paste-error explanation + use case number 6 seems most likely.

## But we already have ActiveSupport.escape_html_entities_in_json, do we still need this?

Yes, because...

1. the JSON string might be encoded in a different library:

```html

```

2. we fight with other libraries over `to_json`, so in some cases calling `to_json` might go through a different encoder

3. if we don't have this, this is what most people would probably end up doing:

```html

```

...which would be bad if something sets `ActiveSupport.escape_html_entities_in_json` to false, or if something overrode `#to_json`, or if they used a third-party encoder and the author "forgot" to honor `ActiveSupport.escape_html_entities_in_json`.

Since the operation (if the bug is fixed) is idempotent, it seems safer that we suggest doing this all the time:

```html

```