Add forward slash as escape character

[EiNSTeiN-]

As per the spec, the forward slash should be escaped in json strings. Not escaping the forward slash causes a security issue when a json object is interpolated inside of a script tag.

For instance:

irb(main):013:0> include ActionView::Helpers::TagHelper
=> Object
irb(main):014:0> include ActionView::Helpers::JavaScriptHelper
=> Object
irb(main):015:0> puts javascript_tag 'var json='+JSON.generate({"key"=>"</script><script>whatever()//"})
<script>
//<![CDATA[
var json={"key":"</script><script>whatever()//"}
//]]>
</script>

In javascript </script> has precedence over everything else, so in this case the script tag is terminated and whatever() is executed even though it should be part of the string.

The correct way to defend against this security issue is to escape the forward slash, so that <\/script> does not terminate the original script tag and the json string stays a string.

@flori

[fw42]

@flori @nobu can you take a look please? this is a somewhat critical security issue

[nobu]

yes, seems a security issue.

[EiNSTeiN-]

@flori, for review please?

[EiNSTeiN-]

@flori an answer would be really nice!

[flori]

Well, according to the spec the forward slash may be escaped, not should be escaped. It's also not a security issue of JSON itself but of the browser interpreting Javascript in an inline script tag. You really have to try hard though and combine several bad practices: like not checking/sanitising user input and using inline script tags in the first place.

On the other hand always escaping the forward slash would make all JSON output containing URLs, filepathes, etc. to be unreadable even though JSON is often stored in flat files, databases, used in over the wire protocols and returned by web APIs that have nothing at all to do with inline script tags.

The question is, is that really worth it? Other JSON implementations usually don't offer the escaping or do it only on demand. On the ones I found that do it in general it is often reported as a bug. I would prefer it if you could make this configurable and not the default output, so people can use the escaping if they need to without making the JSON unreadable in every other application.

[EiNSTeiN-]

I would prefer it if you could make this configurable and not the default output, so people can use the escaping if they need to without making the JSON unreadable in every other application.

Sounds reasonable, I'll work on it.

Thanks for the feedback, much appreciated.

[EiNSTeiN-]

@flori

I have added a configuration option that is disabled by default but where users of this gem will be able to opt-in the new behavior of escaping slashes. Can you review/give feedback on this?

[rafaelfranca]

@flori are you ok with this PR? If so we will rebase it.

[tenderlove]

Bump.

[casperisfine]

I rebased this and opened another PR: #405

[marcandre]

Should be closed.