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

Updates configuring.md to be more clear on framework defaults #37586

Closed
wants to merge 2 commits into from

Conversation

systemnate
Copy link
Contributor

@systemnate systemnate commented Oct 29, 2019

Summary

I was confused in looking through the documentation. It seems that for brand new applications, config.action_controller.default_protect_from_forgery is true. Only when upgrading, and before you flip the new framework defaults, it is false.

Other Information

Currently, the docs say:

config.action_controller.default_protect_from_forgery determines whether forgery protection is added on ActionController::Base. This is false by default.

However, a little bit further it says (in https://edgeguides.rubyonrails.org/configuring.html#with-5-2):

config.action_controller.default_protect_from_forgery: true

Additionally, config/initializers/new_framework_defaults_5_2.rb says:

This file contains migration options to ease your Rails 5.2 upgrade.
Once upgraded flip defaults one by one to migrate to the new default.
...
Rails.application.config.action_controller.default_protect_from_forgery = true

It seems to me that the default is actually true.

I was confused in looking through the documentation.  It seems that for brand new applications, `config.action_controller.default_protect_from_forgery` is `true`.  Only when upgrading, and before you flip the new framework defaults, it is `false`.

Currently, the docs say:
> config.action_controller.default_protect_from_forgery determines whether forgery protection is added on ActionController::Base. This is false by default.

However, a little bit further it says (in https://edgeguides.rubyonrails.org/configuring.html#with-5-2):

> config.action_controller.default_protect_from_forgery: true

Additionally, `config/initializers/new_framework_defaults_5_2.rb` says:

> This file contains migration options to ease your Rails 5.2 upgrade.
> Once upgraded flip defaults one by one to migrate to the new default.
...
> # Rails.application.config.action_controller.default_protect_from_forgery = true

It seems to me that the default is actually `true`.
@rails-bot rails-bot bot added the docs label Oct 29, 2019
@systemnate systemnate changed the title Update configuring.md Updates configuring.md to be be more clear on framework defaults Oct 29, 2019
@bogdanvlviv
Copy link
Contributor

By default default_protect_from_forgery is set to false, see

self.default_protect_from_forgery = false

To apply the framework defaults we use load_defaults method.
The guide explaines what defaults are applied when you call the method with '5.2' or '6.0', etc. (Note that every version includes all defaults of previous versions)

@systemnate
Copy link
Contributor Author

systemnate commented Oct 29, 2019

Thanks - I did not see that. I'm still not completely clear. What calls #load_defaults? If you create a brand new Rails 6 application using rails new, what is the value of default_protect_from_forgery? Are you saying it is false?

Thanks in advance! Somehow I am not understanding what I read from the docs.

Edit: I now see that #load_defaults is called in config/application.rb. I guess the confusion is really that when you generate a new app, these defaults automatically get loaded. To me, therefore, the default is true. I get that it is technically false momentarily, but 🤷‍♂ .

Here is the exact scenario that led to my confusion:

  • A teammate upgraded an app to Rails 5.2. During the PR review, I saw that he removed protect_from_forgery in ApplicationController. I asked about this, and he mentioned that the Rails 5.2 default is true for config.action_controller.default_protect_from_forgery. Curious to learn more, I opened up the Rails docs where the first thing I read is:

config.action_controller.default_protect_from_forgery determines whether forgery protection is added on ActionController::Base. This is false by default.

@systemnate systemnate changed the title Updates configuring.md to be be more clear on framework defaults Updates configuring.md to be more clear on framework defaults Oct 29, 2019
@bogdanvlviv
Copy link
Contributor

bogdanvlviv commented Oct 30, 2019

@systemnate Please read #34855 (comment). I also opened PR to clarify docs of load_defaults in #37595. I hope it'll help users.

@systemnate
Copy link
Contributor Author

systemnate commented Oct 30, 2019

Thanks for the additional history and new PR. I think it is an improvement; however, I would still be confused if I were to read it for the first time again. In the comment you linked to, @rafaelfranca said:

Sorry but I disagree. We should talk about the default value of the configuration when no railtie code is applied. So the current documentation is correct. If we tell that the default is true for config.action_controller.default_protect_from_forgery and the user see that config being set to true in the application they will think it is safe to remove when it is not.

Can you help me understand why it is essential to talk about the defaults when no railtie code is applied? It appears that the reason is to avoid someone removing protect_from_forgery when they should not. For example, a user on a version of Rails before 5.2 might think they could remove it when they should not. To me, this confusion can be avoided by saying something like "The default for Rails versions >= 5.2 is true when config.load_defaults "5.2" (or higher) is used; otherwise, the value is false". I believe this would eliminate the ambiguity, especially for someone reading this guide for the first time.

Existing verbiage:

config.action_controller.default_protect_from_forgery determines whether forgery protection is added on ActionController:Base. This is false by default.

@@ -440,7 +440,7 @@ The schema dumper adds two additional configuration options:

* `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.
* `config.action_controller.default_protect_from_forgery` determines whether forgery protection is added on `ActionController::Base`. For new applications, the default is 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 can follow other places and simply say:

This is true by default.

We change a lot of these options, and in this guide we should just mention what the defaults are.

@rails-bot
Copy link

rails-bot bot commented Feb 17, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Feb 17, 2020
@rails-bot rails-bot bot closed this Feb 24, 2020
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.

None yet

3 participants