terrbear.org

Let me preface this post by saying that I’m a huge fan of dynamic programming. Code that writes code is awesome, and can save a lot of time. I got a little crazy today with some class method magic around polymorphism using eval. Specifically, I wrote something like this (simplified for the sake of argument):

def self.describe(type)
eval("#{type}.new.describe")
end

Now, this code is meant to take a subclass (string), instantiate it, and call its describe method (which, if you’re curious, makes a describe call against Salesforce). Pass it something like “Contact”, and you’ll get everything just fine and dandy. Now, by itself, this isn’t much of a security vulnerability.

But, where does the “type” variable get passed in from? A controller. More specifically, it comes from an HTTP parameter. So, let’s say someone was being particularly devious, and passed a parameter that resolved to this:

class Pwn; def describe; OMG_ARBITRARY_CODE; end; end; Pwn"=> "class Pwn; def describe; puts 'you=pwnt'; end; end; Pwn

That guy just got OMG_ARBITRARY_CODE executed on my machine :/

As such, I’m pretty confident that any code that can *ever* see the outside shouldn’t ever allow execution of strings. I’m sure anyone who does all their work in Java or .NET is appalled that strings can be used for this purpose, but I digress…

So I’ve thought about a few different solutions. I’ve been struggling back and forth with whether or not it’s up to my model to enforce security on something like this, or if it’s up to the controller not to pass bad information to the model. I’ve decide that it’s probably best to clean up both sides.

So, first, here’s the new describe method:

def self.describe(type)
type.new.describe
end

Now I require an actual class to be passed in. This seems like a good improvement. The problem is, if I’m doing describe calls all over the place (don’t worry, they’re cached), do I need to check the parameters each time and figure out which class to pass in? That kind of sucks.

I could pass the string in, and make a method that does something to resolve the string to the correct type in my model class, but then I’ve got this horrible new requirement in my code - any time I want to add a subclass, I have to change the base class.  That kind of sucks, too.

My current solution (which I’m not thrilled with, and am hoping one of you tell me how to do it better), is to put a constant in my startup script for Rails, and then check that before using eval to instantiate the type. Here’s the code:

def self.resolve_type(type)
if ALLOWED_PERSON_INSTANCES.include?(type.upcase)
eval(type)
else
raise TypeError.new("only allow lead or contact")
end
end

So, what do you think? Have a better way? Put it in the comments, por favor.