document that `javascript_include_tag` will html escape sources. #8228

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@senny
Member
senny commented Nov 15, 2012

This is a follow up to my PR #7932. As @NZKoz pointed out, it is not a framework matter but left to the user to mark strings as html_safe when they contain '&' characters. I added the necessary documentation to javascript_include_tag to make the behavior clear.

Closes #7931

@senny
Member
senny commented Nov 15, 2012

I intentionally didn't use docrails because I wanted to get feedback on the documentation change.

@rafaelfranca what do you think? Also should such documentation patches be backported?

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Nov 15, 2012
actionpack/lib/action_view/helpers/asset_tag_helper.rb
@@ -48,6 +48,13 @@ module AssetTagHelper
# javascript_include_tag "http://www.example.com/xmlhr.js"
# # => <script src="http://www.example.com/xmlhr.js"></script>
#
+ # Note that sources you pass to <tt>javascript_include_tag</tt> will be html escaped.
+ # If you have a source, which contains special characters like '&' make sure to use
+ # <tt>sanitize</tt>
+ #
+ # javascript_include_tag "http://www.example.com/xmlhr.js?key=KEY&param=true"
@rafaelfranca
rafaelfranca Nov 15, 2012 Member

Don't we need to sanitize or mark as html safe to it work?

@senny
senny Nov 15, 2012 Member

I did try it with sanitize and it worked. I thought it would be the safer route to take. I could be wrong though.

@rafaelfranca
Member

Yes, documentation changes can be backported, but I not sure if this works with Rails 3.2.x

@rafaelfranca
Member

I think it is good. @NZKoz want to add anything?

@fxn
Member
fxn commented Nov 15, 2012

Don't quite understand the purpose of this patch.

You want the ampersands in the generated src attribute to be escaped using entities, that is, you want the HTML to look like this

<script src="http://www.example.com/xmlhr.js?key=KEY&amp;param=true"></script>

I don't know which would be a use case for a URL string that already has its entities escaped, since a URL as data looks normal, and escaping its entities is something you only do when enconding that string for HTML or XML. Having a URL with entities not escaped looks suspicious to me, it means someone didn't decode the entities in some format to yield the actual data string.

Not sure this patch should be applied, don't see a use case at first sight that justifies it, but please let me know if I am missing something.

(Editorial note: "HTML" is all uppercase.)

@NZKoz
Member
NZKoz commented Nov 15, 2012

Yeah, the escaping in that case is actually fine, it generally would only be broken if you've got something where the values are already escaped. e.g. you've pasted something from the internet which has the &amp;.

As for the example code, you don't sanitize, you call raw() or .html_safe.

@senny
Member
senny commented Nov 15, 2012

The work here was started because of the issue #7931. If that issue actually is no valid concern I don't see a reason to add anything to the documentation.

@NZKoz @fxn can you read through the issue and let me know if we should simply close it ar adjust the documentation.

@fxn
Member
fxn commented Nov 16, 2012

Closing here as agreed in #7932, thanks for the contribution anyway ❤️.

@fxn fxn closed this Nov 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment