Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False-Positive Mass Assignment warning #372

Closed
markijbema opened this issue Jul 23, 2013 · 3 comments · Fixed by #384
Closed

False-Positive Mass Assignment warning #372

markijbema opened this issue Jul 23, 2013 · 3 comments · Fixed by #384
Milestone

Comments

@markijbema
Copy link

In our app we have different scopes for different mass assignment. In our admin part we can just assign almost everything. However, brakeman seems to disapprove. Is this intentional? And if so, what would be a better way to do this? Because to me this seems quite secure:

class User
  include Mongoid::Document

  field :username
  field :admin,       type: Boolean, default: false

  attr_accessible :username
  attr_accessible :username, :admin, as: :admin

end
@themetric
Copy link
Contributor

Yes any kind of role-based authorization to protect these attributes would be secure. Brakeman will throw a false positive many times because we haven't yet checked to see if attributes like this one are purposely exposed and then protected through authorization. This is difficult to detect with a static scanner due to the many different ways to authorize certain actions. CanCan for example does authorization very well and the latest 2.0 version supports protecting resource attributes based on conditions like a user's role.

@presidentbeef
Copy link
Owner

Seems like this is easily fixed by checking for a hash argument with :as.

I'm more surprised that this class would be treated like a model. Is this the actual class that generated the warning? Edit: nevermind, any class in app/models is treated like a model.

@markijbema
Copy link
Author

Also, a Mongoid::Document is a model, so treating it as model is correct behaviour as far as I'm concerned.

Repository owner locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants