Usage of --safe-methods to silence 'Dynamic Render Path' false positives

[ddalcino]

Hi! I'm asking this in the Q&A section because I'm not sure if:

• I'm trying to do something that's not supported,
• I'm misusing the CLI option, or
• I've encountered a bug.

Here's the relevant code, in a controller (from a Rails project):

  render action: (allowed_partial(params[:partial]) || "show")

Assume that the allowed_partial method is passing the params[:partial] parameter through an allow-list, that makes sure that this dynamic render path cannot render anything that it should not be able to. It returns nil if the parameter is not allowed, so the controller would just render the "show" page.

I am passing allowed_partial to the --safe-methods CLI flag, like this:

$ brakeman --safe-methods allowed_partial .

I expected that Brakeman would not report anything for the render line, but it always reports a weak-confidence "Dynamic Render Path" warning.

I know I can add a config/brakeman.ignore file or raise the confidence threshold, but I would prefer not to solve the problem that way. The weak-confidence warnings are often useful, so I don't want to turn them off.

Please let me know what you think I should do. I'm happy to file an issue, if you think that's appropriate.

[presidentbeef]

It's a mis-use of the option, but that's because of poor naming and not your fault. --safe-methods is really only for use with cross-site scripting checks. There are also --sql-safe-methods and --url-safe-methods, but to stop a proliferation of these options, I have adopted the policy of "use the ignore file instead". Another option is to turn off that check, if it's not useful to you.

Some day, depending on how complicated allowed_partial is, Brakeman might be able to recognize it as an allow-listing method. But it's not there yet.

[ddalcino]

Thank you for the quick response, Mr. President! That makes sense; I guess an ignore file isn't much harder to maintain than a list of 'safe methods'.

I wonder how difficult it would be to implement an "allow-list classifier". Are you aware of any existing examples of such code, either in Brakeman or elsewhere? I might be willing to take a crack at it if I had some idea of where to start.