FIX: Better and more secure validation of periods for TopicQuery

FIX: Better and more secure validation of periods for TopicQuery

Co-authored-by: Martin Brennan mjrbrennan@gmail.com

diff --git a/app/controllers/embed_controller.rb b/app/controllers/embed_controller.rb
index 490ff8c..4bbd779 100644
--- a/app/controllers/embed_controller.rb
+++ b/app/controllers/embed_controller.rb
@@ -57,8 +57,13 @@ class EmbedController < ApplicationController
     end
 
     topic_query = TopicQuery.new(current_user, list_options)
-    top_period = params[:top_period]&.to_sym
-    valid_top_period = TopTopic.periods.include?(top_period)
+    top_period = params[:top_period]
+    begin
+      TopTopic.validate_period(top_period)
+      valid_top_period = true
+    rescue Discourse::InvalidParameters
+      valid_top_period = false
+    end
 
     @list = if valid_top_period
       topic_query.list_top_for(top_period)
diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb
index b4fba96..b0d3f6b 100644
--- a/app/controllers/list_controller.rb
+++ b/app/controllers/list_controller.rb
@@ -218,6 +218,8 @@ class ListController < ApplicationController
     @atom_link = "#{Discourse.base_url}/top.rss"
     @description = I18n.t("rss_description.top")
     period = params[:period] || SiteSetting.top_page_default_timeframe.to_sym
+    TopTopic.validate_period(period)
+
     @topic_list = TopicQuery.new(nil).list_top_for(period)
 
     render 'list', formats: [:rss]
diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb
index 9394615..912a60c 100644
--- a/app/controllers/tags_controller.rb
+++ b/app/controllers/tags_controller.rb
@@ -90,6 +90,8 @@ class TagsController < ::ApplicationController
 
       if filter == :top
         period = params[:period] || SiteSetting.top_page_default_timeframe.to_sym
+        TopTopic.validate_period(period)
+
         @list = TopicQuery.new(current_user, list_opts).public_send("list_top_for", period)
         @list.for_period = period
       else
diff --git a/app/models/top_topic.rb b/app/models/top_topic.rb
index 739f6ce..3f09fda 100644
--- a/app/models/top_topic.rb
+++ b/app/models/top_topic.rb
@@ -45,6 +45,17 @@ class TopTopic < ActiveRecord::Base
                                    all: 6)
   end
 
+  def self.score_column_for_period(period)
+    TopTopic.validate_period(period)
+    "#{period}_score"
+  end
+
+  def self.validate_period(period)
+    if period.blank? || !periods.include?(period.to_sym)
+      raise Discourse::InvalidParameters.new("Invalid period. Valid periods are #{periods.join(", ")}")
+    end
+  end
+
   private
 
   def self.sort_orders
diff --git a/app/models/topic.rb b/app/models/topic.rb
index a618b56..601c975 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -463,7 +463,7 @@ class Topic < ActiveRecord::Base
   # Returns hot topics since a date for display in email digest.
   def self.for_digest(user, since, opts = nil)
     opts = opts || {}
-    score = "#{ListController.best_period_for(since)}_score"
+    period = ListController.best_period_for(since)
 
     topics = Topic
       .visible
@@ -483,8 +483,12 @@ class Topic < ActiveRecord::Base
     end
 
     if !!opts[:top_order]
-      topics = topics.joins("LEFT OUTER JOIN top_topics ON top_topics.topic_id = topics.id")
-        .order(TopicQuerySQL.order_top_with_notification_levels(score))
+      topics = topics.joins("LEFT OUTER JOIN top_topics ON top_topics.topic_id = topics.id").order(<<~SQL)
+          COALESCE(topic_users.notification_level, 1) DESC,
+          COALESCE(category_users.notification_level, 1) DESC,
+          COALESCE(top_topics.#{TopTopic.score_column_for_period(period)}, 0) DESC,
+          topics.bumped_at DESC
+      SQL
     end
 
     if opts[:limit]
diff --git a/lib/topic_query.rb b/lib/topic_query.rb
index 122a344..82b76cf 100644
--- a/lib/topic_query.rb
+++ b/lib/topic_query.rb
@@ -266,18 +266,22 @@ class TopicQuery
   end
 
   def list_top_for(period)
-    if !TopTopic.periods.include?(period.to_sym)
-      raise Discourse::InvalidParameters.new("Invalid period. Valid periods are #{TopTopic.periods.join(", ")}")
-    end
-
-    score = "#{period}_score"
+    score_column = TopTopic.score_column_for_period(period)
     create_list(:top, unordered: true) do |topics|
       topics = remove_muted_categories(topics, @user)
-      topics = topics.joins(:top_topic).where("top_topics.#{score} > 0")
+      topics = topics.joins(:top_topic).where("top_topics.#{score_column} > 0")
       if period == :yearly && @user.try(:trust_level) == TrustLevel[0]
-        topics.order(TopicQuerySQL.order_top_with_pinned_category_for(score))
+        topics.order(<<~SQL)
+          CASE WHEN (
+             COALESCE(topics.pinned_at, '1900-01-01') > COALESCE(tu.cleared_pinned_at, '1900-01-01')
+          ) THEN 0 ELSE 1 END,
+          top_topics.#{score_column} DESC,
+          topics.bumped_at DESC
+        SQL
       else
-        topics.order(TopicQuerySQL.order_top_for(score))
+        topics.order(<<~SQL)
+          COALESCE(top_topics.#{score_column}, 0) DESC, topics.bumped_at DESC
+        SQL
       end
     end
   end
@@ -676,7 +680,9 @@ class TopicQuery
     # If we are sorting by category, actually use the name
     if sort_column == 'category_id'
       # TODO forces a table scan, slow
-      return result.references(:categories).order(TopicQuerySQL.order_by_category_sql(sort_dir))
+      return result.references(:categories).order(<<~SQL)
+        CASE WHEN categories.id = #{SiteSetting.uncategorized_category_id.to_i} THEN '' ELSE categories.name END #{sort_dir}
+      SQL
     end
 
     if sort_column == 'op_likes'
diff --git a/lib/topic_query_sql.rb b/lib/topic_query_sql.rb
deleted file mode 100644
index c96f46c..0000000
--- a/lib/topic_query_sql.rb
+++ /dev/null
@@ -1,60 +0,0 @@
-# frozen_string_literal: true
-#
-#  SQL fragments used when querying a list of topics.
-#
-module TopicQuerySQL
-
-  class << self
-
-    def lowest_date
-      "1900-01-01"
-    end
-
-    def order_by_category_sql(dir)
-      -"CASE WHEN categories.id = #{SiteSetting.uncategorized_category_id.to_i} THEN '' ELSE categories.name END #{dir}"
-    end
-
-    # If you've cleared the pin, use bumped_at, otherwise put it at the top
-    def order_with_pinned_sql
-      -"CASE
-        WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
-          THEN topics.pinned_at + interval '9999 years'
-          ELSE topics.bumped_at
-       END DESC"
-    end
-
-    # If you've cleared the pin, use bumped_at, otherwise put it at the top
-    def order_nocategory_with_pinned_sql
-      -"CASE
-        WHEN topics.pinned_globally
-         AND (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
-          THEN topics.pinned_at + interval '9999 years'
-          ELSE topics.bumped_at
-       END DESC"
-    end
-
-    def order_basic_bumped
-      "CASE WHEN (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
-    end
-
-    def order_nocategory_basic_bumped
-      "CASE WHEN topics.pinned_globally AND (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
-    end
-
-    def order_top_for(score)
-      -"COALESCE(top_topics.#{score}, 0) DESC, topics.bumped_at DESC"
-    end
-
-    def order_top_with_pinned_category_for(score)
-      # display pinned topics first
-      -"CASE WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}')) THEN 0 ELSE 1 END,
-       top_topics.#{score} DESC,
-       topics.bumped_at DESC"
-    end
-
-    def order_top_with_notification_levels(score)
-      -"COALESCE(topic_users.notification_level, 1) DESC, COALESCE(category_users.notification_level, 1) DESC, COALESCE(top_topics.#{score}, 0) DESC, topics.bumped_at DESC"
-    end
-
-  end
-end
diff --git a/lib/zeitwerk_config.rb b/lib/zeitwerk_config.rb
index 92a066b..0c5882f 100644
--- a/lib/zeitwerk_config.rb
+++ b/lib/zeitwerk_config.rb
@@ -11,7 +11,6 @@ module ActiveSupport::Dependencies::ZeitwerkIntegration::Inflector

[... diff too long, it was truncated ...]

GitHub sha: 7b45a5ce55f9d15e605dd47a09fc2f3f5eba25f1

This commit appears in #13834 which was approved by Falco. It was merged by eviltrout.