This project is archived and is in readonly mode.
ActiveRecord Association Proxy/Collection create method incorrectly merges attributes
Reported by C. Bedard | November 29th, 2010 @ 08:58 PM
Given the following associated models:
class Client < ActiveRecord::Base
has_many :orders
end
class Order < ActiveRecord::Base
belongs_to :client
end
And let's say I have an orders controller in which a Client creates their orders:
class OrdersController < ApplicationController
def create
@client.orders.create(params[:order])
end
end
This mass-assignment, in Rails 2.X.X, would always ensure that
the Order Object belonged to the association owner, by forcing the
foreign key (client_id) to that of
@client.
With Rails 3.0.3, the behavior has changed, making it possible
to inject a foreign key simply by setting it. So, in this example,
if params[:order] happens to have a
:client_id key, that value will override the
associations's owner id. In other words, the AR scope that is
specified by @client.orders doesn't enforce itself
when creating associated objects.
This may also represent a "security" vulnerability, as anyone can easily infer the foreign key column name (Rails makes it pretty easy) and inject a value submitted in a form, that can create objects belonging to other objects that should not be acessible in this way.
Also, calling @client.orders.build(params[:order])
does enforce the correct scope attributes (as would be
expected).
I am not familiar with the internals of the new AR querying
interface, but having investigated a bit, it all seems to originate
in ActiveRecord::Base#with_scope, where improper
merging of attributes occurs with construct_scope attributes.
Comments and changes to this ticket
-
C. Bedard November 30th, 2010 @ 07:59 PM
- Tag changed from “activerecord rails3, associations, build, create, scope, with_scope” to “activerecord rails3, associations, build, create, patch, scope, with_scope”
More on this issue:
I initially thought the problem was in
ActiveRecord::Base#with_scopebut now after more digging, I found out the problem originates inActiveRecord::Base#initialize, where 2 lines should be reversed in order for scope attributes to be assigned after the attributes Hash that is received as a parameter. Currently the method goes like this:def initialize(attributes = nil) .... .... ensure_proper_type # THE FOLLOWING 2 LINES SHOULD BE REVERSED! populate_with_current_scope_attributes self.attributes = attributes unless attributes.nil? result = yield self if block_given? _run_initialize_callbacks result endThe reason is simple: as it is right now, the scope attributes are first assigned, but then attributes are overwritten with the attributes Hash passed as a parameter. Reversing those to lines solves the problem.
Attached is the patch acting on
ActiveRecord::Base#initialize, which simply assigns scope attributes after the attributes parameter, therby enforcing the scope attributes over the parameter attributes.This is, by the way, the way it was done in Rails 2. Maybe there is a reason for this in Rails 3, but it is not readily apparent.
-
Mike Ragalie December 4th, 2010 @ 07:28 PM
I confirm that this bug exists in edge. And it is potentially a problem, since I can see someone doing something like this:
post = Post.where(:title => "My Favorite Things") post_dup = Post.where(:title => "My Favorite Thingz") comment = post_dup.comments.first post.comments.create(comment.attributes)However, the above patch breaks other AR functionality (some of the tests fail) so I created tests for the problem and I've attached a new fix that passes all tests. In short, the build methods on has_one/has_many call #set_belongs_to_association_for but the create methods do not. I refactored the has_one create methods to operate similarly to the has_many create methods and added in a call to #set_belongs_to_association_for for both association types.
-
Jesse Storimer January 29th, 2011 @ 04:01 AM
+1 for Mike's patch. I definitely agree that this is unexpected behaviour and might raise a security eyebrow.
-
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
C. Bedard