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?

wallerjake
  • 3,999
  • 3
  • 25
  • 32

1 Answers1

17

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,078
  • 12
  • 73
  • 92
  • I am guessing that it will cause the code to be slower though correct? – wallerjake 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. – wallerjake 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