From 25778d98611226593a44e979ea98021720546923 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 16 Jul 2024 10:30:04 +0800 Subject: [PATCH] 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 | 32 ++++-- lib/topic_query_params.rb | 7 +- spec/requests/list_controller_spec.rb | 139 ++++++++++++++++++++++---- 3 files changed, 147 insertions(+), 31 deletions(-) diff --git a/lib/topic_query.rb b/lib/topic_query.rb index e69eb6eec4d..036254f1868 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -8,8 +8,8 @@ class TopicQuery include PrivateMessageLists - PG_MAX_INT ||= 2_147_483_647 - DEFAULT_PER_PAGE_COUNT ||= 30 + PG_MAX_INT = 2_147_483_647 + 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 diff --git a/lib/topic_query_params.rb b/lib/topic_query_params.rb index 4195c3709aa..db8ba7cff3c 100644 --- a/lib/topic_query_params.rb +++ b/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) - val = options[key] = params[key] + if params.key?(key) && (val = params[key]).present? + 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 diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 620d5489076..fc18c743bab 100644 --- a/spec/requests/list_controller_spec.rb +++ b/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 - get "/latest?min_posts=bob" - expect(response.status).to eq(400) + context "when params are invalid" do + it "should return a 400 response when `page` param is a string that represent a negative integer" do + get "/latest?page=-1" + expect(response.status).to eq(400) + end - get "/latest?max_posts=bob" - expect(response.status).to eq(400) + it "should return a 400 response when `page` param is a string larger than maximum integer value" do + get "/latest?page=2147483648" + expect(response.status).to eq(400) - get "/latest?max_posts=1111111111111111111111111111111111111111" - expect(response.status).to eq(400) + get "/latest?page=1111111111111111111111111111111111111111" + expect(response.status).to eq(400) + end - get "/latest?page=-1" - expect(response.status).to eq(400) + it "should return a 400 response when `before` param is not a string represetning an integer" do + get "/latest?before[1]=haxx" + expect(response.status).to eq(400) + end - get "/latest?page=2147483648" - expect(response.status).to eq(400) + it "should return a 400 response when `bumped_before` param is not a string representing an integer" do + get "/latest?bumped_before[1]=haxx" + expect(response.status).to eq(400) + end - get "/latest?page=1111111111111111111111111111111111111111" - expect(response.status).to eq(400) + it "should return a 400 response when `topic_ids` param is not a string representing an integer" do + get "/latest?topic_ids[1]=haxx" + expect(response.status).to eq(400) + end - get "/latest?tags[1]=hello" - expect(response.status).to eq(400) + it "should return a 400 response when `category` param is not a string representing an integer" do + get "/latest?category[1]=haxx" + expect(response.status).to eq(400) + end - get "/latest?before[1]=haxx" - expect(response.status).to eq(400) + it "should return a 400 response when `order` param is not a string" do + get "/latest?order[1]=haxx" + expect(response.status).to eq(400) + end - get "/latest?bumped_before[1]=haxx" - expect(response.status).to eq(400) + it "should return a 400 response when `ascending` param is not a string that is either `true` or `false`" do + 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