Don't convert empty arrays to nils when deep munging params #16924

Merged
merged 1 commit into from Dec 15, 2014

Conversation

Projects
None yet
8 participants
@Sinjo
Contributor

Sinjo commented Sep 15, 2014

This commit tweaks the behaviour of deep_munge in light of changes made to improve security in ActiveRecord.

Previously, when an empty array was passed as an argument to a where or find_by query, it would generate SQL with an IS NULL clause. This lead to vulnerabilities due to records being returned in cases where app code didn't expect it. These are documented in:

Now (example using Rails 4.1.5 and Postgres 9.2.8), ActiveRecord generates a query like:

[1] pry(main)> User.find_by_email([])

---
  User Load (29.3ms)  SELECT  "users".* FROM "users"  WHERE 1=0 LIMIT 1

which never returns any rows.

This new behaviour makes it possible to change the behaviour of deep_munge in what seems like a preferable way. While nils are still stripped from arrays in params, empty arrays won't be converted into nil. Apps would no longer need to work around this behaviour by re-parsing JSON if they want to distinguish between nil and [].

Conveniently, this would fix rails/strong_parameters#192 as well.

I realise it wouldn't be appropriate to target this change at 4.2 this late in the day, but it seems like it would be a nice improvement for 4.3.

@spastorino

This comment has been minimized.

Show comment
Hide comment
Member

spastorino commented Sep 15, 2014

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Sep 15, 2014

Member

It would need careful review, just in case and I'd do it for 5.0. There's not going to be a 4.3 version.

Member

spastorino commented Sep 15, 2014

It would need careful review, just in case and I'd do it for 5.0. There's not going to be a 4.3 version.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Sep 15, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 15, 2014

Member

Related with #13420

Member

rafaelfranca commented Sep 15, 2014

Related with #13420

@Sinjo

This comment has been minimized.

Show comment
Hide comment
@Sinjo

Sinjo Sep 15, 2014

Contributor

Glad to see this get the thumbs up, and agreed on the review. Feels very much worth making sure there are automated tests around ActiveRecord in the relevant places too (and have them run against all officially supported database adapters).

Contributor

Sinjo commented Sep 15, 2014

Glad to see this get the thumbs up, and agreed on the review. Feels very much worth making sure there are automated tests around ActiveRecord in the relevant places too (and have them run against all officially supported database adapters).

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Sep 15, 2014

Member

This change looks fine to me, however I think we should also revisit the munging of [nil] (not here, some other pull request).

It was initially a suprising API which when combined with .blank? ended up doing crazy things, but perhaps we can simply document that behaviour now and move on?

Member

NZKoz commented Sep 15, 2014

This change looks fine to me, however I think we should also revisit the munging of [nil] (not here, some other pull request).

It was initially a suprising API which when combined with .blank? ended up doing crazy things, but perhaps we can simply document that behaviour now and move on?

@Sinjo

This comment has been minimized.

Show comment
Hide comment
@Sinjo

Sinjo Dec 7, 2014

Contributor

Just spotted master is open for 5.0.0 work, so I've rebased against that.

Is this good for merge now? Let me know if there's anything you'd like changed.

Contributor

Sinjo commented Dec 7, 2014

Just spotted master is open for 5.0.0 work, so I've rebased against that.

Is this good for merge now? Let me know if there's anything you'd like changed.

@Sinjo

This comment has been minimized.

Show comment
Hide comment
@Sinjo

Sinjo Dec 15, 2014

Contributor

@spastorino Any update? Keen to get this merged. Just rebased again for changelog conflicts. Let me know if there's anything else you'd like changing.

Contributor

Sinjo commented Dec 15, 2014

@spastorino Any update? Keen to get this merged. Just rebased again for changelog conflicts. Let me know if there's anything else you'd like changing.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Dec 15, 2014

Member

👍 from me!

Member

tenderlove commented Dec 15, 2014

👍 from me!

chancancode added a commit that referenced this pull request Dec 15, 2014

Merge pull request #16924 from Sinjo/params-deep-munge-empty-array
Don't convert empty arrays to nils when deep munging params

@chancancode chancancode merged commit 488aefe into rails:master Dec 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@mtparet

This comment has been minimized.

Show comment
Hide comment

mtparet commented Feb 25, 2015

❤️

@mehulkar

This comment has been minimized.

Show comment
Hide comment
@mehulkar

mehulkar Mar 24, 2015

Is this patch available in any of 4.x? We're running 4.0.4 and wouldn't mind updating a minor revision to get this.

Is this patch available in any of 4.x? We're running 4.0.4 and wouldn't mind updating a minor revision to get this.

@Sinjo

This comment has been minimized.

Show comment
Hide comment
@Sinjo

Sinjo Mar 24, 2015

Contributor

This went in for 5.0, as it was a breaking change and 4.2 was too close to release.

Contributor

Sinjo commented Mar 24, 2015

This went in for 5.0, as it was a breaking change and 4.2 was too close to release.

@jonatack jonatack referenced this pull request in activerecord-hackery/ransack Mar 24, 2015

Closed

searching with _in predicate if the argument is empty returns All records #524

sikachu added a commit to rails/actionpack-xml_parser that referenced this pull request Apr 17, 2015

Update assertion on nil stripping
The behavior of `deep_munge` was changed in rails/rails#16924 for
security reason, so we should expect a different outcome when running
against Rails 5.0+.

sikachu added a commit to rails/actionpack-xml_parser that referenced this pull request Apr 17, 2015

Update assertion on nil stripping
The behavior of `deep_munge` was changed in rails/rails#16924 for
security reason, so we should expect a different outcome when running
against Rails 5.0+.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

@janvarljen janvarljen referenced this pull request in rails-api/active_model_serializers Feb 26, 2016

Closed

Deserializing Empty HasMany Relationship Array Params in JSONAPI Adapter #1532

@namack namack referenced this pull request in rails-api/active_model_serializers Feb 26, 2016

Closed

Deserialize empty hasmany #1541

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