Is Brakeman too opinionated on the argument type of load_defaults?

[TomNaessens]

Hi all, 👋

I had a strange false positive when bumping Brakeman to 6.0.0 on my Rails app. I had Rails defaults for 6.0 loaded, which include the default_protect_from_forgery config option set to true, but yet, Brakeman was telling me I should add it to my ApplicationController specifically.

After some debugging, it turns out that Brakeman skips loading defaults when the load_defaults argument ("6.0" in my case) isn't a number:

brakeman/lib/brakeman/tracker/config.rb

Line 192 in 6af53c6

return unless number? tracker.config.rails[:load_defaults]

Rails itself is not that opinionated on what type it is, they just call to_s on the argument: https://github.com/rails/rails/blob/e10e35dd32aff48457a76b83e244e8baad34a9ec/railties/lib/rails/application/configuration.rb#L107

I get why Brakeman wants it to be a number (as some number comparisons are done on it later), so I did not want to submit this as a bug report. On the other hand, it triggered a false positive that took a bit to find out why it was happening.

Anyway, I'm fine with either answer to the discussion title and I've changed the load_defaults in my Rails config to a float.

[presidentbeef]

Simple answer is yes... as long as the argument to load_defaults is a value Brakeman can figure out (e.g., not a variable/constant) then it should be supported. Especially the simple case of a string instead of a float.

[presidentbeef]

This was addressed a while back in #1785