[Rails-core] strong parameters safety issue enables accidental mass assignment
Hello, Recently I was looking into upgrading one of our Rails 3.2 apps to use strong_parameters. I encountered what seems like a flaw to me and I would like to spark discussion about this, hoping for personal learning and potential improvement of the rails framework. The switch from protected attributes to strong parameters looks from the outside like access control for mass assignment moves from the model to the controllers (where, I agree, they belong). However looking at the implementation of strong parameters, it seems easy for developers to accidentally introduce bugs. How is that? @params in a controller is of type ActiveController::Parameters, which inherits from HashWithIndifferentAccess which in turn inherits from a Ruby Hash. Strong Parameters extends ActiveController::Parameters to have a 'permitted?' function, essentially acting as the whitelist of permitted parameters for mass assignment [1]. Strong Parameters also overrides ActiveRecords sanitize_for_mass_assignment function, where it checks if the attributes respond to 'permitted?'. The implementation assumes that if the attributes Hash does not respond to 'permitted?' the mass assignment decision should be deferred to ActiveRecord. If it responds to 'permitted?' and the mass assigned attributes are not on the whitelist it will raise an exception [2]. The 'permitted?' function here acts as a weird capability [3]. It is weird because a capability that is not present will usually deny an action (mass assignment) while here the action gets permitted in the absence of the capability. Why does that matter? It matters because it is possible for a developer to accidentally lose that capability accidentally very easily on the way from the controller (where permit happened and the capability gets created) to the model (where the capability gets used). This loss does happen silently and effectively disables mass assignment protection. How does that happen? The only class aware of the 'permitted?' capability is ActiveController::Parameters, if we call a method that is not aware of the capability it can get lost as a side effect: class SomeModel ActiveRecord::Base #fields :name, :protected_secret_field include ActiveModel::ForbiddenAttributesProtection end #imagine a request w/ params = {'name' = 1, 'protected_secret_field' = 2}: params.reject!{|k,_|['controller', 'action'].include? k} params.permit(:name) SomeModel.new(params) #Exception, world is OK SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten symbolize_keys returns an object of type Hash, that no longer has the 'permitted?' function, so strong parameter protection is effectively disabled. Both of these patterns are found quite a bit in our codebases - often in a not such simple form though. In some sense the problem is, that there is non-framework code on the codepath between where the whitelist is defined and where it is used, and it is easy to accidentally lose the whitelist which disables mass assignment protection. This problem did not exist with protected attributes, since there was no codepath. Using 'attr_accessible :name' on the model would have prevented these problems, however I am under the impression that strong_parameters is to replace this old way. Am I the only one worried about this (I have not found any discussion online) or is this a known problem? If it is known, what is the proposed solution? I understand that developers need to take care when handling user input, and such should not call e.g. symbolize_keys. But the same is true for mass assignment vulnerabilities as a whole - in a world where programmers do not make mistakes mass assignment errors do not exist. A possible solution could be to not attach this capability onto the parameters Hash but store it separately and then retrieve it at mass assignment time - so it is not the developers responsibility to babysit it. In an alternative implementation 'permit/permit!' could be used to declare a list of mass assignable parameters for a given action in a controller (optionally on a per model basis, default empty list) and store it on the request or as a thread local variable. The mass_assignment part on the model would then read this variable and decide if the mass assignment should be allowed or not. This way the capability/whitelist can not be accidentally lost by transforming the parameters hash anymore. best, Johannes [1] https://github.com/rails/strong_parameters/blob/master/lib/action_controller/parameters.rb [2] https://github.com/rails/strong_parameters/blob/master/lib/active_model/forbidden_attributes_protection.rb [3] http://en.wikipedia.org/wiki/Capability-based_security -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
Generally speaking I believe developers should be careful/responsible for handling what they are sending to their models for mass assignment, and there's where strong params help. The ideal solution indeed would be for Parameters not to inherit from Hash, which is something Rails will likely be changing in the near future. This would avoid people calling methods that are inherited from Hash in the Parameters object, such as symbolize_keys. It won't avoid people actually converting the Parameters object into a Hash with, say, a to_h/to_hash call, but at least that would make it more explicit. Thanks for sharing your thoughts on that. On Wed, Aug 6, 2014 at 1:51 PM, johannes.schlumber...@appfolio.com wrote: Hello, Recently I was looking into upgrading one of our Rails 3.2 apps to use strong_parameters. I encountered what seems like a flaw to me and I would like to spark discussion about this, hoping for personal learning and potential improvement of the rails framework. The switch from protected attributes to strong parameters looks from the outside like access control for mass assignment moves from the model to the controllers (where, I agree, they belong). However looking at the implementation of strong parameters, it seems easy for developers to accidentally introduce bugs. How is that? @params in a controller is of type ActiveController::Parameters, which inherits from HashWithIndifferentAccess which in turn inherits from a Ruby Hash. Strong Parameters extends ActiveController::Parameters to have a 'permitted?' function, essentially acting as the whitelist of permitted parameters for mass assignment [1]. Strong Parameters also overrides ActiveRecords sanitize_for_mass_assignment function, where it checks if the attributes respond to 'permitted?'. The implementation assumes that if the attributes Hash does not respond to 'permitted?' the mass assignment decision should be deferred to ActiveRecord. If it responds to 'permitted?' and the mass assigned attributes are not on the whitelist it will raise an exception [2]. The 'permitted?' function here acts as a weird capability [3]. It is weird because a capability that is not present will usually deny an action (mass assignment) while here the action gets permitted in the absence of the capability. Why does that matter? It matters because it is possible for a developer to accidentally lose that capability accidentally very easily on the way from the controller (where permit happened and the capability gets created) to the model (where the capability gets used). This loss does happen silently and effectively disables mass assignment protection. How does that happen? The only class aware of the 'permitted?' capability is ActiveController::Parameters, if we call a method that is not aware of the capability it can get lost as a side effect: class SomeModel ActiveRecord::Base #fields :name, :protected_secret_field include ActiveModel::ForbiddenAttributesProtection end #imagine a request w/ params = {'name' = 1, 'protected_secret_field' = 2}: params.reject!{|k,_|['controller', 'action'].include? k} params.permit(:name) SomeModel.new(params) #Exception, world is OK SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten symbolize_keys returns an object of type Hash, that no longer has the 'permitted?' function, so strong parameter protection is effectively disabled. Both of these patterns are found quite a bit in our codebases - often in a not such simple form though. In some sense the problem is, that there is non-framework code on the codepath between where the whitelist is defined and where it is used, and it is easy to accidentally lose the whitelist which disables mass assignment protection. This problem did not exist with protected attributes, since there was no codepath. Using 'attr_accessible :name' on the model would have prevented these problems, however I am under the impression that strong_parameters is to replace this old way. Am I the only one worried about this (I have not found any discussion online) or is this a known problem? If it is known, what is the proposed solution? I understand that developers need to take care when handling user input, and such should not call e.g. symbolize_keys. But the same is true for mass assignment vulnerabilities as a whole - in a world where programmers do not make mistakes mass assignment errors do not exist. A possible solution could be to not attach this capability onto the parameters Hash but store it separately and then retrieve it at mass assignment time - so it is not the developers responsibility to babysit it. In an alternative implementation 'permit/permit!' could be used to declare a list of mass assignable parameters for a given action in a controller (optionally on a per model basis, default empty list) and store it on the request or as a thread local variable. The
[Rails-core] Re: reality of fixtures as a default?
Hi, Is there anybody out there who is really using fixtures exclusively? And if so, how do you deal with scenarios when there is a need for multiple fixture sets? (datasets that are mutually exclusive) Or is that the part where the more popular factories come in? For those who are wondering why I even bother, I am in a situation where I have plenty of real data to test and debug with, and if these could be extracted to fixtures which are maintainable, there might be some value in this approach for integration tests. Thanks -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
On Aug 6, 2014, at 12:51 PM, johannes.schlumber...@appfolio.com wrote: [snip] Why does that matter? It matters because it is possible for a developer to accidentally lose that capability accidentally very easily on the way from the controller (where permit happened and the capability gets created) to the model (where the capability gets used). This loss does happen silently and effectively disables mass assignment protection. How does that happen? The only class aware of the 'permitted?' capability is ActiveController::Parameters, if we call a method that is not aware of the capability it can get lost as a side effect: class SomeModel ActiveRecord::Base #fields :name, :protected_secret_field include ActiveModel::ForbiddenAttributesProtection end #imagine a request w/ params = {'name' = 1, 'protected_secret_field' = 2}: params.reject!{|k,_|['controller', 'action'].include? k} params.permit(:name) SomeModel.new(params) #Exception, world is OK SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten This is a bug in `symbolize_keys`. It appears to have been fixed (accidentally? on purpose?) on master: https://github.com/rails/rails/commit/f1bad130d0c9bd77c94e43b696adca56c46a66aa by starting the loop with `self.class.new` instead of `{}`. There’s some additional future changes coming up in Ruby 2.2.0 with methods like `reject`: https://www.ruby-lang.org/en/news/2014/03/10/regression-of-hash-reject-in-ruby-2-1-1/ that seem likely to further complicate the situation. —Matt Jones signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [Rails-core] Re: reality of fixtures as a default?
This list is for rails core discussion, so this question is probably better suited for rubyonrails-talk. But I'll answer anyway (it would be great to get something like fixture_builder into Rails someday, none of the alternatives are good IMO): I use factories + fixtures via https://github.com/rdy/fixture_builder As for mutually exclusive datasets, depending on your app, maybe they can co-exist in the same fixture set. E.g., have them be separate accounts or projects. If not, you can pass flags into fixture_builder to tell it which set of fixtures to build. -- Chad On Wed, Aug 6, 2014 at 5:16 AM, Pruss Wan pruss...@gmail.com wrote: Hi, Is there anybody out there who is really using fixtures exclusively? And if so, how do you deal with scenarios when there is a need for multiple fixture sets? (datasets that are mutually exclusive) Or is that the part where the more popular factories come in? For those who are wondering why I even bother, I am in a situation where I have plenty of real data to test and debug with, and if these could be extracted to fixtures which are maintainable, there might be some value in this approach for integration tests. Thanks -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Re: reality of fixtures as a default?
We have been using fixtures lately in some projects at Plataformatec and it's been work great. The ability to have a pre-defined set of fixtures and modify them for each test works very well for most of our scenarios, and when it does not or it gets a little complex we build helpers to generate the scenario we need (generally by modifying existing fixtures, or generating new records based off the current fixtures attributes). Plus being able to use transactions most of the time is really good. One of the most common problem I've seen with fixtures is that people keep growing and growing them by adding new records every time a new scenario appears - and I think that's somewhat related to the factores background, where adding a new trait was just easy and didn't impact the world. In case of fixtures it's generally better imo to modify your set rather than adding new records, so that you can have a small and very controlled world all the time, and work with it. Anyway, as Chad said this question is more suited for the talk mailing list, so I'd recommend you posting there :) On Wed, Aug 6, 2014 at 5:20 PM, Chad Woolley thewoolley...@gmail.com wrote: This list is for rails core discussion, so this question is probably better suited for rubyonrails-talk. But I'll answer anyway (it would be great to get something like fixture_builder into Rails someday, none of the alternatives are good IMO): I use factories + fixtures via https://github.com/rdy/fixture_builder As for mutually exclusive datasets, depending on your app, maybe they can co-exist in the same fixture set. E.g., have them be separate accounts or projects. If not, you can pass flags into fixture_builder to tell it which set of fixtures to build. -- Chad On Wed, Aug 6, 2014 at 5:16 AM, Pruss Wan pruss...@gmail.com wrote: Hi, Is there anybody out there who is really using fixtures exclusively? And if so, how do you deal with scenarios when there is a need for multiple fixture sets? (datasets that are mutually exclusive) Or is that the part where the more popular factories come in? For those who are wondering why I even bother, I am in a situation where I have plenty of real data to test and debug with, and if these could be extracted to fixtures which are maintainable, there might be some value in this approach for integration tests. Thanks -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- At. Carlos Antonio -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.