Redirect routes escape path parameters wrongly #13110

Closed
tomhughes opened this Issue Nov 30, 2013 · 2 comments

Comments

Projects
None yet
3 participants

Using a redirect route like this:

get '/user/:display_name/edits', :to => redirect('/user/%{display_name}/history')

causes a path where :display_name contains a space to be redirected to a path where query string escaping has been applied and the space has been replaced with a plus sign.

This is wrong - spaces in the path should be % escaped and using a plus sign will cause the subsequent fetch to fail.

The problem is here:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/redirection.rb#L73

which is using Rack::Utils.escape and changing it to URI.escape works although I guess it really needs to figure out if the value is being substituted in the query string or the path.

@tomhughes tomhughes added a commit to tomhughes/openstreetmap-website that referenced this issue Nov 30, 2013

@tomhughes tomhughes Monkey patch escaping in redirect routes
The correct method of escaping depends on whether the parameter
is being substituted in the path or the query, but all our ones
are substitued in the path so use URI.escape instead of the
standard Rack::Utils.escape which does query escaping.

rails/rails#13110
309831a

tomhughes referenced this issue in openstreetmap/openstreetmap-website Nov 30, 2013

Closed

After the redesign, rails redirects to a broken URL. #552

Member

senny commented Dec 1, 2013

pixeltrix was assigned Dec 1, 2013

Owner

pixeltrix commented Dec 1, 2013

On reflection this is intentional - it's quite possible that the parameter may be used in a query parameter so it needs to be escaped for this situation. If you need the escape semantics for path then you can use the option redirect style, e.g:

get '/user/:display_name/edits', :to => redirect(path: '/user/%{display_name}/history')

This will handle + in a path segment correctly.

pixeltrix closed this Dec 1, 2013

@pixeltrix pixeltrix referenced this issue in openstreetmap/openstreetmap-website Dec 1, 2013

@tomhughes tomhughes Preserve the bounding box when redirecting history URLs
Using the options style for the redirect seems to cause parameters
to be preserved, which the path style of redirect does not.
61bb31e

pixeltrix reopened this Dec 2, 2013

pixeltrix closed this in d2e1caa Dec 2, 2013

@fluxusfrequency fluxusfrequency added a commit to fluxusfrequency/rails that referenced this issue Dec 4, 2013

@pixeltrix @fluxusfrequency pixeltrix + fluxusfrequency Try to escape each part of a path redirect route correctly
A path redirect may contain any and all parts of a url which have different
escaping rules for each part. This commit tries to escape each part correctly
by splitting the string into three chunks - path (which may also include a host),
query and fragment; then it applies the correct escape pattern to each part.

Whilst using `URI.parse` would be better, unfortunately the possible presence
of %{name} parameters in the path redirect string prevents us from using it so
we have to use a regular expression instead.

Fixes #13110.
6453f06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment