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"?

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

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? :slight_smile:

>
> 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? :slight_smile:

I certainly think this is deserving of a 1.2.2.

Jeremy

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.

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

Patch at http://dev.rubyonrails.org/ticket/7401. attr_accessible is
good practice, but secure by default is a better one.

Jeremy

Hi --

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 :slight_smile:

David

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