False positive when permitted params are used conditionally

[arolling-sa]

Background

Brakeman version: 6.1.2 through 7.0.2
Rails version: 7.1.5.1
Ruby version: 3.3.6

Issue

Using MyModel.find_by(params.permit(:foo_uid)) is flagged as a high-confidence SQL injection issue when it is one of a set of possible queries, based on conditional logic.

Full warning from Brakeman:

Confidence: High
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: MyModel.find_by((params.permit(:foo_uid) or { :id => params.require(:model_id) }))

Relevant code with warning:

if params[:foo_uid].present?
  field_name = :foo_uid
  query = params.permit(:foo_uid)
elsif params[:model_id].present?
  field_name = :model_id
  query = { id: params.require(:model_id) }
end

MyModel.find_by(query) if field_name

Equivalent code with no warning:

if params[:foo_uid].present?
  field_name = :foo_uid
  query = params.permit(:foo_uid)
  MyModel.find_by(query)
elsif params[:model_id].present?
  field_name = :model_id
  query = { id: params.require(:model_id) }
  MyModel.find_by(query)
end

# Or, alternatively
if params[:foo_uid].present?
  field_name = :foo_uid
  query = { foo_uid: params.require(:foo_uid) }
elsif params[:model_id].present?
  field_name = :model_id
  query = { id: params.require(:model_id) }
end

MyModel.find_by(query) if field_name

In our use case, there are two different identifiers (one legacy) that can be sent in the params to identify the record and we wanted to streamline the logic and reduce duplication. However, this was flagged by Brakeman while the individual queries were not. It appears that when the arg.node_type is or, it doesn't use the list of allowable methods that are defined for a single request argument here. Instead, it eventually calls has_immediate_user_input? (via unsafe_sql? -> find_dangerous_value ) which marks it as an unsafe usage of params. Then unsafe_sql? calls safe_value?, which doesn't use the allow list either.

I'm not sure if params.permit should be marked as safe in all situations, but in this one - where the arglist passed to check_query_arguments is a set of possible options for the query - the outcome should be the same as if each option were individually evaluated.