Merge branch 'security-enforce-max_page-validation' into 'master'

Enforce per_page validation for Branches/TagsFinders

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/3966



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Michał Zając <mzajac@gitlab.com>
Approved-by: Hunter Stewart <hustewart@gitlab.com>
Approved-by: Costel Maxim <cmaxim@gitlab.com>
Reviewed-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Co-authored-by: Vasilii Iakliushin <viakliushin@gitlab.com>

app/finders/branches_finder.rb

@@ -24,10 +24,6 @@ def names
     @params[:names].presence
   end
 
-  def per_page
-    @params[:per_page].presence
-  end
-
   def regex
     @params[:regex].to_s.presence
   end
@@ -37,10 +33,6 @@ def page_token
     "#{Gitlab::Git::BRANCH_REF_PREFIX}#{@params[:page_token]}" if @params[:page_token]
   end
 
-  def pagination_params
-    { limit: per_page, page_token: page_token }
-  end
-
   def by_names(branches)
     return branches unless names
 

app/finders/git_refs_finder.rb

@@ -33,6 +33,18 @@ def sort
     @params[:sort].to_s.presence || 'name'
   end
 
+  def pagination_params
+    { limit: per_page, page_token: page_token }
+  end
+
+  def per_page
+    return if params[:per_page].blank?
+
+    Gitlab::PaginationDelegate.new(
+      per_page: params[:per_page].presence, page: nil, count: nil
+    ).limit_value
+  end
+
   def filter_refs(refs, term)
     regex_string = RE2::Regexp.escape(term.downcase)
     regex_string = unescape_regex_operators(regex_string) if regex_search?

app/finders/tags_finder.rb

@@ -23,15 +23,7 @@ def total
 
   private
 
-  def per_page
-    params[:per_page].presence
-  end
-
   def page_token
     "#{Gitlab::Git::TAG_REF_PREFIX}#{@params[:page_token]}" if params[:page_token]
   end
-
-  def pagination_params
-    { limit: per_page, page_token: page_token }
-  end
 end

lib/gitlab/pagination_delegate.rb

@@ -53,9 +53,15 @@ def offset
     private
 
     def sanitize_per_page(per_page)
-      return @options[:default_per_page] unless per_page && per_page > 0
+      limit = begin
+        Integer(per_page)
+      rescue ArgumentError, TypeError
+        nil
+      end
 
-      [@options[:max_per_page], per_page].min
+      return @options[:default_per_page] unless limit && limit > 0
+
+      [@options[:max_per_page], limit].min
     end
 
     def sanitize_page(page)

spec/finders/branches_finder_spec.rb

@@ -304,6 +304,20 @@
 
           expect(result.map(&:name)).to eq(["'test'", '2-mb-file'])
         end
+
+        context 'when per_page is over the limit' do
+          let(:params) { { per_page: 3 } }
+
+          before do
+            stub_const('Gitlab::PaginationDelegate::MAX_PER_PAGE', 2)
+          end
+
+          it 'limits the maximum number of elements' do
+            result = subject
+
+            expect(result.map(&:name)).to match_array(["'test'", '2-mb-file'])
+          end
+        end
       end
 
       context 'by page_token only' do

spec/finders/tags_finder_spec.rb

@@ -168,6 +168,20 @@ def load_tags(params, gitaly_pagination: false)
 
           expect(result.map(&:name)).to eq(%w[v1.0.0 v1.1.0])
         end
+
+        context 'when per_page is over the limit' do
+          let(:params) { { per_page: 3 } }
+
+          before do
+            stub_const('Gitlab::PaginationDelegate::MAX_PER_PAGE', 2)
+          end
+
+          it 'limits the maximum number of elements' do
+            result = subject
+
+            expect(result.map(&:name)).to eq(%w[v1.0.0 v1.1.0])
+          end
+        end
       end
 
       context 'by page_token only' do

spec/lib/gitlab/pagination_delegate_spec.rb

@@ -154,4 +154,12 @@
                                  count: 1).current_page).to eq(1)
     end
   end
+
+  context 'with an invalid per_page value' do
+    it 'has a default per page' do
+      expect(described_class.new(page: nil,
+                                 per_page: { wrong: :value },
+                                 count: 0).limit_value).to eq(described_class::DEFAULT_PER_PAGE)
+    end
+  end
 end