FIX: Better and more secure validation of periods for TopicQuery

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

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)

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]

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

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

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]

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'

lib/topic_query_sql.rb

@@ -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

lib/zeitwerk_config.rb

@@ -11,7 +11,6 @@ module ActiveSupport::Dependencies::ZeitwerkIntegration::Inflector
     'pop3_polling_enabled_setting_validator' => 'POP3PollingEnabledSettingValidator',
     'regular' => 'Jobs',
     'scheduled' => 'Jobs',
-    'topic_query_sql' => 'TopicQuerySQL',
     'version' => 'Discourse',
   }
 

spec/requests/embed_controller_spec.rb

@@ -116,6 +116,25 @@ describe EmbedController do
         expect(response.body).to match("data-referer=\"https://example.com/evil-trout\"")
       end
 
+      it "returns a list of topics if the top_period is not valid" do
+        topic1 = Fabricate(:topic)
+        topic2 = Fabricate(:topic)
+        good_topic = Fabricate(:topic, like_count: 1000, posts_count: 100)
+        TopTopic.refresh!
+        TopicQuery.any_instance.expects(:list_top_for).never
+
+        get '/embed/topics?discourse_embed_id=de-1234&top_period=decadely', headers: {
+          'REFERER' => 'https://example.com/evil-trout'
+        }
+        expect(response.status).to eq(200)
+        expect(response.headers['X-Frame-Options']).to be_nil
+        expect(response.body).to match("data-embed-id=\"de-1234\"")
+        expect(response.body).to match("data-topic-id=\"#{good_topic.id}\"")
+        expect(response.body).to match("data-topic-id=\"#{topic1.id}\"")
+        expect(response.body).to match("data-topic-id=\"#{topic2.id}\"")
+        expect(response.body).to match("data-referer=\"https://example.com/evil-trout\"")
+      end
+
       it "wraps the list in a custom class" do
         topic = Fabricate(:topic)
         get '/embed/topics?discourse_embed_id=de-1234&embed_class=my-special-class', headers: {

spec/requests/tags_controller_spec.rb

@@ -717,6 +717,11 @@ describe TagsController do
       topic_ids = response.parsed_body["topic_list"]["topics"].map { |topic| topic["id"] }
       expect(topic_ids).to eq([tag_topic.id])
     end
+
+    it "raises an error if the period is not valid" do
+      get "/tag/#{tag.name}/l/top.json?period=decadely"
+      expect(response.status).to eq(400)
+    end
   end
 
   describe '#search' do