False Positive: SQL Injection on string interpolation in #pretty_print

[ShadSterling]

Background

Brakeman version: 5.0.0 Rails version: 5.2.4.5 Ruby version: 2.6.5p114

Link to Rails application code: (not public)

False Positive

Full warning from Brakeman:

Confidence: Weak
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: pp.group(1, "<#{tag}", ">")
File: app/services/product_attributes_by_oms_service.rb
Line: 37

Confidence: Weak
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: pp.group(1, "<#{tag}", ">")
File: app/services/product_attributes_by_oms_service.rb
Line: 96

Relevant code:

class ProductAttributesByOmsService < HttpServiceBaseV2
    ## ...
    class ServiceResult
        ## ...
        def pretty_print pp=PP.new
            pp.group 1, "<#{tag}", ">" do  ## WARNING HERE, Line 37
                pp.breakable
                pp.text "errors="
                pp.pp errors
                pp.breakable
                pp.text "warnings="
                pp.pp warnings
                pp.breakable
                pp.text "result="
                pp.pp result
                pp.breakable
            end
        end
    end
    ## ...
    class ProductAttributes
        ## ...
        def pretty_print pp=PP.new
            pp.group 1, "<#{tag}", ">" do  ## WARNING HERE, Line 96
                pp.breakable
                pp.text "oms_id="
                pp.pp    oms_id
                pp.breakable
                pp.text "sku_number="
                pp.pp    sku_number
                pp.breakable
                pp.text "sku_info="
                pp.pp    sku_info
                pp.breakable
                pp.text "sku_alternates="
                pp.pp    sku_alternates
                pp.breakable
            end
        end
        ## ...
    end
    ## ...
end

## The tag method is what we use in our inspect methods:
class Object
    def tag
        base=self.class.superclass
        "#{self.class}#{base ? "<#{base}" : ""}@0x#{(object_id*2).to_s(16).rjust(16,"0")}"
    end
end

The pretty_print method is the helper for pretty_inspect; see https://docs.ruby-lang.org/en/3.0.0/PP.html#class-PP-label-Output+Customization

Why might this be a false positive?

There's no SQL here, we're generating strings for pretty_inspect for log messages

[presidentbeef]

Thank you for reporting this @ShadSterling.

Finding just calls on ActiveRecord objects is pretty hard, so Brakeman is a bit aggressive about identifying potential DB queries.

This case can be addressed, though.

[presidentbeef]

So, actually to address this particular case Brakeman needs to do something with default argument values and that's kind of a big change. I'll have to ponder this.