Merge pull request #16924 from Sinjo/params-deep-munge-empty-array

Don't convert empty arrays to nils when deep munging params
rails · Dec 15, 2014 · 488aefe · 488aefe
2 parents 485723e + 8f8ccb9
commit 488aefe
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 23 deletions.
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
@@ -1,3 +1,14 @@
+*   Stop converting empty arrays in `params` to `nil`
+
+    This behaviour was introduced in response to CVE-2012-2660, CVE-2012-2694
+    and CVE-2013-0155
+
+    ActiveRecord now issues a safe query when passing an empty array into
+    a where clause, so there is no longer a need to defend against this type
+    of input (any nils are still stripped from the array).
+
+    *Chris Sinjakli*
+
 *   Fixed usage of optional scopes in URL helpers.
 
     *Alex Robbin*

diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb
@@ -53,15 +53,6 @@ def unpermitted_parameters(event)
       end
     end
 
-    def deep_munge(event)
-      debug do
-        "Value for params[:#{event.payload[:keys].join('][:')}] was set "\
-        "to nil, because it was one of [], [null] or [null, null, ...]. "\
-        "Go to http://guides.rubyonrails.org/security.html#unsafe-query-generation "\
-        "for more information."\
-      end
-    end
-
     %w(write_fragment read_fragment exist_fragment?
        expire_fragment expire_page write_page).each do |method|
       class_eval <<-METHOD, __FILE__, __LINE__ + 1

diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb
@@ -16,10 +16,6 @@ def deep_munge(hash, keys = [])
             when Array
               v.grep(Hash) { |x| deep_munge(x, keys) }
               v.compact!
-              if v.empty?
-                hash[k] = nil
-                ActiveSupport::Notifications.instrument("deep_munge.action_controller", keys: keys)
-              end
             when Hash
               deep_munge(v, keys)
             end

diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -39,15 +39,15 @@ def teardown
 
   test "nils are stripped from collections" do
     assert_parses(
-      {"person" => nil},
+      {"person" => []},
       "{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' }
     )
     assert_parses(
       {"person" => ['foo']},
       "{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' }
     )
     assert_parses(
-      {"person" => nil},
+      {"person" => []},
       "{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' }
     )
   end

diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb b/actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -95,8 +95,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
 

diff --git a/guides/source/action_controller_overview.md b/guides/source/action_controller_overview.md
@@ -112,8 +112,8 @@ NOTE: The actual URL in this example will be encoded as "/clients?ids%5b%5d=1&id
 
 The value of `params[:ids]` will now be `["1", "2", "3"]`. Note that parameter values are always strings; Rails makes no attempt to guess or cast the type.
 
-NOTE: Values such as `[]`, `[nil]` or `[nil, nil, ...]` in `params` are replaced
-with `nil` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation)
+NOTE: Values such as `[nil]` or `[nil, nil, ...]` in `params` are replaced
+with `[]` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation)
 for more information.
 
 To send a hash you include the key name inside the brackets:

diff --git a/guides/source/security.md b/guides/source/security.md
@@ -942,7 +942,7 @@ unless params[:token].nil?
 end
 ```
 
-When `params[:token]` is one of: `[]`, `[nil]`, `[nil, nil, ...]` or
+When `params[:token]` is one of: `[nil]`, `[nil, nil, ...]` or
 `['foo', nil]` it will bypass the test for `nil`, but `IS NULL` or
 `IN ('foo', NULL)` where clauses still will be added to the SQL query.
 
@@ -953,9 +953,9 @@ request:
 | JSON                              | Parameters               |
 |-----------------------------------|--------------------------|
 | `{ "person": null }`              | `{ :person => nil }`     |
-| `{ "person": [] }`                | `{ :person => nil }`     |
-| `{ "person": [null] }`            | `{ :person => nil }`     |
-| `{ "person": [null, null, ...] }` | `{ :person => nil }`     |
+| `{ "person": [] }`                | `{ :person => [] }`     |
+| `{ "person": [null] }`            | `{ :person => [] }`     |
+| `{ "person": [null, null, ...] }` | `{ :person => [] }`     |
 | `{ "person": ["foo", null] }`     | `{ :person => ["foo"] }` |
 
 It is possible to return to old behaviour and disable `deep_munge` configuring