Capture push rule regex errors and present them to user

app/services/merge_requests/merge_service.rb

@@ -56,6 +56,9 @@ module MergeRequests
       end
 
       true
+    rescue PushRule::MatchError => e
+      handle_merge_error(log_message: e.message, save_message_on_model: true)
+      false
     end
 
     private

changelogs/unreleased-ee/dm-push-rule-regexp-error.yml

+---
+title: Capture push rule regex errors and present them to user
+merge_request: 4102
+author:
+type: fixed

ee/app/models/push_rule.rb

 class PushRule < ActiveRecord::Base
+  MatchError = Class.new(StandardError)
+
   belongs_to :project
 
   validates :project, presence: true, unless: "is_sample?"
@@ -98,6 +100,8 @@ class PushRule < ActiveRecord::Base
     else
       true
     end
+  rescue RegexpError => e
+    raise MatchError, "Regular expression '#{regex}' is invalid: #{e.message}"
   end
 
   def read_setting_with_global_default(setting)

lib/gitlab/checks/change_access.rb

@@ -193,6 +193,8 @@ module Gitlab
             end
           end
         end
+      rescue PushRule::MatchError => e
+        raise GitAccess::UnauthorizedError, e.message
       end
 
       def branch_name_allowed_by_push_rule?(push_rule)

spec/models/push_rule_spec.rb → spec/ee/spec/models/push_rule_spec.rb

@@ -49,6 +49,24 @@ describe PushRule do
     end
   end
 
+  methods_and_regexes = {
+    commit_message_allowed?: :commit_message_regex,
+    branch_name_allowed?: :branch_name_regex,
+    author_email_allowed?: :author_email_regex,
+    filename_blacklisted?: :file_name_regex
+  }
+
+  methods_and_regexes.each do |method_name, regex_attr|
+    describe "##{method_name}" do
+      it 'raises a MatchError when the regex is invalid' do
+        push_rule[regex_attr] = '+'
+
+        expect { push_rule.public_send(method_name, 'foo') } # rubocop:disable GitlabSecurity/PublicSend
+          .to raise_error(PushRule::MatchError, /\ARegular expression '\+' is invalid/)
+      end
+    end
+  end
+
   describe '#commit_signature_allowed?' do
     let!(:premium_license) { create(:license, plan: License::PREMIUM_PLAN) }
     let(:signed_commit) { double(has_signature?: true) }

spec/lib/gitlab/checks/change_access_spec.rb

@@ -252,17 +252,23 @@ describe Gitlab::Checks::ChangeAccess do
       end
 
       context 'commit message rules' do
-        let(:push_rule) { create(:push_rule, :commit_message) }
+        let!(:push_rule) { create(:push_rule, :commit_message) }
 
         it_behaves_like 'check ignored when push rule unlicensed'
 
         it 'returns an error if the rule fails' do
           expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'")
         end
+
+        it 'returns an error if the regex is invalid' do
+          push_rule.commit_message_regex = '+'
+
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /\ARegular expression '\+' is invalid/)
+        end
       end
 
       context 'author email rules' do
-        let(:push_rule) { create(:push_rule, author_email_regex: '.*@valid.com') }
+        let!(:push_rule) { create(:push_rule, author_email_regex: '.*@valid.com') }
 
         before do
           allow_any_instance_of(Commit).to receive(:committer_email).and_return('mike@valid.com')
@@ -282,10 +288,16 @@ describe Gitlab::Checks::ChangeAccess do
 
           expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Author's email 'joan@invalid.com' does not follow the pattern '.*@valid.com'")
         end
+
+        it 'returns an error if the regex is invalid' do
+          push_rule.author_email_regex = '+'
+
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /\ARegular expression '\+' is invalid/)
+        end
       end
 
       context 'branch name rules' do
-        let(:push_rule) { create(:push_rule, branch_name_regex: '^(w*)$') }
+        let!(:push_rule) { create(:push_rule, branch_name_regex: '^(w*)$') }
         let(:ref) { 'refs/heads/a-branch-that-is-not-allowed' }
 
         it_behaves_like 'check ignored when push rule unlicensed'
@@ -294,6 +306,12 @@ describe Gitlab::Checks::ChangeAccess do
           expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Branch name does not follow the pattern '^(w*)$'")
         end
 
+        it 'returns an error if the regex is invalid' do
+          push_rule.branch_name_regex = '+'
+
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /\ARegular expression '\+' is invalid/)
+        end
+
         context 'when the ref is not a branch ref' do
           let(:ref) { 'a/ref/thats/not/abranch' }
 
@@ -346,13 +364,19 @@ describe Gitlab::Checks::ChangeAccess do
       context 'file name rules' do
         # Notice that the commit used creates a file named 'README'
         context 'file name regex check' do
-          let(:push_rule) { create(:push_rule, file_name_regex: 'READ*') }
+          let!(:push_rule) { create(:push_rule, file_name_regex: 'READ*') }
 
           it_behaves_like 'check ignored when push rule unlicensed'
 
           it "returns an error if a new or renamed filed doesn't match the file name regex" do
             expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File name README was blacklisted by the pattern READ*.")
           end
+
+          it 'returns an error if the regex is invalid' do
+            push_rule.file_name_regex = '+'
+
+            expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /\ARegular expression '\+' is invalid/)
+          end
         end
 
         context 'blacklisted files check' do