#3178 Mass Assignment Security Refactoring - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3178 ✓resolved

Mass Assignment Security Refactoring

Reported by Eric Chapweske | September 9th, 2009 @ 10:15 PM | in 3.0.2

This patch provides a rewrite that is hopefully more understandable and modular than the existing code. This should allow developers and plugin authors to experiment a bit more without introducing massive security holes. E.g., it's pretty trivial to move responsibility over to a controller by including ActiveRecord::MassAssignmentSecurity and adding a couple helper methods. This is my first contribution attempt so hopefully I didn't mess up too much ;)

Comments and changes to this ticket

You flagged this item as spam.
Eric Chapweske September 9th, 2009 @ 10:16 PM
- Tag changed from “patch” to “activerecord, patch”
- mass_assignment_patch.diff 30.6 KB
Michael Koziarski September 14th, 2009 @ 12:05 AM
- Milestone cleared.
- Assigned user set to “Michael Koziarski”
Hey Eric,

This looks like a really awesome refactoring.

I especially like the way they're tested independently of active record's own attributes= method. Could you also include an example in the documentation for the sanitizer how you'd suggest using it in a controller.

CCing jeremy pratik and josh as I think this is good to go but would like to get more eyes on it.

Great work
You flagged this item as spam.
Eric Chapweske September 29th, 2009 @ 07:49 PM
Thanks for the feedback! I've added the example documentation and also updated the implementation so that it can be extended for context-sensitive sanitization, as shown in the example in mass_assignment_security.rb. I would like to find a cleaner solution for this, but I think this approach requires the fewest API changes.
- mass_assignment_patch_2.diff 32.1 KB
You flagged this item as spam.
Eric Chapweske October 5th, 2009 @ 12:30 AM
Round 3. Hmmm, refactored the refactoring and expanded the documentation a bit. This simplifies the implementation significantly. Don't plan to make any other changes unless requested.

Also added mass_assignment_security/context.rb. I'd be happy to move it to a plugin, although I think it's pretty useful, and it's optional to include it.

Also rewrote multi-parameter assignment, the ticket is at http://rails.lighthouseapp.com/projects/8994/tickets/3317
You flagged this item as spam.
Eric Chapweske October 5th, 2009 @ 12:32 AM
And the patch:
- mass_assignment_3.diff 33 KB
You flagged this item as spam.
Eric Chapweske January 12th, 2010 @ 08:18 PM
Any chance of this making it in the next release? Hoping to utilize this in the near future.

Thanks!
-Eric
Rizwan Reza January 16th, 2010 @ 08:53 AM
- Tag changed from “activerecord, patch” to “activerecord, bugmash, patch”
The patch doesn't apply cleanly anymore.
You flagged this item as spam.
Eric Chapweske January 29th, 2010 @ 03:36 AM
New patch against master.
- mass_assign.diff 25.7 KB
You flagged this item as spam.
Eric Chapweske January 29th, 2010 @ 03:37 AM
Oh, this patch also modifies the documentation. Removed advise about using Hash.slice, and replaced it with this:
```
# Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_accessible+ 
# to sanitize attributes won't provide sufficient protection.
```
Edited by Rohit Arondekar for formating.
Jeremy Kemper January 29th, 2010 @ 05:47 PM
- State changed from “new” to “open”
This is nice and it works for me. The code style is different from the rest of Rails though: extra spaces padding class declarations, no indentation after protected/private. It's nit-picking, I know, but let's keep it uniform.
You flagged this item as spam.
Eric Chapweske January 30th, 2010 @ 01:56 AM
Woops. Adding indentation for private/protected and no spaces between modules and classes.
- mass_assign_clean.diff 25.7 KB
You flagged this item as spam.
Eric Chapweske January 30th, 2010 @ 02:10 AM
Bleh, scratch that -- this patch has a bug in it -- but I expect the style is correct :)
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “activerecord, bugmash, patch” to “activerecord, patch”
[bulk edit]

joshsz April 24th, 2010 @ 08:49 PM

FWIW, this patch is no longer applying cleanly to master:

$ git apply /tmp/mass_assign_clean.diff -v
(...)
Checking patch activerecord/lib/active_record/base.rb...
error: while searching for:
    extend ActiveModel::Naming
    extend QueryCache::ClassMethods
    extend ActiveSupport::Benchmarkable

    include Validations
    include Locking::Optimistic, Locking::Pessimistic

error: patch failed: activerecord/lib/active_record/base.rb:2378
error: activerecord/lib/active_record/base.rb: patch does not apply
(...)

Rohit Arondekar July 1st, 2010 @ 10:33 AM
- Importance changed from “” to “Medium”
Any updates to this ticket? Eric did you manage to fix that bug?
José Valim July 8th, 2010 @ 06:16 PM
- Assigned user changed from “Michael Koziarski” to “José Valim”
- State changed from “open” to “resolved”
Hoooray, applied!
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]