FIX: Validate page/limit params for directory, user-badges and groups (#22877)

We'll now return a 400 error instead of 500. 400 is a better description of the issue, and also avoids creating unnecessary noise in the logs.

app/controllers/application_controller.rb

@@ -1086,18 +1086,29 @@ class ApplicationController < ActionController::Base
   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
+    fetch_int_from_params(:limit, params: params, default: default, max: max)
+  end
 
-    if params.has_key?(:limit)
-      limit =
+  def fetch_int_from_params(key, params: self.params, default:, min: 0, max: nil)
+    key = key.to_sym
+
+    if default.present? && ((max.present? && default > max) || (min.present? && default < min))
+      raise "default #{key.inspect} is not between #{min.inspect} and #{max.inspect}"
+    end
+
+    if params.has_key?(key)
+      value =
         begin
-          Integer(params[:limit])
+          Integer(params[key])
         rescue ArgumentError
-          raise Discourse::InvalidParameters.new(:limit)
+          raise Discourse::InvalidParameters.new(key)
         end
 
-      raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > max
-      limit
+      if (min.present? && value < min) || (max.present? && value > max)
+        raise Discourse::InvalidParameters.new(key)
+      end
+
+      value
     else
       default
     end

app/controllers/directory_items_controller.rb

@@ -58,7 +58,7 @@ class DirectoryItemsController < ApplicationController
     end
 
     result = result.includes(:user_stat) if period_type == DirectoryItem.period_types[:all]
-    page = params[:page].to_i
+    page = fetch_int_from_params(:page, default: 0)
 
     user_ids = nil
     if params[:name].present?

app/controllers/groups_controller.rb

@@ -88,7 +88,7 @@ class GroupsController < ApplicationController
     # count the total before doing pagination
     total = groups.count
 
-    page = params[:page].to_i
+    page = fetch_int_from_params(:page, default: 0)
     page_size = MobileDetection.mobile_device?(request.user_agent) ? 15 : 36
     groups = groups.offset(page * page_size).limit(page_size)
 

app/controllers/user_badges_controller.rb

@@ -27,9 +27,8 @@ class UserBadgesController < ApplicationController
       grant_count = badge.user_badges.where(user_id: user_id).count
     end
 
-    if offset = params[:offset]
-      user_badges = user_badges.offset(offset.to_i)
-    end
+    offset = fetch_int_from_params(:offset, default: 0)
+    user_badges = user_badges.offset(offset) if offset > 0
 
     user_badges_topic_ids = user_badges.map { |user_badge| user_badge.post&.topic_id }.compact
 

spec/requests/directory_items_controller_spec.rb

@@ -33,6 +33,16 @@ RSpec.describe DirectoryItemsController do
     include_examples "invalid limit params", "/directory_items.json", described_class::PAGE_SIZE
   end
 
+  context "with page parameter" do
+    it "only accepts valid page numbers" do
+      get "/directory_items.json", params: { period: "all", page: -1 }
+      expect(response.status).to eq(400)
+
+      get "/directory_items.json", params: { period: "all", page: 0 }
+      expect(response.status).to eq(200)
+    end
+  end
+
   context "with exclude_groups parameter" do
     before { DirectoryItem.refresh! }
 

spec/requests/groups_controller_spec.rb

@@ -38,6 +38,17 @@ RSpec.describe GroupsController do
       expect(body["load_more_groups"]).to eq("/groups?page=2")
     end
 
+    it "only accepts valid page numbers" do
+      get "/groups.json", params: { page: -1 }
+      expect(response.status).to eq(400)
+
+      get "/groups.json", params: { page: 0 }
+      expect(response.status).to eq(200)
+
+      get "/groups.json", params: { page: 1 }
+      expect(response.status).to eq(200)
+    end
+
     context "when group directory is disabled" do
       before { SiteSetting.enable_group_directory = false }
 

spec/requests/user_badges_controller_spec.rb

@@ -31,6 +31,14 @@ RSpec.describe UserBadgesController do
       get "/user_badges.json", params: { badge_id: badge.id }
       expect(response.status).to eq(404)
     end
+
+    it "only accepts valid offset params" do
+      get "/user_badges.json", params: { badge_id: badge.id, offset: -1 }
+      expect(response.status).to eq(400)
+
+      get "/user_badges.json", params: { badge_id: badge.id, offset: 100 }
+      expect(response.status).to eq(200)
+    end
   end
 
   describe "#index" do