Merge branch 'improve_sort_validation_for_branches' into 'master'

Validate sort parameter for branches page and branches API

See merge request gitlab-org/gitlab!79476

app/controllers/projects/branches_controller.rb

@@ -131,11 +131,28 @@ class Projects::BranchesController < Projects::ApplicationController
   private
 
   def sort_value_for_mode
-    return params[:sort] if params[:sort].present?
+    custom_sort || default_sort
+  end
+
+  def custom_sort
+    sort = params[:sort].presence
+
+    unless sort.in?(supported_sort_options)
+      flash.now[:alert] = _("Unsupported sort value.")
+      sort = nil
+    end
 
+    sort
+  end
+
+  def default_sort
     'stale' == @mode ? sort_value_oldest_updated : sort_value_recently_updated
   end
 
+  def supported_sort_options
+    [nil, sort_value_name, sort_value_oldest_updated, sort_value_recently_updated]
+  end
+
   # It can be expensive to calculate the diverging counts for each
   # branch. Normally the frontend should be specifying a set of branch
   # names, but prior to

lib/api/branches.rb

@@ -24,7 +24,7 @@ module API
     helpers do
       params :filter_params do
         optional :search, type: String, desc: 'Return list of branches matching the search criteria'
-        optional :sort, type: String, desc: 'Return list of branches sorted by the given field'
+        optional :sort, type: String, desc: 'Return list of branches sorted by the given field', values: %w[name_asc updated_asc updated_desc]
       end
     end
 

locale/gitlab.pot

@@ -38452,6 +38452,9 @@ msgstr ""
 msgid "Unsubscribes from this %{quick_action_target}."
 msgstr ""
 
+msgid "Unsupported sort value."
+msgstr ""
+
 msgid "Unsupported todo type passed. Supported todo types are: %{todo_types}"
 msgstr ""
 

spec/controllers/projects/branches_controller_spec.rb

@@ -657,6 +657,36 @@ RSpec.describe Projects::BranchesController do
       end
     end
 
+    context 'sorting', :aggregate_failures do
+      let(:sort) { 'name_asc' }
+
+      before do
+        get :index, format: :html, params: {
+          namespace_id: project.namespace, project_id: project, state: 'all', sort: sort
+        }
+      end
+
+      it { expect(assigns[:sort]).to eq('name_asc') }
+
+      context 'when sort is not provided' do
+        let(:sort) { nil }
+
+        it 'uses a default sort without an error message' do
+          expect(assigns[:sort]).to eq('updated_desc')
+          expect(controller).not_to set_flash.now[:alert]
+        end
+      end
+
+      context 'when sort is not supported' do
+        let(:sort) { 'unknown' }
+
+        it 'uses a default sort and shows an error message' do
+          expect(assigns[:sort]).to eq('updated_desc')
+          expect(controller).to set_flash.now[:alert].to(/Unsupported sort/)
+        end
+      end
+    end
+
     context 'when gitaly is not available' do
       before do
         allow_next_instance_of(Gitlab::GitalyClient::RefService) do |ref_service|

spec/requests/api/branches_spec.rb

@@ -188,6 +188,24 @@ RSpec.describe API::Branches do
       end
     end
 
+    context 'when sort parameter is passed' do
+      it 'sorts branches' do
+        get api(route, user), params: { sort: 'name_asc', per_page: 10 }
+
+        sorted_branch_names = json_response.map { |branch| branch['name'] }
+
+        project_branch_names = project.repository.branch_names.sort.take(10)
+
+        expect(sorted_branch_names).to eq(project_branch_names)
+      end
+
+      context 'when sort value is not supported' do
+        it_behaves_like '400 response' do
+          let(:request) { get api(route, user), params: { sort: 'unknown' }}
+        end
+      end
+    end
+
     context 'when unauthenticated', 'and project is public' do
       before do
         project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)