Attr reader #10

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@pitluga
pitluga commented Feb 27, 2012

This is a solid first whack at allowing you to tighten the constraints on your model. There are some weird things though.

  • I made it only assign instance variables if there is a method that responds to the attribute. This could get weird. I wonder if there is some concept we are missing.
  • version is still an attr_writer. I wasn't sure how this was used. We should talk about it.
  • I found and fixed what could be a bug around created_at/updated_at where we were using the unmigrated values.

I'm cool with waiting to pull it in if you think the respond_to? thing it too weird.

@pgr0ss pgr0ss was assigned Feb 27, 2012
@pgr0ss
Member
pgr0ss commented Feb 27, 2012

I think this looks good. Here's a few comments:

  • I think the respond_to is a little weird, but probably ok for now.

  • I think this looks weird: object.instance_variable_set("@id", data_store.save(hash).key)

    Since we are setting the id outside of the model, it seems like it should be public. Maybe we can add a setter that only allows setting once to prevent ids from changing? What are your thoughts on this?

  • Good catch on the created_at/updated_at bug.

@dkubb dkubb commented on an outdated diff Feb 28, 2012
lib/curator/model.rb
attr_writer :version
end
def initialize(args = {})
args.each do |attribute, value|
send("#{attribute}=", value) if respond_to?("#{attribute}=")
+ instance_variable_set("@#{attribute}", value) if respond_to?(attribute.to_s)
@dkubb
dkubb Feb 28, 2012

One thing to keep in mind is that in #respond_to? creates a Symbol with whatever is passed into it, and then uses that to see if the method exists for the object. Symbol objects aren't GC'd, so if you're passing in user input into the constructor it would be trivially easy for an attacker to DOS a machine this way. The same problem exists with Class#public_method_defined?.

Of course this assumes user input is being passed in directly, which is probably a bad idea, but since it's such a common idiom I figured I'd mention it.

This was a problem that affected both Sequel and DataMapper (which I'm the maintainer of) in the past. Both use the same general approach to work-around it: a whitelist of allowed methods is checked before allowing an attribute to be set.

@pitluga
pitluga commented Feb 29, 2012

dkubb, Thanks for pointing this out. Never would have thought of that. This latest commit addresses that issue in what is probably a very poorly performing way.

@dkubb
dkubb commented Feb 29, 2012

@pitluga Yeah, that's basically how I did it in DataMapper. The only thing different is I have a class method that memoizes the list of allowed methods, so the work was only done once per class.

I also used a Set instead of an Array for the list of allowed methods because Set#include? is O(n), although I should probably benchmark the difference between Array and Set because when "n" is small Array#include? might actually be faster. Even still I usually prefer using a data-structure that communicates my intent, which in this case is that it be a list of unique strings.

The other thing I did, which I borrowed from Sequel, is that I filter out cases where the attribute matches the libraries' own methods that end in "=". So for example in DM you can say obj.attributes = { ... }, so I filter out "attributes=" from the list. This also includes filtering out "==", "taguri=", etc. A more recent implementation for a DataMapper 2 library can be found here: https://github.com/solnic/virtus/blob/master/lib/virtus/class_methods.rb#L88-96

@pitluga pitluga Merge branch 'master' into attr_reader
Conflicts:
	lib/curator/repository.rb
87faa79
@pitluga
pitluga commented Mar 2, 2012

rebase and merged onto master.

@pitluga pitluga closed this Mar 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment