This project is archived and is in readonly mode.

#4765 ✓resolved
Bruno Michel

Link_to should escape & in the query string

Reported by Bruno Michel | June 4th, 2010 @ 02:33 PM | in 3.0.2


the link_to helper should escape & in the query string to&. This is the behaviour described in the help of link_to:

link_to "Nonsense search", searches_path(:foo => "bar", :baz => "quux")
# => <a href="/searches?foo=bar&amp;baz=quux">Nonsense search</a>

But, it seems to be not the case. For example, one test case in actionpack/test/template/url_helper_test.rb (line 349 in master) asserts that:

assert_equal %{<a href="/?order=desc&page=2\">Showing</a>},
  link_to_unless_current("Showing", hash_for(:order => "desc", :page => 2))

If someone can confirm this bug, I'll make a patch with tests.

Comments and changes to this ticket

  • Bruno Michel

    Bruno Michel June 26th, 2010 @ 04:38 PM

    Here is the patch and its tests.

    By the way, I think that the behaviour of url_for for escaping is weird. It escapes strings by default, but not hashes, even if the documentation wasn't saying that. It seems that there is a purpose for that as there was a commit specially for that (1b3195b63ca44f0a70b61b75fcf4991cb2fbb944), so I do not have modified this. But I was tempted.

  • Bruno Michel

    Bruno Michel June 26th, 2010 @ 04:51 PM

    • Tag changed from escape, link_to, rails3 to escape, link_to, patch, rails3
  • chaitanyav

    chaitanyav June 27th, 2010 @ 05:01 AM

    I checked this and it does escape the ampersand

    =link_to "Clickme", url_for(:controller => "posts", :action => "index")
    =link_to "Clickme", url_for(:controller => "posts", :action => "index", :foo => "1", :bar => "2")
    =link_to "Clickme", url_for(:controller => "posts", :action => "new", :foo => "1", :bar => "2")

    <a href="/posts">Clickme</a> <a href="/posts?bar=2&amp;foo=1">Clickme</a> <a href="/posts/new?bar=2&amp;foo=1">Clickme</a>

    I am using Rails 3 beta 4.

  • chaitanyav

    chaitanyav June 27th, 2010 @ 05:27 AM

    It generated this url,

    =link_to_unless_current("Showing", {:order => "desc", :page => 2})
    <a href="/posts?order=desc&page=2">Showing</a>


    def hash_for(opts = {})

    {:controller =&gt; &quot;foo&quot;, :action =&gt; &quot;bar&quot;}.merge(opts)
    hash_for does not escape, its a method in the url_helper_test.rb, url_for does the escaping
  • chaitanyav

    chaitanyav June 27th, 2010 @ 05:36 AM

    def hash_for(opts = {})
      {:controller => "foo", :action => "bar"}.merge(opts)
  • Andrew White

    Andrew White June 27th, 2010 @ 09:36 AM

    • Assigned user set to “José Valim”
    • State changed from “new” to “open”
    • Milestone cleared.
    • Importance changed from “” to “High”

    I think that commit was to prevent double escaping of hash urls but that seems to have been corrected elsewhere. I think we need to restore the escaping by default of hash urls since that's what the HTML specs recommend.

    Attached patch includes a test to make sure hash urls are not double escaped.

  • Repository

    Repository June 28th, 2010 @ 02:37 PM

    • State changed from “open” to “resolved”

    (from [0b6ce3422370647cad3e91263a291f69b313d65b]) Restores the escaping of urls generated from hashes. [#4765 state:resolved]

    HTML specifications recommend the escaping of urls in web pages,
    which url_for does by default for string urls and consquently
    urls generated by path helpers as these return strings.

    Hashes passed to url_for are not escaped by default and this
    commit reverses this default so that they are escaped.

    Undoes the changes of this commit:

    Signed-off-by: José Valim

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2

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=""></a>

Referenced by