Default protect from forgery #29742

lugray · 2017-07-10T15:27:21Z

Summary

Protect from forgery by default: Rather than protecting from forgery in the generated ApplicationController, add it to ActionController::Base by config. This configuration defaults to false to support older versions which have removed it from their ApplicationController, but is set to true for Rails 5.2.

Also adds ActionController::Base.skip_forgery_protection as a wrapper to skip_before_action :verify_authenticity_token for disabling forgery protection.

lugray · 2017-07-10T17:30:12Z

r? @rafaelfranca

rafaelfranca · 2017-07-10T17:32:12Z

actionpack/lib/action_controller/railtie.rb

@@ -69,5 +69,13 @@ class Railtie < Rails::Railtie #:nodoc:
        config.compile_methods! if config.respond_to?(:compile_methods!)
      end
    end
+
+    initializer "action_controller.request_forgery_protection" do |app|
+      ActiveSupport.on_load(:action_controller) do


This is also going to run for ActionController::Api, so it is better to use on_load(:action_controller_base here. That way we can do:

ActiveSupport.on_load(:action_controller_base) do if app.config.action_controller.default_protect_from_forgery protect_from_forgery with: :exception end end

rafaelfranca · 2017-07-10T17:40:37Z

railties/lib/rails/application/configuration.rb

@@ -96,6 +96,10 @@ def load_defaults(target_version)
            active_support.use_authenticated_message_encryption = true
          end

+          if respond_to?(:action_controller)
+            action_controller.default_protect_from_forgery = true


We need to add this config to the configurations.md guide.

Edouard-chin · 2017-07-10T18:59:09Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

+      #
+      #   skip_before_action :verify_authenticity_token
+      #
+      # See skip_before_action for allowed options.


Small nitpicks, might use plus sign +skip_before_action+ for rdoc

rafaelfranca

Last thing. Could you add a CHANGELOG entry?

eugeneius · 2017-07-10T19:25:24Z

railties/test/application/configuration_test.rb

+    test "config.action_controller.default_protect_from_forgery is true by default on production" do
+      app "production"
+
+      ActionController::Base.object_id # force lazy load hooks to run


This isn't necessary, since referencing ActionController::Base on the next line will run the lazy load hooks. It's only needed when testing ActionController::Parameters options, as loading that class won't trigger the hooks.

eugeneius · 2017-07-10T19:27:32Z

railties/test/application/configuration_test.rb

@@ -1209,6 +1209,15 @@ def create
      assert_equal false, ActionController::Parameters.action_on_unpermitted_parameters
    end

+    test "config.action_controller.default_protect_from_forgery is true by default on production" do


The behaviour here doesn't depend on the environment, so I'd ✂️ "on production" from the test name.

eugeneius · 2017-07-10T19:32:38Z

railties/test/application/configuration_test.rb

@@ -1209,6 +1209,15 @@ def create
      assert_equal false, ActionController::Parameters.action_on_unpermitted_parameters
    end

+    test "config.action_controller.default_protect_from_forgery is true by default on production" do
+      app "production"


The rest of the tests in this file set this as "development" when the behaviour doesn't depend on the particular environment; we should probably do the same here for consistency.

eugeneius · 2017-07-10T19:34:18Z

guides/source/configuring.md

@@ -401,6 +401,8 @@ The schema dumper adds one additional configuration option:

 * `config.action_controller.per_form_csrf_tokens` configures whether CSRF tokens are only valid for the method/action they were generated for.

+* `config.action_controller.default_protect_from_forgery` determines whether forgery protection is added on ActionController:Base.  This is false by default, but enabled when loading defaults for Rails 5.2.


ActionController:Base -> ActionController::Base (with backticks 😊)

eugeneius · 2017-07-10T19:38:08Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -85,6 +85,10 @@ module RequestForgeryProtection
      config_accessor :per_form_csrf_tokens
      self.per_form_csrf_tokens = false

+      # Controls whether forgery protection is enabled by default


This line needs a full stop at the end.

eugeneius · 2017-07-10T19:38:12Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -128,6 +132,15 @@ def protect_from_forgery(options = {})
        append_after_action :verify_same_origin_request
      end

+      # Turn off request forgery protection. This is a wrapper for


I think there should be a colon here:

This is a wrapper for:

lugray · 2017-07-10T19:51:35Z

Thanks @eugeneius, updated as per your review.

eugeneius · 2017-07-10T20:06:55Z

This looks great! 👏

My only remaining question is whether there should be an entry in new_framework_defaults_5_2.rb for this, so that upgraded apps can opt-in to the new behaviour if they want it. My understanding of that file is that it should mirror the options from Rails::Application::Configuration#load_defaults.

Rather than protecting from forgery in the generated ApplicationController, add it to ActionController::Base by config. This configuration defaults to false to support older versions which have removed it from their ApplicationController, but is set to true for Rails 5.2.

Since we now default to `protect_from_forgery with: :exception`, provide a wrapper to `skip_before_action :verify_authenticity_token` for disabling forgery protection.

lugray · 2017-07-10T20:41:44Z

@eugeneius That makes sense. I added to new_framework_defaults_5_2.rb.tt

UlrichThomasGabor · 2017-07-24T17:13:14Z

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt

+
+# Add default protection from forgery to ActionController::Base instead of in
+# ApplicationController.
+# Rails.applocation.config.action_controller.default_protect_from_forgery = true


There is a typo applocation

Thanks, I'll get a fix up.

Actually, it already got fixed in e01b240.

wnm · 2018-01-24T11:25:54Z

This configuration defaults to false to support older versions which have removed it from their ApplicationController, but is set to true for Rails 5.2.

@lugray don't older versions have it in ApplicationController, and the new default is there to move it to ActionController::Base so we can remove it from ApplicationController?

lugray · 2018-01-24T13:59:09Z

@wnm By default, older versions do have it in ApplicationController. The point I'm making is that the way to turn it off in older versions would have been to remove the line from the generated ApplicationController. If someone has actively removed it because they don't want it, upgrading should not change the behavior. But new applications will get the 5.2 defaults, which set it to true. If you're upgrading an older version of rails, and you want to remove it from ApplicationController, you'll need to change the configuration you use, since you won't have the 5.2 defaults.

wnm · 2018-01-24T14:14:45Z

@lugray ah I see... totally makes sense. thanks for the quick response!!

rails/rails#29742

octavpo · 2018-04-24T16:38:21Z

I think it would be better if the default_protect_from_forgery config could take the :with parameter as a value.

There's also an inconsistency in approach that if you use default_protect_from_forgery = true in config it defaults to :exception, while if you use protect_from_forgery with no parameters in the controller it defaults to :null_session.

collimarco · 2018-06-12T09:57:46Z

There's also an inconsistency in approach that if you use default_protect_from_forgery = true in config it defaults to :exception, while if you use protect_from_forgery with no parameters in the controller it defaults to :null_session.

I totally agree! It is not clear from the docs that the default behavior is :exception.

Rails 5.1 has the CSRF protection disabled by default but rails 5.2 enable it again. rails/rails#29742 - Remove the fancy ajax delete. - Use jquery_ujs for the delete action (it must be included by the host app at the moment). - Add `csrf_meta_tags` on the layout.

Rails 5.2 added this behavior in rails/rails#29742 however it is false by default for backwards compatibility.

Rails 5.2 added this behavior in rails/rails#29742

swrobel · 2018-08-22T22:45:24Z

@lugray can you explain this?

There's also an inconsistency in approach that if you use default_protect_from_forgery = true in config it defaults to :exception, while if you use protect_from_forgery with no parameters in the controller it defaults to :null_session.

rafaelfranca · 2018-08-22T22:56:26Z

Yes, we change the default to be :exception because :null_session was a bad default.

swrobel · 2018-08-22T23:02:48Z

@rafaelfranca that does not seem to be the case:
https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L138
https://api.rubyonrails.org/classes/ActionController/RequestForgeryProtection/ClassMethods.html#method-i-protect_from_forgery

rafaelfranca · 2018-08-22T23:05:51Z

It was the case already before this patch. https://github.com/rails/rails/pull/29742/files#diff-8380e0e11d4efeb2b0b142ae9e4cf512L3 Even that the default of the method is null_session, rails already generated the application using exception. That was the new default for applications not for the method.

swrobel · 2018-08-22T23:19:18Z

@rafaelfranca I understand what you're saying, but for the 99.9% of Rails users who don't dig into the source code at this level, they're going to look at the docs for protect_from_forgery and see that null_session is the default and not understand why their 5.2 app is using exception. I don't see this anywhere in the 5.2 release notes, and while it's mentioned in the security guide I still think this creates a huge opportunity for confusion.

Would you at least be open to changing the default for protect_from_forgery to exception in 6.0 so it's consistent?

rafaelfranca · 2018-08-28T03:30:53Z

I think it is worth considering it, but it will be a really annoying experience to most of the users given they will have to be explicit about the default to remove the deprecation warning. But I guess it will avoid confusion.

not sure how ember-cli generates these, but we're unable to deploy some TED frontends with this filter in place.

Thanks to rails#29742, newly created Rails apps after Rails 5.2 don't have to deep dive into CSRF problems, and they don't require direct call of `protect_from_forgery`. However, some projects sill have to consider the case where `protect_from_forgery` should be called. Calling `protect_from_forgery` without `:with` option treats `:with` as `:null_session` by default, not `:exception`, so I was a bit confused by the inconsistency between `default_protect_from_forgery` and `protect_from_forgery`. Maybe, changing the default `:with` to `:exception` will bring significant breaking changes, so I want to suggest adding a notice to the method.

lugray force-pushed the default_protect_from_forgery branch from 591df00 to 3aeaf53 Compare July 10, 2017 16:41

rails-bot assigned rafaelfranca Jul 10, 2017

rafaelfranca requested changes Jul 10, 2017

View reviewed changes

lugray force-pushed the default_protect_from_forgery branch from 3aeaf53 to 4bb3085 Compare July 10, 2017 18:08

Edouard-chin reviewed Jul 10, 2017

View reviewed changes

lugray force-pushed the default_protect_from_forgery branch from 4bb3085 to 393b333 Compare July 10, 2017 19:16

rafaelfranca requested changes Jul 10, 2017

View reviewed changes

lugray force-pushed the default_protect_from_forgery branch from 393b333 to 5d72432 Compare July 10, 2017 19:25

eugeneius reviewed Jul 10, 2017

View reviewed changes

lugray force-pushed the default_protect_from_forgery branch 2 times, most recently from e9b8b74 to 58af2b8 Compare July 10, 2017 19:50

Lisa Ugray added 2 commits July 10, 2017 16:23

Add ActionController::Base.skip_forgery_protection

73b944e

Since we now default to `protect_from_forgery with: :exception`, provide a wrapper to `skip_before_action :verify_authenticity_token` for disabling forgery protection.

lugray force-pushed the default_protect_from_forgery branch from 58af2b8 to 73b944e Compare July 10, 2017 20:24

eugeneius approved these changes Jul 10, 2017

View reviewed changes

rafaelfranca merged commit 48cb8b3 into rails:master Jul 10, 2017

rafaelfranca mentioned this pull request Jul 10, 2017

Adding protect_from_forgery into ActionController::Base #27362

Closed

lugray deleted the default_protect_from_forgery branch July 10, 2017 21:27

UlrichThomasGabor reviewed Jul 24, 2017

View reviewed changes

shimbaco added a commit to annict/annict that referenced this pull request Mar 27, 2018

Skip verify_authenticity_token to avoid InvalidAuthenticityToken error

545ef40

rails/rails#29742

enrico mentioned this pull request Jun 20, 2018

Doorkeeper application controllers and Rails 5.2 protect_from_forgery default behavior doorkeeper-gem/doorkeeper#1110

Closed

chionyan mentioned this pull request Jun 29, 2018

Twitterログアウト機能の実装 chionyan/awesome_events#6

Merged

ceritium mentioned this pull request Jul 5, 2018

Make it works with CSRF token opsidao/featurer_web#3

Open

jkeck added a commit to sul-dlss/vatican_exhibits that referenced this pull request Jul 21, 2018

Protect from forgery by default

1548ee7

Rails 5.2 added this behavior in rails/rails#29742 however it is false by default for backwards compatibility.

jkeck added a commit to sul-dlss/vatican_exhibits that referenced this pull request Jul 21, 2018

Protect from forgery by default

7424765

Rails 5.2 added this behavior in rails/rails#29742

jkeck mentioned this pull request Jul 21, 2018

Protect from forgery by default sul-dlss/vatican_exhibits#274

Merged

camillevilla pushed a commit to sul-dlss/vatican_exhibits that referenced this pull request Jul 23, 2018

Protect from forgery by default (#274)

1499ab0

Rails 5.2 added this behavior in rails/rails#29742

alexdean referenced this pull request in tedconf/front_end_builds Apr 4, 2019

don't require an authenticity token when posting

255a043

not sure how ember-cli generates these, but we're unable to deploy some TED frontends with this filter in place.

louim mentioned this pull request May 6, 2019

Rails 5.2 POST to 404 results in 500 due to missing CSRF token yuki24/rambulance#48

Closed

drcapulet mentioned this pull request May 21, 2019

Rails 5.2 Breaks protect_with_forgery in Rails Engine Controllers #36317

Closed

EduardoGHdez mentioned this pull request Jun 12, 2020

Default forgery protection strategy #39608

Closed

sohymg mentioned this pull request Jul 30, 2020

[ci skip] Add clarity on protect_from_forgery documentation #39950

Merged

brandondees mentioned this pull request Sep 13, 2020

Update Rails to 5.2.4.4 railslink/railslink#133

Merged

5 tasks

yykamei mentioned this pull request Sep 25, 2023

Add notice to documentation for protect_from_forgery's :with option #49374

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default protect from forgery #29742

Default protect from forgery #29742

lugray commented Jul 10, 2017

lugray commented Jul 10, 2017

rafaelfranca Jul 10, 2017

rafaelfranca Jul 10, 2017

lugray Jul 10, 2017

Edouard-chin Jul 10, 2017 •

edited

lugray Jul 10, 2017

rafaelfranca left a comment

eugeneius Jul 10, 2017

eugeneius Jul 10, 2017

eugeneius Jul 10, 2017

eugeneius Jul 10, 2017

eugeneius Jul 10, 2017

eugeneius Jul 10, 2017

lugray commented Jul 10, 2017

eugeneius commented Jul 10, 2017

lugray commented Jul 10, 2017

UlrichThomasGabor Jul 24, 2017

lugray Jul 24, 2017

lugray Jul 24, 2017

wnm commented Jan 24, 2018

lugray commented Jan 24, 2018 •

edited

wnm commented Jan 24, 2018

octavpo commented Apr 24, 2018

collimarco commented Jun 12, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 22, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 22, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 28, 2018

		@@ -401,6 +401,8 @@ The schema dumper adds one additional configuration option:

		* `config.action_controller.per_form_csrf_tokens` configures whether CSRF tokens are only valid for the method/action they were generated for.

		* `config.action_controller.default_protect_from_forgery` determines whether forgery protection is added on ActionController:Base. This is false by default, but enabled when loading defaults for Rails 5.2.

Default protect from forgery #29742

Default protect from forgery #29742

Conversation

lugray commented Jul 10, 2017

Summary

lugray commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Edouard-chin Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lugray commented Jul 10, 2017

eugeneius commented Jul 10, 2017

lugray commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnm commented Jan 24, 2018

lugray commented Jan 24, 2018 • edited

wnm commented Jan 24, 2018

octavpo commented Apr 24, 2018

collimarco commented Jun 12, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 22, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 22, 2018

swrobel commented Aug 22, 2018

rafaelfranca commented Aug 28, 2018

Edouard-chin Jul 10, 2017 •

edited

lugray commented Jan 24, 2018 •

edited