Rails Ruby on Rails → 2.1.1

Comments and changes to this ticket

• You flagged this item as spam.
  

  Alex MacCaw May 8th, 2008 @ 07:29 PM

  +1

• 

  Michael Koziarski May 8th, 2008 @ 10:48 PM

  Here's the patch as a git patch.

  Can you provide a little more insight into why this works? It should help reviewers.

  Alex, what about the patch did you like?

  • route-globbing.diff 3.4 KB

• You flagged this item as spam.
  

  Alex MacCaw May 9th, 2008 @ 12:00 AM

  Well, I worked with Chris closely on this patch, and we've certainly found escaping of 'globbed' params an issue. I agree that the patch isn't immediately obvious, mostly because Rails routing is incredibly complicated and ugly (Incidentally Paul Battley has a very quick graph based version). Rails already escapes slashes in params for normal routes, so you'd expect it to do the same for globbed ones, which currently it doesn't. Basically this patch rectifies that by escaping PathSegment#hash (I presume PathSegment is always an array) which happens when the URL is being built.

• 

  Michael Koziarski May 9th, 2008 @ 03:00 AM

  • Milestone set to “2.1.1”
  • Assigned user set to “Michael Koziarski”

  There's certainly a lot to be improved about the current routing implementation, and it sounds like Paul Battley's change is interesting, got a url for it?

  But it would probably help matters if there was at least some comments about why removing the escaping code from interpolation_chunk and adding an extract_value method have the result of adding escaping to the dynamic segment ;)

  I certainly like the look of this in general, but want to see some slightly wider testing.

• You flagged this item as spam.
  

  Chris Roos May 9th, 2008 @ 09:30 AM

  Here's an attempt at an explanation...

  At a really basic level, the current implementation escapes too late. The array of *additional values is turned into a string joined by slashes (Array#to_param) before the escaping takes place. This is why the escaping in PathSegment#interpolation_chunk specifically avoids escaping the slashes (the RESERVED_PCHAR constant marks them as 'safe'). We need to escape the *additional values before joining them with slashes.

  In slightly more detail...

  Route#write_generation calls #generation_extraction and #generation_structure in order to compile the generate_raw method.

  Route#generation_extraction calls Segment#extraction_code for each segment. For DynamicSegments, including PathSegments, #extraction_code calls DynamicSegment#extract_value which simply checks for the existence of the key in the hash of params and calls #to_param on it. The effect of this is to turn our array of *additional values into a string joined by slashes. At this point, there's no way to tell the difference between a slash in one of our values and a slash that joins the values (so, ['foo/bar', 'baz'].to_param becomes 'foo/bar/baz').

  Route#generation_structure calls, eventually, Segment#interpolation_chunk. PathSegment#interpolation_chunk escapes all unsafe chars except the slash (it's added to Segment::RESERVED_PCHAR in PathSegment::RESERVED_PCHAR) which is why the slash in our 'foo/bar' value never gets escaped.

  The solution is to move the escaping up to the extract_value method, by overriding the DynamicSegment implementation in PathSegment. Then, because we're already all escaped up, we can just return the escaped value in the PathSegment#interpolation_chunk.

  Does that all make sense and sound about right?

  I'd be willing to add more tests if you could maybe suggest some that you'd like to see?

• 

  Michael Koziarski May 10th, 2008 @ 03:57 AM

  • State changed from “new” to “resolved”

  That is, perhaps, the most perfect explanation I've come across so far :). I'm bookmarking this one for referencing later when requesting clarification.

  Also, fixed in 6776edccf6fb553eb0ac6db55e1d30df1b5b6589

• You flagged this item as spam.
  

  floehopper May 17th, 2008 @ 09:07 PM

  Michael: Paul Battley has written about his alternative routing implementation on his blog...

  http://po-ru.com/diary/rerouting...

  http://po-ru.com/diary/rerouting...

  http://po-ru.com/diary/rerouting...

• 

  Michael Koziarski May 18th, 2008 @ 07:09 AM

  Ah yes, I remember that, sadly it doesn't look like anything came of it. Perhaps we can put some more effort, post 2.1, to seperate the mapping from the recognition / generation process to make this kind of thing more feasible.

• 

  Andrew White May 21st, 2008 @ 12:19 AM

  Unfortunately this patch now forces me to pass path segments as arrays to url helper functions, e.g:

  map.theme 'themes/:theme/:type/*file',

  :controller => 'theme',

  :action => 'show'

  >

  theme_path('default', 'stylesheets', 'shared/layout.css')

  => /themes/default/stylesheets/shared%2Flayout.css

  >

  theme_path('default', 'stylesheets', ['shared', 'layout.css'])

  => /themes/default/stylesheets/shared/layout.css

  Perhaps the patch can be amended so that if the parameter is a string then it doesn't escape forward slashes - I can do this if it is acceptable.

• 

  Andrew White May 21st, 2008 @ 12:26 AM

  (Second attempt without crazy blockquote formatting)

  Unfortunately this patch now forces me to pass path segments as arrays to url helper functions, e.g:

  map.theme 'themes/:theme/:type/*file', :controller => 'theme', :action => 'show'

  theme_path('default', 'stylesheets', 'shared/layout.css') => /themes/default/stylesheets/shared%2Flayout.css

  theme_path('default', 'stylesheets', ['shared', 'layout.css']) => /themes/default/stylesheets/shared/layout.css

  Perhaps the patch can be amended so that if the parameter is a string then it doesn't escape forward slashes - I can do this if it is acceptable.

• 

  Michael Koziarski May 21st, 2008 @ 12:39 AM

  • State changed from “resolved” to “open”

  I'm not sure what I think about that string special case, but at the very least we should document this new behaviour in the release notes.

  Can you see any downsides to the string special case Chris and Alex?

• You flagged this item as spam.
  

  Chris Roos May 21st, 2008 @ 09:33 AM

  Personally, I'd probably avoid adding a special case with the defence that the added complexity isn't warranted. In my opinion, supplying the path components as an array makes the code more readable (that is just an opinion though...)

• 

  Andrew White May 21st, 2008 @ 10:00 AM

  I'm not to fussed about it since I can work around it using the

  following helper:

  def theme_path(theme, type, filename)

  super(theme, type, filename.split('/'))

  end

  But I do agree that the change in behavior should be documented

• 

  Pratik May 21st, 2008 @ 10:56 AM

  • Title changed from “[PATCH] Bug in Rails Route Globbing” to “Bug in Rails Route Globbing”
• 

  Pratik May 21st, 2008 @ 01:54 PM

  Removing "patch" keyword.

  Thanks.

• 

  Michael Koziarski May 23rd, 2008 @ 01:04 AM

  Adding a releasenotes tag so we remember to mention it.

• 

  Michael Koziarski May 23rd, 2008 @ 01:06 AM

  We'll live with the new behaviour. If you did :some_non_globbed_params => 'foo/bar' it'd do the same thing.

• 

  DHH May 31st, 2008 @ 10:38 PM

  • State changed from “open” to “resolved”

• You flagged this item as spam.
  

  Francis Irving September 24th, 2008 @ 08:20 PM

  • Tag set to “actionpack, bug, edge, releasenotes, routing, tested”

  My application has stopped working because of this fix.

  I am using Apache, with proxy passes to Mongrel. When you have a URL with %2f in a path component, Apache immediately returns an error.

  See http://www.webmasterworld.com/ap...

  It turns out that Rails is now breaking RFC 2396 http://www.ietf.org/rfc/rfc2396.txt (see section 3.3). << Within a path segment, the characters "/", ";", "=", and "?" are reserved. >>

  I can see why this bug fix is tempting, and I'm not sure what to suggest instead, but it does break both Internet standards and a popular web server.

  Meanwhile I've worked round it by adding a url.sub!("%2F", "/") in my code.

• You flagged this item as spam.
  

  Francis Irving September 24th, 2008 @ 08:26 PM

  I mean url.gsub!("%2F", "/") for the workaround.

• 

  Michael Koziarski September 26th, 2008 @ 12:02 PM

  This seems to be fine with the RFC. We're escaping the / chars according to section 2?

• You flagged this item as spam.
  

  Francis Irving September 26th, 2008 @ 12:31 PM

  Alas it is more complex than that. See section 2.4.2. When to Escape and Unescape

  "each component may have its own set of characters that are reserved ... a URI must be separated into its components before the escaped characters within those components can be safely decoded."

  And section 3.3:

  "Within a path segment, the characters "/", ";", "=", and "?" are reserved."

  To give an example on the Ruby on Rails website itself. The URL http://www.rubyonrails.org/foo/bar gives a Ruby on Rails error message, whereas http://www.rubyonrails.org/foo%2... gives an Apache error.

• 

  Michael Koziarski September 26th, 2008 @ 12:40 PM

  Right, I'd expect different results because they're pointing at different resources.

  It's looking for the literal file 'foo/bar' in the root directory. The non escaped version is looking for the file bar within directory foo.

  Is this a bug in apache that prevents it from handling directories and files with escaped reserved characters?

• You flagged this item as spam.
  

  Chris Roos September 26th, 2008 @ 03:07 PM

  "It turns out that Rails is now breaking RFC 2396 http://www.ietf.org/rfc/rfc2396.txt (see section 3.3). << Within a path segment, the characters "/", ";", "=", and "?" are reserved. >>"

  I don't believe this patch is breaking RFC 2396. Those characters are reserved, meaning that you cannot expect to use them unencoded in a path segment. However, you can encode them and use them, so %2f instead of / is fine.

  I would suggest you enable the AllowEncodedSlashes[1] directive in Apache to solve your problem. From my limited understanding, I believe that Apache rejects any requests that contain an encoded slash in the path as a security precaution[2]. So, unless we consider Rails to be amongst the "CGI scripts which don't perform sanity checking on input variables" I think it's safe to enable.

  Incidentally, it seems that the reason the AllowEncodedSlashes directive was originally added was to make it harder for people to determine the web server[3]. From that article I'd guess that Apache is the only web server that treats encoded slashes in this special way (i.e. rejecting requests that include them outright).

  [1] http://httpd.apache.org/docs/2.2... [2] http://www.apacheweek.com/issues... [3] http://www.devshed.com/c/a/Apach...

• You flagged this item as spam.
  

  Francis Irving September 26th, 2008 @ 04:19 PM

  Ace - I didn't know about AllowEncodedSlashes.

  Thanks for clearing this up!