From f41908ad5b589edfbe27700ec15c5954ae45149f Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Thu, 22 Jul 2021 16:31:53 +1000
Subject: [PATCH] SECURITY: Validate period param for top topic routes (#13818)

Fixes a possible SQL injection vector
---
 lib/topic_query.rb                    |  4 ++++
 spec/components/topic_query_spec.rb   | 18 ++++++++++++++++++
 spec/requests/list_controller_spec.rb |  5 +++++
 3 files changed, 27 insertions(+)

diff --git a/lib/topic_query.rb b/lib/topic_query.rb
index 12e759a22c8..122a344d647 100644
--- a/lib/topic_query.rb
+++ b/lib/topic_query.rb
@@ -266,6 +266,10 @@ 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"
     create_list(:top, unordered: true) do |topics|
       topics = remove_muted_categories(topics, @user)
diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb
index 041f519cd60..b4cc1efa27f 100644
--- a/spec/components/topic_query_spec.rb
+++ b/spec/components/topic_query_spec.rb
@@ -355,6 +355,24 @@ describe TopicQuery do
     end
   end
 
+  context "#list_top_for" do
+    it "lists top for the week" do
+      Fabricate(:topic, like_count: 1000, posts_count: 100)
+      TopTopic.refresh!
+      expect(topic_query.list_top_for(:weekly).topics.count).to eq(1)
+    end
+
+    it "only allows periods defined by TopTopic.periods" do
+      expect { topic_query.list_top_for(:all) }.not_to raise_error
+      expect { topic_query.list_top_for(:yearly) }.not_to raise_error
+      expect { topic_query.list_top_for(:quarterly) }.not_to raise_error
+      expect { topic_query.list_top_for(:monthly) }.not_to raise_error
+      expect { topic_query.list_top_for(:weekly) }.not_to raise_error
+      expect { topic_query.list_top_for(:daily) }.not_to raise_error
+      expect { topic_query.list_top_for("some bad input") }.to raise_error(Discourse::InvalidParameters)
+    end
+  end
+
   context 'mute_all_categories_by_default' do
     fab!(:category) { Fabricate(:category_with_definition) }
     fab!(:topic) { Fabricate(:topic, category: category) }
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index 762d9aeff2f..e84888a00a8 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -410,6 +410,11 @@ RSpec.describe ListController do
       expect(response.media_type).to eq('application/rss+xml')
     end
 
+    it 'errors for invalid periods on top RSS' do
+      get "/top.rss?period=decadely"
+      expect(response.status).to eq(400)
+    end
+
     TopTopic.periods.each do |period|
       it "renders #{period} top RSS" do
         get "/top.rss?period=#{period}"