From 39794037817703575c35a75f1961b01b83791191 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 7 Dec 2015 09:46:56 -0500 Subject: [PATCH] Change the `protect_from_forgery` prepend default to `false` Per this comment https://github.com/rails/rails/pull/18334#issuecomment-69234050 we want `protect_from_forgery` to default to `prepend: false`. `protect_from_forgery` will now be insterted into the callback chain at the point it is called in your application. This is useful for cases where you want to `protect_from_forgery` after you perform required authentication callbacks or other callbacks that are required to run after forgery protection. If you want `protect_from_forgery` callbacks to always run first, regardless of position they are called in your application, then you can add `prepend: true` to your `protect_from_forgery` call. Example: ```ruby protect_from_forgery prepend: true ``` --- actionpack/CHANGELOG.md | 23 +++++++++++++++++++ .../metal/request_forgery_protection.rb | 14 +++++------ .../request_forgery_protection_test.rb | 4 ++-- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index cab7d85ee7..b8563d5076 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,26 @@ +* Change the `protect_from_forgery` prepend default to `false` + + Per this comment + https://github.com/rails/rails/pull/18334#issuecomment-69234050 we want + `protect_from_forgery` to default to `prepend: false`. + + `protect_from_forgery` will now be insterted into the callback chain at the + point it is called in your application. This is useful for cases where you + want to `protect_from_forgery` after you perform required authentication + callbacks or other callbacks that are required to run after forgery protection. + + If you want `protect_from_forgery` callbacks to always run first, regardless of + position they are called in your application then you can add `prepend: true` + to your `protect_from_forgery` call. + + Example: + + ```ruby + protect_from_forgery prepend: true + ``` + + * Eileen M. Uchitelle* + * In url_for, never append a question mark to the URL when the query string is empty anyway. (It used to do that when called like `url_for(controller: 'x', action: 'y', q: {})`.) diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index 836ed892dc..26c4550f89 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -102,13 +102,13 @@ module ActionController #:nodoc: # # Valid Options: # - # * :only/:except - Only apply forgery protection to a subset of actions. Like only: [ :create, :create_all ]. + # * :only/:except - Only apply forgery protection to a subset of actions. For example only: [ :create, :create_all ]. # * :if/:unless - Turn off the forgery protection entirely depending on the passed Proc or method reference. - # * :prepend - By default, the verification of the authentication token is added to the front of the - # callback chain. If you need to make the verification depend on other callbacks, like authentication methods - # (say cookies vs OAuth), this might not work for you. Pass prepend: false to just add the - # verification callback in the position of the protect_from_forgery call. This means any callbacks added - # before are run first. + # * :prepend - By default, the verification of the authentication token will be added at the position of the + # protect_from_forgery call in your application. This means any callbacks added before are run first. This is useful + # when you want your forgery protection to depend on other callbacks, like authentication methods (Oauth vs Cookie auth). + # + # If you need to add verification to the beginning of the callback chain, use prepend: true. # * :with - Set the method to handle unverified request. # # Valid unverified request handling methods are: @@ -116,7 +116,7 @@ module ActionController #:nodoc: # * :reset_session - Resets the session. # * :null_session - Provides an empty session during request but doesn't reset it completely. Used as default if :with option is not specified. def protect_from_forgery(options = {}) - options = options.reverse_merge(prepend: true) + options = options.reverse_merge(prepend: false) self.forgery_protection_strategy = protection_method_class(options[:with] || :null_session) self.request_forgery_protection_token ||= :authenticity_token diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb index 2a3704c300..87a8ed3dc9 100644 --- a/actionpack/test/controller/request_forgery_protection_test.rb +++ b/actionpack/test/controller/request_forgery_protection_test.rb @@ -540,10 +540,10 @@ class PrependProtectForgeryBaseControllerTest < ActionController::TestCase assert_equal(expected_callback_order, @controller.called_callbacks) end - def test_verify_authenticity_token_is_prepended_by_default + def test_verify_authenticity_token_is_not_prepended_by_default @controller = PrependDefaultController.new get :index - expected_callback_order = ["verify_authenticity_token", "custom_action"] + expected_callback_order = ["custom_action", "verify_authenticity_token"] assert_equal(expected_callback_order, @controller.called_callbacks) end end