Bug in Rails Route Globbing

+1

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?

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.

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: 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...

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.

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.

(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.

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...)

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

Removing "patch" keyword.

Thanks.

Adding a releasenotes tag so we remember to mention it.

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

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

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

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.

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?

"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...

Ace - I didn't know about AllowEncodedSlashes.

Thanks for clearing this up!