Rails Ruby on Rails → 3.0.6

Comments and changes to this ticket

• 

  Martin Gamsjaeger (snusnu) June 3rd, 2010 @ 10:09 PM

  While this maybe isn't the exact issue Jan encountered, I noticed something similar, where #content_tag_as_string completely ignores the escape param on the content. This leads to errors with the engineyard/rails_metrics plugin for example, and basically, for every call to #content_tag where the 2nd param is given as a String.

  Patch can be found at: http://github.com/snusnu/rails/commit/7f1b5bc6452cd1ca65dae3283c4af...

  I'll keep on looking if there's already another ticket for that specific issue, or maybe it's even the same issue as this ticket describes anyway. If not, I'll probably open another ticket and link it back to this one too

• You flagged this item as spam.
  

  Jan Berkel June 3rd, 2010 @ 10:33 PM

  that issue is unrelated. the problem is in actionpack/lib/action_dispatch/routing/route_set.rb:

  
  # route_set.rb
  def url_for(options)
    ...
    rewritten_url << "##{Rack::Utils.escape(options[:anchor].to_param.to_s)}" if options[:anchor]
  end
  
  # actionpack/lib/action_view/helpers/url_helper.rb
  def url_for(option)
    ...
    escape  = options.key?(:escape) ? options.delete(:escape) : false
    super
  end
  

  So what happens is that while url_for in url_helper.rb accepts an escape option, it doesn't get passed through route_set.rb (the key is deleted).

  I could fix this but wanted to check first if it the desired behaviour, to me it looks like a bug.

• 

  Neeraj Singh June 4th, 2010 @ 01:04 AM

  The culprit is following code

  rewritten_url << "##{Rack::Utils.escape(options[:anchor].to_param.to_s)}" if options[:anchor]
  

  As you can see anchor tag is always escaped. And it does not depend on options that is passed to this method.

  http://github.com/rails/rails/blob/master/actionpack/lib/action_dis...

  I tried to look at HTTP RFC and could not find any reference that anchor tag should always be escaped. If no such reference is found then it should be considered a bug. The patch and test can be generated easily.

• You flagged this item as spam.
  

  Jan Berkel June 4th, 2010 @ 01:41 AM

  @neeraj: anchor tags are not even sent to the server, they are purely local. it still makes sense to escape the anchor tag to produce valid urls, but the content is more free form than for example the query string, so it should be possible to have that part not escaped automatically.

  my concrete use case is google analytics tracking. there's an option to append tracking parameters to the anchor instead of having them as query parameters.

  http://example.com?utm_campaign=bar
  http://example.com/#utm_campaign=bar
  

  However if the content of the anchor gets escaped the tracking stops working.

• 

  Neeraj Singh June 25th, 2010 @ 09:42 PM

  • Assigned user set to “José Valim”

  Assigning it to rails core team member to get a verdict on whether anchor value should be escaped or not?

• 

  José Valim June 25th, 2010 @ 11:22 PM

  :anchor (as everything else on url_for) should respect the :escape parameter. Patches please! :)

• 

  Andrew White June 26th, 2010 @ 02:02 AM

  • Milestone cleared.
  • State changed from “new” to “open”

  The :escape option is only for HTML entity escaping and not URL escaping which is what's happening here. However there is a bug in that the :anchor is being over-escaped. The attached patch fixes this issue and you won't need to pass :escape => false to get your Google Analytics use case working.

  • 0001-dont-escape-safe-chars-in-fragment.diff 2.2 KB
• 

  Repository June 26th, 2010 @ 02:06 AM

  • State changed from “open” to “resolved”

  (from [bba19603c27e0439eb22a9bce7e3adf6924b224b]) URL fragments should not have safe characters escaped. Ref: Appendix A, http://tools.ietf.org/rfc/rfc3986.txt

  [#4762 state:resolved]

  Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
  http://github.com/rails/rails/commit/bba19603c27e0439eb22a9bce7e3ad...

• You flagged this item as spam.
  

  Jan Berkel June 29th, 2010 @ 11:07 AM

  • Importance changed from “” to “High”

  thanks! i backported the fix to 2-3-stable. any chance this can be applied? we're not ready for 3.x yet.

  • 4762-2_3_stable_backport.diff 2.9 KB
• 

  Santiago Pastorino June 29th, 2010 @ 06:35 PM

  • Milestone set to “2.3.9”
  • State changed from “resolved” to “open”
• 

  Repository June 30th, 2010 @ 12:27 PM

  (from [f8f4872fccbc6ba2b4970e4e9eab9ce7f0d19986]) Backported patch from [#4762]

  URL fragments should not have safe characters escaped. Ref: Appendix A,
  http://tools.ietf.org/rfc/rfc3986.txt

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/f8f4872fccbc6ba2b4970e4e9eab9c...

• 

  José Valim June 30th, 2010 @ 12:27 PM

  • State changed from “open” to “committed”

• You flagged this item as spam.
  

  Shayan Guha November 4th, 2010 @ 09:55 PM

  From http://tools.ietf.org/html/rfc3986#section-3.5, "/" and "?" should also be considered "safe" characters for the anchor. The patch doesn't look like it handles those characters. Also it looks like '%' should not be allowed.

• You flagged this item as spam.
  

  Shayan Guha November 4th, 2010 @ 11:15 PM

  This fix also needs to be applied to the rewrite_url function on the 2.3 branch. The :anchor option is still being escaped using CGI.escape. https://github.com/rails/rails/blob/v2.3.10/actionpack/lib/action_c...

• You flagged this item as spam.
  

  iGEL February 16th, 2011 @ 04:17 PM

  I agree with Shayan: / and ? shouldn't be escaped. I would like to use URLs like www.server.com/controller/action#tab=controller/action, but Rails 3.0.3 escapes the /. Any chance to reopen this ticket?

• 

  Santiago Pastorino February 16th, 2011 @ 04:32 PM

  is this an issue in 3-0-stable and/or master?

• 

  Andrew White February 16th, 2011 @ 04:43 PM

  It's an issue in 2-3-stable, 3-0-stable and master. 3-0-stable and master use Rack::Mount::Utils.escape_uri which doesn't have '/' and '?' in the list since those need escaping for routes. 2-3-stable uses URI.escape directly which Rack::Mount::Util.escape_uri does as well, so we might as well use URI.escape directly in 3-0-stable and master.

• 

  Santiago Pastorino February 16th, 2011 @ 04:55 PM

  • State changed from “committed” to “open”
  • Milestone changed from “2.3.9” to “3.0.5”

• You flagged this item as spam.
  

  iGEL February 16th, 2011 @ 05:36 PM

  Thank you. It's an issue on 3-0-stable and master. I've modified the tests, so it fails, if / or ? are escaped, but I don't dare to write a patch.

  • dont_escape_safe_chars.diff 1.2 KB
  • dont_escape_safe_chars_3-0.diff 1.2 KB

• You flagged this item as spam.
  

  iGEL February 16th, 2011 @ 06:41 PM

  Well, I dared after all, and put Andrews suggestion into a patch. Hope, everything is alright the way I did it.

  • dont_escape_safe_chars_3-0.diff 2.7 KB
  • dont_escape_safe_chars.diff 2.7 KB
• 

  Andrew White February 16th, 2011 @ 09:32 PM

  You need to double check the unsafe char list you are escaping - the default for URI.escape is UNSAFE, which includes anything not in the UNRESERVED and RESERVED list. The spec says it should be PCHAR + '/?' - this means #,[ & ] should be escaped but the default URI.escape doesn't encode them.

• 

  Santiago Pastorino February 17th, 2011 @ 01:29 AM

  Hey Andrew, can I leave the ticket for you to push when you think the fix is ready?

• 

  Andrew White February 17th, 2011 @ 05:35 AM

  • Assigned user changed from “José Valim” to “Andrew White”

  Sure, I'll handle it.

• 

  Santiago Pastorino February 27th, 2011 @ 03:15 AM

  • Milestone changed from “3.0.5” to “3.0.6”

  [bulk edit]