labml icon indicating copy to clipboard operation
labml copied to clipboard

.map.join pattern is not considered string_building

Open Fryguy opened this issue 4 years ago • 0 comments

Background

Brakeman version: 5.1.1 Rails version: 6.0.4 Ruby version: 2.7.2

Link to Rails application code: https://github.com/ManageIQ/manageiq-schema (specifically these lines in this method: https://github.com/ManageIQ/manageiq-schema/blob/580494b457aa68fa84ad0ef79a99f7347acf57bf/lib/migration_helper.rb#L222-L223 )

Possibly related to #1591 and #1231

Issue

What problem are you seeing?

One of the helper methods that we use for building a SQL statement is flagged as a SQL injection. However, if I rearrange the method it is no longer flagged as a SQL injection.

I believe what is happening is that .map {}.join is not considered string_building? when it hits this line. However, when refactoring the code to look like the After below, I believe it passes because it is now using << which is considered an acceptable method here.


Before code (fails):

      condition_list = mapping.keys.map { |s| connection.quote(s) }.join(',')
      when_clauses = mapping.map { |before, after| "WHEN #{connection.quote(before)} THEN #{connection.quote(after)}" }.join(' ')

After code (succeeds):

      condition_list = ""
      mapping.keys.each { |s| condition_list << connection.quote(s) << "," }
      condition_list.chomp!(",")
      when_clauses = ""
      mapping.each { |before, after| when_clauses << "WHEN #{connection.quote(before)} THEN #{connection.quote(after)} " }
      when_clauses.chomp!(" ")

Fryguy avatar Jul 26 '21 18:07 Fryguy