This project is archived and is in readonly mode.
Rails not handling links with & correctly
Reported by Anil Wadghule | May 10th, 2008 @ 02:24 PM | in 3.0.2
Certain browsers including Firefox does not convert links with & amp ; in it to &. That causes problems further in rails applications, where params gets created using parse_query_parameters method.
The query strings with & amp ; get parsed as key part e.g. query sting with & amp ;name=david would get converted to params as params[amp;name] instead of params[:name]. Ideally browser should convert & amp ; to & whenever it parses query strings. But that is not the case for current browsers.
This could be serious issue. The rails helper page.redirect_to outputs window.location links to have & amp ; in it. So page.redirect_to having additional params(other than controller, action, id) simply doesn't work.
I have attached related fix diff file.
Comments and changes to this ticket
-
Stephen Celis May 10th, 2008 @ 05:19 PM
- Title changed from “[PATCH] Rails not handling links with & correctly” to “Rails not handling links with & correctly”
Please use the "patch" tag now rather than preface with "[PATCH]"
-
-
-
Pratik May 11th, 2008 @ 08:40 PM
- Assigned user changed from “DHH” to “Pratik”
-
Daniel Morrison May 13th, 2008 @ 03:23 AM
-1
& shouldn't be in your URLs, and if it is, it should be there for a reason.
Rails is acting appropriately.
If there's a bug in page.redirect_to, it should be fixed there, not here.
-
Pratik May 13th, 2008 @ 10:46 AM
- State changed from “new” to “incomplete”
-
Anil Wadghule May 18th, 2008 @ 01:32 PM
okay, I will fix page.redirect_to. The url passed to it should not be html escaped as it is directly given to window.location, which is browser location bar.
-
Mike Breen March 26th, 2009 @ 01:49 PM
- Tag set to “actionpack, bug, edge, patch, tests”
Here's a patch that fixes this in page.redirect.
-
Pratik March 26th, 2009 @ 01:56 PM
- State changed from “incomplete” to “open”
-
joshsz April 24th, 2010 @ 05:33 PM
The patch works for me. I'm unsure of the utility of this though. I think you're probably doing something wrong if you have to write out urls with & amp ;, but I'd have to examine your specific case to make a real judgement.
-
Rizwan Reza May 15th, 2010 @ 03:38 PM
- Tag changed from “actionpack, bug, edge, patch, tests” to “actionpack, bug, bugmash, edge, patch, tests”
-
Rizwan Reza May 15th, 2010 @ 03:38 PM
- Milestone cleared.
+1 verified
I have attached a patch without trailing whitespace error. It preserves Mike's authorship.
-
Rizwan Reza May 15th, 2010 @ 03:39 PM
- no changes were found...
-
Rizwan Reza May 15th, 2010 @ 03:44 PM
- State changed from “open” to “invalid”
-
Xavier Noria May 15th, 2010 @ 03:52 PM
- State changed from “invalid” to “open”
Just a clarification.
& per se does not belong to URLs. You output & when the URL lives inside of an HTML page. Reason is attribute values have to be HTML-escaped as content is.
So a URL should be HTML-escaped if it goes into a href attribute, and should not if it goes in a HTTP header, belongs to the body of a text mail, or any other context.
You should escape also quotes and angles, except you won't see those in a URL, so in practice the simple gsub may do (not 100% sure, but I have no counterxample by now).
I think it would be better to include the ampersand in the regexp though, since the string "amp;" may appear by itself, as in http://www.example.com?foo=amp;bar=woo. And to add a test that asserts such a query string remains untouched.
-
Xavier Noria May 15th, 2010 @ 04:04 PM
Mike, who is passing a location argument that it is HTML-escaped? HTML-escaping should only happen in the view layer, it is suspicious that location is escaped at all. Better understand that before we consider this patch.
-
Santiago Pastorino May 15th, 2010 @ 05:32 PM
Yeah i was about to said the same Xavier said if the patch is going to be valid it's safer to do gsub('&', '&')
-
Santiago Pastorino May 15th, 2010 @ 05:35 PM
Sorry i didn't check the formatting. Hehe do this gsub('&' + 'amp;', '&') but without the +, all toghether
-
Anil Wadghule May 15th, 2010 @ 08:26 PM
I've attached corrected patch.
Basically it replaces "& amp ;"(without spaces) character with the & character. "& amp ;" is valid character entity reference for &. See 'Character Entity Reference' section in http://www.w3.org/TR/REC-html40/charset.html
-
Anil Wadghule May 15th, 2010 @ 09:58 PM
Attached is a app which demonstrates this issue.
See the pastie for the behavior before patch and after patch
To answer Xavier, I remember earlier Rails versions used to generate urls with & amp ; in it, which triggered this ticket. But looks like it is not the case now.
And when such HTML-escaped urls passed to page.redirect_to helper which basically has just "window.location.href = url". It would just pass wrong HTML-escaped link as it is.
That's why this patch looks necessary.
PS - Rails has html_escape defined in rails/activesupport/lib/active_support/core_ext/string/output_safety.rb
I think if we can have html_unescape, we can use it to HTML unescape string urls in redirect_to method.Thouhts?
-
José Valim May 15th, 2010 @ 10:29 PM
- State changed from “open” to “invalid”
Anil, thanks for the patch but the question is: how, in the first place, the url ended up with "&"? Rails urls are properly generated so I cannot see an use case for automatically removing &.
-
Rizwan Reza May 16th, 2010 @ 02:10 AM
- Tag changed from “actionpack, bug, bugmash, edge, patch, tests” to “actionpack, bug, edge, patch, tests”
-
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “Low”
-
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>
Anil Wadghule
blj
Daniel Morrison
DHH
Jeremy Kemper
Joe Goldwasser
José Valim
Mike Breen
Rizwan Reza
Stephen Celis
Xavier Noria