Detect ActiveModel::ForbiddenAttributesProtection

[tilsammans]

Forgive me for creating an issue and not a pull request (yet) but it seems Brakeman does not detect when the 'strong_parameters' gem is used and ActiveModel::ForbiddenAttributesProtection is included in a model. This will be the Rails 4-way of protecting mass assignment and I think Brakeman should see it, right?

[presidentbeef]

Hi Joost,

Thank you for bringing this up.

It does not seem to be widely-used at the moment, but it does not look difficult to support.

Also, Rails 4 support is not a top priority until there is at least a release candidate :)

[tilsammans]

Agreed on low priority.

I am working on an entirely new app which definitely will run Rails 4 most of its life. It seemed smart to implement the new way of protecting mass attributes. Now my brakeman scan is full of high priority warnings while it should be 100% clean. :)

The gem is at 1.0.3 and does work with Rails 3.2+. I am not sure if it is widely used at this time though.

Op 5 mei 2012 om 00:46 heeft Justin reply@reply.github.com het volgende geschreven:

Hi Joost,

Thank you for bringing this up.

It does not seem to be widely-used at the moment, but it does not look difficult to support.

Also, Rails 4 support is not a top priority until there is at least a release candidate :)

---

Reply to this email directly or view it on GitHub:
#88 (comment)

[presidentbeef]

This is the only thing in Rails 4 that I am relatively certain will affect Brakeman, so plan to get done before Brakeman 2.0

[brynary]

As a very early first cut of this, could we just skip CheckModelAttributes and CheckMassAssignment completely if the strong_parameters gem is present? If so, I could take a stab at that.

In the time since it was released, that approach has become popular even in Rails 3 apps, anecdotally.

[presidentbeef]

Is that all we need to do? If someone is using the gem or Rails 4, do we just not warn on mass assignment issues? If not, what do we warn on?

[presidentbeef]

Okay, looks like if the app is using the gem, it needs to also include ActiveModel::ForbiddenAttributesProtection which is easy to check.

I'll see about getting to this today.

[brynary]

Yes, that sounds like a good start for Rails 3.

In Rails 4 I believe this will become a config setting (default to on). If someone flips that off (which may be especially likely during early Rails 3 -> 4 migrations), then we'll want the current/old behavior.

Finally, looking at the strong_parameters gem, there seems to be a method #permit! which allows all params through, so we may want to emit a warning if that is called. I've never used it, and hopefully if someone uses it they know what they are doing, but it removes all filtering so it's a bit dangerous.

[brynary]

Does that require each model include ActiveModel::ForbiddenAttributesProtection? The common/best way is to include it into AR::Base at boot time in an initializer.

[presidentbeef]

Yes, it does. Do you have example code for the initializer? I can add it.

[brynary]

class ActiveRecord::Base
  include ActiveModel::ForbiddenAttributesProtection
end

or

ActiveRecord::Base.class_eval do
  include ActiveModel::ForbiddenAttributesProtection
end

[presidentbeef]

class_eval? 😞 Okay, will add.

[brynary]

Eh, yeah. I actually often use class_eval because if something goes wrong an the class being extended was not previously loaded then class_eval will raise an error and the other form will just create a new (broke) class.

[presidentbeef]

Okay, updated. Just checking for include ActiveModel::ForbiddenAttributesProtection in an initializer, which covers both cases. Hopefully no one will include that in some random class. (This is because initializers aren't included in the current CallIndex, so checking them for stuff uses the old, lame way.)

[brynary]

Sounds good!

[CarlosEspejo]

I learned this one from Ryan Bates

ActiveRecord::Base.send(:include, ActiveModel::ForbiddenAttributesProtection)

[brynary]

Yes, we should catch that too.

[skandragon]

Using permit! is safe if you are protecting your code in other ways, such as requiring that the user be an admin, so all fields are permissible. Even then, it's dangerous as it also allows access to the user_id fields, for instance, which you likely don't want to grant. I'd make permit! a warning of some sort, but make it easy to turn it off for those who think they know better.

[presidentbeef]

@skandragon Can you open a separate issue for this, maybe with some more details or links about permit!?