New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a more useful definition of json_escape #6094

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@jfirebaugh
Contributor

jfirebaugh commented Apr 30, 2012

The existing definition removes double quote characters, and hence returns invalid JSON, making it unsuitable for the most common use case: bootstrapping JSON within a <script> element.

I am unaware of any use cases satisfied by the current behavior, which was previously discussed on lighthouse without coming to a satisfactory resolution. The original commit at 0ff7a2d does not indicate that the double quote behavior was intentional. It seems likely that it was simply an oversight after copy and pasting the definition of html_escape.

Since Rails does not make it easy to correctly escape bootstrapped JSON, incorrect and insecure methods are widespread and incorrectly recommended: 1, 2, 3. This change, together with community education, would alleviate the situation.

It's worth discussion whether json_escape should always return HTML-safe strings, such that it can be used without explicitly calling html_safe:

<script>
  var data = <%=j @data.to_json %>;
</script>

Also related is the discussion of the other j helper at #3578.

Provide a more useful definition of json_escape
The previous definition removed double quote characters,
and hence returned invalid JSON, making it unsuitable
for the most common use case: bootstrapping JSON in
a <script> element.

The original definition was at 0ff7a2d,
without indication that the double quote behavior was intentional.
It seems likely that it was simply an oversight after
copy and pasting the definition of html_escape.

Furthermore, since json_escape does not return a HTML-safe
string if not passed one, it is unnecessary for it to escape
characters other than the slash.
@colszowka

This comment has been minimized.

Show comment
Hide comment
@colszowka

colszowka commented May 9, 2012

+1

@rapind

This comment has been minimized.

Show comment
Hide comment
@rapind

rapind commented Jun 21, 2012

+1

@anbotero

This comment has been minimized.

Show comment
Hide comment
@anbotero

anbotero Aug 8, 2012

Contributor

+1

Contributor

anbotero commented Aug 8, 2012

+1

@ay

This comment has been minimized.

Show comment
Hide comment
@ay

ay Sep 2, 2012

Contributor

+1

Contributor

ay commented Sep 2, 2012

+1

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Sep 8, 2012

So, what's the state with this? The current escape_json is one freaky bastard.

moll commented Sep 8, 2012

So, what's the state with this? The current escape_json is one freaky bastard.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 8, 2012

Member

The current state is 'nobody from core has reviewed it.'

Member

steveklabnik commented Sep 8, 2012

The current state is 'nobody from core has reviewed it.'

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 17, 2012

Member

This needs a rebase.

Member

steveklabnik commented Nov 17, 2012

This needs a rebase.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 26, 2013

Contributor

@jfirebaugh are you able to rebase this so that it can be reviewed? Looks like a useful change 👍

Contributor

parndt commented Feb 26, 2013

@jfirebaugh are you able to rebase this so that it can be reviewed? Looks like a useful change 👍

@conzett

This comment has been minimized.

Show comment
Hide comment
@conzett

conzett commented Mar 13, 2013

+1

@nertzy

This comment has been minimized.

Show comment
Hide comment
@nertzy

nertzy Apr 6, 2013

Contributor

+1, I've often worried about </script> leaking in via user input. It's much better to know how to address this.

Contributor

nertzy commented Apr 6, 2013

+1, I've often worried about </script> leaking in via user input. It's much better to know how to address this.

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Apr 18, 2013

-1

the widely used $("#form").html("<%= j render(:partial => ....) %>") no longer works

lidaobing commented Apr 18, 2013

-1

the widely used $("#form").html("<%= j render(:partial => ....) %>") no longer works

@gabetax

This comment has been minimized.

Show comment
Hide comment
@gabetax

gabetax Apr 18, 2013

@lidaobing The already-merged #3578 removed the j alias to json_escape, and leaves it pointing to escape_javascript, which the docs say is for your use case. I don't think this change should affect your cited use case.

gabetax commented Apr 18, 2013

@lidaobing The already-merged #3578 removed the j alias to json_escape, and leaves it pointing to escape_javascript, which the docs say is for your use case. I don't think this change should affect your cited use case.

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing commented Apr 19, 2013

@gabetax got it, thanks

@Hebo

This comment has been minimized.

Show comment
Hide comment
@Hebo

Hebo Jun 4, 2013

+1, ran into this issue, and it would be great for Rails to have a way of handling this.

Hebo commented Jun 4, 2013

+1, ran into this issue, and it would be great for Rails to have a way of handling this.

@gerwitz

This comment has been minimized.

Show comment
Hide comment
@gerwitz

gerwitz commented Jun 16, 2013

+1

@MCodyB

This comment has been minimized.

Show comment
Hide comment
@MCodyB

MCodyB Feb 5, 2014

I'm not sure if this is the right place but I thought I should point out that the rubydocs don't reflect this change
JSON_ESCAPE_REGEXP = /[&"><]/

instead of the new regex

JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u

Similarly it's still
JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }

instead of the new

JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c', "\u2028" => '\u2028', "\u2029" => '\u2029' }

http://rubydoc.info/docs/rails/ERB/Util#json_escape-class_method
http://api.rubyonrails.org/classes/ERB/Util.html

MCodyB commented Feb 5, 2014

I'm not sure if this is the right place but I thought I should point out that the rubydocs don't reflect this change
JSON_ESCAPE_REGEXP = /[&"><]/

instead of the new regex

JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u

Similarly it's still
JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }

instead of the new

JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c', "\u2028" => '\u2028', "\u2029" => '\u2029' }

http://rubydoc.info/docs/rails/ERB/Util#json_escape-class_method
http://api.rubyonrails.org/classes/ERB/Util.html

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 5, 2014

Member

This is only fixed on master (and 4.1.beta1), you can see it on
http://edgeapi.rubyonrails.org/classes/ERB/Util.html

On Wed, Feb 5, 2014 at 11:56 AM, MCodyB notifications@github.com wrote:

I'm not sure if this is the right place but I thought I should point out
that the rubydocs don't reflect this change
JSON_ESCAPE_REGEXP = /[&"><]/

instead of the new regex

JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u

Similarly it's still
JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }

instead of the new

JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c',
"\u2028" => '\u2028', "\u2029" => '\u2029' }

http://rubydoc.info/docs/rails/ERB/Util#json_escape-class_method
http://api.rubyonrails.org/classes/ERB/Util.html


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/6094#issuecomment-34230671
.

Member

chancancode commented Feb 5, 2014

This is only fixed on master (and 4.1.beta1), you can see it on
http://edgeapi.rubyonrails.org/classes/ERB/Util.html

On Wed, Feb 5, 2014 at 11:56 AM, MCodyB notifications@github.com wrote:

I'm not sure if this is the right place but I thought I should point out
that the rubydocs don't reflect this change
JSON_ESCAPE_REGEXP = /[&"><]/

instead of the new regex

JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u

Similarly it's still
JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }

instead of the new

JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c',
"\u2028" => '\u2028', "\u2029" => '\u2029' }

http://rubydoc.info/docs/rails/ERB/Util#json_escape-class_method
http://api.rubyonrails.org/classes/ERB/Util.html


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/6094#issuecomment-34230671
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment