Did rails used to do this?

uhhhh.md

Using rails 3.2.2, btw.

1.9.3-p125 :005 > a = Article.first
 => #<Article id: 1, title: "Sample Article Title", body: "This is the text for my article, woo hoo!", created_at: "2012-04-13 00:16:04", updated_at: "2012-04-13 00:16:04"> 
1.9.3-p125 :006 > a.comments
 => [four comments] 
1.9.3-p125 :007 > c = a.comments.new
 => #<Comment id: nil, article_id: 1, author_name: nil, body: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :008 > a.comments
 => [four comments, #<Comment id: nil, article_id: 1, author_name: nil, body: nil, created_at: nil, updated_at: nil>] 

This becomes a problem when you want to do something like this:

  def show
    @article = Article.find(params[:id])
    @comment = @article.comments.new
  end

and in the view

<%= render :partial => 'comment', :collection => @article.comments %>
<h3>Post a Comment</h3>

<%= form_for @comment do |f| %>
...

Because you have to make the comment for the form, but then it tries to render it in the collection.

Is this new? Have I just never noticed it?

That always happened unfortunately. try something like :collection => @article.comments.reject(&:new_record?)

Or don't build the comment with the article attribute.

What if you do: Comment.new(:article => @Article) ?

@claco in that case it wont show up

@oscardelben I wonder how I've never run into it before.

@claco doesn't work, and if I do :article_id => @article.id, it throws mass assignment errors.

The simplest way to accomplish this seems to be

    @comment = Comment.new
    @comment.article_id = @article.id

I think you can do

:collection => @article.comments.scoped

Confirmed 3.1.4 does the same.

@claco that won't work if you're properly protecting against mass-assignment.

@steveklabnik Now THAT seems broken to me. It works in other cases of relations.

I do thing.relation.create(:article => @articleinstance) all the time. Maybe it's a "new" thing.

@steveklabnik FWIW, I usually skirt the issue by using a presenter which builds a blank comment and associates it via explicit assignment. Ugh.

@stevenharman Huh? That's not a mass assignment at all. I mean, it's not like @Article is coming from params[:article]

regardless if mass assignment protection, I'd expect Comment.new(:article => @Article) to just do :article_id = @article_id and walk away. Doesn't seem unsafe.

@stevenharman I guess this is why we need to mark variables as tainted/untainted.

Comment.new(:article => params[:article]) # params is tainted. no mass assignment!
Comment.new(:article => @article_i_loaded_myself) #untained. Proceed!!!!

@claco, that's mass assigment, you could have passed that from user input.

@steveklabnik I found it while making my first blgo platform in rails back in Rails 1 or 2. If you use Model.associations.build it will get included in your associations objects, unless you filter it out.

@oscardelben is there a doc covering this? Because http://guides.rubyonrails.org/security.html#mass-assignment doesn't appear to mention the act of pass an object in as a relation. IT only focuses on hash attributes from params.

@claco,

rails doesn't know if the hash you're passing comes from params or not, it just sees a hash. You can take a look at this article I wrote about mass assignment internals. And I know this is confusing but I hope it'll make sense to you!

@oscardelben

Thanks for the link!

So, in theory, now that it's empty whitelist, you could in fact probably whitelist :article, then Comment.new(:article => @Article) would work?

That's probably defeating the purpose of course. I'm just surprised that something that used to work falls into the whitelist restrictions because :article is not an attribute in the the context of self.attributes, it's a relationship assignment akin to Comment.new.article = @Article.

Also: See the Step 1: ...Taint section of this:

http://jkfill.com/2012/03/10/preventing-mass-assignment-injection-in-rails/

@claco I'm not sure I follow that (late here :)). In any case, you can always do this:

Comment.new({:article => @article)}, :without_protection => true)

@technoweenie wrote a great blog post recently that explains how Github approached the issue.