False positive SQL injection for quoted_primary_key, quoted_table_name #884

Closed
md5 opened this Issue May 20, 2016 · 3 comments

Projects

None yet

2 participants

@md5
md5 commented May 20, 2016

I have some code that is dynamically constructing SQL using quoted_primary_key and quoted_table_name from ActiveRecord::ModelSchema::ClassMethods. Brakeman is reporting these as a possible SQL injection, which is not the case with the default implementation of these methods. I suppose someone could override them for their class and introduce an SQL injection vector, but this seems like a false positive to me.

@presidentbeef
Owner

Hi Mike,

You are right, at the moment Brakeman ignores quoted_table_name but not quoted_primary_key. I'm guessing it is warning about the latter. Can you provide an example? Thanks!

@md5
md5 commented May 23, 2016
def self.self_and_descendants_for(id)
    where(<<-SQL, id: id)
      #{quoted_table_name}.#{quoted_primary_key} IN (
        WITH RECURSIVE descendant_tree(#{quoted_primary_key}, path) AS (
            SELECT #{quoted_primary_key}, ARRAY[#{quoted_primary_key}]
            FROM #{quoted_table_name}
            WHERE #{quoted_primary_key} = :id
          UNION ALL
            SELECT #{quoted_table_name}.#{quoted_primary_key}, descendant_tree.path || #{quoted_table_name}.#{quoted_primary_key}
            FROM descendant_tree
            JOIN #{quoted_table_name} ON #{quoted_table_name}.parent_id = descendant_tree.#{quoted_primary_key}
            WHERE NOT #{quoted_table_name}.#{quoted_primary_key} = ANY(descendant_tree.path)
        )
        SELECT #{quoted_primary_key}
        FROM descendant_tree
        ORDER BY path
      )
    SQL
end
@md5
md5 commented Jun 2, 2016

👍

@presidentbeef presidentbeef locked and limited conversation to collaborators Oct 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.