Refactor Trends::Query to avoid brakeman sql injection warnings (#25881)
				
					
				
			This commit is contained in:
		
							parent
							
								
									8d20afe0aa
								
							
						
					
					
						commit
						1b1ecf8ee2
					
				
					 2 changed files with 18 additions and 29 deletions
				
			
		| 
						 | 
				
			
			@ -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
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -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,
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
		Loading…
	
		Reference in a new issue