This project is archived and is in readonly mode.
Deprecate 'escape' parameter/option in three helper functions
Reported by Brian Morearty | February 12th, 2011 @ 05:29 PM | in 3.1
From this Rails Core comment by Santiago: https://mail.google.com/a/morearty.org/#inbox/12e0c4594370424a
Now that we have the html_safe function, let's depcrecate the
'escape' option (which is sometimes an option, sometimes a
parameter) in many of the helper functions in 3-0-stable. The
default in most cases should now be to escape the text by calling
html_safe in the helper. Let people call html_safe before calling
the helper if they want to avoid escaping.
These are the helpers that have either an escape
parameter or :escape option that we can deprecate:
FormTagHelper#text_area_tag. Remove the :escape option. Santiago suggested removing this one.TagHelper#content_tag. Remove the escape param, which currently doesn't work. See below for details.TagHelper#tag. Remove escape param, which currently defaults to true.
Santiago asked me to go ahead and make a patch for this
deprecation, which I will do.
Here's why I said the escape param for content_tag
currently doesn't work:
In a 3.0.1 Rails project the output is never escaped but html_safe? always returns true:
rails console
Loading development environment (Rails 3.0.1)
ruby-1.8.7-p330 :001 > helper.content_tag :div do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :002 > (helper.content_tag :div do '<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :003 > helper.content_tag :div,nil,nil,true do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :004 > (helper.content_tag :div,nil,nil,true do '<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :005 > helper.content_tag :div,nil,nil,false do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :006 > (helper.content_tag :div,nil,nil,false do '<b>hello</b>' end).html_safe?
=> true
In a Rails 3.0.2 project the content is always escaped and html_safe? always returns true:
rails console
Loading development environment (Rails 3.0.2)
ruby-1.8.7-p330 :001 > helper.content_tag :div do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :002 > (helper.content_tag :div do '<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :003 > helper.content_tag :div,nil,nil,true do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :004 > (helper.content_tag :div,nil,nil,true do '<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :005 > helper.content_tag :div,nil,nil,false do '<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :006 > (helper.content_tag :div,nil,nil,false do '<b>hello</b>' end).html_safe?
=> true
So at least in Rails 3.0.2 the html_safe? function is reporting the truth. But content_tag escapes the output even when escape=false (see the last two lines).
This is the commit that changed the behavior. After this change, escape=true doesn't work but calling html_safe still does.
Comments and changes to this ticket
-
Santiago Pastorino February 12th, 2011 @ 06:49 PM
- State changed from “new” to “open”
- Assigned user set to “Santiago Pastorino”
- Importance changed from “” to “Low”
-
Brian Morearty February 13th, 2011 @ 12:37 AM
I'm writing a patch with deprecation notices for 3-0-stable. But I have a question: what branch do I use for the post-deprecation code? (The code that no longer supports the escape parameter at all, rather than just giving a deprecation warning.)
-
Santiago Pastorino February 13th, 2011 @ 04:51 AM
- Milestone set to “3.1”
3-0-stable for the one that deprecates it and master for the one that removes it.
-
Brian Morearty February 13th, 2011 @ 10:11 PM
Cool, so this patch is done, tested, and ready for you to apply. The 3 helper functions mentioned above (
FormTagHelper#text_area_tag,TagHelper#content_tag, andTagHelper#tag) will have the following behavior after these patches are applied:- In 3.0.5, using the 'escape' parameter or option will show a deprecation message.
- In 3.1 the 'escape' parameter or option to these 3 methods is
removed. The correct way to specify escaping behavior is to call
(or not call)
html_safeon the parameters. Both the rdoc and the tests are updated.
The attached zipfile contains two patches:
- 3-0-stable-deprecate-escape.diff is a patch for the 3-0-stable branch
- master-remove-escape.diff is a patch for the master branch
I put a full day's work in to this and was careful. Please let me know if anything looks amiss.
-
Santiago Pastorino February 17th, 2011 @ 06:57 PM
Hey Brian thanks for your work, can you please upload the patch files directly, don't wrap them in a zip is much easier for me to apply them and test.
-
-
-
Santiago Pastorino February 17th, 2011 @ 11:15 PM
Hey Brian, first of all thanks for your work ...
About the master patch ...
+ # tag("img", {:src => "open & shut.png".html_safe})I think is not a good example, mark as safe a thing that is going to be escaped otherwise.
- final_value = value.is_a?(Array) ? value.join(" ") : value - final_value = ERB::Util.html_escape(final_value) if escape + final_value = value.is_a?(Array) ? value.map{|v|ERB::Util.html_escape(v)}.join(" ") : ERB::Util.html_escape(value)I think is better if you just do the same as before but remove the if escape, seems more readable to me.
- unless options[:sanitize] == false + if options.fetch(:sanitize, true) link_text = sanitize(link_text) href = sanitize(href) + else + link_text = link_text.html_safe + href = href.html_safe end - content_tag(:a, link_text, link_attributes.merge('href' => href), !!options[:sanitize]) + punctuation.reverse.join('') + content_tag(:a, link_text, link_attributes.merge('href' => href)) + punctuation.reverse.join('')That's wrong we should avoid marking html_safe unsafe content, sanitize != escape
http://github.com/rails/rails/blob/master/actionpack/lib/action_vie...
-
Brian Morearty February 18th, 2011 @ 12:54 AM
Thanks for taking a detailed look at it, Santiago.
+ # tag("img", {:src => "open & shut.png".html_safe})You make a good point. I'll change it to something better, like a string with an HTML tag in it.
- final_value = value.is_a?(Array) ? value.join(" ") : value - final_value = ERB::Util.html_escape(final_value) if escape + final_value = value.is_a?(Array) ? value.map{|v|ERB::Util.html_escape(v)}.join(" ") : ERB::Util.html_escape(value)At first I did it the way you suggested--just leave the old code in place but always call html_escape. It caused problems because (I think) joining an array of strings, some of which are unsafe, results in a string that's unsafe. Then we escape it so all the content from all the array elements gets escaped.
So if there's an array and you want one of the elements to be considered safe you have to mark them all safe. If I remember correctly. This approach was a fix for that problem.
- unless options[:sanitize] == false + if options.fetch(:sanitize, true) link_text = sanitize(link_text) href = sanitize(href) + else + link_text = link_text.html_safe + href = href.html_safe end - content_tag(:a, link_text, link_attributes.merge('href' => href), !!options[:sanitize]) + punctuation.reverse.join('') + content_tag(:a, link_text, link_attributes.merge('href' => href)) + punctuation.reverse.join('')Yeah, this is a tricky case. I knew that sanitize != escape but the old code was passing !!options[:sanitize] to content_tag's "escape" parameter, so the effect was:
1. If sanitize was true we also escaped it after sanitizing. Now the sanitize function calls html_safe at the end (not as part of my change), so this old behavior is probably broken.
2. If sanitize was false we didn't escape it. My call to html_safe in the 'else' clause keeps this behavior. But I think you're right, I shouldn't call html_safe here. I should let the content be output as an unsafe string.I'll revisit this code and let you know what I find.
-
Brian Morearty February 18th, 2011 @ 05:25 PM
Hi Santiago,
Thanks again for the help! Here is a new patch for the master branch. This is what I did:
Re: your first point, you are right--the example wasn't good. I have fixed the
tagexample to something that makes more sense. The only case I could think of where you would ever put a<or>in an attribute was JavaScript so I did that.Re: your second point, for the correct behavior I think it needs to be left the way it was in my patch. Joining a mix of html-safe and unsafe strings results in a single unsafe string, and escaping that would result in escaping some parts that were already safe. This irb session illustrates the difference:
# This is what my change does ruby-1.8.7-p330 :011 > [ERB::Util.html_escape('a<b'),ERB::Util.html_escape('b<c'.html_safe)].join(" ") => "a<b b<c" # This is what your proposed change does--it escapes b<c even though it's safe ruby-1.8.7-p330 :012 > ERB::Util.html_escape(['a<b','b<c'.html_safe].join(" ")) => "a<b b<c"Re: your third point, I now remember why I added the html_safe calls. I tried reverting them them but the pre-existing
test_auto_link_should_not_sanitize_input_when_sanitize_option_is_falsetest failed because all the ampersands are turned into ampersand-amp;. Theauto_linkhelper has never escaped ampersands. This makes sense because when you use auto_link the input is an HTML string already, so any escaping that the caller wants is up to them. You said "we should avoid marking html_safe unsafe content" but the regex that recognizes a URL stops when it reaches a less-than or greater-than character, so the URL is always safe. My addition of html_safe is required to keep the behavior consistent because theescapeparameter was removed from the content_tag call on the next line (since it's no longer supported).In a side note, it turns out the
sanitizeparameter toauto_linkis most likely irrelevant because the regexes used for both email and url recognition stop when they reach the first angle bracket. So there will never be anything to sanitize in a url or email. But that's a discussion for another day.So to sum it up, in the end the only change from the first patch was the example for
tag. -
Brian Morearty February 23rd, 2011 @ 09:07 PM
Hi,
I was wondering if there's anything I can do to help push this along. I saw that a 3.0.5rc1 was pushed out today, and the original plan was to put the 'escape' deprecations in 3.0.5.
Would it help if I add some more tests to "document" why I made the changes we discussed in the last couple of posts above?
Or would it help if someone else put another pair of eyes on the patch so they could weigh in on the right approach?
I'm willing to do more work to get this patch in. It would be a shame to let it die. Thanks.
-
Santiago Pastorino February 24th, 2011 @ 12:23 AM
Hey Brian, I'm really sorry for the delay, but I'm running out of time.
It takes a lot of time to review patches :( at least for me.Anyways the idea of this patch is to go for 3.1 and to one 3.0.x but we are late to put it on 3.0.5
I will take a look ASAP.
Thanks. -
Brian Morearty February 24th, 2011 @ 12:40 AM
Hey, Santiago, no problem. I don't even know how you find time to do everything you do. I know reviewing patches is time-consuming. :-)
It's not a big deal if it's too late for the deprecations to make it into 3.0.5. There's always another version.
-
Ed Ruder March 20th, 2011 @ 02:34 AM
+1 This would be a nice change to have, to finalize the migration to html_safe in Rails
-
tieunguyet March 22nd, 2011 @ 08:50 AM
Today the company to build a website for a job so important that can not simply say yes or no to it. Website has become the media and the leading business tool for every business, it brings an undeniable advantage. Here are the reasons to start building a website: Website
* Establish business presence on the internet, the opportunity to interact with customers anywhere and at any time. ホームページ製作 * Introduction of products and services in a lively and interactive. ホームページ制作 * Create opportunities to sell goods in a professional manner without costly. ホームページ作成 * The opportunity to serve customers better, achieve greater satisfaction from customers. ホームページ作成ソフト * Create a professional image in public, effective tool to implement marketing and PR campaigns. ホームページセミナー * The website is simply not a cause of failure of the business. CMS Your website is not only eye-catching, fast loading, meet customer needs, user-friendly but also friendly to Google, Yahoo, Bing (the popular search engines today) to website is always a high position on search engines this. Web promotion would be simpler if your website is programmed with the search engine-friendly from the beginning. With the development of talented engineers, professionals, OSS will design you a website with everything you need to exploit the strengths of the internet, making the reader interested in learning more about:
* The mission of your company. ビジネスブログ * Services of your company. * Help you identify the object. * Create the form and feel according to your specific needs. * Making site is always lively and eye-catching access. -
-
Brian Morearty March 29th, 2011 @ 05:56 AM
Hi again,
Sorry to bump this but here goes: is there anything I can do to help get this deprecation of the 'escape' parameter done and approved? It's so close. The patch is in. Santiago has already reviewed it once, gave three suggestions, I implemented one of his suggestions and tried my best to explain why I think the other two should remain as is.
Would it help if I contribute more tests? Is there someone else who could approve the change? Should I provide more explanation of the changes I made? Anything?
It would really be a shame if this work went to waste since (a) it fixes real problems (the escape parameter doesn't work anymore) and (b) there seems to be general agreement that it should be done and no one has expressed opposition.
I'll help any way I can.
Thanks.
Brian
-
Santiago Pastorino March 29th, 2011 @ 02:46 PM
Brian I need time to review a bit more and push I'm really really busy. Don't worry this will go in before 3.1 release.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
Brian Morearty
Chris Mear
Jeremy Kemper
Manfred Stienstra
Piotr Sarnacki