Skip to content
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

Add forward slash as escape character #235

Closed
wants to merge 2 commits into from
Closed

Conversation

EiNSTeiN-
Copy link

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

@EiNSTeiN- EiNSTeiN- force-pushed the master branch 4 times, most recently from 1009911 to a41ce74 Compare February 9, 2015 23:49
@fw42
Copy link

fw42 commented Feb 10, 2015

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

@nobu
Copy link
Member

nobu commented Feb 11, 2015

yes, seems a security issue.

@EiNSTeiN-
Copy link
Author

@flori, for review please?

@EiNSTeiN-
Copy link
Author

@flori an answer would be really nice!

@flori
Copy link
Member

flori commented Apr 27, 2015

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-
Copy link
Author

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-
Copy link
Author

@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
Copy link

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

@tenderlove
Copy link
Member

Bump.

@casperisfine
Copy link

I rebased this and opened another PR: #405

@marcandre
Copy link
Member

Should be closed.

@EiNSTeiN- EiNSTeiN- closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants