From 7b45a5ce55f9d15e605dd47a09fc2f3f5eba25f1 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Fri, 23 Jul 2021 13:52:35 -0400 Subject: [PATCH] FIX: Better and more secure validation of periods for TopicQuery Co-authored-by: Martin Brennan --- app/controllers/embed_controller.rb | 9 +++- app/controllers/list_controller.rb | 2 + app/controllers/tags_controller.rb | 2 + app/models/top_topic.rb | 11 +++++ app/models/topic.rb | 10 +++-- lib/topic_query.rb | 24 +++++++---- lib/topic_query_sql.rb | 60 -------------------------- lib/zeitwerk_config.rb | 1 - spec/requests/embed_controller_spec.rb | 19 ++++++++ spec/requests/tags_controller_spec.rb | 5 +++ 10 files changed, 68 insertions(+), 75 deletions(-) delete mode 100644 lib/topic_query_sql.rb diff --git a/app/controllers/embed_controller.rb b/app/controllers/embed_controller.rb index 490ff8c698c..4bbd7796abc 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 b4fba96cb3b..b0d3f6bd100 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 9394615aaa2..912a60c9c77 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 739f6ce66cc..3f09fda10e7 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 a618b569232..601c9759775 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 122a344d647..82b76cf28a9 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 c96f46c68c6..00000000000 --- 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 92a066b8339..0c5882f8cab 100644 --- a/lib/zeitwerk_config.rb +++ b/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', } diff --git a/spec/requests/embed_controller_spec.rb b/spec/requests/embed_controller_spec.rb index 44b21a9c63c..e95a78c550a 100644 --- a/spec/requests/embed_controller_spec.rb +++ b/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: { diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb index a9836194636..ea6334232d2 100644 --- a/spec/requests/tags_controller_spec.rb +++ b/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