rails/rails — pull request #13157 by imanel (2013-12-04 09:49:50+00:00)
Title: Remove deep munge

### Rationale:

After implementing `strong_params` in Rails 4.0 `deep_munge` become rather issue than solution. Instead of fixing one [simple example](https://groups.google.com/forum/#!topic/rubyonrails-security/t1WFuuQyavI) it introduced series of bugs that prevented a lot of people in making [valid API](https://github.com/rails/rails/pull/11044)'s and even [nested attributes](https://github.com/rails/rails/issues/8832). This lead to some pretty [insane solutions](https://github.com/rails/rails/pull/8862) that introduce even more confusion. In the mean time a lot of applications are still on Rails 3.2.10 just to prevent invalid JSON parsing, or applying [patch](https://github.com/rails/rails/issues/8832#issuecomment-12131852) to remove `deep_munge`.

### Why deep_munge was introduced:

`deep_munge` was introduced as solution to simple example:

```ruby
unless params[:token].nil?
user = User.find_by_token(params[:token])
user.reset_password!
end
```

The problem was situation where attacker crafted request so it will set `params[:token] = [nil]`.

`deep_munge` solution to it is compacting all arrays and changing empty array to nil:

```ruby
def deep_munge(hash)
hash.each do |k, v|
case v
when Array
v.grep(Hash) { |x| deep_munge(x) }
v.compact!
hash[k] = nil if v.empty?
when Hash
deep_munge(v)
end
end

hash
end
```

### What kind of bugs deep_munge introduced:

This table shows what kind of behavior will we get when passing JSON:

JSON | Hash
-----------------------------|----------------------
{"person":null} | {'person' => nil}
{"person":[]} | {'person' => nil}
{"person":[null]} | {'person' => nil}
{"person":[null, null, ...]} | {'person' => nil}
{"person":["foo", null]} | {'person' => ["foo"]}

It's obvious that current behavior prevents all `nil`s in params. A lot of API's allows them, so braking JSON spec is probably not a solution. It also makes confusion by changing type of passed argument from Array to NilClass - that prevents strong params to work in some cases.

Second type of error is breaking [nested attributes](https://github.com/rails/rails/issues/8832) - at this point passing empty array will error. To explain it further: without proper hack to default behavior it's impossible to pass nested attributes for empty collection. So we have core functionality that is not working.

### What kind of bugs deep_munge is not supporting:

Let's stay with example that was shown as CVE-2013-0155 and behavior with `deep_munge` in place:

```ruby
unless params[:token].nil?
user = User.find_by_token(params[:token])
user.reset_password!
end
```

If this code stays then it creates false feeling of security, while it made hacking application just a little more harder to hack. Let's consider that attacker knows how long token is - he can pass array of tokens and try to hit one of them. For longer tokens it will be harder, but thanks to no limit of POST request he can try thousands of tokens in one batch - until he hit one of them. It's harder to guess, but still far from calling "secure". In properly written application this code would look like that:

```ruby
User.find_by_token!(params[:token].to_s)
user.reset_password!
```

This assures only one key is passed and default type is not relevant. I'm pretty sure that most of advanced developers are doing it this way (or using something similar) instead of checking `nil?`.

For new developers we should invest in educating them instead of dumbing them down with false security feeling and reinforcing bad habits. CVE-2013-0155 is only one of multiple situations when newcomers should learn convention and good habits instead of using half-measures to cover potential bugs in code - we can't cover them all.

### Related issues:

- #11044
- #8832
- #8862
- #8845
- #8855
- #8870

It's been 11 months since @rafaelfranca said that [you are working on fix](https://github.com/rails/rails/pull/8870#issuecomment-12107571) - since this time all I can see is spreading this bug instead of fixing it. `deep_munge` was supposed to be feature (fix for bug that in reality was just programmer writing bad code), but instead it's bug that introduces a lot of other problems (including invalid handling of JSON by default which is not obvious and always hard to debug). Please just remove it.