redirect() in routes.rb incorrectly behaves like a catch-all route #8018

Closed
lsimoneau opened this Issue Oct 24, 2012 · 8 comments

Projects

None yet
@lsimoneau

This similar to the issue described in #2430, but the solution described there is incomplete. Placing the redirect route later in the routemap only prevents the redirect from catching another route so long as that other route is specified correctly.

However, if for example a parameter is missing or the route doesn't exist, the routemap will swallow the routing error and simply use the redirect route with action and controller as parameters.

As an example, we have the following in our routes.rb:

match "/myresources" => redirect("/deals")

Now if you incorrectly specify another non-existent route like this:

url_for(:controller => "blargh", :action => "blargh")

Instead of getting a Routing Error, it returns:

"http://localhost/myresources?action=blargh&controller=blargh"

This is incorrect.

@sevenseacat
Contributor

Huh... I didn't believe this until I tried it out myself. Changing the redirect to a normal route matcher indeed removes the problem.

@guilleiguaran
Member
@tenderlove tenderlove was assigned Oct 27, 2012
@schneems
Member
schneems commented Dec 6, 2012

@pixeltrix @carlosantoniodasilva, this is related to #2430 but slightly different, can you take a look?

@NullVoxPopuli

This is what I'm experiencing:

in my routes:

match "sections/sort_sections", to: redirect("sections#sort")

in my controller

format.html { redirect_to(:controller => "skins",
    :action => 'for_account',
    :account => @skin.account_id) }

resulting url:

http://admin.local.vhost/api/v1/sections/sort_sections?account=1&action=for_account&controller=skins

testing the redirect:

  response.should redirect_to(admin_skins_for_account_url(@account.id))

test failure
Expected response to be a redirect to http://admin.app.vhost/skins/for_account/5 but was a redirect to http://admin.app.vhost/api/v1/sections/sort_sections?account=5&action=for_account&controller=admin%2Fskins.

result of rake routes | grep skins/for_account

                    admin_skins_for_account          /skins/for_account/:id(.:format)                            admin/skins#for_account

Note, that manually navigating to the /skins/for_account/:id url works just fine.

Using rails 2.3.18

@stereoscott
Contributor

Just hit this issue today using Rails 4. If I try to generate a url for an action that does not have a matching route, rails falls back on the first route in routes.rb that uses redirect(). Unfortunately I don't know enough about the internal routing code so I can't offer much help here, but I'm happy to test or help out in any way I can.

@pixeltrix pixeltrix added a commit that referenced this issue Jul 16, 2013
@pixeltrix pixeltrix Skip Rack applications and redirects when generating urls
When generating an unnamed url (i.e. using `url_for` with an options
hash) we should skip anything other than standard Rails routes otherwise
it will match the first mounted application or redirect and generate a
url with query parameters rather than raising an error if the options
hash doesn't match any defined routes.

Fixes #8018
(cherry picked from commit 1555a18)

Conflicts:
	actionpack/CHANGELOG.md
aba0a17
@pixeltrix pixeltrix added a commit that closed this issue Jul 16, 2013
@pixeltrix pixeltrix Skip Rack applications and redirects when generating urls
When generating an unnamed url (i.e. using `url_for` with an options
hash) we should skip anything other than standard Rails routes otherwise
it will match the first mounted application or redirect and generate a
url with query parameters rather than raising an error if the options
hash doesn't match any defined routes.

Fixes #8018
1555a18
@pixeltrix pixeltrix closed this in 1555a18 Jul 16, 2013
@arunagw
Member
arunagw commented Oct 23, 2013

@pixeltrix I found a test started failing after this change.

I am using a helper method to generate meta tags.

    raw [
      tag('meta', property: 'site:url', content: url_for(only_path: false)),
      tag('meta', property: 'site:title', content: page_title),
    ].join("\n")

I have a test for this to just test helper return values.

When I tried using rc3 I found that this url_for started giving error

ActionController::UrlGenerationError:
       No route matches {}
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_dispatch/journey/formatter.rb:39:in `generate'
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_dispatch/routing/route_set.rb:601:in `generate'
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_dispatch/routing/route_set.rb:631:in `generate'
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_dispatch/routing/route_set.rb:667:in `url_for'
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_dispatch/routing/url_for.rb:155:in `url_for'
     # /Users/arunagw/checkouts/rails/actionpack/lib/action_view/routing_url_for.rb:83:in `url_for'

But in Rails 4.0 it's passing.

I know that we need to pass controller and action to make this work which is true in development mode and it works fine.

Reporting this here as I found in helper test broke after this. I think I need to fix helper test only and pass params to this.

@pixeltrix pixeltrix was assigned Oct 23, 2013
@kuahyeow
Contributor

Just hit this issue in Rails 3.2.16. It looks like the above fix is only in master, and it will be hard to use redirect() or any rack mounts in routes.rb with this issue, unfortunately. :(

@korny korny referenced this issue in rails/journey Apr 21, 2015
Closed

redirect() incorrectly behaves like a catch-all route (backport) #73

0 of 1 task complete
@korny
Contributor
korny commented Apr 21, 2015

I made a backport for journey 1.0.4:

gem 'journey', '~> 1.0.4', :github => 'sofatutor/journey', :branch => 'v1.0.4-fix-8018' 
@crazymykl crazymykl added a commit to RBM-Technologies/journey that referenced this issue Mar 2, 2016
@crazymykl crazymykl Backport fix for rails/rails#8018 4b40844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment