Brakeman doesn't understand `map!`

[MrJoy]

Background

Brakeman version: 5.0.1 Rails version: 6.1.3.2 Ruby version: 2.7.3

Link to Rails application code: N/A

Issue

I got a false-positive from Brakeman today that involved a couple layers of incorrect behavior. One of those layers is somewhat clear and unambiguous, the other is more complex:

pieces = something.underscore.split(".")
pieces.map! { |piece| ActiveRecord::Base.connection.quote_column_name(piece) }
something_clean = pieces.join(".")
some_relation.order(Arel.sql("#{something_clean} #{sort_direction}"))

According to the output from Brakeman, that got reduced to:

Arel.sql("#{something.underscore.split(".").join(".")} #{"asc"}")

Of course, adding map! to the method call chain results in the call to quote_column_name being included in the reduced code as one would expect.

The other, murkier problem: That doesn't resolve the apparent false-positive, as it seems that Brakeman also doesn't seem to understand quote_column_name -- or at least doesn't understand how I'm using it here. The change that caused this code to go from passing Brakeman checks to failing them is the change from just doing:

something_clean = ActiveRecord::Base.connection.quote_column_name(something.underscore)
some_relation.order(Arel.sql("#{something_clean} #{sort_direction}"))

So the fundamental problem I'm facing seems to be that joining a set of sanitized values together does not produce what's considered to be a sanitized value. Maybe that's intended behavior because of the ".", in which case feel free to disregard.

That said, it seems to me that Brakeman not understanding the modify-in-place collection methods does qualify as a defect. Perhaps a relatively obscure one for most, but those of us using RuboCop checks to help ensure our code isn't too sloppy about performance are gonna get hit by this one (especially since not all the modify-in-place methods actually return the collection in all circumstances -- e.g. uniq! -- making it impossible to reliably used the method-chaining approach with them in some cases).

[MrJoy]

Ah, I see my second issue is likely the same as #1231 . Since that doesn't seem like it's going anywhere, I'll plan around that.

[presidentbeef]

For your second example, Brakeman is probably warning about sort_direction, not the use of quote_column_name. The highlighted code (purple in my terminal) indicates which value Brakeman is warning about.

Brakeman is pretty aggressive when warning about any kind of string building for SQL, so even if it doesn't know what a value is it will still raise a warning (medium instead of high confidence though).

[MrJoy]

The value for sort_direction gets replaced as :BRAKEMAN_SAFE_LITERAL, or "asc" (depending on... well, I'm not entirely sure what -- maybe the fact that I moved the ||= "asc" higher up) when I run Brakeman. The variable is checked against a list of constants earlier in the method. (raise ... unless SOME_LIST_OF_CONSTANTS.include?(sort_direction)), which seems to be something Brakeman correctly detects as constraining the values to ones that are safe. And that variable was referenced in the same way, in the code prior to the change that caused this message to appear:

something_clean = ActiveRecord::Base.connection.quote_column_name(something.underscore)
some_relation.order(Arel.sql("#{something_clean} #{sort_direction}"))

Like I said, I think that problem is me running into the same issue as #1231. The issue here is that when I use map! in an unchained manner, instead of seeing Brakeman interpolate that down to something like:

Arel.sql("#{something.underscore.split(".").map! { |x| ActiveRecord::Base.connectionquote_column_name(x) }.join(".")} #{"asc"}")

I instead see:

Arel.sql("#{something.underscore.split(".").join(".")} #{"asc"}")

I get the correct reduced code if I use the chained version of map!.

Or, to be more explicit, the following two pieces of code are equivalent, but Brakeman sees them as different:

pieces = something.underscore.split(".")

pieces.map! { |piece| ActiveRecord::Base.connection.quote_column_name(piece) } # Brakeman ignores this.

something_clean = pieces.join(".")
some_relation.order(Arel.sql("#{something_clean} #{sort_direction}"))

pieces = something.underscore.split(".").map! { |piece| ActiveRecord::Base.connection.quote_column_name(piece) } # Brakeman properly recognizes it here.

something_clean = pieces.join(".")
some_relation.order(Arel.sql("#{something_clean} #{sort_direction}"))

It's specifically the splitting-and-joining that's resulting in the kvetching about a SQL injection.

The full method, as it presently exists:

  def apply_sorting(result, sort_column, sort_direction)
    return result if sort_column.blank?

    # We should be using enum types for sort_direction, but this guard is here to prevent
    # SQL injection in the off-chance that somebody gets lazy/forgetful.
    sort_direction ||= "asc"
    raise ArgumentError, "Invalid column order!" unless ALLOWED_DIRS.include?(sort_direction)

    # We should be using enum types for sort_column, but this guard is here to prevent
    # SQL injection in the off-chance that somebody gets lazy/forgetful.
    conn = ActiveRecord::Base.connection
    sort_column = sort_column
                  .underscore
                  .split(".")
                  .map { |piece| conn.quote_column_name(piece) }
                  .join(".")
    result&.order(Arel.sql("#{sort_column} #{sort_direction}"))
  end

This still produces a SQL injection warning which I've had to add to the ignore-list.

[presidentbeef]

Hi @MrJoy sorry for the confusion. I was only addressing this part

That doesn't resolve the apparent false-positive, as it seems that Brakeman also doesn't seem to understand quote_column_name

The likelihood of Brakeman understanding this code:

    sort_column = sort_column
                  .underscore
                  .split(".")
                  .map { |piece| conn.quote_column_name(piece) }
                  .join(".")

and understanding that it is "safe" for SQL is pretty low, unfortunately.

[MrJoy]

Totally. I've gone ahead and just marked it as a false positive in my repo. If the general policy is one where Brakeman errs on the side of caution and things like map operations in general are going to scare it into speaking up, then this bug report is probably kinda moot.