Refactor `Trends::Query` to avoid brakeman sql injection warnings (#25881)
This commit is contained in:
parent
ecd8e0d612
commit
1ef014802b
|
@ -68,12 +68,10 @@ class Trends::Query
|
||||||
alias to_a to_ary
|
alias to_a to_ary
|
||||||
|
|
||||||
def to_arel
|
def to_arel
|
||||||
tmp_ids = ids
|
if ids_for_key.empty?
|
||||||
|
|
||||||
if tmp_ids.empty?
|
|
||||||
klass.none
|
klass.none
|
||||||
else
|
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.offset(@offset) if @offset.present?
|
||||||
scope = scope.limit(@limit) if @limit.present?
|
scope = scope.limit(@limit) if @limit.present?
|
||||||
scope
|
scope
|
||||||
|
@ -95,8 +93,22 @@ class Trends::Query
|
||||||
self
|
self
|
||||||
end
|
end
|
||||||
|
|
||||||
def ids
|
def ids_for_key
|
||||||
redis.zrevrange(key, 0, -1).map(&:to_i)
|
@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
|
end
|
||||||
|
|
||||||
def perform_queries
|
def perform_queries
|
||||||
|
|
|
@ -23,29 +23,6 @@
|
||||||
],
|
],
|
||||||
"note": ""
|
"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_type": "Cross-Site Scripting",
|
||||||
"warning_code": 2,
|
"warning_code": 2,
|
||||||
|
|
Reference in New Issue