False positive: where(foo) if foo.is_a?(Hash)

[akimd]

Background

Brakeman version: 4.3.1 Rails version: 4.2.10 Ruby version: 2.5.1p57

False Positive

Full warning from Brakeman:

Possible SQL injection near line 175: AccessRule.preload_disp_names.smart_orderer(params[:sort_by]).where(params[:conditions])

171 | @restricted_fields = []
172 | @access_rules ||= AccessRule # let protected actions called before preset scopes
173 | @access_rules = @access_rules.preload_disp_names
174 | @access_rules = @access_rules.smart_orderer(params[:sort_by]) if params[:sort_by].is_a?(String)
175 | @access_rules = @access_rules.where(params[:conditions]) if params[:conditions].is_a?(Hash) # accepts hash filters by url
176 | @access_rules = @access_rules.joins(@joins_fields) if defined?(@join_fields)
177 | per_page = params[:per_page].blank? ? nil : params[:per_page].to_i
178 | per_page ||= (cookies[:access_rules_per_page] \|\| 50).to_i
179 | @access_rules = @access_rules.page(params[:page].to_i > 0 ? params[:page].to_i : 1).per_page(per_page)
180 | @access_rules = @access_rules.page(@access_rules.total_pages) if @access_rules.total_pages < @access_rules.current_page

Why might this be a false positive?

Brakeman thinks this is dangerous, because it is not aware that params[:conditions] is actually a hash. I can avoid the warning by changing 175 to use to_h:

@access_rules = @access_rules.where(params[:conditions].to_h) if params[:conditions].is_a?(Hash)

but I'd like to avoid the avoidance :)

Cheers!

[presidentbeef]

Hi Akim,

Yes, I agree. I have some thoughts on fixing this that I'll need to test.