ActiveRecord::AttributeMethods #respond_to?

rubyonrails-core
#1

I have a model class

class Foo < ActiveRecord::Base
  attr_protected :bar
end

I would think that Foo.new.respond_to?(:bar) would be false, but it
returns true. This is because the definition of :respond_to? checks
whether @attributes.include?(method_name), but doesn't take attribute
protection into account.

Foo thus does not abide by the general contract of respond_to?:
"respond_to?(:baz) returns true if and only if call(:baz) does not
raise a NoMethodError."

Will fixing this break tons and tons of code?

#2

I would think that Foo.new.respond_to?(:bar) would be false, but it
returns true.

It should be true as the Foo instance responds to a method called bar.

This is because the definition of :respond_to? checks
whether @attributes.include?(method_name), but doesn't take attribute
protection into account.

It doesn't check attributes, it checks the actual methods on the
object. It is independent of Rails.

Foo thus does not abide by the general contract of respond_to?:
"respond_to?(:baz) returns true if and only if call(:baz) does not
raise a NoMethodError."

Hu? Foo.new *has* a bar method. It just doesn't allow you to set it in
mass-assignment.

Jonathan

#3

Sorry, you're right. I phrased my question wrong. Let's try this:

class Foo < ActiveRecord::Base

  private

  # really protect attribute bar
  def bar
    read_attribute :bar
  end
  def bar=(val)
    write_attribute :bar, val
  end

end

Now Foo.new.respond_to?(:bar) really _should_ return false, but it
doesn't. This method isn't checked before @attributes is.

-Gaius

#4

class Foo < ActiveRecord::Base

private

# really protect attribute bar
def bar
   read_attribute :bar
end
def bar=(val)
   write_attribute :bar, val
end

end

Now Foo.new.respond_to?(:bar) really _should_ return false

respond_to? is a Ruby core object method, it's not implemented by Rails.

#5

It _is_ a core Ruby object, with a contract defined in Object. The
problem is that Rails overrides that definition, accounting for
ActiveRecord attributes. (It _should_ override, since it overrides
method_missing.)

All I'm saying is that the override devined in
ActiveRecord::AttributeMethods is inconsistent with the core Ruby
contract.

-Gaius

#6

This is because the definition of :respond_to? checks
whether @attributes.include?(method_name), but doesn't take attribute
protection into account.

It doesn't check attributes, it checks the actual methods on the
object. It is independent of Rails.

Well, not really. ActiveRecord does override #respond_to? to take
attributes into account (the "actual methods" are defined lazily).

Foo thus does not abide by the general contract of respond_to?:
"respond_to?(:baz) returns true if and only if call(:baz) does not
raise a NoMethodError."

Hu? Foo.new *has* a bar method. It just doesn't allow you to set it in
mass-assignment.

I agree. The current behavior is the same as the pure-Ruby one:

$ irb
irb(main):001:0> class Foo
irb(main):002:1> def bar
irb(main):003:2> end
irb(main):004:1> protected :bar
irb(main):005:1> end
=> Foo
irb(main):006:0> Foo.new.respond_to? :bar
=> true

#7

Ok, but what about

$ irb
irb(main):001:0> class Foo
irb(main):002:1> def bar
irb(main):003:2> end
irb(main):004:1> private:bar
irb(main):005:1> end
=> Foo
irb(main):006:0> Foo.new.respond_to? :bar
=> false

-Gaius

#8

Well, we are talking about attr_protected, right?

#9

No, sorry. I'm trying to _truly_ make an attribute private. I want
nobody but my instance (self) to be able to access this attribute.
There's nothing like attr_private to my knowledge, though, so I
declared private methods.

The problem is the basic order of the way ActiveRecord redefines :respond_to?

1. if regular respond_to? true, then return true
2. if matches an attribute, return true
3. return regular respond_to?

There's no way for me to _prevent_ an attribute from being exposed
publicly as far as respond_to? is concerned.

-Gaius

#10

No, sorry. I'm trying to _truly_ make an attribute private. I want
nobody but my instance (self) to be able to access this attribute.
There's nothing like attr_private to my knowledge, though, so I
declared private methods.

I'd question *why* you want that to be completely private, what is it
you're hoping to achieve. Users of your code will *always* be able to
call your method, there's simply no way for you to stop them:

@object.__send__(:your_secret_method) # PWN3D!!!!

There's currently no way to mark an attribute as private, and we could
probably add that as an enhancement, but I'm a little confused what
the use case is.

#11

This is slightly tangential, but since we're on the subject, I'd like
to point out a related issue with private methods. It's early, and I'm
still working on my first cup of coffee, so please just ignore me if
I'm too off topic here :). The problem I am seeing is that a private
method works as expected when called directly on an object, but when
the object is accessed through an association_proxy 'private' is
ignored (because the association_proxy is 'send'ing on
method_missing).

Here's an example:

Class User < AR::Base
  private
  def secret_stuff ; "foo" end
end

Class Purchase < AR::Base
  has_one :user
end

User.first.secret_stuff

=> NoMethodError: private method `secret_stuff' called for #<User:
0x3d446dc> # as expected

Purchase.first.user.secret_stuff

=> "foo" # d'oh!

Because the association_proxy does a blind send, the privacy check is
bypassed. When I first came across this, it really threw me for a loop
-- in one context my 'private' declaration was working fine and dandy,
and in another, it was being ignored! I wanted to use 'private'
because I had just modified my class in such a way that it made sense,
encapsulation-wise, to only have the class itself access the
particular method. I wasn't looking for true protection against
malicious code, I just wanted to verify my encapsulation properties
and find code that was violating those properties. In the end I
decided it wasn't worth the effort to make the two code paths behave
the same so instead I did a manual code comb to find outside classes
who were accessing my private method.

So, I guess I'm just offering this as another data point. It's another
place where the rails metaprogramming slightly modifies the expected
behavior of the underlying code. I don't think this is a major problem
or anything, just thought it fit in with the current topic. Maybe this
behavior is covered in the docs and I just missed it?

Cheers,
acechase

#12

acechase: I think your case is different, but extremely close to the
issue I'm getting at. Thank you for bringing it up.

Koz: are you prepared to argue that Ruby shouldn't have private
methods at all because programmers can get around them with __send__?
I want to make a method private so other programmers don't do stupid
stuff. I want it to be one more thing in the way of writing insecure,
unreliable code. There's no such thing as bulletproof code, but if I
can add more padding without adding conceptual weight, I'm all for it.

Also, I'd just like Rails Models to act like normal Ruby Objects.

-Gaius

#13

Gaius,

I applaud your desire to be safe with your user's information and
protect them from inadvertent programming errors. On my website, we
have a forum where sensitive matters are frequently discussed and so
we provide our users the option of posting anonymously. Of course, we
still need to know who posted it so that they can edit it later so I
had a similar conundrum that you're facing now. I didn't want to ever
let a developer accidentally reveal their identity when the anonymous
flag was set on their post. So I did little meta-programming to allow
me to make the user and user_id nil when the post was anonymous, but
still have access to the information through aliased methods if we
needed it.

http://gist.github.com/8654

Ruby on rails is great, but so is Ruby itself, and with a little
diligence, we can bend the world to our will to achieve a desired API.

-chris

P.S. For those not inclined to click through here's the rough sketch:
# == Schema Information

#14

Koz: are you prepared to argue that Ruby shouldn't have private
methods at all because programmers can get around them with __send__?
I want to make a method private so other programmers don't do stupid
stuff. I want it to be one more thing in the way of writing insecure,
unreliable code. There's no such thing as bulletproof code, but if I
can add more padding without adding conceptual weight, I'm all for it.

No, my point is simply that whatever safety you think you're receiving
by fixing this respond_to? block is no substitute for code review.
While this is definitely strange behaviour and a little counter
intuitive I can't imagine a situation where you'd actually have a
security issue caused by this code. I find it hard to think a
developer will be sitting in an irb session saying "does this
respond_to? :foo" rather than looking at the source to that model to
find the method they want :slight_smile:

If you'd like to submit a patch for respond_to to check the private /
public status of methods defined by attributes, I'd be happy to apply
it.

#15

User.first.secret_stuff

=> NoMethodError: private method `secret_stuff' called for #<User:
0x3d446dc> # as expected

Purchase.first.user.secret_stuff

=> "foo" # d'oh!

Yeah, this is also a bug. If you want to whip up a patch for it, I'll
gladly apply it.

#16

Also, I'd just like Rails Models to act like normal Ruby Objects.

It would be pretty weird to make attr_accessor methods private in a
normal Ruby object, since the writer method could never be called
except with a self.send. If some field was meant to be private, you'd
probably just use the instance variables directly and not have reader
or writer methods at all.

It seems like the closest analog in Active Record would be to not
define reader and writer methods for the attribute, only exposing it
via read_attribute and write_attribute (and [] and []=).

-hume.

#17

John, this is a _fantastic_ point. It's certainly true that there
would be no way to use the "setter" half of a private attr_accessor in
a regular Ruby object:

self.bar = 'baz' # violates private
bar = 'baz' # sets a local variable

But you can use the "getter" half:

return bar.dup # return a defensive copy of the instance variable

I consider Ruby's definition of "private" fairly crippled, but that's
not the fault of the Rails community, nor can they (we?) do much about
it.

Maybe my actual use case will help clear up why I want a private
attribute. I have a single-table-inheritance model:

table products:
  integer version
  string name

class Product < ActiveRecord::Base
  attr_private :version # if such a method existed
end

class VersionedProduct < Foo
  attr_accessor :version # reveal the method
end

I wouldn't normally ask this list for usage advice, but perhaps this
case will inform design.

Thanks,
Gaius

#18

John, this is a _fantastic_ point. It's certainly true that there
would be no way to use the "setter" half of a private attr_accessor in
a regular Ruby object:

self.bar = 'baz' # violates private
bar = 'baz' # sets a local variable

But you can use the "getter" half:

return bar.dup # return a defensive copy of the instance variable

I consider Ruby's definition of "private" fairly crippled, but that's
not the fault of the Rails community, nor can they (we?) do much about
it.

I still think you misunderstand and mix Ruby's private and Rails'
attr_protected/attr_accessible.

class A
  private
  def foo
    "foo"
  end
end

a = A.new
a.foo

NoMethodError: private method `foo' called for #<A:0x6bae4>

So no crippling here. Note that you can still access foo if you really wan:

a.send(:foo)
=> "foo"

This is Ruby where we WANT to be able to do things like that.

Rails' attr_protected/attr_accessible are only there to protect you
from mass-assignment and have noting to do with private/protected.
Ruby's attr_accessor :foo will just create a getter and setter for you
if you are too lazy.

Maybe my actual use case will help clear up why I want a private
attribute. I have a single-table-inheritance model:

table products:
integer version
string name

class Product < ActiveRecord::Base
attr_private :version # if such a method existed
end

class VersionedProduct < Foo
attr_accessor :version # reveal the method
end

I wouldn't normally ask this list for usage advice, but perhaps this
case will inform design.

The simple answer is: Just don't call version on products! It's your
code and you know where you use it.

If you really want to stop using version:

class Product < ActiveRecord::Base

  def version
    raise 'private'
  end

end

class VersionedProduct < Product

  def version
    attributes[:version]
  end

end

You could do the same for the setter or even write your attr_private
method that will generate the getters/setters.

But again, I see no point in doing so.

Jonathan

#19

Jonathan Weiss said,
<<<
The simple answer is: Just don't call version on products! It's your
code and you know where you use it.

It's my code right now, but not after I leave and some other coder
gets assigned to the project. I want to help future maintainers
prevent accidents. In Java, this sort of defensive programming is
very easy.

I want the protection offered by attr_protected (or the lack of
attr_accessible) AND the protection offered by restricted visibility.
I want the most protection I can possibly get without making the code
impossible to read.

All I'm saying is that Rails doesn't respect Ruby's native visibility.
John Hume brought up an even more important example of this.

-Gaius

#20

All I'm saying is that Rails doesn't respect Ruby's native visibility.
John Hume brought up an even more important example of this.

As I mentioned earlier in the thread, these are both valid points and
you should send us a patch for them. While I may not agree with you
about the use of private for this purpose, you're right that there's
no valid reason to misbehave here, and there's no downside to doing
so.

More Resources

Keep up to date with Rails on Twitter and This Week in Rails

Policies: Conduct, License, Maintenance, Security, Trademarks