Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActionDispatch::Routing::RouteSet#recognize_path (assert_recognize) leaks parameters #28398

Closed
libc opened this issue Mar 13, 2017 · 2 comments
Closed

Comments

@libc
Copy link
Contributor

libc commented Mar 13, 2017

ActionDispatch::Routing::RouteSet#recognize_path does not reset parameters between trying different constraints out. It only adds parameters. Normal matching doesn't exhibit the same behavior and there's a test to make sure of it

Steps to reproduce

Add the following code to actionpack/test/controller/routing_test.rb

  def test_recognize_path_does_not_leak_params
    rs.draw do
      get ':foo/bar' => 'bar#foo', constraints: lambda { |*| false }
      get 'foo/:bar' => 'foo#bar'
    end

    assert_equal({ :controller => "foo", :action => 'bar', :bar => 'bar' }, rs.recognize_path("/foo/bar"))
  end

As you can see the :foo parameter was leaked from the first route with non-matching constraint.

Expected behavior

No failure

Actual behavior

  1) Failure:
LegacyRouteSetTests#test_recognize_path_does_not_leak_params [test/controller/routing_test.rb:673]:
--- expected
+++ actual
@@ -1 +1 @@
-{:controller=>"foo", :action=>"bar", :bar=>"bar"}
+{:controller=>"foo", :action=>"bar", :foo=>"foo", :bar=>"bar"}

System configuration

Rails version: 5.0.2, master

Ruby version: 2.3.3

@maclover7
Copy link
Contributor

cc @pixeltrix

@pixeltrix pixeltrix self-assigned this Mar 15, 2017
pixeltrix added a commit that referenced this issue Mar 15, 2017
In 9b654d4 some params munging was added to ensure that they were
set whenever `recognize_path` would call either a proc or callable
constraint. Since we no longer mutate the environment hash within
the method it's now unnecessary and actually causes params to leak
between route matches before checking constraints.

Fixes #28398.

(cherry picked from commit 886085d)
@pixeltrix
Copy link
Contributor

Backported the fix to 5-0-stable in 090adf5

It appears that 4-2-stable isn't affected - probably due to duping of params somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants