Migrate mass-assignment security to strong_parameters #1609

Merged
merged 4 commits into from Aug 6, 2014

Conversation

Projects
None yet
5 participants
Member

ferrous26 commented Jul 23, 2014

Ahead of the upgrade to Rails 4, this change removes the use of attr_accessible and attr_protected as mass assignment protection mechanisms in favour of whitelisting input attributes at the controller level.

See http://blog.markusproject.org/?p=5588 for more details.

All tests are passing, and some playing around with a local server seemed to work correctly. At this point I have some confidence that this is done correctly.

@houndci-bot houndci-bot commented on an outdated diff Jul 23, 2014

app/controllers/assignments_controller.rb
@@ -1,3 +1,7 @@
+Dir.glob("app/models/*_submission_rule.rb").each do |rule|
@houndci-bot

houndci-bot Jul 23, 2014

Prefer single-quoted strings when you don't need string interpolation or special symbols.

Contributor

lvwrence commented Jul 23, 2014

So basically all controller actions that have to do with creating and editing resources have parameters go through the whitelist at resource_params, correct?

Member

ferrous26 commented Jul 23, 2014

@oneohtrix yes, the idea is to make sure that if you are just going to pass an arbitrary list of variables through initialize/update a model, the list should be filtered to only allow attributes which users should be allowed to modify. Obviously not a silver bullet, input still needs to be sanitized; this is just another layer of defence.

Alternatively, models can be, and in some cases are, updated like so https://github.com/MarkUsProject/Markus/blob/d2bcc92788ceed880e113c59ce5bcf4ac3cb7112/app/controllers/annotations_controller.rb#L50, which does not use strong parameters because it makes tis own list of which attributes to allow and extracts those attributes manually.

@zhangsu zhangsu commented on an outdated diff Jul 23, 2014

app/controllers/assignments_controller.rb
unless potential_rule && potential_rule.ancestors.include?(SubmissionRule)
- raise I18n.t('assignment.not_valid_submission_rule',
- type: params[:assignment][:submission_rule_attributes][:type])
+ raise SubmissionRule::InvalidRuleType.new(rule_name)
@zhangsu

zhangsu Jul 23, 2014

Member

The Ruby style guide recommends raising through two arguments to raise instead of explicitly using the initializer:

raise SubmissionRule::InvalidRuleType, rulename

@zhangsu zhangsu commented on an outdated diff Jul 23, 2014

app/controllers/assignments_controller.rb
end
- assignment.submission_rule.type = params[:assignment][:submission_rule_attributes][:type]
+ assignment.submission_rule.type = rule_attributes[:type]
+ assignment.submission_rule.update_attributes(submission_rule_params)
@zhangsu

zhangsu Jul 23, 2014

Member

I might have misunderstood this one, but why was the previous code not updating the attributes?

Contributor

david-yz-liu commented Jul 25, 2014

@ferrous26 Thanks for doing this!

@zhangsu's last comment about the submission rules prompted me to do some digging... There's a lot of funny stuff going on with the submission rules, but one bug that your PR introduces for sure is that we lose the ability to "Remove" periods (all three rule types). That is, when you click the Remove checkbox and submit, the "Successful update" message is displayed, but the period is still there.

Contributor

david-yz-liu commented Jul 25, 2014

@ferrous26 Here's another fun one: if section due dates are used, every time the settings page gets updated, redundant section due date entries are added into the database, and the redundancies are displayed after the page refreshes.

Member

ferrous26 commented Aug 4, 2014

So....submission rules are handled by a bunch of hacks:

    # Was the SubmissionRule changed?  If so, switch the type of the
    # SubmissionRule. This little conditional has to do some hack-y
    # workarounds, since accepts_nested_attributes_for is a little...dumb.

I played around with it a bit and found that what happens is that the code expects to nuke every period on a submission rule update, and then create new periods. However, the old periods were referencesd in the submitted parameters, and they would get filtered out by strong_parameters because I did not allow the id parameter on the period of the submission_rule_attributes of the assignment_attributes.

It seems weird that those values are getting submitted if they are not being changed, and they are not coming through with a _destroy marker to indicate they should be deleted. The only solution I can think of right now is just to rewrite the hacks to actually work properly with strong_parameters.

Contributor

david-yz-liu commented Aug 4, 2014

Yeah, that's probably the best option. Is it possible to revert to the old params passing for the submission rules and merge the other changes, and then continue work on submission rules in another PR?

Member

ferrous26 commented Aug 5, 2014

Strong parameters is an all or nothing deal. There is an option to simply white list all input or a subset of input (without regard for what that subset actually contains).

Though, I can put the submission rule stuff in a separate commit, if that helps.

@houndci-bot houndci-bot commented on an outdated diff Aug 5, 2014

app/controllers/assignments_controller.rb
+ if assignment.submission_rule.class != potential_rule
+
+ # In this case, the easiest thing to do is nuke the old rule along
+ # with all the periods and a new submission rule...this may cause
+ # issues with foreign keys in the future, but not with the current
+ # schema
+ assignment.submission_rule.delete
+ assignment.submission_rule = potential_rule.new
+
+ # this part of the update is particularly hacky, because the incoming
+ # data will include some mix of the old periods and new periods; in
+ # the case of purely new periods the input is only an array, but in
+ # the case of a mixture the input is a hash, and if there are no
+ # periods at all then the periods_attributes will be nil
+ periods = submission_rule_params[:periods_attributes]
+ periods = if periods.kind_of?(Hash)
@houndci-bot

houndci-bot Aug 5, 2014

Prefer Object#is_a? over Object#kind_of?.

@houndci-bot houndci-bot commented on an outdated diff Aug 5, 2014

app/controllers/assignments_controller.rb
+ # schema
+ assignment.submission_rule.delete
+ assignment.submission_rule = potential_rule.new
+
+ # this part of the update is particularly hacky, because the incoming
+ # data will include some mix of the old periods and new periods; in
+ # the case of purely new periods the input is only an array, but in
+ # the case of a mixture the input is a hash, and if there are no
+ # periods at all then the periods_attributes will be nil
+ periods = submission_rule_params[:periods_attributes]
+ periods = if periods.kind_of?(Hash)
+ # in this case, we do not care about the keys, because
+ # the new periods will have nonsense values for the key
+ # and the old periods are being discarded
+ periods.map { |_, p| p }.reject { |p| p.has_key?(:id) }
+ elsif periods.kind_of?(Array)
@houndci-bot

houndci-bot Aug 5, 2014

Prefer Object#is_a? over Object#kind_of?.

Member

ferrous26 commented Aug 5, 2014

@david-yz-liu GH-1622 and all submission rule updating funkiness that I could find has been addressed in c40f051

ferrous26 added some commits Jun 16, 2014

@ferrous26 ferrous26 Migrate mass-assignment security to strong_parameters
Ahead of the upgrade to Rails 4, this change removes the use of
attr_accessible and attr_protected as mass assignment protection
mechanisms in favour of whitelisting input attributes at the
controller level.

See http://blog.markusproject.org/?p=5588 for more details.
fe7928e
@ferrous26 ferrous26 Handle submission rule changes correctly in assignment editing
Resolves GH-1622.
5d1c404
Contributor

david-yz-liu commented Aug 5, 2014

@ferrous26 Nice job, thanks! Once you fix the bug with the dubplicating Section Due Dates, I will merge this.

@ferrous26 ferrous26 Fix section due date duplication
They were not being properly whitelisted previously, and the
workaround did not check if the due dates already existed before
creating new due dates.

This fix allows Rails to simply Do The Right Thing.
63c2793
Member

ferrous26 commented Aug 5, 2014

Member

ferrous26 commented Aug 6, 2014

There are some test failures now, but they appear to be false positives. I am looking into it now.

@houndci-bot houndci-bot commented on an outdated diff Aug 6, 2014

test/functional/assignments_controller_test.rb
assert_not_nil new_assignment
assert_equal true, new_assignment.section_due_dates_type
due_date_attributes.each do |key, value|
- assert new_assignment.section_due_dates[key].
- due_date.to_s.include?(value['due_date'])
+ date = Time.parse(value['due_date'])
+ due_date =
+ new_assignment.section_due_dates
+ .find { |d| d.due_date == date}
@houndci-bot

houndci-bot Aug 6, 2014

Prefer detect over find.
Space missing inside }.

@houndci-bot houndci-bot commented on an outdated diff Aug 6, 2014

test/functional/assignments_controller_test.rb
due_date_attributes = {
- 0 => { 'section_id' => "#{@section1.id}",
- 'due_date' => '2011-10-27 00:00' },
- 1 => { 'section_id' => "#{@section2.id}",
- 'due_date' => '2011-10-29 00:00' }
+ '0' => { 'section_id' => "#{@section1.id}",
@houndci-bot

houndci-bot Aug 6, 2014

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

@houndci-bot houndci-bot commented on an outdated diff Aug 6, 2014

test/functional/assignments_controller_test.rb
due_date_attributes = {
- 0 => { 'section_id' => "#{@section1.id}",
- 'due_date' => nil },
- 1 => { 'section_id' => "#{@section2.id}",
- 'due_date' => '2011-10-29 00:00' }
+ '0' => { 'section_id' => "#{@section1.id}",
@houndci-bot

houndci-bot Aug 6, 2014

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

@ferrous26 ferrous26 Really delete old section due dates when they get turned off
And fix broken tests which were providing false failures.
1172027

@houndci-bot houndci-bot commented on the diff Aug 6, 2014

test/functional/assignments_controller_test.rb
assert_not_nil new_assignment
assert_equal true, new_assignment.section_due_dates_type
due_date_attributes.each do |key, value|
- assert new_assignment.section_due_dates[key].
- due_date.to_s.include?(value['due_date'])
+ date = Time.parse(value['due_date'])
+ due_date =
+ new_assignment.section_due_dates
+ .find { |d| d.due_date == date }
@houndci-bot

houndci-bot Aug 6, 2014

Prefer detect over find.

Member

ferrous26 commented Aug 6, 2014

Ok, all fixed up now.

Though, I strongly disagree with Hound on the matter of #find vs. #detect. When did #detect get in vogue?

@david-yz-liu david-yz-liu added a commit that referenced this pull request Aug 6, 2014

@david-yz-liu david-yz-liu Merge pull request #1609 from ferrous26/strong_params
Migrate mass-assignment security to strong_parameters
89a2a30

@david-yz-liu david-yz-liu merged commit 89a2a30 into MarkUsProject:master Aug 6, 2014

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