Regression with empty nested attributes in parameters #8832

OmarSkalli · 2013-01-09T00:40:32Z

Rails 3.2.11 is causing a regression when nested attributes have an empty value. For example, the following code works in 3.2.10 but throws an exception in 3.2.11:

            $.ajax('/home/index', {
                type: 'POST',
                contentType: "application/json; charset=utf-8",
                dataType: "json",
                data: JSON.stringify({ post: { comments_attributes: [] }}),
                error: function(response, status, jqxhr) { $("#response").html(response.responseText); },
                success: function(response, status, jqxhr) { $("#response").html("Request successful"); }
            });
        });

You can reproduce the bug with the following repro application:

Clone: https://github.com/Chetane/rails-3.2.11-nested-attributes-bug
Rake db:migrate, rails server
Go to home/index controller and follow steps

This is breaking my application, and other will most likely encounter this exception. This commit is causing the regression: d5cd97b

hash[k] = nil if v.empty

The text was updated successfully, but these errors were encountered:

mattgreen · 2013-01-09T00:52:08Z

+1

We had to work around this in production just after we deployed 3.2.11. An empty array is a perfectly reasonable value.

ienders · 2013-01-09T01:11:07Z

+1

I don't usually comment on issues, but I'm sorry, this is pretty upsetting. An empty array and nil are not the same thing at all.

akavi · 2013-01-09T01:21:24Z

+1

Why is the fix in JSON parsing instead of in query generation?

Note that this fix also strips nils from within arrays (eg. [1, 2, 3, nil] => [1, 2, 3])

PikachuEXE · 2013-01-09T02:14:55Z

Should be related to #8831

OmarSkalli · 2013-01-09T02:18:02Z

Definitely related. It's worth pointing however that in this case, an exception is thrown. In #8831, it silently strips nil. Both should definitely be fixed.

jeremy · 2013-01-09T04:29:41Z

Please do investigate! Need a fix that doesn't result in args injection. See the tests @ d5cd97b#L7R27

homakov · 2013-01-09T04:33:28Z

tough question: leave nils and expect injection in find methods or remove them and get JSON broken. I don't know..

mikebaldry · 2013-01-09T09:28:46Z

+1

nicolasblanco · 2013-01-09T10:04:35Z

+1.
My app is broken because of this patch. And this application does not even use ActiveRecord.

homakov · 2013-01-09T11:52:55Z

ok, +1 too

simonx1 · 2013-01-09T14:48:32Z

+1

jeremy · 2013-01-09T15:40:55Z

We have +7 but no code proposed. Please do investigate!

ceterumnet · 2013-01-09T16:02:02Z

+1

jlsync · 2013-01-09T16:07:13Z

+1 on this issue. Perhaps the solution is to delete the key from the params hash rather than setting the value to nil (which is my current work-around in a before_filter ).

TheBerg · 2013-01-09T17:18:36Z

+1

rainulf · 2013-01-09T17:47:56Z

+1

TheBerg · 2013-01-09T17:52:23Z

After investigating it some more, I totally understand why this was implemented the way it was implemented. Rails needs to be secure by default.

Maybe we should add a params_from_post_data method that would still let people get around this. Thoughts?

I am investigating if this would work in my own applications. If it works and people feel like it is a good work around I'll submit a patch.

TheBerg · 2013-01-09T18:05:05Z

Params from post data would look kinda like this:

  def json_params
    @json_params ||= ActionController::Parameters.new(ActiveSupport::JSON.decode(request.body))
  end

But would need to handle XML, JSON, etc. Thoughts?

mattgreen · 2013-01-09T18:11:09Z

@TheJeff : I'm not sure why empty JSON arrays are a security problem in ActionPack.

They may be a problem for AR, but but that's AR's problem, not ActionPack's.

TheBerg · 2013-01-09T18:11:11Z

Oh, ActionController::Parameters.new is part of StrongParameters & Rails4, not in Rails 3.

TheBerg · 2013-01-09T18:15:09Z

@mattgreen I agree, but don't see a way to fix this issue without severely crippling ActiveRecord (basically disallowing searching for nil values).

Personally, I feel like this is a per-application security vulnerability, but at the same time I respect the Rails Cores team working to make Rails secure by default.

Another way to fix this would be to extend StrongParameters so that a require or allow would make sure that that value is not an Array or Hash, but that would require people to move to StrongParameters.

mattgreen · 2013-01-09T18:27:18Z

Requiring people to use StrongParameters in Rails 4 would be an acceptable resolution to this bug.

But mutilating data that comes in to cover the persistence layer's weaknesses is ridiculous. I'm assuming this bug was an oversight.

wpeterson · 2013-01-09T18:34:30Z

+1 This will likely break things for us as well.

TheBerg · 2013-01-09T18:36:55Z

@mattgreen I believe it was an oversight, but also I don't really know of any other way to solve this issue without doing it. I am racking my brain for a solution and I just can't think of another one.

Unpakt · 2013-01-09T18:56:01Z

We've created a before filter that will fix this issue until there is a new actionpack release see the gist here:

https://gist.github.com/4495772

abuggia · 2013-01-09T19:02:46Z

+1

edawerd · 2013-01-09T19:36:33Z

+1

xdaveyx · 2013-01-09T20:10:50Z

+1

imanel · 2013-11-11T22:53:35Z

I would opt for removal of this - it's no longer necessary after permitted params were introduced.

joshwlewis · 2013-11-13T21:59:27Z

Looks like #11044 should solve this.

jasonmp85 · 2013-11-13T22:52:48Z

LOL @ milestone 4.0.2.

"Hey guys, remember that time we broke compliance with JSON in a patch release? Well we're going to fix it in a patch release, too. Hope you didn't write any hacks to get around the asinine behavior."

rafaelfranca · 2013-11-13T23:24:42Z

Like I said, the milestone doesn't mean it will be fixed on that milestone,
it is just to make sure we will revisit this issue when release that
milestone.
On Nov 13, 2013 8:52 PM, "Jason Petersen" notifications@github.com wrote:

LOL @ milestone 4.0.2.

"Hey guys, remember that time we broke compliance with JSON in a patch
release? Well we're going to fix it in a patch release, too. Hope you
didn't write any hacks to get around the asinine behavior."

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/8832#issuecomment-28442476
.

jasonmp85 · 2013-11-14T00:47:17Z

Ah. Serves me right to open my mouth before reading all the comments (though in all honestly I'm squelching this thread now because it's just been 👍s for months now).

OmarSkalli · 2013-11-14T00:51:21Z

Can a contributor add the "Regression" label to this issue please?

rafaelfranca · 2013-11-14T00:51:26Z

Agree with you we have to take a decision here. This is why I added the
milestone. I'll make sure we come with some solution before the 4.0.2
release. But it can be included only on 4.1.
On Nov 13, 2013 10:47 PM, "Jason Petersen" notifications@github.com wrote:

Ah. Serves me right to open my mouth before reading all the comments
(though in all honestly I'm squelching this thread now because it's just
been [image: 👍]s for months now).

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/8832#issuecomment-28450070
.

imanel · 2013-11-14T08:21:32Z

@rafaelfranca what is the reason for deep munge after implementation of permitted params. From what I remember the reason was passing array instead of string, which is now prevented by default...

This works around a rails bug; see rails/rails#8832 for more details

melcher · 2013-12-11T19:48:25Z

+1

mikeric · 2013-12-12T19:11:36Z

+1

luisobo · 2013-12-13T23:46:05Z

👍

vincentwoo · 2013-12-15T00:04:09Z

+1

11449 · 2013-12-17T16:26:27Z

+1

chancancode · 2013-12-20T10:50:50Z

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 stop responding with +1s and "me too"s. Those comments are only going to create noise for everyone and slow down the progress. We heard you loud and clear, and yes, we definitely care a lot about your opinion on this.

Instead, please head over to #13420 for the background and contribute your ideas. Thanks again! ❤️ 💚 💙 💛 💜

This enables PUT-ing and GET-ing of full dashboard definitions through /<slug>.json. In this API, the 'dashboards_json' database field is converted from a JSON string to a native JSON suboject of the Rails Dashboard model JSON. The full externally exposed JSON looks like this: { "name": "Test Dashboard", "created_at": "2014-11-07T00:25:57.000Z", "updated_at": "2014-11-07T00:38:23.000Z", "dashboard_json": { "globalConfig": { // ...global configuration... }, "widgets": [ // ...widget definitions... ] } } Upon PUT, the 'dashboards_json' field is validated against the JSON-schema definition in the file 'dashboard_schema.json' and then saved again as a string in a single database field. Since Rails converts JSON empty arrays to "nil" values by default, this also upgrades PromDash to Rails 4.1.1, which supports an option to turn off this automatic conversion. See the following issues about this regarding security implications (which for now we shouldn't need to worry about in PromDash): rails/rails#8832 rails/rails#13420 Further, as we're exposing the dashboard_json as an external API for the first time, this also includes a lot of naming cleanups in the JSON fields and the related code.

This enables JSON-schema-validated PUT-ing and GET-ing of full dashboard definitions through /<slug>.json. In the external part of this API, the 'dashboards_json' database field is converted from a JSON-encoded string to a native JSON subobject of the Rails Dashboard model JSON representation. The full externally exposed JSON for a dashboard looks like this: { "name": "Test Dashboard", "created_at": "2014-11-07T00:25:57.000Z", "updated_at": "2014-11-07T00:38:23.000Z", "dashboard_json": { "globalConfig": { // ...global configuration... }, "widgets": [ // ...widget definitions... ] } } Upon PUT, the 'dashboards_json' field is validated against the JSON-schema definition in the file 'dashboard_schema.json' and then saved again as a serialized string in a single database field. Since Rails converts empty JSON arrays to "nil" values by default, this also upgrades PromDash to Rails 4.1.1, which supports an option to turn off this automatic conversion. See the following issues about this regarding security implications (which for now we shouldn't need to worry about in PromDash): rails/rails#8832 rails/rails#13420 Further, as we're exposing the dashboard_json as an external API for the first time, this also includes a lot of naming cleanups in the JSON fields and the related code.

This is breaking the JSON parsing for empty arrays, which we use with the magic crowbar-deep-merge-template feature when creating the initial crowbar proposal. With deep munge enabled, we cannot remove the nagios proposal, which results in failures when nagios barclamp is not available. See: rails/rails#8832 rails/rails#13420 We can revert this once switching to rails 5 (see comments in second issue).

jeremy mentioned this issue Jan 9, 2013

Rails 3.2.11 introduces bug in JSON params parsing #8831

Closed

yalon mentioned this issue Jan 9, 2013

Rails 3.2.11 regression with non-object JSON content #8845

Closed

imanel mentioned this issue Dec 4, 2013

Remove deep munge #13157

Closed

carpeliam added a commit to pophealth/popHealth that referenced this issue Dec 5, 2013

set selected_measure_ids to [] if it's set to nil

d4cc720

This works around a rails bug; see rails/rails#8832 for more details

carpeliam added a commit to pophealth/popHealth that referenced this issue Dec 5, 2013

set selected_measure_ids to [] if it's set to nil

49cb82a

This works around a rails bug; see rails/rails#8832 for more details

carpeliam mentioned this issue Dec 5, 2013

set selected_measure_ids to [] if it's set to nil pophealth/popHealth#80

Closed

imanel mentioned this issue Dec 6, 2013

[Concept] Always require permit before fetching #13215

Closed

carpeliam added a commit to pophealth/popHealth that referenced this issue Dec 6, 2013

set selected_measure_ids to [] if it's set to nil

345a8aa

This works around a rails bug; see rails/rails#8832 for more details

chancancode closed this as completed Dec 20, 2013

waldenraines mentioned this issue Mar 4, 2014

Content Views: handle composite content views. Katello/katello#3771

Merged

rails locked and limited conversation to collaborators Aug 15, 2014

Regression with empty nested attributes in parameters #8832

Regression with empty nested attributes in parameters #8832

Comments

OmarSkalli commented Jan 9, 2013

mattgreen commented Jan 9, 2013

ienders commented Jan 9, 2013

akavi commented Jan 9, 2013

PikachuEXE commented Jan 9, 2013

OmarSkalli commented Jan 9, 2013

jeremy commented Jan 9, 2013

homakov commented Jan 9, 2013

mikebaldry commented Jan 9, 2013

nicolasblanco commented Jan 9, 2013

homakov commented Jan 9, 2013

simonx1 commented Jan 9, 2013

jeremy commented Jan 9, 2013

ceterumnet commented Jan 9, 2013

jlsync commented Jan 9, 2013

TheBerg commented Jan 9, 2013

rainulf commented Jan 9, 2013

TheBerg commented Jan 9, 2013

TheBerg commented Jan 9, 2013

mattgreen commented Jan 9, 2013

TheBerg commented Jan 9, 2013

TheBerg commented Jan 9, 2013

mattgreen commented Jan 9, 2013

wpeterson commented Jan 9, 2013

TheBerg commented Jan 9, 2013

Unpakt commented Jan 9, 2013

abuggia commented Jan 9, 2013

edawerd commented Jan 9, 2013

xdaveyx commented Jan 9, 2013

imanel commented Nov 11, 2013

joshwlewis commented Nov 13, 2013

jasonmp85 commented Nov 13, 2013

rafaelfranca commented Nov 13, 2013

jasonmp85 commented Nov 14, 2013

OmarSkalli commented Nov 14, 2013

rafaelfranca commented Nov 14, 2013

imanel commented Nov 14, 2013

melcher commented Dec 11, 2013

mikeric commented Dec 12, 2013

luisobo commented Dec 13, 2013

vincentwoo commented Dec 15, 2013

11449 commented Dec 17, 2013

chancancode commented Dec 20, 2013