Fix error-prone SQL queries

[ClearlyClaire]

Following the news of the compromise of one infamous Mastodon fork, I have been reviewing Mastodon's source code for SQL injection vulnerabilities.

So far, I have found none, but I have found a few queries that had me scratch my heard pretty hard and that could easily introduce vulnerabilities in the future if we are not careful.

This PR should make them more robust.

[ClearlyClaire]

Removed unneeded space characters around the generated tsquery, I do not think they serve any purpose, I tried a bunch of tsqueries terms, and encoding them with or without spaces always result in the same tsquery.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ClearlyClaire]

I still think this is a worthy change.

[mohe2015]

I don't know ruby that much but this should really get merged. You need to rebase though.

[mohe2015]

I don't know ruby but the previous here looks quite dangerous as far as I can tell. Are the tag_ids sanitized somewhere else or is there another reason why this is not an sql injection?

[Gargron]

The ids are not passed from user input

[mohe2015]

I haven't fully understood this query but couldn't this check in sql tag_id IN (:tag_ids)

[ClearlyClaire]

No. That's what tagged_with does (returning statuses that are tagged with any of the given tags). tagged_with_all needs to return tagged with all the given tags, there might be better way to query that, but right now I don't know which, and tag_id IN (:tag_ids) isn't one of them. Though tagged_with_none can most probably be rewritten in a better way using tag_id IN (:tag_ids).

[ClearlyClaire]

I have rewritten tagged_with_none in a way that seems slightly more efficient and does not involve creating names from any input. However, tagged_with_all remains unchanged for the moment.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ClearlyClaire]

I still think this is a good change. Rebased.

Again, this PR:

• slightly improves readability of Account.search_for
• slightly improves performance, and greatly improves readability of Status.tagged_with_none
• fixes a bug when both Status.tagged_with_all and Status.tagged_with_none are used at the same time
• while it does not fix any existing security vulnerability, it fixes 4 brakeman warnings and improve error-prone code, including some (Status.tagged_with_all / Status.tagged_with_none) that could be called in an inappropriate way without brakeman catching it

[Gargron]

I think I mentioned this before: This does not yield the same results as ''' ' || #{terms} || ' ''' || ':*'

[ClearlyClaire]

Which different results? ''' ' || #{terms} || ' ''' || ':*' (when #{terms} get substituted by a string surrounded by single-quotes and not containing some characters) is a way to write the string ' #{terms} ':* in SQL (it's tricky because of the single-quotes), "' #{terms} ':*" results in the same string, which cannot be pasted directly in SQL, but it's ok because it's used as a parameter to a parameterized query instead.

So the difference between the two strings is just the spaces surrounding #{terms}. I understand the query is from https://github.com/Casecommons/pg_search/blob/master/lib/pg_search/features/tsearch.rb#L100-L138 but I don't understand what those extra spaces would mean, I could not understand it from PostgreSQL's documentation and did not observe any difference on testing data.

[ClearlyClaire]

Since I could not understand the reason for the surrounding spaces in the built query, I ended up comparing the generated tsqueries (at the SQL level) using the following code:

DISALLOWED_TSQUERY_CHARACTERS = /['?\\:‘’]/.freeze

def compare_tsqueries(unsanitized_terms)
  terms = unsanitized_terms.gsub(DISALLOWED_TSQUERY_CHARACTERS, ' ')
  inline_terms = Arel.sql(ActiveRecord::Base.connection.quote(terms))
  inline_tsquery = "to_tsquery('simple', ''' ' || #{inline_terms} || ' ''' || ':*')"
  tsquery_source = "'#{terms}':*"

  sql = "SELECT #{inline_tsquery} = to_tsquery('simple', $1)"
  ActiveRecord::Base.connection.select_all(sql, nil, [[nil, tsquery_source]]).rows[0][0]
end

def random_ascii_string(length)
  value = ''
  length.times do
    value  << (32 + rand(126-32)).chr
  end
  value
end

1000000.times.map { random_ascii_string(10) }.filter { |x| !compare_tsqueries(x) }

While I don't understand that result, it seems to_tsquery('simple', query) works slightly differently if the first term of query starts with ./, ../ or .. . I don't know how to explain that change, so I'm introducing the spaces back, even though I do not think these discrepancies (one case searching for a leading . less) would cause any issue at all.