This project is archived and is in readonly mode.
Rails not handling links with & correctly
-
Stephen Celis- 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]"
-
bljI am pretty sure this is invalid.
-
PratikWhy can't the fix be in page.redirect_to ?
-
Pratik
- Assigned user changed from DHH to Pratik
-
Daniel Morrison-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
- State changed from new to incomplete
-
Anil Wadghuleokay, 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- Tag set to actionpack, bug, edge, patch, tests
Here's a patch that fixes this in page.redirect.
-
fix_amp_prob_in_redirect.diff
1.85 KB
-
Pratik
- State changed from incomplete to open
-
joshszThe 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- Tag changed from actionpack, bug, edge, patch, tests to actionpack, bug, bugmash, edge, patch, tests
-
Rizwan Reza
- Milestone cleared.
+1 verified
I have attached a patch without trailing whitespace error. It preserves Mike's authorship.
-
-
Rizwan Reza
- State changed from open to invalid
-
Xavier Noria- 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
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
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
Sorry i didn't check the formatting. Hehe do this gsub('&' + 'amp;', '&') but without the +, all toghether
-
Anil Wadghule
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
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?
-
fixamp.zip
100 KB
-
-
José Valim- 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
- Tag changed from actionpack, bug, bugmash, edge, patch, tests to actionpack, bug, edge, patch, tests
-
-
hgj
thanks you very much for your information