Empty array becomes nil in params #13766

Closed
dnagir opened this Issue Jan 20, 2014 · 4 comments

Comments

Projects
None yet
3 participants
@dnagir
Contributor

dnagir commented Jan 20, 2014

When the following JSON data is posted to a Rails controller:

{"property_grouping":{"project_id":289,"name":"test group2","property_ids":[]}}

(NOTE the empty array).

It gets converted into the following params in the controller:

{"property_grouping"=>{"project_id"=>289, "name"=>"test group2", "property_ids"=>nil, }, "action"=>"...", "controller"=>"...", "format"=>"json"}

The main problem with it is that strong_parameters (defined as params.require(:property_grouping).permit(..., property_ids: [])) will eliminate the property_ids from the hash (because it is nil).

As a result it will be impossible to clear the collection of property_ids (or generally impossible to clear any other has_many collection).

It could be a strong_parameters' issue but it doesn't appear to me that way because the actual value that is posted ([] - empty array) gets converted into a different one (nil).

This is happening in Rails 4.1.0.beta1 but likely (not confirmed though) to happen in Rails 4.0.2 as well.

@dnagir

This comment has been minimized.

Show comment
Hide comment
@dnagir

dnagir Jan 20, 2014

Contributor

The workaround is to permit property_ids as a scalar value when it is nil, otherwise permit as an array:

  def property_grouping_params
    allowed = [:name, :exclusive, :project_id]

    grouping_params = params[:property_grouping]
    if grouping_params.has_key?(:property_ids) and grouping_params[:property_ids].nil?
      allowed << :property_ids # property_ids is nil - permit it as a scalar value (nil is scalar and doesn't classify as array)
    else
      allowed << {property_ids: []} # permit as a normal array
    end
    params.require(:property_grouping).permit(*allowed)
  end
Contributor

dnagir commented Jan 20, 2014

The workaround is to permit property_ids as a scalar value when it is nil, otherwise permit as an array:

  def property_grouping_params
    allowed = [:name, :exclusive, :project_id]

    grouping_params = params[:property_grouping]
    if grouping_params.has_key?(:property_ids) and grouping_params[:property_ids].nil?
      allowed << :property_ids # property_ids is nil - permit it as a scalar value (nil is scalar and doesn't classify as array)
    else
      allowed << {property_ids: []} # permit as a normal array
    end
    params.require(:property_grouping).permit(*allowed)
  end
@bradleypriest

This comment has been minimized.

Show comment
Hide comment
@bradleypriest

bradleypriest Jan 20, 2014

Contributor

@dnagir this is caused by deep_munge. Check out the discussion at #13420

Contributor

bradleypriest commented Jan 20, 2014

@dnagir this is caused by deep_munge. Check out the discussion at #13420

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jan 20, 2014

Member

Hi @dnagir, I know this is surprising, but bare with me - this is actually a feature, not a bug ;) This has been added somewhere in the 3.2 release series in response to a security issue. We are well aware that this is problematic in some cases. I'm sorry that you ran into one of those problematic cases. We understand this sucks, and we definitely welcome solutions to make the situation better. See #13420 for details.

There is also a new config option in 4.1 (introduced in #13188) to disable this feature, but if you do choose that route please make sure you understand the security implications!

I'm closing this for now, but feel free to participate in #13420 to explore solutions to make this better!

Member

chancancode commented Jan 20, 2014

Hi @dnagir, I know this is surprising, but bare with me - this is actually a feature, not a bug ;) This has been added somewhere in the 3.2 release series in response to a security issue. We are well aware that this is problematic in some cases. I'm sorry that you ran into one of those problematic cases. We understand this sucks, and we definitely welcome solutions to make the situation better. See #13420 for details.

There is also a new config option in 4.1 (introduced in #13188) to disable this feature, but if you do choose that route please make sure you understand the security implications!

I'm closing this for now, but feel free to participate in #13420 to explore solutions to make this better!

@dnagir

This comment has been minimized.

Show comment
Hide comment
@dnagir

dnagir Jan 20, 2014

Contributor

@chancancode thanks for the explanation.

I agree with what's said in #13157 and will use disable deep munge as per #13188.

The parameters should not be touched by Rails IMO. The Rack application knows nothing about how they can be used and shouldn't make any assumption about it.

Replacing empty arrays with nil is a bit silly way of "fixing" the security issues introduced by other layers (AR).
The developer should know well enough not to blindly trust the user input. And the strong_parameters are already helping with that.

The "fix" with with deep munge fixes the wrong problem on a wrong layer and introduces too many issues that go against users' expectations and general common sense.

Contributor

dnagir commented Jan 20, 2014

@chancancode thanks for the explanation.

I agree with what's said in #13157 and will use disable deep munge as per #13188.

The parameters should not be touched by Rails IMO. The Rack application knows nothing about how they can be used and shouldn't make any assumption about it.

Replacing empty arrays with nil is a bit silly way of "fixing" the security issues introduced by other layers (AR).
The developer should know well enough not to blindly trust the user input. And the strong_parameters are already helping with that.

The "fix" with with deep munge fixes the wrong problem on a wrong layer and introduces too many issues that go against users' expectations and general common sense.

@inukshuk inukshuk referenced this issue in samvera/sufia Apr 19, 2015

Merged

Add Arkivo API integration #1022

thenickcox added a commit to WikiEducationFoundation/WikiEduDashboard that referenced this issue Dec 11, 2015

[multiline] When [] is passed for training_module_ids, set the attr a…
…ccordingly

When the API sends `[]` for training_module_ids for a block, ActiveRecord
sets it to `nil` because of the possiblity for unsafe query generation
(see
http://guides.rubyonrails.org/security.html#unsafe-query-generation).
Unfortunately, then the controller doesn't accept that as a valid param
in the whitelist (because it's expecting an array), so it ignores the
param. (See [this github
issue](rails/rails#13766 (comment))
for more details.)

This PR conditionally adds whitelisting for `[]` for a block's
training_module_ids, and thus allows clearing them out.

Closes #525.

thenickcox added a commit to WikiEducationFoundation/WikiEduDashboard that referenced this issue Dec 11, 2015

[multiline] When [] is passed for training_module_ids, set the attr a…
…ccordingly

When the API sends `[]` for training_module_ids for a block, ActiveRecord
sets it to `nil` because of the possiblity for unsafe query generation
(see
http://guides.rubyonrails.org/security.html#unsafe-query-generation).
Unfortunately, then the controller doesn't accept that as a valid param
in the whitelist (because it's expecting an array), so it ignores the
param. (See [this github
issue](rails/rails#13766 (comment))
for more details.)

This PR conditionally adds whitelisting for `[]` for a block's
training_module_ids, and thus allows clearing them out.

Closes #525.

@blevine blevine referenced this issue in rmosolgo/graphql-ruby Oct 10, 2017

Closed

Passing empty array in a variable returns an error #986

darraghbuckley added a commit to pledgeaction/web that referenced this issue Apr 27, 2018

Update gems to clear security warnings.
Updates haven't been taken in a while and there are a few security warnings.

This change updates gems.

It also:

- Remove gem version pinning.

- Removes `config.active_record.raise_in_transactional_callbacks` as it's no
longer and option.

- Updates rails_test tests using named parameters.

Beyond that, the need for providing more than just an empty array in the
controller test is here:

rails/rails#13766

@darraghbuckley darraghbuckley referenced this issue in pledgeaction/web Apr 27, 2018

Merged

Update gems to clear security warnings. #30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment