From 1ef014802b6ed281100087a7951b26f6f7423418 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,