Parse '{"person":[]}' JSON/XML as {'person' => []} #8862

Closed
wants to merge 1 commit into
from

Projects

None yet
@ndbroadbent
Contributor

We're also affected by #8832, so this is an attempt at solving that issue. Here's how JSON/XML will now be 'munged':

JSONHash
{"person":[]} {'person' => []}
{"person":[null]} {'person' => nil}
{"person":[null, null, ...]} {'person' => nil}
{"person":["foo", null]} {'person' => ["foo"]}

I know there must be a reason for replacing [] with nil, but I can't figure out how this would be a security risk. Could someone please help me understand the 'args injection' problem?

FWIW, the AR tests are still passing at d5cd97b#L7R27.

@TheBerg
TheBerg commented Jan 10, 2013

This still exposes an issue. Say you have this:

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

and pass in {"token": []} then params[:token].nil? would be false, but User.find_by_token(params[:token]) would run SELECTusers.* FROMusersWHEREpeople.tokenIN (NULL) LIMIT 1.

So I think this still needs a patch to ActiveRecord. I don't think that User.find_by_token([]) should ever be valid.

That is what the security issue (CVE-2013-0155) is about: https://bugzilla.redhat.com/show_bug.cgi?id=892866

@TheBerg
TheBerg commented Jan 10, 2013

Sorry, my bad. Active Record is fixed (although it still does a SQL query which is weird):

[4] pry(main)> User.find_by_token([])
  Person Load (0.6ms)  SELECT `users`.* FROM `users` WHERE `users`.`token` IN (NULL) LIMIT 1
=> nil
@ndbroadbent
Contributor

OK, thanks for the explanation! But isn't that already tested for in ActiveRecord? d5cd97b#L7R32

def test_where_with_table_name_and_empty_array
  assert_equal 0, Post.where(:id => []).count
end
@TheBerg
TheBerg commented Jan 10, 2013

+1

@jmccartie
Contributor

+1

@zhubert
Contributor
zhubert commented Jan 10, 2013

+1

@TheBerg
TheBerg commented Jan 10, 2013

@jeremy please check out this fix for regression #8832

@antoinegrant

+1

@vjt
vjt commented on 43a732e Jan 10, 2013

👍!

@varchar
varchar commented Jan 10, 2013

great table, describing the bug!

@joe1chen

+1

@imanel
Contributor
imanel commented Jan 10, 2013

I believe that correct code in this situation should use blank? instead of nil?:

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

This will remove problem with passing empty array and empty hash. Unfortunately it will not remove problem with array with nil inside.

The bigger problem is that for every situation where you would like to disallow nil in array there is another situation where nil is perfectly valid value and should be allowed. Unfortunately this is application-specific problem and should be handled differently in every situation. We might want to introduce another method that will munge params, but definitely it should not be on by default(ask all API-based applications that were broken yesterday why).

That's why I believe we should abandon this solution, do revert(as in #8870) and try to find easy to understand(and apply) method that will be optional instead of default.

@TheBerg
TheBerg commented Jan 10, 2013

If you throw this into an initializer it will apply this fix until something is in Rails:

module ActionDispatch
  class Request < Rack::Request
    def deep_munge(hash)
      hash.each do |k, v|
        case v
        when Array
          if v.size > 0 && v.all?(&:nil?)
            hash[k] = nil
            next
          end
          v.grep(Hash) { |x| deep_munge(x) }
          v.compact!
        when Hash
          deep_munge(v)
        end
      end

      hash
    end
  end
end

If you still need {"test": [nil]} to work, it won't but {"test": []} will, which was enough of a fix for my applications.

@gnufied
Contributor
gnufied commented Jan 17, 2013

Any word on merging this into 3-2-stable? @rafaelfranca already kinda confirmed (and thanks for that!), but it will be awesome if this gets merged and we have a new release.

@ndbroadbent
Contributor

Just mentioning that our app is still broken until this (or an alternative) is merged. In the meantime, we've included this patch as an initializer: https://gist.github.com/ndbroadbent/4758944

@antoinegrant

Still broken in our app as well!

@parndt
Contributor
parndt commented Apr 14, 2013

Seems like this should be merged. 👍

@daviddavis daviddavis referenced this pull request in Katello/katello May 20, 2013
Merged

959895 - Handling nil for products #2330

@mikeric
mikeric commented Jun 21, 2013

+1

@jbeard4
jbeard4 commented Jul 7, 2013

+1

@robin850
Member

Hello world!

This pull request is targeting the 3-2-stable branch but 3.2.x is not under maintenance anymore so I'm going to close it.

@ndbroadbent : Thank you for the pull request! As far as I can see, this is still a problem on Rails 4.0.0. Could you reopen this pull request against the master branch please ? ❤️ It would be awesome

Thank you everyone.

@robin850 robin850 closed this Oct 28, 2013
@imanel imanel referenced this pull request Dec 4, 2013
Closed

Remove deep munge #13157

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