From 2ae33ce39cced79452bd2b985bfbf85aef36882c Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Wed, 3 Mar 2021 15:08:53 +0100
Subject: [PATCH] Fix error-prone SQL queries in Account search

While this code seems to not present an actual vulnerability, one could
easily be introduced by mistake due to how the query is built.

This PR parameterises the `to_tsquery` input to make the query more robust.
---
 app/models/account.rb | 50 +++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/app/models/account.rb b/app/models/account.rb
index 238ea1d655..344926e621 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -428,6 +428,9 @@ class Account < ApplicationRecord
   end
 
   class << self
+    DISALLOWED_TSQUERY_CHARACTERS = /['?\\:‘’]/.freeze
+    TEXTSEARCH = "(setweight(to_tsvector('simple', accounts.display_name), 'A') || setweight(to_tsvector('simple', accounts.username), 'B') || setweight(to_tsvector('simple', coalesce(accounts.domain, '')), 'C'))"
+
     def readonly_attributes
       super - %w(statuses_count following_count followers_count)
     end
@@ -438,68 +441,68 @@ class Account < ApplicationRecord
     end
 
     def search_for(terms, limit = 10, offset = 0)
-      textsearch, query = generate_query_for_search(terms)
+      tsquery = generate_query_for_search(terms)
 
       sql = <<-SQL.squish
         SELECT
           accounts.*,
-          ts_rank_cd(#{textsearch}, #{query}, 32) AS rank
+          ts_rank_cd(#{TEXTSEARCH}, to_tsquery('simple', :tsquery), 32) AS rank
         FROM accounts
-        WHERE #{query} @@ #{textsearch}
+        WHERE to_tsquery('simple', :tsquery) @@ #{TEXTSEARCH}
           AND accounts.suspended_at IS NULL
           AND accounts.moved_to_account_id IS NULL
         ORDER BY rank DESC
-        LIMIT ? OFFSET ?
+        LIMIT :limit OFFSET :offset
       SQL
 
-      records = find_by_sql([sql, limit, offset])
+      records = find_by_sql([sql, limit: limit, offset: offset, tsquery: tsquery])
       ActiveRecord::Associations::Preloader.new.preload(records, :account_stat)
       records
     end
 
     def advanced_search_for(terms, account, limit = 10, following = false, offset = 0)
-      textsearch, query = generate_query_for_search(terms)
+      tsquery = generate_query_for_search(terms)
 
       if following
         sql = <<-SQL.squish
           WITH first_degree AS (
             SELECT target_account_id
             FROM follows
-            WHERE account_id = ?
+            WHERE account_id = :id
             UNION ALL
-            SELECT ?
+            SELECT :id
           )
           SELECT
             accounts.*,
-            (count(f.id) + 1) * ts_rank_cd(#{textsearch}, #{query}, 32) AS rank
+            (count(f.id) + 1) * ts_rank_cd(#{TEXTSEARCH}, to_tsquery('simple', :tsquery), 32) AS rank
           FROM accounts
-          LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = ?)
+          LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = :id)
           WHERE accounts.id IN (SELECT * FROM first_degree)
-            AND #{query} @@ #{textsearch}
+            AND to_tsquery('simple', :tsquery) @@ #{TEXTSEARCH}
             AND accounts.suspended_at IS NULL
             AND accounts.moved_to_account_id IS NULL
           GROUP BY accounts.id
           ORDER BY rank DESC
-          LIMIT ? OFFSET ?
+          LIMIT :limit OFFSET :offset
         SQL
 
-        records = find_by_sql([sql, account.id, account.id, account.id, limit, offset])
+        records = find_by_sql([sql, id: account.id, limit: limit, offset: offset, tsquery: tsquery])
       else
         sql = <<-SQL.squish
           SELECT
             accounts.*,
-            (count(f.id) + 1) * ts_rank_cd(#{textsearch}, #{query}, 32) AS rank
+            (count(f.id) + 1) * ts_rank_cd(#{TEXTSEARCH}, to_tsquery('simple', :tsquery), 32) AS rank
           FROM accounts
-          LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = ?) OR (accounts.id = f.target_account_id AND f.account_id = ?)
-          WHERE #{query} @@ #{textsearch}
+          LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = :id) OR (accounts.id = f.target_account_id AND f.account_id = :id)
+          WHERE to_tsquery('simple', :tsquery) @@ #{TEXTSEARCH}
             AND accounts.suspended_at IS NULL
             AND accounts.moved_to_account_id IS NULL
           GROUP BY accounts.id
           ORDER BY rank DESC
-          LIMIT ? OFFSET ?
+          LIMIT :limit OFFSET :offset
         SQL
 
-        records = find_by_sql([sql, account.id, account.id, limit, offset])
+        records = find_by_sql([sql, id: account.id, limit: limit, offset: offset, tsquery: tsquery])
       end
 
       ActiveRecord::Associations::Preloader.new.preload(records, :account_stat)
@@ -523,12 +526,13 @@ class Account < ApplicationRecord
 
     private
 
-    def generate_query_for_search(terms)
-      terms      = Arel.sql(connection.quote(terms.gsub(/['?\\:]/, ' ')))
-      textsearch = "(setweight(to_tsvector('simple', accounts.display_name), 'A') || setweight(to_tsvector('simple', accounts.username), 'B') || setweight(to_tsvector('simple', coalesce(accounts.domain, '')), 'C'))"
-      query      = "to_tsquery('simple', ''' ' || #{terms} || ' ''' || ':*')"
+    def generate_query_for_search(unsanitized_terms)
+      terms = unsanitized_terms.gsub(DISALLOWED_TSQUERY_CHARACTERS, ' ')
 
-      [textsearch, query]
+      # The final ":*" is for prefix search.
+      # Not sure what surrounding spaces are for, but they were there in the
+      # original code.
+      "' #{terms} ':*"
     end
   end