rails brakeman order sql injection

Question

How can I avoid a brakeman warning in Rails when constructing an order method from parameters?

def index
  @methods = [:name, :manager, :deadline]
  assignments = Assignment.order(sort_column(@methods) + " " + sort_direction).received(current_user).root
end

def sort_column(column_names)
  column_names.each do |column|
    return column if column == params[:sort]
  end
  return 'updated_at'
end

def sort_direction
  params[:direction] == 'asc' ? 'asc' : 'desc'
end

I'm working hard to avoid ever putting user-generated code directly into the query, but brakeman still alerts (medium confidence) that this is a SQL injection vulnerability.

Is this a false positive? If not, how do I correct the vulnerability?

If so, is there an easy way to avoid the false positive?

Answer 1

Okay, this is too long for a comment.

From my testing, moving the string building into a method like this does make the warning go away:

def index
  @methods = [:name, :manager, :deadline]
  assignments = Assignment.order(sort_order).received(current_user).root
end

def sort_order
  sort_column(@methods) + " " + sort_direction
end

However, that's just hiding the problem. I would suggest adding something like this to the Assignment model instead:

class Assignment < ActiveRecord::Base

  def self.sorted_by(column, direction)
    direction = direction.downcase == 'asc' ? 'asc' : 'desc'
    column = sanitize_sql(column)
    order("#{column} #{direction}")
  end

end

Just keep in mind that sometimes you have to choose between keeping a tool happy and keeping your code reasonable. As for the false positive, I don't see this particular issue being resolved, since it is not simple to inspect sort_column and know it is safe.

Answer 2

You could add a sanitize method on the order by clause

 assignments = Assignment.order(ActiveRecord::Base::sanitize(sort_column(@methods) + " " + sort_direction)).received(current_user).root