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

Already on GitHub? Sign in to your account

Fix authorization mass assignment #12

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

jjb commented Nov 12, 2011

In my app I have accessible attributes set to empty globally, which forces each model to define its own whitelist.

ActiveRecord::Base.send(:attr_accessible, nil)

This is a pretty common practice AFAIK. Because of this, creating a new OAuth2::Model::Authorization in OAuth2::Model::Authorization.for_response_type was failing for me.

This patch accommodates such environments.

Contributor

jcoglan commented Dec 1, 2011

I'm not quite sure what this achieves. You make all attributes inaccessible for mass-assignment, and then work around that by essentially reimplementing find_or_create_by* to paper over the restriction. What value does this add? Do you need to do this for all the ActiveRecord libraries you use?

Contributor

jjb commented Dec 3, 2011

You make all attributes inaccessible for mass-assignment

yes

and then work around that by essentially reimplementing find_or_create_by* to paper over the restriction.

I'm not sure what you mean here.

Let me reexplain from the beginning. It's a very common, standard, recommended practice in Rails to make an initializer which does this:

ActiveRecord::Base.send(:attr_accessible, nil)

And then use attr_accessible in each and every model to specify the whitelist of attributes which may be mass-assigned (that is, the set of attributes that a user is allowed to control from, say, the create and edit web forms for a given resource).

So given that, there's a problem using oauth2-provider without this patch.

Does that make sense?

Contributor

jcoglan commented Jan 13, 2012

What I'm saying is that with the find_or_new method, you seem to have reimplemented find_or_create_by_owner_and_client in order to work around a security restriction you put in, and this makes the security restriction itself seem less valuable. The restriction is designed to make developers writing forms unable to expose fields the user should not be able to change, but if you provide an alternative API that works around this restriction the holes come back.

I also wonder whether you need to patch all the ActiveRecord libraries you use -- surely they don't all assume all the attributes have been locked down and do everything using the verbose model.attr = value syntax?

Have you tried doing this to make the fields writable:

class OAuth2::Model::Authorization
  attr_accessible :owner, :client
end
Contributor

jjb commented Jan 13, 2012

What I'm saying is that with the find_or_new method, you seem to have reimplemented find_or_create_by_owner_and_client in order to work around a security restriction you put in, and this makes the security restriction itself seem less valuable. The restriction is designed to make developers writing forms unable to expose fields the user should not be able to change, but if you provide an alternative API that works around this restriction the holes come back.

The restriction is so if any part of the system throws params into a create or update, the ownership aspects of the model can't be changed if a nefarious person puts in extra vars. So, no part of your library is an offender, but your library still has to exist within implementations which set ActiveRecord::Base.send(:attr_accessible, nil)

I also wonder whether you need to patch all the ActiveRecord libraries you use -- surely they don't all assume all the attributes have been locked down and do everything using the verbose model.attr = value syntax?

That's an interesting point... But yeah, I've never had this problem before.

Have you tried doing this to make the fields writable:

class OAuth2::Model::Authorization
 attr_accessible :owner, :client
end

If we did that then the model would be vulnerable to the params injection attack :-D

Contributor

jjb commented Jan 13, 2012

If we did that then the model would be vulnerable to the params injection attack :-D

Hmm actually, since they aren't _id that might be fine... hmm

bugant commented Mar 23, 2012

The attr_accessible solution should prevent injection attack, right? That is, only owner and client attributes could be mass-assigned, but this should be fine.

Contributor

jcoglan commented Aug 22, 2012

This is now fixed by @pixeltrix. Sorry it took so long.

@jcoglan jcoglan closed this Aug 22, 2012

@sectioneight sectioneight pushed a commit to sectioneight/oauth2-provider that referenced this pull request Jan 7, 2013

@tomafro tomafro Merge pull request #12 from alevy/master
Fix for ActiveRecord 3.1
de9e8ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment