SECURITY: Impose a upper bound on limit params in various controllers

What is the problem here?

In multiple controllers, we are accepting a `limit` params but do not
impose any upper bound on the values being accepted. Without an upper
bound, we may be allowing arbituary users from generating DB queries
which may end up exhausing the resources on the server.

What is the fix here?

A new `fetch_limit_from_params` helper method is introduced in
`ApplicationController` that can be used by controller actions to safely
get the limit from the params as a default limit and maximum limit has
to be set. When an invalid limit params is encountered, the server will
respond with the 400 response code.

app/controllers/admin/api_controller.rb

@@ -4,9 +4,11 @@ class Admin::ApiController < Admin::AdminController
   # Note: in the REST API, ApiKeys are referred to simply as "key"
   # If we used "api_key", then our user provider would try to use the value for authentication
 
+  INDEX_LIMIT = 50
+
   def index
     offset = (params[:offset] || 0).to_i
-    limit = (params[:limit] || 50).to_i.clamp(1, 50)
+    limit = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
 
     keys =
       ApiKey

app/controllers/admin/reports_controller.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class Admin::ReportsController < Admin::StaffController
+  REPORTS_LIMIT = 50
+
   def index
     reports_methods =
       ["page_view_total_reqs"] +
@@ -108,10 +110,7 @@ class Admin::ReportsController < Admin::StaffController
     facets = nil
     facets = report_params[:facets].map { |s| s.to_s.to_sym } if Array === report_params[:facets]
 
-    limit = nil
-    if report_params.has_key?(:limit) && report_params[:limit].to_i > 0
-      limit = report_params[:limit].to_i
-    end
+    limit = fetch_limit_from_params(params: report_params, default: nil, max: REPORTS_LIMIT)
 
     filters = nil
     filters = report_params[:filters] if report_params.has_key?(:filters)

app/controllers/admin/staff_action_logs_controller.rb

@@ -1,11 +1,13 @@
 # frozen_string_literal: true
 
 class Admin::StaffActionLogsController < Admin::StaffController
+  INDEX_LIMIT = 200
+
   def index
     filters = params.slice(*UserHistory.staff_filters + %i[page limit])
 
     page = (params[:page] || 0).to_i
-    page_size = (params[:limit] || 200).to_i.clamp(1, 200)
+    page_size = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
 
     staff_action_logs = UserHistory.staff_action_records(current_user, filters)
     count = staff_action_logs.count

app/controllers/application_controller.rb

@@ -1084,4 +1084,22 @@ class ApplicationController < ActionController::Base
         end
       end
   end
+
+  def fetch_limit_from_params(params: self.params, default:, max:)
+    raise "default limit cannot be greater than max limit" if default.present? && default > max
+
+    if params.has_key?(:limit)
+      limit =
+        begin
+          Integer(params[:limit])
+        rescue ArgumentError
+          raise Discourse::InvalidParameters.new(:limit)
+        end
+
+      raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > max
+      limit
+    else
+      default
+    end
+  end
 end

app/controllers/directory_items_controller.rb

@@ -81,8 +81,7 @@ class DirectoryItemsController < ApplicationController
       end
     end
 
-    limit = [params[:limit].to_i, PAGE_SIZE].min if params[:limit].to_i > 0
-    limit ||= PAGE_SIZE
+    limit = fetch_limit_from_params(default: PAGE_SIZE, max: PAGE_SIZE)
 
     result_count = result.count
     result = result.limit(limit).offset(limit * page).to_a

app/controllers/drafts_controller.rb

@@ -5,11 +5,17 @@ class DraftsController < ApplicationController
 
   skip_before_action :check_xhr, :preload_json
 
+  INDEX_LIMIT = 50
+
   def index
     params.permit(:offset)
-    params.permit(:limit)
 
-    stream = Draft.stream(user: current_user, offset: params[:offset], limit: params[:limit])
+    stream =
+      Draft.stream(
+        user: current_user,
+        offset: params[:offset],
+        limit: fetch_limit_from_params(default: nil, max: INDEX_LIMIT),
+      )
 
     render json: { drafts: stream ? serialize_data(stream, DraftSerializer) : [] }
   end

app/controllers/groups_controller.rb

@@ -230,15 +230,16 @@ class GroupsController < ApplicationController
     render "posts/latest", formats: [:rss]
   end
 
+  MEMBERS_LIMIT = 1_000
+
   def members
     group = find_group(:group_id)
 
     guardian.ensure_can_see_group_members!(group)
 
-    limit = (params[:limit] || 50).to_i
+    limit = fetch_limit_from_params(default: 50, max: MEMBERS_LIMIT)
     offset = params[:offset].to_i
 
-    raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > 1000
     raise Discourse::InvalidParameters.new(:offset) if offset < 0
 
     dir = (params[:asc] && params[:asc].present?) ? "ASC" : "DESC"

app/controllers/notifications_controller.rb

@@ -5,6 +5,8 @@ class NotificationsController < ApplicationController
   before_action :ensure_admin, only: %i[create update destroy]
   before_action :set_notification, only: %i[update destroy]
 
+  INDEX_LIMIT = 50
+
   def index
     user =
       if params[:username] && !params[:recent]
@@ -25,8 +27,7 @@ class NotificationsController < ApplicationController
     end
 
     if params[:recent].present?
-      limit = (params[:limit] || 15).to_i
-      limit = 50 if limit > 50
+      limit = fetch_limit_from_params(default: 15, max: INDEX_LIMIT)
 
       include_reviewables = false
 

app/controllers/post_action_users_controller.rb

@@ -1,13 +1,15 @@
 # frozen_string_literal: true
 
 class PostActionUsersController < ApplicationController
+  INDEX_LIMIT = 200
+
   def index
     params.require(:post_action_type_id)
     params.require(:id)
     post_action_type_id = params[:post_action_type_id].to_i
 
     page = params[:page].to_i
-    page_size = (params[:limit] || 200).to_i
+    page_size = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
 
     # Find the post, and then determine if they can see the post (if deleted)
     post = Post.with_deleted.where(id: params[:id].to_i).first

app/controllers/posts_controller.rb

@@ -662,13 +662,15 @@ class PostsController < ApplicationController
     render body: nil
   end
 
+  DELETED_POSTS_MAX_LIMIT = 100
+
   def deleted_posts
     params.permit(:offset, :limit)
     guardian.ensure_can_see_deleted_posts!
 
     user = fetch_user_from_params
     offset = [params[:offset].to_i, 0].max
-    limit = [(params[:limit] || 60).to_i, 100].min
+    limit = fetch_limit_from_params(default: 60, max: DELETED_POSTS_MAX_LIMIT)
 
     posts = user_posts(guardian, user.id, offset: offset, limit: limit).where.not(deleted_at: nil)
 

app/controllers/tag_groups_controller.rb

@@ -79,7 +79,10 @@ class TagGroupsController < ApplicationController
       matches = matches.where("lower(NAME) in (?)", params[:names].map(&:downcase))
     end
 
-    matches = matches.order("name").limit(params[:limit] || 5)
+    matches =
+      matches.order("name").limit(
+        fetch_limit_from_params(default: 5, max: SiteSetting.max_tag_search_results),
+      )
 
     render json: {
              results:

app/controllers/tags_controller.rb

@@ -292,13 +292,8 @@ class TagsController < ::ApplicationController
       exclude_has_synonyms: params[:excludeHasSynonyms],
     }
 
-    if params[:limit]
-      begin
-        filter_params[:limit] = Integer(params[:limit])
-        raise Discourse::InvalidParameters.new(:limit) if !filter_params[:limit].positive?
-      rescue ArgumentError
-        raise Discourse::InvalidParameters.new(:limit)
-      end
+    if limit = fetch_limit_from_params(default: nil, max: SiteSetting.max_tag_search_results)
+      filter_params[:limit] = limit
     end
 
     filter_params[:category] = Category.find_by_id(params[:categoryId]) if params[:categoryId]

app/controllers/users_controller.rb

@@ -1182,6 +1182,8 @@ class UsersController < ApplicationController
     end
   end
 
+  SEARCH_USERS_LIMIT = 50
+
   def search_users
     term = params[:term].to_s.strip
 
@@ -1204,10 +1206,11 @@ class UsersController < ApplicationController
       params[:include_staged_users],
     )
     options[:last_seen_users] = !!ActiveModel::Type::Boolean.new.cast(params[:last_seen_users])
-    if params[:limit].present?
-      options[:limit] = params[:limit].to_i
-      raise Discourse::InvalidParameters.new(:limit) if options[:limit] <= 0
+
+    if limit = fetch_limit_from_params(default: nil, max: SEARCH_USERS_LIMIT)
+      options[:limit] = limit
     end
+
     options[:topic_id] = topic_id if topic_id
     options[:category_id] = category_id if category_id
 
@@ -1733,6 +1736,8 @@ class UsersController < ApplicationController
     render json: success_json
   end
 
+  BOOKMARKS_LIMIT = 20
+
   def bookmarks
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
@@ -1740,7 +1745,15 @@ class UsersController < ApplicationController
 
     respond_to do |format|
       format.json do
-        bookmark_list = UserBookmarkList.new(user: user, guardian: guardian, params: params)
+        bookmark_list =
+          UserBookmarkList.new(
+            user: user,
+            guardian: guardian,
+            search_term: params[:q],
+            page: params[:page],
+            per_page: fetch_limit_from_params(default: nil, max: BOOKMARKS_LIMIT),
+          )
+
         bookmark_list.load
 
         if bookmark_list.bookmarks.empty?
@@ -1789,10 +1802,9 @@ class UsersController < ApplicationController
         UserBookmarkList.new(
           user: current_user,
           guardian: guardian,
-          params: {
-            per_page: USER_MENU_LIST_LIMIT - reminder_notifications.size,
-          },
+          per_page: USER_MENU_LIST_LIMIT - reminder_notifications.size,
         )
+
       bookmark_list.load do |query|
         if exclude_bookmark_ids.present?
           query.where("bookmarks.id NOT IN (?)", exclude_bookmark_ids)