11

In my rails application I am getting the following security warning from brakeman. Unsafe reflection method constantize called with model attribute. Here is what my code is doing.


chart_type = Chart.where(
  id: chart_id,
).pluck(:type).first

begin
  ChartPresenter.new(chart_type.camelize.constantize.find(chart_id))
rescue
  raise "Unable to find the chart presenter"
end

From my research I haven't found any concrete solution. I have heard that you can make a whitelist but I am not sure what brakeman is looking for. I tried to create an array and to check against that before calling constantize and breakman still complains. Any help with this would be great. If you feel it's not a needed fix can you give details as to why it shouldn't be a concern?

Jacob Waller
  • 4,119
  • 3
  • 26
  • 32

1 Answers1

19

You can go the other way around, finding the class whose name is of chart_type:

chart_class = [User, Category, Note, Post].find { |x| x.name == chart_type.classify }
if chart_class.nil?
  raise "Unable to find the chart presenter"
end
ChartPresenter.new(chart_class.find(chart_id))

This way Brakeman should be happy, and you are more secure...

Uri Agassi
  • 36,848
  • 14
  • 76
  • 93
  • I am guessing that it will cause the code to be slower though correct? – Jacob Waller May 19 '14 at 16:11
  • That list [User, Category, Note, Post] is going to be a dynamic list though is that going to be a problem? Meaning I will still have to constantize right? The list is a little to long to type it all out. Also since it's dynamic we would have to keep updating it. – Jacob Waller May 19 '14 at 16:50
  • You can try it, I don't know what Brakeman will make of it... You can try to explore your model using `ActiveRecord::Base.descendants` (see here: http://stackoverflow.com/a/10712838/1120015) – Uri Agassi May 19 '14 at 17:01