Merge branch 'sh-handle-invalid-comparison' into 'master'

Reject invalid branch names in repository compare controller

Closes #51003

See merge request gitlab-org/gitlab-ce!22186

app/controllers/projects/compare_controller.rb

@@ -16,6 +16,8 @@ class Projects::CompareController < Projects::ApplicationController
   before_action :define_diff_notes_disabled, only: [:show, :diff_for_path]
   before_action :define_commits, only: [:show, :diff_for_path, :signatures]
   before_action :merge_request, only: [:index, :show]
+  # Validation
+  before_action :validate_refs!
 
   def index
   end
@@ -63,6 +65,21 @@ class Projects::CompareController < Projects::ApplicationController
 
   private
 
+  def valid_ref?(ref_name)
+    return true unless ref_name.present?
+
+    Gitlab::GitRefValidator.validate(ref_name)
+  end
+
+  def validate_refs!
+    valid = [head_ref, start_ref].map { |ref| valid_ref?(ref) }
+
+    return if valid.all?
+
+    flash[:alert] = "Invalid branch name"
+    redirect_to project_compare_index_path(@project)
+  end
+
   def compare
     return @compare if defined?(@compare)
 

changelogs/unreleased/sh-handle-invalid-comparison.yml

+---
+title: Reject invalid branch names in repository compare controller
+merge_request: 22186
+author:
+type: fixed

spec/controllers/projects/compare_controller_spec.rb

@@ -97,6 +97,30 @@ describe Projects::CompareController do
         expect(assigns(:commits)).to eq([])
       end
     end
+
+    context 'when the target ref is invalid' do
+      let(:target_ref) { "master%' AND 2554=4423 AND '%'='" }
+      let(:source_ref) { "improve%2Fawesome" }
+
+      it 'shows a flash message and redirects' do
+        show_request
+
+        expect(flash[:alert]).to eq('Invalid branch name')
+        expect(response).to have_http_status(302)
+      end
+    end
+
+    context 'when the source ref is invalid' do
+      let(:source_ref) { "master%' AND 2554=4423 AND '%'='" }
+      let(:target_ref) { "improve%2Fawesome" }
+
+      it 'shows a flash message and redirects' do
+        show_request
+
+        expect(flash[:alert]).to eq('Invalid branch name')
+        expect(response).to have_http_status(302)
+      end
+    end
   end
 
   describe 'GET diff_for_path' do