From 6e8e3c3151581b20e88d13b3bbfdedb5d7750037 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 31 Jul 2023 15:00:05 +0100 Subject: [PATCH] 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 | 25 +++++++++++++------ app/controllers/directory_items_controller.rb | 2 +- app/controllers/groups_controller.rb | 2 +- app/controllers/user_badges_controller.rb | 5 ++-- .../directory_items_controller_spec.rb | 10 ++++++++ spec/requests/groups_controller_spec.rb | 11 ++++++++ spec/requests/user_badges_controller_spec.rb | 8 ++++++ 7 files changed, 51 insertions(+), 12 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 79c652a3fe4..6aeed83ee5d 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/directory_items_controller.rb b/app/controllers/directory_items_controller.rb index 7d70b1eeae7..fb1531c19e8 100644 --- a/app/controllers/directory_items_controller.rb +++ b/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? diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index a3a267815f0..bbd465858a2 100644 --- a/app/controllers/groups_controller.rb +++ b/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) diff --git a/app/controllers/user_badges_controller.rb b/app/controllers/user_badges_controller.rb index ac0e78be29a..e06701e6ee7 100644 --- a/app/controllers/user_badges_controller.rb +++ b/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 diff --git a/spec/requests/directory_items_controller_spec.rb b/spec/requests/directory_items_controller_spec.rb index 94eaec500b8..070c4c32cfe 100644 --- a/spec/requests/directory_items_controller_spec.rb +++ b/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! } diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 001b8c8cb4f..738545d3609 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/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 } diff --git a/spec/requests/user_badges_controller_spec.rb b/spec/requests/user_badges_controller_spec.rb index 67f585540f1..f21149d02de 100644 --- a/spec/requests/user_badges_controller_spec.rb +++ b/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