1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

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

Don't convert empty arrays to nils when deep munging params
This commit is contained in:
Godfrey Chan 2014-12-15 14:46:32 -08:00
commit 488aefe742
7 changed files with 21 additions and 23 deletions

View file

@ -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. * Fixed usage of optional scopes in URL helpers.
*Alex Robbin* *Alex Robbin*

View file

@ -53,15 +53,6 @@ module ActionController
end end
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? %w(write_fragment read_fragment exist_fragment?
expire_fragment expire_page write_page).each do |method| expire_fragment expire_page write_page).each do |method|
class_eval <<-METHOD, __FILE__, __LINE__ + 1 class_eval <<-METHOD, __FILE__, __LINE__ + 1

View file

@ -16,10 +16,6 @@ module ActionDispatch
when Array when Array
v.grep(Hash) { |x| deep_munge(x, keys) } v.grep(Hash) { |x| deep_munge(x, keys) }
v.compact! v.compact!
if v.empty?
hash[k] = nil
ActiveSupport::Notifications.instrument("deep_munge.action_controller", keys: keys)
end
when Hash when Hash
deep_munge(v, keys) deep_munge(v, keys)
end end

View file

@ -39,7 +39,7 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest
test "nils are stripped from collections" do test "nils are stripped from collections" do
assert_parses( assert_parses(
{"person" => nil}, {"person" => []},
"{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' } "{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' }
) )
assert_parses( assert_parses(
@ -47,7 +47,7 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest
"{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' } "{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' }
) )
assert_parses( assert_parses(
{"person" => nil}, {"person" => []},
"{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' } "{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' }
) )
end end

View file

@ -95,8 +95,8 @@ class QueryStringParsingTest < ActionDispatch::IntegrationTest
assert_parses({"action" => nil}, "action") assert_parses({"action" => nil}, "action")
assert_parses({"action" => {"foo" => nil}}, "action[foo]") 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" => { "bar" => nil }}}, "action[foo][bar][]") assert_parses({"action" => {"foo" => { "bar" => [] }}}, "action[foo][bar][]")
assert_parses({"action" => {"foo" => nil }}, "action[foo][]") assert_parses({"action" => {"foo" => [] }}, "action[foo][]")
assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]") assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end end

View file

@ -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. 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 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) with `[]` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation)
for more information. for more information.
To send a hash you include the key name inside the brackets: To send a hash you include the key name inside the brackets:

View file

@ -942,7 +942,7 @@ unless params[:token].nil?
end 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 `['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. `IN ('foo', NULL)` where clauses still will be added to the SQL query.
@ -953,9 +953,9 @@ request:
| JSON | Parameters | | JSON | Parameters |
|-----------------------------------|--------------------------| |-----------------------------------|--------------------------|
| `{ "person": null }` | `{ :person => nil }` | | `{ "person": null }` | `{ :person => nil }` |
| `{ "person": [] }` | `{ :person => nil }` | | `{ "person": [] }` | `{ :person => [] }` |
| `{ "person": [null] }` | `{ :person => nil }` | | `{ "person": [null] }` | `{ :person => [] }` |
| `{ "person": [null, null, ...] }` | `{ :person => nil }` | | `{ "person": [null, null, ...] }` | `{ :person => [] }` |
| `{ "person": ["foo", null] }` | `{ :person => ["foo"] }` | | `{ "person": ["foo", null] }` | `{ :person => ["foo"] }` |
It is possible to return to old behaviour and disable `deep_munge` configuring It is possible to return to old behaviour and disable `deep_munge` configuring