Rails - Jan 2007 - cattr_accessor and ActiveRecord attribute mass assignment

Looks like cattr_accessors, defined for ActiveRecord::Base can be set
through mass assignment of attributes for new model instances.
Following example uses table "model (id serial, name text)", and with
script/console :
>> class Model < ActiveRecord::Base; end
=> nil
>> ActiveRecord::Base.allow_concurrency
=> false
>> m = Model.new(''allow_concurrency'' =>
''true'', ''colorize_logging'' =>
''oh yes!'', ''name'' =>
''try001'')
=> #<Model:0xb75f63f8
@attributes={"name"=>"try001"},
@new_record=true>
>> ActiveRecord::Base.allow_concurrency
=> "true"

I wonder, if this is a bug, or just hidden feature of
"cattr_accessor"?

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/25/07, Lugovoi Nikolai
<meadow.nnick-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
>
> Looks like cattr_accessors, defined for ActiveRecord::Base can be set
> through mass assignment of attributes for new model instances.
> Following example uses table "model (id serial, name text)", and
with
> script/console :
> >> class Model < ActiveRecord::Base; end
> => nil
> >> ActiveRecord::Base.allow_concurrency
> => false
> >> m = Model.new(''allow_concurrency'' =>
''true'', ''colorize_logging'' =>
> ''oh yes!'', ''name'' =>
''try001'')
> => #<Model:0xb75f63f8
@attributes={"name"=>"try001"}, @new_record=true>
> >> ActiveRecord::Base.allow_concurrency
> => "true"
>
> I wonder, if this is a bug, or just hidden feature of
"cattr_accessor"?
It''s a hidden feature of any writer method.  Use attr_protected or
attr_accessible to set which may be set through a mass assignment.

http://rails.rubyonrails.org/classes/ActiveRecord/Base.html#M001003

-- 
Rick Olson
http://weblog.techno-weenie.net
http://mephistoblog.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

2007/1/25, Rick Olson
<technoweenie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>
> It''s a hidden feature of any writer method.  Use attr_protected or
> attr_accessible to set which may be set through a mass assignment.
>

Ok, it''s a hidden feature of all writer methods.
But not adding fields, defined as "cattr_accessor" in
ActiveRecord::Base
to attributes_protected_by_default is, IMHO, ugly bug.

As using Model.create(params[:model]) or build(params[:item])
isn''t (yet?) considered bad practice, and is, probably, quite
widespread,
one can just add, for example, "comment[verification_timeout]=none" to
post parameters
and turn server to permanent error state, filling log with:

ArgumentError (comparison of Fixnum with String failed):
   
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:92:in
`>''
   
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:92:in
`verify!''
   
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:107:in
`verify_active_connections!''
   
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:106:in
`each_value''
   
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:106:in
`verify_active_connections!''
    .//vendor/rails/railties/lib/dispatcher.rb:111:in
`prepare_application''
[snip]
Rendering script/../config/../public/500.html (500 Error)

Shall we expect Rails 1.2.2 soon or just become paranoic and
add attr_accessible (with almost full list of columns) to all models
meanwhile? :)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/25/07, Lugovoi Nikolai
<meadow.nnick-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
>
> 2007/1/25, Rick Olson
<technoweenie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >
> > It''s a hidden feature of any writer method.  Use
attr_protected or
> > attr_accessible to set which may be set through a mass assignment.
> >
> Ok, it''s a hidden feature of all writer methods.
> But not adding fields, defined as "cattr_accessor" in
ActiveRecord::Base
> to attributes_protected_by_default is, IMHO, ugly bug.
I agree, it''s a ugly bug and a likely security issue in most Rails
apps.  It would be better to have attributes_accessable_by_default
instead, defaulting to only the tables content_columns (except primary
key and magic columns).

The Scaffolding Extensions plugin isn''t vulnerable to this because it
only update attributes that are shown on the form.
>
> As using Model.create(params[:model]) or build(params[:item])
> isn''t (yet?) considered bad practice, and is, probably, quite
widespread,
> one can just add, for example,
"comment[verification_timeout]=none" to
> post parameters
> and turn server to permanent error state, filling log with:
>
> ArgumentError (comparison of Fixnum with String failed):
>    
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:92:in
> `>''
>    
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:92:in
> `verify!''
>    
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:107:in
> `verify_active_connections!''
>    
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:106:in
> `each_value''
>    
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_specification.rb:106:in
> `verify_active_connections!''
>     .//vendor/rails/railties/lib/dispatcher.rb:111:in
`prepare_application''
> [snip]
> Rendering script/../config/../public/500.html (500 Error)
>
> Shall we expect Rails 1.2.2 soon or just become paranoic and
> add attr_accessible (with almost full list of columns) to all models
> meanwhile? :)
I certainly think this is deserving of a 1.2.2.

Jeremy

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

> I certainly think this is deserving of a 1.2.2.
PDI a patch.  I add attr_accessible to all my models anyway.  I think
that''s a good practice to get into.

-- 
Rick Olson
http://weblog.techno-weenie.net
http://mephistoblog.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

I have no patch, but I was about to write something to set
attr_accessible to a reasonable default to avoid explicitly writing
long lists of attributes.
I thought maybe I could share this code to collect ideas and opinions.
So I turned it in a plugin.

If someone is interested the download is at

http://rubyforge.org/frs/?group_id=2585

the file to download is

activerecord_auto_accessible-0.0.1.tgz

the actual code are 34 lines.
any comment is really appreciated.

Please note I haven''t yet properly tested it

Paolo

On 25/01/07, Rick Olson
<technoweenie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
>
> > I certainly think this is deserving of a 1.2.2.
>
> PDI a patch.  I add attr_accessible to all my models anyway.  I think
> that''s a good practice to get into.
>
> --
> Rick Olson
> http://weblog.techno-weenie.net
> http://mephistoblog.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/25/07, Rick Olson
<technoweenie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
>
> > I certainly think this is deserving of a 1.2.2.
>
> PDI a patch.  I add attr_accessible to all my models anyway.  I think
> that''s a good practice to get into.
Patch at http://dev.rubyonrails.org/ticket/7401.  attr_accessible is
good practice, but secure by default is a better one.

Jeremy

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Hi --

On Thu, 25 Jan 2007, Rick Olson wrote:
>
> On 1/25/07, Lugovoi Nikolai
<meadow.nnick-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> Looks like cattr_accessors, defined for ActiveRecord::Base can be set
>> through mass assignment of attributes for new model instances.
>> Following example uses table "model (id serial, name text)",
and with
>> script/console :
>>>> class Model < ActiveRecord::Base; end
>> => nil
>>>> ActiveRecord::Base.allow_concurrency
>> => false
>>>> m = Model.new(''allow_concurrency'' =>
''true'', ''colorize_logging'' =>
>> ''oh yes!'', ''name'' =>
''try001'')
>> => #<Model:0xb75f63f8
@attributes={"name"=>"try001"}, @new_record=true>
>>>> ActiveRecord::Base.allow_concurrency
>> => "true"
>>
>> I wonder, if this is a bug, or just hidden feature of
"cattr_accessor"?
>
> It''s a hidden feature of any writer method.
I''d have to agree that it''s a bug -- at least, it''s
contrary to the
documentation:

   AR::Base.new: ...valid attribute keys are determined by the column
   names of the associated table  hence you cant have attributes
   that arent part of the table columns.

and, in general, "attributes" and "all the attributes" and
such
phrases are used interchangeably in read and write contexts, without
suggesting that they''re different.

I know that attr_accessible is available (and probably underutilized
by me and others), but based on the documentation, it sounds like even
the worst/loosest case should still not pull in the cattr values.

(I know the discussion has progressed to patch-hood, etc. -- I just
now was catching up though :-)


David

-- 
Q. What is THE Ruby book for Rails developers?
A. RUBY FOR RAILS by David A. Black (http://www.manning.com/black)
    (See what readers are saying!  http://www.rubypal.com/r4rrevs.pdf)
Q. Where can I get Ruby/Rails on-site training, consulting, coaching?
A. Ruby Power and Light, LLC (http://www.rubypal.com)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/27/07, dblack-TKXtfPMJ4Ozk1uMJSBkQmQ@public.gmane.org
<dblack-TKXtfPMJ4Ozk1uMJSBkQmQ@public.gmane.org>
wrote:
> Hi --
>
> On Thu, 25 Jan 2007, Rick Olson wrote:
>
> >
> > On 1/25/07, Lugovoi Nikolai
<meadow.nnick-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> Looks like cattr_accessors, defined for ActiveRecord::Base can be
set
> >> through mass assignment of attributes for new model instances.
> >> Following example uses table "model (id serial, name
text)", and with
> >> script/console :
> >>>> class Model < ActiveRecord::Base; end
> >> => nil
> >>>> ActiveRecord::Base.allow_concurrency
> >> => false
> >>>> m = Model.new(''allow_concurrency'' =>
''true'', ''colorize_logging'' =>
> >> ''oh yes!'', ''name'' =>
''try001'')
> >> => #<Model:0xb75f63f8
@attributes={"name"=>"try001"}, @new_record=true>
> >>>> ActiveRecord::Base.allow_concurrency
> >> => "true"
> >>
> >> I wonder, if this is a bug, or just hidden feature of
"cattr_accessor"?
> >
> > It''s a hidden feature of any writer method.
>
> I''d have to agree that it''s a bug -- at least,
it''s contrary to the
> documentation:
>
>    AR::Base.new: ...valid attribute keys are determined by the column
>    names of the associated table — hence you can''t have attributes
>    that aren''t part of the table columns.
>
> and, in general, "attributes" and "all the attributes"
and such
> phrases are used interchangeably in read and write contexts, without
> suggesting that they''re different.
>
> I know that attr_accessible is available (and probably underutilized
> by me and others), but based on the documentation, it sounds like even
> the worst/loosest case should still not pull in the cattr values.
>
> (I know the discussion has progressed to patch-hood, etc. -- I just
> now was catching up though :-)
This security issue was fixed in trunk with changesets 6050 and 6051.
However, neither of these has yet been merged to stable.  Is this an
oversight, or was this intentional?  If an oversight, can someone in
core merge the changes to 1.2-stable so that this security issue will
be fixed in 1.2.2?  If intentional, can someone in core explain why?

Thanks,
Jeremy

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---