labml
labml copied to clipboard
labmlai
prevent rails 5.2+ without defaults being oked
On rails 5.2+ installations that are missing the defaults, or where upgraded to rails 5.2+ without applying all the defaults - the default_protect_from_forgery? method returns the wrong result.
if the defaults are not present, dig returns nil which also does not match Sexp.new(:false) and the original method returns true.
This new logic both simplifies the logic and returns correctly false when the defaults are not present.
It appears this problem was introduced here https://github.com/presidentbeef/brakeman/pull/1138
Hi @montdidier! Thank you for bringing this up.
I'm not 100% convinced the current state of Brakeman is wrong, but the wording of the changes is a bit confusing.
However, testing with a clean Rails 5.2 app (also tested with 5.2.0):
Loading development environment (Rails 5.2.4.4)
2.5.1 :001 > Rails.configuration.action_controller.default_protect_from_forgery
=> true
The configuration is not explicitly set anywhere. So the default is true and Brakeman should look for the configuration setting to be explicitly set to false. I have confirmed that is what it does now.
Also confirmed config.action_controller.default_protect_from_forgery = true does not generate a warning but config.action_controller.default_protect_from_forgery = false does.
Apologies for the confusing wording. I will try again to explain what I mean.
The problem as I have observed is that in apps that have been upgraded to 5.2 the defaults are not necessarily present. This needs to be done explicitly. In a clean fresh app, no problem. While there is no argument upgraders should do this, they don't necessarily do it. As I figured part of brakemans job is to protect developers from themselves I thought this would be a valuable change.
See https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults. You can see that unless done explicitly it is quite possible to end up without defaults when upgrading.
Knowing that, what is the behaviour of the current code when config.action_controller.default_protect_from_forgery returns nil?
Note that load_defaults was only added from rails 5 onward. https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults
Okay, I've got it now. This only applies to upgraded apps.
This seems like a bit of a catch-22. If the app is upgraded, it needs to have the configuration option set explicitly, or else CSRF protection is not enabled by default. But if the app was created as a Rails 5.2 app or newer, then it does not need to have the configuration option set explicitly, but will instead default to enabling CSRF protection. :thinking:
Is there a way to reasonably check which case applies? Would you expect config/initializers/new_framework_defaults_5_2.rb to be present for upgraded apps?
doesn't my change just work for all cases? or am I missing something?
if config.action_controller.default_protect_from_forgery is nil as what might expect in an upgraded app, it will not match and fallback to false further down.
if config.action_controller.default_protect_from_forgery is false in an app then CSRF is not enabled by default, it will not match and fallback to false further down.
if config.action_controller.default_protect_from_forgery is true it matches and returns true.
I think we can trust the config.action_controller.default_protect_from_forgery value? It might not be enabled explicitly in the project code but it is in the framework somewhere for new apps. It's happening in the load_defaults I think which just populates this and others.
Presumably if the upgrader wants to set the defaults up piecemeal one can do so in a file named config/initializers/new_framework_defaults_5_2.rb (although the naming is a recommendation not a guarantee) and if
config.action_controller.default_protect_from_forgery = true is explicitly set here we'd see it represented within the default_protect_from_forgery? method?
Okay, I think I see the disconnect here.
if config.action_controller.default_protect_from_forgery is nil as what might expect in an upgraded app, [...]
It might not be enabled explicitly in the project code but it is in the framework somewhere for new apps.
It will be nil from Brakeman's point of view in both a fresh and upgraded app, because there is no code that Brakeman sees that sets the value. What Brakeman has in tracker.config.rails[...] is only what the application explicitly sets. If no value is set, then Brakeman can't see it.
However, I do see that load_defaults is called here in config/application.rb:
module Rails52
class Application < Rails::Application
# Initialize configuration defaults for originally generated Rails version.
config.load_defaults 5.2
So we could use that call to figure out what default configs to set. Unfortunately, this is not picked up right now because Brakeman is looking for calls like config.load_defaults = ... but that could easily be added in https://github.com/presidentbeef/brakeman/blob/main/lib/brakeman/processors/lib/rails3_config_processor.rb
Opened https://github.com/presidentbeef/brakeman/pull/1531 to at least load the config into tracker.config.rails[...].
It will be
nilfrom Brakeman's point of view in both a fresh and upgraded app, because there is no code that Brakeman sees that sets the value. What Brakeman has intracker.config.rails[...]is only what the application explicitly sets. If no value is set, then Brakeman can't see it.
OK good to know. I am not by any means au fait with how brakeman works. I'll have a think about what you have said and see if I can contribute anything further meaningful.
I have a branch that will do what you expect. Stay tuned, I'll have a PR up soon.
@montdidier I've merged into main. Can you rebase and test that behavior is what you expect? Some actual tests would be nice, too, if you have time.