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

Deprecate :controller and :action path parameters #23980

Merged
merged 1 commit into from
Mar 1, 2016

Conversation

pixeltrix
Copy link
Contributor

Allowing :controller and :action values to be specified via the path in config/routes.rb has been an underlying cause of a number of issues in Rails that have resulted in security releases. In light of this it's
better that controllers and actions are explicitly whitelisted rather than trying to blacklist or sanitize 'bad' values.

WDYT? @dhh @rafaelfranca @tenderlove

Allowing :controller and :action values to be specified via the path
in config/routes.rb has been an underlying cause of a number of issues
in Rails that have resulted in security releases. In light of this it's
better that controllers and actions are explicitly whitelisted rather
than trying to blacklist or sanitize 'bad' values.
@dhh
Copy link
Member

dhh commented Mar 1, 2016

👍

1 similar comment
@fxn
Copy link
Member

fxn commented Mar 1, 2016

👍

@rafaelfranca
Copy link
Member

:shipit:

pixeltrix added a commit that referenced this pull request Mar 1, 2016
…ents

Deprecate :controller and :action path parameters
@pixeltrix pixeltrix merged commit f2c707a into master Mar 1, 2016
@pixeltrix pixeltrix deleted the deprecate-controller-action-segments branch March 1, 2016 17:31
jonatack added a commit to jonatack/rails that referenced this pull request Mar 3, 2016
Follow-up to rails#23980.

- Fix grammar: "be remove" -> "be removed".

- Wrap lines at 80 chars.

Lurvely ;-)
lucasmazza added a commit to heartcombo/devise that referenced this pull request Mar 7, 2016
This was deprecated on rails/rails#23980.

We now generate scope and provider specific routes, like `user_facebook_omniauth_callback`
or `user_github_omniauth_callback`.

We could deprecate the `omniauth_authorize_path` in favor of the generated routes, but
the `shared/links.html.erb` depends on it to generate all omniauth links at once.

Closes #3983.
y-yagi added a commit to y-yagi/rails that referenced this pull request Mar 13, 2016
jonatack added a commit to activerecord-hackery/ransack that referenced this pull request Mar 14, 2016
Using dynamic :controller or :action segments in routes has been a
source of a number of security issues in production and is deprecated
in Rails 5.0 since this [pull request] and is planned to be removed in
Rails 5.1.

Ransack is still maintaining compatibility with legacy Rails 3 and 4
versions, so this commit silences the deprecation messages for now
while running the test suite.

[pull request]: rails/rails#23980
yui-knk added a commit to yui-knk/rails that referenced this pull request Mar 30, 2016
"Using a dynamic :controller (or :action) segment in a route is deprecated"
by 6520ea5 (rails#23980).
@yui-knk yui-knk mentioned this pull request Mar 30, 2016
@bogdan
Copy link
Contributor

bogdan commented Mar 30, 2016

hallelujah!

y-yagi added a commit to y-yagi/rails that referenced this pull request May 21, 2016
prathamesh-sonpatki added a commit that referenced this pull request May 21, 2016
maclover7 pushed a commit to maclover7/rails that referenced this pull request Jun 6, 2016
maclover7 pushed a commit to maclover7/rails that referenced this pull request Jun 6, 2016
@elliotwesoff
Copy link

elliotwesoff commented Sep 6, 2016

Is there some other way to draw dynamic routes in Rails 5.1? I'm maintaining an enormous Rails app that was built on Rails 1 and almost every link_to uses this convention. Adding the standard resources definitions to the routes file for every controller and changing every link_to is simply out of the question. Are there any other options?

lizconlan added a commit to mysociety/alaveteli that referenced this pull request Mar 8, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Mar 8, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Mar 8, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Mar 8, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Mar 8, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Apr 2, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so I've added a new route for that here as well.
lizconlan added a commit to mysociety/alaveteli that referenced this pull request Apr 11, 2019
Passing in a dynamic action is deprecated in Rails 5.1 (for security
reasons) and will be removed in 5.2.:

rails/rails#23980

(We're seeing the deprecation notice in the latest build of 5.0.)

As we would like to keep this functionality, I've adopted a version of
the fix suggested - by the author of the code that introduced the
deprecation - in this issue thread in the Rails repo:

rails/rails#27231 (comment)

Essentially, this adds a filter to the allowable action name for safety
and uses :template as a proxy for :action. The main downside is that
themes that call `help_general_url` will need to pass in :template
instead of :action.

Also this caused (revealed?) a glitch which wasn't `/help/unhappy` route
- on its own without the optional `:url_title` part - wasn't being
recognised so that has also been updated to make :url_title optional
artur-intech pushed a commit to internetee/registry that referenced this pull request Oct 16, 2019
@jonathanhefner jonathanhefner mentioned this pull request Mar 17, 2020
1 task
@henrik
Copy link
Contributor

henrik commented Jul 2, 2020

Ran into this updating an old app. This was our workaround:

-        get "/:action", controller: "sections", constraints: ->(request) {
-          action = request.path_parameters[:action]
-          AdminCompanySection::ALL.include?(action)
-        }, as: :section
+        # Also see `Admin::BaseController.admin_company_section_path`.
+        AdminCompanySection::ALL.each do |section|
+          get section, controller: "sections", action: section, as: "#{section}_section"
+        end

And in Admin::BaseController:

helper_method \
def admin_company_section_path(company, section, **args)
  public_send("admin_company_#{section}_section_path", company, **args)
end

@ashrafalzyoud
Copy link

resources :invoices do
  member do
    get :client_view
  end
  collection do
    get :auto_complete
    get :bulk_edit, :context_menu
    post :bulk_edit, :bulk_update
    delete :bulk_destroy
    get :recurring
  end
  resources :invoice_mails, :only => [:new] do
    collection do
      post :send_mail, :preview_mail
    end
  end
end

resources :invoices do
  resources :invoice_payments, :as => :payments
end

resources :invoice_payments, :only => [:show, :index]
resources :projects do
  resources :invoice_templates, :only => [:new, :create]

  resources :invoice_imports, :only => [:new, :create, :show] do
    member do
      get :settings
      post :settings
      get :mapping
      post :mapping
      get :run
      post :run
    end
  end

  resources :expense_imports, :only => [:new, :create, :show] do
    member do
      get :settings
      post :settings
      get :mapping
      post :mapping
      get :run
      post :run
    end
  end
end

resources :invoice_templates do
  member do
    get :preview
  end
  collection do
    post :add
  end
end

resources :invoice_reports, only: [:new, :create] do
  collection do
    get :create
    get :autocomplete
    get :preview
  end
end

resources :expenses do
    collection do
      get :bulk_edit, :context_menu
      post :bulk_edit, :bulk_update
      delete :bulk_destroy
    end
end

resources :invoice_imports, :only => [:new, :create]
resources :expense_imports, :only => [:new, :create]
resources :invoice_comments, :only => [:create, :destroy]

resources :projects do
resources :invoices, :only => [:index, :new, :create]
  resources :expenses, :only => [:index, :new, :create]
end

match "invoice_time_entries/new", :controller => "invoice_time_entries", :action => 'new', :via => :get
match "invoice_time_entries/create", :controller => "invoice_time_entries", :action => 'create', :via => :post

DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)
DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 6.0. (called from instance_eval at /home/redmine/4.11/config/routes.rb:370)

@henrik
Copy link
Contributor

henrik commented Sep 21, 2020

@ashrafalzyoud Are you asking for help with solving this issue? Please use Stack Overflow for that, referencing this issue, and including the specific part of your routes that the deprecation warning points to – I don't believe it's included in the code you pasted here :)

@ashrafalzyoud
Copy link

@henrik
thx for your time and your help and support

 366  Dir.glob File.expand_path("#{Redmine::Plugin.directory}/*") do |plugin_dir|
 367 file = File.join(plugin_dir, "config/routes.rb")
 368   if File.exists?(file)
 369      begin
 370       instance_eval File.read(file)
 371   rescue SyntaxError, StandardError => e
 372       puts "An error occurred while loading the routes definition of #{File.basename(plugin_dir)}   372  plugin (#{file}): #{e.message}."
373        exit 1
374      end

this code in redmine
config/routes.rb

@henrik
Copy link
Contributor

henrik commented Sep 21, 2020

@ashrafalzyoud Hi! Sorry, I probably wasn't clear – please use https://stackoverflow.com/questions/ask to ask your question. There will be a lot more people available to help you there than in this issue :)

superdev9082 added a commit to superdev9082/ransack that referenced this pull request Feb 16, 2023
Using dynamic :controller or :action segments in routes has been a
source of a number of security issues in production and is deprecated
in Rails 5.0 since this [pull request] and is planned to be removed in
Rails 5.1.

Ransack is still maintaining compatibility with legacy Rails 3 and 4
versions, so this commit silences the deprecation messages for now
while running the test suite.

[pull request]: rails/rails#23980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants