From 21fe7b8c12b2dd6101d5ffe2839d60ea196e1ce0 Mon Sep 17 00:00:00 2001 From: moveson Date: Mon, 24 Aug 2026 09:42:47 -0600 Subject: [PATCH 1/2] Add failing specs for negative page and per_page params Co-Authored-By: Claude Fable 5 --- .../concerns/prepared_params_spec.rb | 15 ++++ spec/presenters/base_presenter_spec.rb | 72 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 spec/presenters/base_presenter_spec.rb diff --git a/spec/controllers/concerns/prepared_params_spec.rb b/spec/controllers/concerns/prepared_params_spec.rb index 68ee83828..c0640e2ce 100644 --- a/spec/controllers/concerns/prepared_params_spec.rb +++ b/spec/controllers/concerns/prepared_params_spec.rb @@ -449,6 +449,21 @@ it { expect(result).to eq(1) } end + context "when page is a negative integer" do + let(:page) { -11 } + it { expect(result).to eq(1) } + end + + context "when page is a negative string" do + let(:page) { "-11" } + it { expect(result).to eq(1) } + end + + context "when page is a SQL injection probe" do + let(:page) { "-11' UNION ALL SELECT NULL,NULL--" } + it { expect(result).to eq(1) } + end + context "when page is integer 1" do let(:page) { 1 } it { expect(result).to eq(1) } diff --git a/spec/presenters/base_presenter_spec.rb b/spec/presenters/base_presenter_spec.rb new file mode 100644 index 000000000..88afd0c5c --- /dev/null +++ b/spec/presenters/base_presenter_spec.rb @@ -0,0 +1,72 @@ +require "rails_helper" + +RSpec.describe BasePresenter do + subject { presenter_class.new(prepared_params) } + + let(:presenter_class) do + Class.new(described_class) do + def initialize(params) + @params = params + end + + private + + attr_reader :params + end + end + + let(:prepared_params) { PreparedParams.new(ActionController::Parameters.new(query_params), [], []) } + + describe "#page" do + let(:result) { subject.page } + + context "when page is not provided" do + let(:query_params) { {} } + it { expect(result).to eq(1) } + end + + context "when page is zero" do + let(:query_params) { { page: "0" } } + it { expect(result).to eq(1) } + end + + context "when page is positive" do + let(:query_params) { { page: "3" } } + it { expect(result).to eq(3) } + end + + context "when page is negative" do + let(:query_params) { { page: "-11" } } + it { expect(result).to eq(1) } + end + + context "when page is a SQL injection probe" do + let(:query_params) { { page: "-11' UNION ALL SELECT NULL,NULL--" } } + it { expect(result).to eq(1) } + end + end + + describe "#per_page" do + let(:result) { subject.per_page } + + context "when per_page is not provided" do + let(:query_params) { {} } + it { expect(result).to eq(described_class::DEFAULT_PER_PAGE) } + end + + context "when per_page is zero" do + let(:query_params) { { per_page: "0" } } + it { expect(result).to eq(described_class::DEFAULT_PER_PAGE) } + end + + context "when per_page is positive" do + let(:query_params) { { per_page: "25" } } + it { expect(result).to eq(25) } + end + + context "when per_page is negative" do + let(:query_params) { { per_page: "-5" } } + it { expect(result).to eq(described_class::DEFAULT_PER_PAGE) } + end + end +end From 74b9bdd00fb5176082f02734572dbf66819c2a99 Mon Sep 17 00:00:00 2001 From: moveson Date: Mon, 24 Aug 2026 09:43:49 -0600 Subject: [PATCH 2/2] Clamp non-positive page and per_page params to their defaults The page guards in PreparedParams and BasePresenter handled zero but not negatives, so SQL-injection scanner probes like page=-11'... (which to_i reduces to -11) flowed into pagy, raised mid-render, and 500ed (~34 times/day on the course best efforts page). Any value below 1 now falls back to the first page (or default per_page), so bots get a 200 and Scout stays quiet. Resolves #2236 Co-Authored-By: Claude Fable 5 --- app/controllers/concerns/prepared_params.rb | 2 +- app/presenters/base_presenter.rb | 4 ++-- spec/presenters/base_presenter_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/prepared_params.rb b/app/controllers/concerns/prepared_params.rb index 41bbda3cf..cf7a65c83 100644 --- a/app/controllers/concerns/prepared_params.rb +++ b/app/controllers/concerns/prepared_params.rb @@ -53,7 +53,7 @@ def original_params def page result = params[:page]&.to_i || FIRST_PAGE - result.zero? ? FIRST_PAGE : result + result < 1 ? FIRST_PAGE : result end def search diff --git a/app/presenters/base_presenter.rb b/app/presenters/base_presenter.rb index ba4f3f345..7b208afd2 100644 --- a/app/presenters/base_presenter.rb +++ b/app/presenters/base_presenter.rb @@ -41,12 +41,12 @@ def genders def page result = params[:page]&.to_i || FIRST_PAGE - result.zero? ? FIRST_PAGE : result + result < 1 ? FIRST_PAGE : result end def per_page result = params[:per_page]&.to_i || DEFAULT_PER_PAGE - result.zero? ? DEFAULT_PER_PAGE : result + result < 1 ? DEFAULT_PER_PAGE : result end def search_text diff --git a/spec/presenters/base_presenter_spec.rb b/spec/presenters/base_presenter_spec.rb index 88afd0c5c..c5000509b 100644 --- a/spec/presenters/base_presenter_spec.rb +++ b/spec/presenters/base_presenter_spec.rb @@ -5,7 +5,7 @@ let(:presenter_class) do Class.new(described_class) do - def initialize(params) + def initialize(params) # rubocop:disable Lint/MissingSuper -- the parent initializer raises NotImplementedError @params = params end