From 1b1ecf8ee2f7a032b67f333d9e2f2d809e3ff50c Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Wed, 12 Jul 2023 08:19:20 -0400
Subject: [PATCH] Refactor `Trends::Query` to avoid brakeman sql injection
 warnings (#25881)

---
 app/models/trends/query.rb | 24 ++++++++++++++++++------
 config/brakeman.ignore     | 23 -----------------------
 2 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/app/models/trends/query.rb b/app/models/trends/query.rb
index cd5571bc62..c4edbba6b8 100644
--- a/app/models/trends/query.rb
+++ b/app/models/trends/query.rb
@@ -68,12 +68,10 @@ class Trends::Query
   alias to_a to_ary
 
   def to_arel
-    tmp_ids = ids
-
-    if tmp_ids.empty?
+    if ids_for_key.empty?
       klass.none
     else
-      scope = klass.joins("join unnest(array[#{tmp_ids.join(',')}]) with ordinality as x (id, ordering) on #{klass.table_name}.id = x.id").reorder('x.ordering')
+      scope = klass.joins(sanitized_join_sql).reorder('x.ordering')
       scope = scope.offset(@offset) if @offset.present?
       scope = scope.limit(@limit) if @limit.present?
       scope
@@ -95,8 +93,22 @@ class Trends::Query
     self
   end
 
-  def ids
-    redis.zrevrange(key, 0, -1).map(&:to_i)
+  def ids_for_key
+    @ids_for_key ||= redis.zrevrange(key, 0, -1).map(&:to_i)
+  end
+
+  def sanitized_join_sql
+    ActiveRecord::Base.sanitize_sql_array(join_sql_array)
+  end
+
+  def join_sql_array
+    [join_sql_query, ids_for_key]
+  end
+
+  def join_sql_query
+    <<~SQL.squish
+      JOIN unnest(array[?]) WITH ordinality AS x (id, ordering) ON #{klass.table_name}.id = x.id
+    SQL
   end
 
   def perform_queries
diff --git a/config/brakeman.ignore b/config/brakeman.ignore
index 4f21944b66..27d8ff8da5 100644
--- a/config/brakeman.ignore
+++ b/config/brakeman.ignore
@@ -23,29 +23,6 @@
       ],
       "note": ""
     },
-    {
-      "warning_type": "SQL Injection",
-      "warning_code": 0,
-      "fingerprint": "30dfe36e87fe1b8f239df9a33d576e44a9863f73b680198d4713be6540ae61d3",
-      "check_name": "SQL",
-      "message": "Possible SQL injection",
-      "file": "app/models/trends/query.rb",
-      "line": 76,
-      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
-      "code": "klass.joins(\"join unnest(array[#{ids.join(\",\")}]) with ordinality as x (id, ordering) on #{klass.table_name}.id = x.id\")",
-      "render_path": null,
-      "location": {
-        "type": "method",
-        "class": "Trends::Query",
-        "method": "to_arel"
-      },
-      "user_input": "ids.join(\",\")",
-      "confidence": "Weak",
-      "cwe_id": [
-        89
-      ],
-      "note": ""
-    },
     {
       "warning_type": "Cross-Site Scripting",
       "warning_code": 2,