Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Always compact array parameters rather than setting them to nil #9569

Closed
wants to merge 1 commit into from

9 participants

@laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.

@laserlemon laserlemon Always compact array parameters rather than setting them to nil
The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.
3f5db8b
@ersatzryan

The behavior of Rails keeps changing. In 3-0-stable passing empty array param results in an empty array but passing an array of just nil or multiple nil values results in nil where I believe most people would still expect it to return []

10513d2

However in 3-1-stable, 3-2-stable, and master both empty array params and passing an array of just nil or multiple nil values both result in nil. Again this seems contrary to what one would expect.

@benhamill

For clarity, can someone give a simple before/after example that illustrates how this affects the day-to-day code a typical Rails developer might write?

@LouisStAmour

It takes {"person":[null]} and turns it into {"person":[]} instead of {"person":nil}. In both this and the previous patch, #9111 it fixes the problem of preserving empty arrays, but this goes father, making sure that you always get an array when given one. To be clear, it ignores past (seldom-expected) behaviour of returning nil when given an array with nil in it, preferring a simpler approach of simply compacting the array, returning an empty array more often.

@laserlemon

@benhamill One example would be a JSON API that accepts POST /users to create a user. If the User model serializes an array of nicknames, the request body might look like:

{
  "name": "Steve Richert",
  "nicknames": ["laserlemon"]
}

…but for a user with no nicknames, we might send:

{
  "name": "Frank Stallone",
  "nicknames": []
}

Currently Rails will try to set the user's serialized array of nicknames to nil. That's one of many possible points of confusion that the current Rails behavior can cause.

tumblr_m0hisv9cPt1qd0uqe

@ersatzryan

it also rears its ugly head if you have a Parent class that accepts_nested_attributes_for :children and pass {"children_attributes": []} as assign_nested_attributes_for_collection_association will raise an ArgumentError because it sends nil instead of an empty array.

https://github.com/rails/rails/blob/v3.2.12/activerecord/lib/active_record/nested_attributes.rb#L375

@rafaelfranca

@tenderlove @jeremy could you guys review this one?

@laserlemon

@jeremy Thanks for taking the time to chat a bit at the RailsConf help desk. Would love to get your opinion on these changes.

@laserlemon

Could somebody please provide me with an example of where compacting array parameters is not sufficient to close the security hole?

@cbankester

What's the status on this? Any reviewing going on? It seems like a small thing, but it really makes a huge difference when posting nested models.

@christhekeele christhekeele referenced this pull request from a commit in christhekeele/rails
@laserlemon laserlemon Always compact array parameters rather than setting them to nil
Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.
7343c01
@christhekeele

I've rebased against master so that this change can get more attention with as little friction in the codebase as possible.

@bhavinjavia

Just ran into this when accepting nested array values (which could be empty) via a JSON API
+1

@christhekeele christhekeele referenced this pull request from a commit in christhekeele/rails
@laserlemon laserlemon Always compact array parameters rather than setting them to nil
Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.
d45f61a
@chancancode
Owner

Hey guys, thank you for submitting this and expressing your concerns about this issue! Speaking for myself, I understand where you are coming from, and I agree we could do better. Unfortunately, this PR does not meet our requirements for an acceptable solution to the problem.

Please head over to #13420 for the background and contribute your ideas. Thanks again! :heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 5, 2013
  1. @laserlemon

    Always compact array parameters rather than setting them to nil

    laserlemon authored
    The CVE-2013-0155 security vulnerability outlines how array parameters
    containing nil values can be dangerous when accepted directly into
    Active Record finder methods. The previous fix has been to detect array
    parameters with nil values and to clobber the entire array into nil.
    
    Instead, compacting the array solves the security issue and causes less
    surprise in the case where array parameters are expected, as is often
    the case when accepting nested attributes for collections.
This page is out of date. Refresh to see the latest.
View
1  actionpack/lib/action_dispatch/http/request.rb
@@ -291,7 +291,6 @@ def deep_munge(hash)
when Array
v.grep(Hash) { |x| deep_munge(x) }
v.compact!
- hash[k] = nil if v.empty?
when Hash
deep_munge(v)
end
View
4 actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -32,7 +32,7 @@ def teardown
test "nils are stripped from collections" do
assert_parses(
- {"person" => nil},
+ {"person" => []},
"{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' }
)
assert_parses(
@@ -40,7 +40,7 @@ def teardown
"{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' }
)
assert_parses(
- {"person" => nil},
+ {"person" => []},
"{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' }
)
end
View
4 actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -84,8 +84,8 @@ def teardown
assert_parses({"action" => nil}, "action")
assert_parses({"action" => {"foo" => nil}}, "action[foo]")
assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar]")
- assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar][]")
- assert_parses({"action" => {"foo" => nil }}, "action[foo][]")
+ assert_parses({"action" => {"foo" => { "bar" => [] }}}, "action[foo][bar][]")
+ assert_parses({"action" => {"foo" => [] }}, "action[foo][]")
assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end
Something went wrong with that request. Please try again.