Expand skip_before_filter check

[brynary]

Right now CheckSkipBeforeFilter only looks for skipping the verify_authenticity_token method.

Although Rails does not have a controller-level authentication solution baked in, most users seem to use one of a small handful of authentication gems. Therefore, most Rails apps use guessable names for their authentication filter method, most commonly #login_required.

My proposal is that CheckSkipBeforeFilter also looks for login_required and the other "this action requires authentication" methods that common gems (e.g. Devise, Authlogic, restful_authentication) use.

Even though these aren't Rails methods, does this seem like it's in scope for Brakeman?

[presidentbeef]

The CheckSkipBeforeFilter doesn't just check for skipping the verify_authenticity_token, it also checks if it uses :except or :only - essentially a whitelist vs. blacklist approach. Is this what you are thinking for these other methods, too? I don't mind checking for things outside of Rails.

[brynary]

Yes. Sorry I wasn't clear. I'm proposing looking for skip_before_filter with :except with a method that provides security (e.g. login_required). This usually has the same issue as what CheckSkipBeforeFilter already picks up for verify_authenticity_token -- that you end up with an insecure-by-default configuration.

[presidentbeef]

That sounds fine to me.

[brynary]

Great. Seems like next step is to survey popular authentication/authorization gems for the names of filter methods that enforce controls. I'll take a stab at that.

[oreoshake]

According to https://www.ruby-toolbox.com/categories/rails_authentication
devise :authenticate_user!
omniauth ???
authlogic :require_user

[brynary]

restful_authentication uses login_required, which is what is the most common I've seen in my experience.

[presidentbeef]

Expanded! And omniauth also uses authenticate_user!