Reconsider check for foreign keys on mass assignment

[JasonBarnabe]

Trying 4.1, and my app gets 91 warnings about "Potentially dangerous key allowed for mass assignment". I would guess that is attempting to prevent the user from submitting a value not presented by the UI and doing something nefarious, but to warn on any foreign key seems a bit much. This is an extremely common pattern when presenting the user with a drop down or radio buttons to select another model in your app (especially things like country_id, *_type_id, etc.)

[csexton]

Just ran into this as well, and went digging to see what made the keys considered dangerous. And I found the change that added check.

Right now breakman assumes any parameter ending in _id is dangerous.

A number of the rails apps I maintain have a number of columns that have columns with _id that are not foreign keys in the database. These attributes are just identifiers from external systems.

[presidentbeef]

Thanks for the feedback.

Just want to point out I forgot to deduplicate the warnings, so the number should be reduced in the next release.

[j15e]

It is raising way too many warnings here too, we have removed the check altogether. I do not understand why an attribute with _id, when allowed by strong params, should be an issue?

We offer relationship choices in forms that validate with the model or just the database foreign key constraint.

Let me know the security rationale behind, I will be glad to learn more. 🏫

[louim]

From what I see in the code, I understand this was done to prevent ownership stealing. Let’s say for example one of your ressources has an owner_id pointing to a user. Having that key available for assignation can create a security breach if there is no validation of who is allowed to change it. But brakeman doesn’t know your rules or what you do with the key once it’s assigned.

I understand it may be overwhelming to whitelist all of the keys that you know are safe, but it should be a one time job. To me, this adds a nice safety net for developers who might be less experienced.

And you can always disable that rule globally if you know that you covered all your keys with safety checks, or they are keys that are actually allowed to be edited by the users.

[michaelrigart]

I can agree with @louim that having this check is good to create awareness about potential ownership stealing. These are great warnings.

But is there a way to work around this warning without just ignoring it. Take for example an associated field that does need that extra validation.
Would it be possible to make brakeman detect that extra validation?

[mort666]

While I agree that this is a worthwhile cause, the check is a blunt instrument that in effect will negate any positive effects as people will disable the check vs go through a ignore.

The check needs to further validate, check that parameter is actually used as foreign key in assignment via the schema via the relationships, maybe look at linking it to 'objects that could be linked to ownership'. User models, i.e. identify a User model by the devise configuration elements. Look at the validation on that an ID prior to saving, for example in our app some 'IDs' that had been identified had been validated in before action filters against a while list.

The thing that makes this change considerably more annoying and more likely to do more harm than good is that the issue description that brakeman links to in the Knowledge base is far from adequate to describe the problem or the reason why. It talks about the older mass assignment issue, given a lot of these are being raised in Strong Parameter usage the reader will read that detailed description and go wtf! and pretty much immediately turn off the check as it will look to them like a blatant False Positive.

[presidentbeef]

Thank you to everyone for your input.
I have updated the documentation a little bit. Please note everyone is welcome to contribute improvements to the Brakeman website.

FWIW, this is a direct translation of an existing check prior to strong params: https://github.com/presidentbeef/brakeman/blob/master/lib/brakeman/checks/check_model_attr_accessible.rb

Take for example an associated field that does need that extra validation. Would it be possible to make brakeman detect that extra validation?

No, not really.

The check needs to further validate, check that parameter is actually used as foreign key in assignment via the schema via the relationships

I don't see how that would be feasible. Maybe if permit is called at the site of the mass assignment, maybe. But the common pattern for strong parameters is to have a helper method that whitelists keys, and the open source version of Brakeman isn't going to be able to connect those helper methods with the actual uses.

Would people like to see the warning about *_id removed? Or changed to weak confidence?

[JasonBarnabe]

I would prefer to have the *_id check removed. It offers no value to me and I would just turn off the check entirely if it were to remain, even if it were with weak confidence.

[wata727]

IMO, brakeman shouldn't remove *_id check. Indeed, this warning may confuse many users. However, it gives us the opportunity to consider security issues again.

As much as possible, brakeman should reduce false positives, but if that is difficult, give warnings to potentially dangerous code, and users should check it.

[presidentbeef]

I think what I'll do is drop the *_id stuff for now and bring it back when it can be checked with fewer false positives. I have some ideas now that I'm not sick and my brain is moving again.