Mass assignment security #718

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants

pda commented Dec 15, 2010

Following on from plataformatec/#716...

I've moved attribute whitelisting from the resource model into the RegistrationController.

Whitelisting uses ActiveModel::MassAssignmentSecurity (thanks José) rather than per-model attr_accessible, so that different sets of attributes can be used in different contexts, and mass-assignment of non-user-submitted attributes can be used from application code.

I've added integration tests which assert arbitrary params sent to RegistrationController do not reach the model. The tests use :facebook_token as an example of a protected attribute, but any database-backed protected attribute would work. The tests were passing, then failed when attr_accessible was removed from User, and now pass again with filtering in RegistrationController.

I investigated the other controllers which pass params hashes into the model, and found that they all filter the params down to authentication_keys via find_or_initialize_with_errors, except for PasswordsController#update which does not use mass assignment.

pda added some commits Dec 15, 2010

Tests assert arbitrary params sent to RegistrationController do not r…
…each the model.

Tests use :facebook_token as an example of a protected attribute, but
any attribute which exists in the database but shouldn't be editable by
the user-facing form works.

Currently this is protected against by attr_accessible whitelisting on
the model.
Attribute whitelist moved from User model to RegistrationController.
Whitelisting uses ActiveModel::MassAssignmentSecurity rather than
per-model attr_accessible, so that different sets of attributes can be
used in different contexts, and mass-assignment of non-user-submitted
attributes can be used from application code.
Owner

josevalim commented Dec 15, 2010

Nice, thanks for the pull request. But I was thinking in including AM::MassAssignmentSecurity in the model and calling it appropriately in each model method. Moving this responsibility to the controller really makes it hard to customize, which is what a lot of people end up doing, and is not backward compatible.

pda commented Jan 31, 2011

I wont have time to look further into this any time soon ... but I do still believe the mass-assignment filtering is a controller concern.

Perhaps you'd like to merge in f19e21f which just contains the tests to assert that filtered data does not make it to the model.

Other than that, how about we close this pull request for now?

Cheers!
Paul

@josevalim josevalim closed this Mar 30, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment