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

Prohibit sneaky custom params from being drawn (Fix #30467) #35236

Merged
merged 1 commit into from Mar 27, 2019

Conversation

@schmijos
Copy link
Contributor

@schmijos schmijos commented Feb 12, 2019

Summary

This PR fixes #30467 by raising an ArgumentError if a resource custom param contains a colon (:).

After this change it's not possible anymore to configure routes like this:

routes.draw do
  resources :users, param: 'name/:sneaky'
end

See the regression test here: 8e3573a

@rails-bot rails-bot bot added the actionpack label Feb 12, 2019
@schmijos schmijos changed the title Prohibit sneaky custom params from being drawn Prohibit sneaky custom params from being drawn (Fix #30467) Feb 13, 2019
@schmijos schmijos force-pushed the fix-30467 branch 2 times, most recently from 09297ed to 9e9b978 Feb 13, 2019
Copy link
Member

@jeremy jeremy left a comment

Thanks @schmijos! Could you squash to a single atomic commit as well?

actionpack/CHANGELOG.md Outdated Show resolved Hide resolved
actionpack/test/dispatch/routing_test.rb Outdated Show resolved Hide resolved
After this change it's not possible anymore to configure routes
like this:

    routes.draw do
      resources :users, param: "name/:sneaky"
    end

Fixes rails#30467.
@schmijos
Copy link
Contributor Author

@schmijos schmijos commented Mar 26, 2019

👋 @jeremy, thank you very much for the review. I rephrased and squashed.

@rafaelfranca rafaelfranca merged commit 93dbbe3 into rails:master Mar 27, 2019
3 checks passed
@schmijos schmijos deleted the fix-30467 branch Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants