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

Fix default cross-site request forgery setting when defaults are not loaded #1776

Merged
merged 3 commits into from Apr 21, 2023

Conversation

presidentbeef
Copy link
Owner

Follow up from #1530, adding a test.

Should fix #1773

@presidentbeef presidentbeef merged commit a301e6a into main Apr 21, 2023
8 checks passed
@presidentbeef presidentbeef deleted the montdidier-hotfix/rails_52_if_no_defaults branch April 21, 2023 06:32
@joshgoebel
Copy link

Yes, I think this inversion on the conditional would fix it! Thanks!

@MaksJS
Copy link

MaksJS commented May 29, 2023

I've got a regression with this one on Rails 7. Rails.configuration.action_controller.default_protect_from_forgery is true and I get the Cross-Site Request Forgery warning. Maybe, at some point, the value of default_protect_from_forgery is not passed to the tracker config when not inlined in the configuration file (true is the default value).

@joshgoebel
Copy link

Where else are you setting the configuration? I guess I'd expect it to run part of the init to get the values - but does it only do static analysis?

@presidentbeef
Copy link
Owner Author

For security reasons, Brakeman does not run any of the code it analyzes.

@MaksJS Where is default_protect_from_forgery set in your application? Is it in an initializer?

@MaksJS
Copy link

MaksJS commented May 30, 2023

@joshgoebel @presidentbeef Nowhere, it's true by default so I don't set it again in the config

@joshgoebel
Copy link

t's true by default

So you're using load_defaults?

@MaksJS
Copy link

MaksJS commented May 30, 2023

@joshgoebel Yes

@presidentbeef
Copy link
Owner Author

@MaksJS I cannot reproduce your issue.

This is what the Rails 7 test app looks like:

module Rails7
  class Application < Rails::Application
    # Initialize configuration defaults for originally generated Rails version.
    config.load_defaults 7.0

It's very hard to guess at the issue. Please share the warning you are getting what your code looks like. Even better, share an example Rails application that demonstrates the issue. Thanks!

@MaksJS
Copy link

MaksJS commented May 31, 2023

@presidentbeef Ok I was able to reproduce it. You're right, with a brand new Rails 7 application it works well.
But on my case, I'm developing a Rails Engine with a dummy application, which have this configuration by default:

module Dummy
  class Application < Rails::Application
    config.load_defaults Rails::VERSION::STRING.to_f
  end

This configuration is generated by this template: https://github.com/rails/rails/blob/main/railties/lib/rails/generators/rails/app/templates/config/application.rb.tt#L15

In this case, I get the Cross-Site Request Forgery warning with Brakeman 6.0.0.

@presidentbeef
Copy link
Owner Author

Ah. In that case Brakeman cannot know for which version of Rails to load the defaults. What version would you expect it to use? Is there a Gemfile.lock with a specific version?

@MaksJS
Copy link

MaksJS commented Jun 2, 2023

Well, Rails::VERSION::STRING.to_f would evaluate to 7.0 here. You can also get the version number with Rails.configuration.loaded_config_version, so maybe it can be available in tracker.config.rails.dig(:loaded_config_version). Or maybe not if the app config is never evaluated, which seems to be the case if I understand correctly this method

def set_rails_version version = nil

But yes there is an entry in Gemfile.lock: rails (~> 7.0.5).

And I correctly get the version number in the report:

[Notice] Detected Rails 7 application
....
Rails Version: 7.0.5
Brakeman Version: 6.0.0
....
== Warnings ==

Confidence: High
Category: Cross-Site Request Forgery
....

@joevin-slq-docto
Copy link
Contributor

joevin-slq-docto commented Jun 15, 2023

Hello,
I'm using Rails 7 with config.load_defaults 6.0 inside application.rb.
Rails console confirms me that Rails.configuration.action_controller.default_protect_from_forgery is set to true but I get the Cross-Site Request Forgery warning since I upgraded to Brakeman 6.0.0.

Repository owner locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False Negative: warning on CSRF in Rails 5.2+ with defaults
5 participants