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

Default protect from forgery #29742

Merged
merged 2 commits into from
Jul 10, 2017

Conversation

lugray
Copy link

@lugray lugray commented Jul 10, 2017

Summary

Fixes #29193.

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 lugray force-pushed the default_protect_from_forgery branch from 591df00 to 3aeaf53 Compare July 10, 2017 16:41
@lugray
Copy link
Author

lugray commented Jul 10, 2017

r? @rafaelfranca

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@lugray lugray force-pushed the default_protect_from_forgery branch from 3aeaf53 to 4bb3085 Compare July 10, 2017 18:08
#
# skip_before_action :verify_authenticity_token
#
# See skip_before_action for allowed options.
Copy link
Member

@Edouard-chin Edouard-chin Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing. Could you add a CHANGELOG entry?

@lugray lugray force-pushed the default_protect_from_forgery branch from 393b333 to 5d72432 Compare July 10, 2017 19:25
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs a full stop at the end.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a colon here:

This is a wrapper for:

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

lugray commented Jul 10, 2017

Thanks @eugeneius, updated as per your review.

@eugeneius
Copy link
Member

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.

Lisa Ugray added 2 commits July 10, 2017 16:23
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 lugray force-pushed the default_protect_from_forgery branch from 58af2b8 to 73b944e Compare July 10, 2017 20:24
@lugray
Copy link
Author

lugray commented Jul 10, 2017

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

@rafaelfranca rafaelfranca merged commit 48cb8b3 into rails:master Jul 10, 2017
@lugray lugray deleted the default_protect_from_forgery branch July 10, 2017 21:27

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo applocation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll get a fix up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it already got fixed in e01b240.

@wnm
Copy link

wnm commented Jan 24, 2018

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
Copy link
Author

lugray commented Jan 24, 2018

@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
Copy link

wnm commented Jan 24, 2018

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

@octavpo
Copy link

octavpo commented Apr 24, 2018

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
Copy link
Contributor

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.

ceritium added a commit to ceritium/featurer_web that referenced this pull request Jul 5, 2018
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.
jkeck added a commit to sul-dlss/vatican_exhibits that referenced this pull request Jul 21, 2018
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
Rails 5.2 added this behavior in rails/rails#29742
camillevilla pushed a commit to sul-dlss/vatican_exhibits that referenced this pull request Jul 23, 2018
@swrobel
Copy link
Contributor

swrobel commented Aug 22, 2018

@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
Copy link
Member

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

@rafaelfranca
Copy link
Member

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
Copy link
Contributor

swrobel commented Aug 22, 2018

@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
Copy link
Member

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.

alexdean referenced this pull request in tedconf/front_end_builds Apr 4, 2019
not sure how ember-cli generates these, but we're unable to deploy some TED
frontends with this filter in place.
yykamei added a commit to yykamei/rails that referenced this pull request Sep 25, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants