Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord adapter bypasses strong_parameters #42

Open
lsimoneau opened this issue Aug 28, 2014 · 3 comments
Open

ActiveRecord adapter bypasses strong_parameters #42

lsimoneau opened this issue Aug 28, 2014 · 3 comments

Comments

@lsimoneau
Copy link

ActiveRecord 4.1.5 addressed a security vulnerability whereby parameters passed to where were not sanitized, so cases where where was used in conjunction with create could potentially allow injecting arbitrary attributes if the conditions hash was user-specified: rails/rails@9456990

orm_adapter's find_all method, because it uses conditions_to_fields and recreates a conditions hash from scratch, discards the original hash, which may have been a strong parameters hash, in which case that information is now lost.

This may not actually be an issue, since vulnerable code would need to be depending on both orm_adapter's and ActiveRecord's API (chaining create onto the return value of find_all), which defeats the purpose of using orm_adapter in the first place, but I thought it worth mentioning, since it's a somewhat surprising behaviour that's different from what ActiveRecord does.

I noticed this inconsistency in an override to a Devise controller that started erroring with ForbiddenAttributes when I updated to Rails 4.1.5. The original Devise controller used find_first and so bypassed strong parameters, while my version used Rails' where().first and threw the error.

Do you think it's worth updating orm_adapter to preserve the strong params information, either as a security concern or just for consistency of behaviour with the equivalent ActiveRecord APIs?

@ianwhite
Copy link
Owner

Hi Louis, thanks for writing this up.

I see no harm in changing conditions_to_fields to return an object of the same type as the conditions parameter, basically a dup, but with the association keys added, and the association objects removed. Do you think that would preserve the strong parameter information?

@lsimoneau
Copy link
Author

Yeah, I think a dup with a bit of extra massaging as you've described would do the trick. Let me see what I can put together, I'll send a PR (most likely tomorrow, it's pretty late here).

@ianwhite
Copy link
Owner

👍 Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants