FIX: Return 400 response codes when topic list query params are invalid (#27930)

This commit updates `TopicQuery.validators` to cover all of the
public options listed in `TopicQuery.public_valid_options`. This is done
to fix the app returning a 500 response code when an invalid value, such
as a hash, is passed as a query param when accessing the various topic
list routes.

lib/topic_query.rb

@@ -8,8 +8,8 @@
 class TopicQuery
   include PrivateMessageLists
 
-  PG_MAX_INT ||= 2_147_483_647
+  PG_MAX_INT = 2_147_483_647
-  DEFAULT_PER_PAGE_COUNT ||= 30
+  DEFAULT_PER_PAGE_COUNT = 30
 
   def self.validators
     @validators ||=
@@ -17,14 +17,32 @@ class TopicQuery
         int = lambda { |x| Integer === x || (String === x && x.match?(/\A-?[0-9]+\z/)) }
         zero_up_to_max_int = lambda { |x| int.call(x) && x.to_i.between?(0, PG_MAX_INT) }
         array_or_string = lambda { |x| Array === x || String === x }
+        string = lambda { |x| String === x }
+        true_or_false = lambda { |x| x == true || x == false || x == "true" || x == "false" }
 
         {
+          page: zero_up_to_max_int,
           before: zero_up_to_max_int,
           bumped_before: zero_up_to_max_int,
-          max_posts: zero_up_to_max_int,
+          topic_ids: array_or_string,
+          category: string,
+          order: string,
+          ascending: true_or_false,
           min_posts: zero_up_to_max_int,
-          page: zero_up_to_max_int,
+          max_posts: zero_up_to_max_int,
+          status: string,
+          filter: string,
+          state: string,
+          search: string,
+          q: string,
+          f: string,
+          subset: string,
+          group_name: string,
           tags: array_or_string,
+          match_all_tags: true_or_false,
+          no_subcategories: true_or_false,
+          no_tags: true_or_false,
+          exclude_tag: string,
         }
       end
   end
@@ -757,7 +775,7 @@ class TopicQuery
     category_id = get_category_id(options[:category])
     @options[:category_id] = category_id
     if category_id
-      if options[:no_subcategories]
+      if ActiveModel::Type::Boolean.new.cast(options[:no_subcategories])
         result = result.where("topics.category_id = ?", category_id)
       else
         result = result.where("topics.category_id IN (?)", Category.subcategory_ids(category_id))
@@ -1010,7 +1028,7 @@ class TopicQuery
     return list if muted_tag_ids.blank?
 
     # if viewing the topic list for a muted tag, show all the topics
-    if !opts[:no_tags] && opts[:tags].present?
+    if !ActiveModel::Type::Boolean.new.cast(opts[:no_tags]) && opts[:tags].present?
       if TagUser
            .lookup(user, :muted)
            .joins(:tag)
@@ -1286,7 +1304,7 @@ class TopicQuery
       end
 
       @options[:tag_ids] = tags
-    elsif @options[:no_tags]
+    elsif ActiveModel::Type::Boolean.new.cast(@options[:no_tags])
       # the following will do: ("topics"."id" NOT IN (SELECT DISTINCT "topic_tags"."topic_id" FROM "topic_tags"))
       result = result.where.not(id: TopicTag.distinct.select(:topic_id))
     end

lib/topic_query_params.rb

@@ -6,17 +6,14 @@ module TopicQueryParams
     params[:tags] = [params[:tag_id], *Array(params[:tags])].uniq if params[:tag_id].present?
 
     TopicQuery.public_valid_options.each do |key|
-      if params.key?(key)
+      if params.key?(key) && (val = params[key]).present?
-        val = options[key] = params[key]
+        options[key] = val
         raise Discourse::InvalidParameters.new key if !TopicQuery.validate?(key, val)
       end
     end
 
     # hacky columns get special handling
     options[:topic_ids] = param_to_integer_list(:topic_ids)
-    options[:no_subcategories] = options[:no_subcategories] == "true" if options[
-      :no_subcategories
-    ].present?
 
     options
   end

spec/requests/list_controller_spec.rb

@@ -12,33 +12,134 @@ RSpec.describe ListController do
   end
 
   describe "#index" do
-    it "does not return a 500 for invalid input" do
+    context "when params are invalid" do
-      get "/latest?min_posts=bob"
+      it "should return a 400 response when `page` param is a string that represent a negative integer" do
-      expect(response.status).to eq(400)
+        get "/latest?page=-1"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?max_posts=bob"
+      it "should return a 400 response when `page` param is a string larger than maximum integer value" do
-      expect(response.status).to eq(400)
+        get "/latest?page=2147483648"
+        expect(response.status).to eq(400)
 
-      get "/latest?max_posts=1111111111111111111111111111111111111111"
+        get "/latest?page=1111111111111111111111111111111111111111"
-      expect(response.status).to eq(400)
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?page=-1"
+      it "should return a 400 response when `before` param is not a string represetning an integer" do
-      expect(response.status).to eq(400)
+        get "/latest?before[1]=haxx"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?page=2147483648"
+      it "should return a 400 response when `bumped_before` param is not a string representing an integer" do
-      expect(response.status).to eq(400)
+        get "/latest?bumped_before[1]=haxx"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?page=1111111111111111111111111111111111111111"
+      it "should return a 400 response when `topic_ids` param is not a string representing an integer" do
-      expect(response.status).to eq(400)
+        get "/latest?topic_ids[1]=haxx"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?tags[1]=hello"
+      it "should return a 400 response when `category` param is not a string representing an integer" do
-      expect(response.status).to eq(400)
+        get "/latest?category[1]=haxx"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?before[1]=haxx"
+      it "should return a 400 response when `order` param is not a string" do
-      expect(response.status).to eq(400)
+        get "/latest?order[1]=haxx"
+        expect(response.status).to eq(400)
+      end
 
-      get "/latest?bumped_before[1]=haxx"
+      it "should return a 400 response when `ascending` param is not a string that is either `true` or `false`" do
-      expect(response.status).to eq(400)
+        get "/latest?ascending=maybe"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `min_posts` param is a string that does not represent an integer" do
+        get "/latest?min_posts=bob"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `max_posts` param is a string that does not represent an integer" do
+        get "/latest?max_posts=bob"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `max_posts` param is a string larger than maximum integer value" do
+        get "/latest?max_posts=1111111111111111111111111111111111111111"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `status` param is not a string" do
+        get "/latest?status%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `filter` param is not a string" do
+        get "/latest?filter%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `state` param is not a string" do
+        get "/latest?state%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `search` param is not a string" do
+        get "/latest?search%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `q` param is not a string" do
+        get "/latest?q%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `f` param is not a string" do
+        get "/latest?f%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `subset` param is not a string" do
+        get "/latest?subset%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `group_name` param is not a string" do
+        get "/latest?group_name%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `tags` param is not an array or string" do
+        get "/latest?tags[1]=hello"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `filter` param is not a string" do
+        get "/latest?filter%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `match_all_tags` param is not a string that is either `true` or `false`" do
+        get "/latest?match_all_tags=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `no_subcategories` param is not a string that is either `true` or `false`" do
+        get "/latest?no_subcategories=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `no_tags` param is not a string that is either `true` or `false`" do
+        get "/latest?no_tags=something"
+        expect(response.status).to eq(400)
+      end
+
+      it "should return a 400 response when `exclude_tag` param is not a string" do
+        get "/latest?exclude_tag%5Bsomehash%5D=something"
+        expect(response.status).to eq(400)
+      end
     end
 
     it "returns 200 for legit requests" do