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”?
On 1/25/07, Lugovoi N. [email protected] wrote:
‘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 O.
http://weblog.techno-weenie.net
http://mephistoblog.com
2007/1/25, Rick O. [email protected]:
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? 
On 1/25/07, Lugovoi N. [email protected] wrote:
2007/1/25, Rick O. [email protected]:
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.
.//vendor/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:92:in
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
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
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 O.
http://weblog.techno-weenie.net
http://mephistoblog.com
On 1/25/07, Rick O. [email protected] 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
Hi –
On Thu, 25 Jan 2007, Rick O. wrote:
=> 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
David
–
Q. What is THE Ruby book for Rails developers?
A. RUBY FOR RAILS by David A. Black (Ruby for Rails)
(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)
On 1/27/07, [email protected] [email protected] wrote:
script/console :
I wonder, if this is a bug, or just hidden feature of “cattr_accessor”?
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