Stop using mass assignment in controllers #82

Closed
augustf opened this Issue May 3, 2012 · 11 comments

Projects

None yet

4 participants

@augustf
Concerto Digital Signage member

Mass assignment is known to be a major cause of security headaches in Rails applications (http://guides.rubyonrails.org/security.html#mass-assignment). As a best practice, we should refactor our controllers to assign attributes individually. We can then either start using a mass assignment whitelisting technique, or even better, the mass assignment sanitizer, both of which would ameliorate the issue.

@bamnet bamnet was assigned May 3, 2012
@bamnet
Concerto Digital Signage member

I'll take this todo post rails upgrade.

@augustf
Concerto Digital Signage member

Here's to the 3.2.2=>3.2.3 upgrade not breaking anything of consequence...

@zr2d2 zr2d2 was assigned Nov 28, 2012
@bamnet
Concerto Digital Signage member

Punting this to Zach who's working on this.

@zr2d2
Concerto Digital Signage member

Mostly complete as of d2b8830. Feel free to reopen if problems exist.

@zr2d2 zr2d2 closed this Jan 9, 2013
@bamnet
Concerto Digital Signage member

Nope, the build is broken. Maybe the plugin model to blame? https://travis-ci.org/concerto/concerto/jobs/4037078

@bamnet bamnet reopened this Jan 9, 2013
@bamnet
Concerto Digital Signage member

I spent a few hours looking into this tonight trying to fix the build. I'm now of the pretty firm belief that mass assignment protection has no place in the model, we should skip ahead and start using Strong Parameters (https://github.com/rails/strong_parameters).

I'm going to revert the commit flurry from tonight and will try to test strong params tomorrow if no one beats me to it by then.

@bamnet bamnet added a commit that referenced this issue Jan 9, 2013
@bamnet bamnet Revert "almost there" per #82.
This reverts commit b7f59ab.

Revert "whitelist the config"

This reverts commit 9337766.

Revert "ignore the configs"

This reverts commit 39ca21f.

Revert "Whitelist Everything (Almost)"

This reverts commit d2b8830.

Revert "Merge master into mass_assignment"

This reverts commit 98c4f03, reversing
changes made to 4431a6c.
ebb5c3c
@bamnet bamnet added a commit that referenced this issue Jan 10, 2013
@bamnet bamnet Strong params for screens per #82. 1745120
@bamnet
Concerto Digital Signage member

1745120 has a good example of the simplest implementation, add all the fields you see in the form to the list, test it manually and then run the tests to make sure nothing broke (it shouldn't have, we have a weak functional / integration test suite)

@bamnet bamnet added a commit that referenced this issue Jan 10, 2013
@bamnet bamnet Feed strong params for #82. a25943c
@zr2d2 zr2d2 pushed a commit that referenced this issue Jan 12, 2013
Zach Rowe strong parameters for groups per #82 4581a86
@zr2d2 zr2d2 pushed a commit that referenced this issue Jan 12, 2013
Zach Rowe Add config SP #82 eadd88b
@zr2d2 zr2d2 pushed a commit that referenced this issue Jan 13, 2013
Zach Rowe style, internationalize, and protect kinds #82 5b430c9
@zr2d2 zr2d2 pushed a commit that referenced this issue Jan 13, 2013
Zach Rowe add strong parameters to membership #82 3cdfe23
@bamnet
Concerto Digital Signage member

@zr2d2 What's the status here? Are all our parameters strong?

@zr2d2
Concerto Digital Signage member

The following models arem't using ForbiddenAttributes: concerto_plugin, field, media, position, submission, subscription, template, user

I am also working on adding strong params to devise

@zr2d2 zr2d2 pushed a commit that referenced this issue Feb 11, 2013
Zach Rowe add strong params for sessions #82 22b0816
@zr2d2 zr2d2 added a commit that referenced this issue Apr 27, 2013
@zr2d2 zr2d2 add strong params to submissions #82 b4f4463
@zr2d2 zr2d2 added a commit that referenced this issue Apr 27, 2013
@zr2d2 zr2d2 add srong params for templates #82 b3ef7a7
@zr2d2 zr2d2 pushed a commit that referenced this issue May 6, 2013
Zach Rowe add strong params to users #82 44f1b90
@zr2d2 zr2d2 pushed a commit that referenced this issue May 12, 2013
Zach Rowe Merge branch 'master' of github.com:concerto/concerto into rails-4
* 'master' of github.com:concerto/concerto:
  add strong params to users #82
9c2ebe1
@mfrederickson
Concerto Digital Signage member

@zr2d2 Heya Zack, here's a little something fer ya, matie! Arrrrr! It wont be allowin' me actions...

Started PUT "/feeds/1/submissions/5" for 10.0.3.16 at 2013-05-16 14:53:40 -0800
Processing by SubmissionsController#update as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"HtKIPLCw2GcrkIN+lDsaJJ4a2/P+JRm9UNgMSTWDGow=", "submission"=>{"moderation_reason"=>"", "moderation_flag"=>"false"}, "commit"=>"Deny", "feed_id"=>"1", "id"=>"5"}
Completed 500 Internal Server Error in 87ms

ActiveModel::ForbiddenAttributes (ActiveModel::ForbiddenAttributes):
  app/controllers/submissions_controller.rb:70:in `block in update'
  app/controllers/submissions_controller.rb:69:in `update'
@zr2d2 zr2d2 pushed a commit that referenced this issue May 17, 2013
Zach Rowe add strong params to submissions #82 da49c51
@zr2d2
Concerto Digital Signage member

@mfrederickson good catch. I added strong params to allow for submission#update

@zr2d2 zr2d2 closed this May 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment