Given the existence of attr_accessible, attr_protected, a global on/off switch for the default behavior and nested attributes, you cannot tell what that line does without knowing the contents of at least two files.
And Rails 3.1 does have the concept of roles (not necessarily of users), baked into the same mass assignment logic into the model via the :on argument.
I'm not saying that this is all bad or a bug even, but I don't see how this is trivial to the reader either.
I agree there are a number of complex issues here, however that line of code still obviously takes ALL content from the outside world and directly updates a model with it. Any developer who does not spot that as a security issue is probably creating many others as well. Rails can not protect against developers not understanding that form submissions can contain any content and should not be trusted or applied directly to models without understanding what's happening. A cautious developer would slice up the submission to update the model with only the expected or allowed fields.
I just learned of the new role feature for attr_accessible because of this controversy. This seems to solve one of the major issues with attr_accessible - that different controllers and users need to update different attributes, so any somewhat complex app would end up widening it's attr_accessible attributes beyond what they should be.
These are still blunt tools though - what if only superadmin users can update a role column to superadmin, but admins can update it to admin or guest. This requires more extensive logic in the controller than simple attribute filtering, demonstrating why this filtering really belongs outside the model. Despite that, I think the new "role" based attr_accessible probably covers most cases and seems quite useful.
For all we know, GitHub may have been using attr_accessible but have expanded it to include columns updatable by admins.
Thank you for the thoughtful comment - perhaps there is hope that HN hasn't been entirely taken over by people talking out of their asses.
Comments
(I didn't downvote you)
> Does exactly what you'd expect it to do.
Given the existence of attr_accessible, attr_protected, a global on/off switch for the default behavior and nested attributes, you cannot tell what that line does without knowing the contents of at least two files.
And Rails 3.1 does have the concept of roles (not necessarily of users), baked into the same mass assignment logic into the model via the :on argument.
I'm not saying that this is all bad or a bug even, but I don't see how this is trivial to the reader either.
I agree there are a number of complex issues here, however that line of code still obviously takes ALL content from the outside world and directly updates a model with it. Any developer who does not spot that as a security issue is probably creating many others as well. Rails can not protect against developers not understanding that form submissions can contain any content and should not be trusted or applied directly to models without understanding what's happening. A cautious developer would slice up the submission to update the model with only the expected or allowed fields.
I just learned of the new role feature for attr_accessible because of this controversy. This seems to solve one of the major issues with attr_accessible - that different controllers and users need to update different attributes, so any somewhat complex app would end up widening it's attr_accessible attributes beyond what they should be.
These are still blunt tools though - what if only superadmin users can update a role column to superadmin, but admins can update it to admin or guest. This requires more extensive logic in the controller than simple attribute filtering, demonstrating why this filtering really belongs outside the model. Despite that, I think the new "role" based attr_accessible probably covers most cases and seems quite useful.
For all we know, GitHub may have been using attr_accessible but have expanded it to include columns updatable by admins.
Thank you for the thoughtful comment - perhaps there is hope that HN hasn't been entirely taken over by people talking out of their asses.