ActiveModel: dangerous use of send()?

Question

In RailsCast 219, the following code is offered for creating a class for ferrying data back and forth from forms, but without any ActiveRecord persistence:

class Message
  include ActiveModel::Validations

  attr_accessor :name, :email, :content

  validates_presence_of :name
  validates_format_of :email, :with => /^[-a-z0-9_+\.]+\@([-a-z0-9]+\.)+[a-z0-9]{2,4}$/i
  validates_length_of :content, :maximum => 500

  def initialize(attributes = {})
    attributes.each do |name, value|
      send("#{name}=", value)
    end
  end
end

I'm new to Ruby, but the send("#{name}=", value) seems like an invitation for an attacker to assign arbitrary values to arbitrary fields. Is this an issue? A few commenters asked similar questions, but no response.

Here's protection: http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html#method-i-attr_accessible — jdoe, Jun 27 '12 at 14:51

score 3 · Answer 1 · answered Jun 27 '12 at 14:52

3

send is a common way of calling methods dynamically (when you don't know in advance what you will call).

If you're concerned about security, you should definitely do some validations. Here's a simple restricting check:

def initialize(attributes = {})
  attributes.each do |name, value|
    if [:name, :email, :content].include?(name)
      send("#{name}=", value)
    end
  end
end

answered Jun 27 '12 at 14:52

Sergio Tulentsev

226,338
43
373
367

I understand what send is doing. My question is: is it dangerous to do what the Rasilscast is suggesting, without extra code to defend against assignment to arbitrary fields? Is there something in Rails that somehow makes this safe by default? Thanks for your time. – George Armhold Jun 29 '12 at 14:28
No, by default it's not safe. – Sergio Tulentsev Jun 29 '12 at 14:30

score 1 · Accepted Answer · edited May 23 '17 at 11:58

When I asked a question related to the same RailsCast recently and was told that the initialiser was dangerous, but sadly no justification was given.

Having delved a little deeper I now believe that the method does not introduce any security weakness for the reasons that jdoe eluded to in his comment to your question. The send method does not circumvent accessor methods, therefore the security of attributes is controlled by accessor declarations as normal.

However, I would recommend a validation check to improve robustness against an attempt to assign inaccessible or non-existant attributes. Similar to Sergio's suggestion but more general:

attributes.each do |name, value|
  send("#{name}=", value) if respond_to?("#{name}=")
end

Testing for `respond_to?` is fine, provided that the class doesn't contain any internal state that shouldn't be settable from the outside (as it's likely to be the case with data transfer objects). However, if it has internal setters, then one needs to whitelist mass-assignable properties. — Sergio Tulentsev, Jul 01 '15 at 15:26

ActiveModel: dangerous use of send()?

2 Answers2