Merge branch 'negative_commit_message_regex' into 'master'

new push rule: reject commit messages based on negative match regex

Closes #5743

See merge request gitlab-org/gitlab-ee!5453

db/schema.rb

@@ -2262,6 +2262,7 @@ ActiveRecord::Schema.define(version: 20180612175636) do
     t.boolean "reject_unsigned_commits"
     t.boolean "commit_committer_check"
     t.boolean "regexp_uses_re2", default: true
+    t.string "commit_message_negative_regex"
   end
 
   add_index "push_rules", ["is_sample"], name: "index_push_rules_on_is_sample", where: "is_sample", using: :btree

doc/push_rules/push_rules.md

@@ -65,6 +65,7 @@ The following options are available.
 | Check whether commit is signed through GPG | **Premium** 10.1 | Reject commit when it is not signed through GPG. Read [signing commits with GPG][signing-commits]. |
 | Prevent committing secrets to Git | **Starter** 8.12 | GitLab will reject any files that are likely to contain secrets. Read [what files are forbidden](#prevent-pushing-secrets-to-the-repository). |
 | Restrict by commit message | **Starter** 7.10 | Only commit messages that match this regular expression are allowed to be pushed. Leave empty to allow any commit message. Uses multiline mode, which can be disabled using `(?-m)`. |
+| Restrict by commit message (negative match)| **Starter** 11.1 | Only commit messages that do not match this regular expression are allowed to be pushed. Leave empty to allow any commit message. Uses multiline mode, which can be disabled using `(?-m)`. |
 | Restrict by branch name | **Starter** 9.3 | Only branch names that match this regular expression are allowed to be pushed. Leave empty to allow any branch name. |
 | Restrict by commit author's email | **Starter** 7.10 | Only commit author's email that match this regular expression are allowed to be pushed. Leave empty to allow any email. |
 | Prohibited file names | **Starter** 7.10 | Any committed filenames that match this regular expression are not allowed to be pushed. Leave empty to allow any filenames. |

ee/app/controllers/admin/push_rules_controller.rb

@@ -24,7 +24,7 @@ class Admin::PushRulesController < Admin::ApplicationController
   end
 
   def push_rule_params
-    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex
+    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex commit_message_negative_regex
                         branch_name_regex force_push_regex author_email_regex
                         member_check file_name_regex max_file_size prevent_secrets]
 

ee/app/controllers/projects/push_rules_controller.rb

@@ -26,7 +26,7 @@ class Projects::PushRulesController < Projects::ApplicationController
 
   # Only allow a trusted parameter "white list" through.
   def push_rule_params
-    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex
+    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex commit_message_negative_regex
                         branch_name_regex force_push_regex author_email_regex
                         member_check file_name_regex max_file_size prevent_secrets]
 

ee/app/models/push_rule.rb

@@ -11,6 +11,7 @@ class PushRule < ActiveRecord::Base
     force_push_regex
     delete_branch_regex
     commit_message_regex
+    commit_message_negative_regex
     author_email_regex
     file_name_regex
     branch_name_regex
@@ -36,6 +37,7 @@ class PushRule < ActiveRecord::Base
 
   def commit_validation?
     commit_message_regex.present? ||
+      commit_message_negative_regex.present? ||
       branch_name_regex.present? ||
       author_email_regex.present? ||
       reject_unsigned_commits ||
@@ -63,6 +65,10 @@ class PushRule < ActiveRecord::Base
     data_match?(message, commit_message_regex, multiline: true)
   end
 
+  def commit_message_blocked?(message)
+    commit_message_negative_regex.present? && data_match?(message, commit_message_negative_regex, multiline: true)
+  end
+
   def branch_name_allowed?(branch)
     data_match?(branch, branch_name_regex)
   end

ee/app/services/ee/merge_requests/merge_service.rb

@@ -26,6 +26,11 @@ module EE
           return false
         end
 
+        if push_rule.commit_message_blocked?(params[:commit_message])
+          handle_merge_error(log_message: "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'", save_message_on_model: true)
+          return false
+        end
+
         unless push_rule.author_email_allowed?(current_user.email)
           handle_merge_error(log_message: "Commit author's email '#{current_user.email}' does not follow the pattern '#{push_rule.author_email_regex}'", save_message_on_model: true)
           return false

ee/app/views/shared/push_rules/_form.html.haml

@@ -34,6 +34,16 @@
     If this field is empty it allows any commit message.
     For example you can require that an issue number is always mentioned in the commit message.
 
+.form-group
+  = f.label :commit_message_negative_regex, "Commit message negative match", class: 'label-light'
+  = f.text_field :commit_message_negative_regex, class: "form-control", placeholder: 'Example: ssh\:\/\/'
+  .form-text.text-muted
+    No commit message is allowed to match this
+    = link_to 'Ruby regular expression', 'http://www.ruby-doc.org/core-2.1.1/Regexp.html'
+    to be pushed.
+    If this field is empty it allows any commit message.
+    For example you can require that no commit message contains any links.
+
 .form-group
   = f.label :branch_name_regex, "Branch name", class: "label-light"
   = f.text_field :branch_name_regex, class: "form-control", placeholder: 'Example: (feature|hotfix)\/*'

ee/changelogs/unreleased/negative_commit_message_regex.yml

+---
+title: Add a new push rule to allow negative matching of commit messages.
+merge_request: 5453
+author: Hannes Rosenögger
+type: added

ee/db/migrate/20180423165301_add_negative_matching_commit_message_push_rule.rb

+class AddNegativeMatchingCommitMessagePushRule < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    add_column :push_rules, :commit_message_negative_regex, :string, null: true
+  end
+
+  def down
+    remove_column :push_rules, :commit_message_negative_regex
+  end
+end
+

ee/lib/api/project_push_rule.rb

@@ -14,12 +14,13 @@ module API
           optional :member_check, type: Boolean, desc: 'Restrict commits by author (email) to existing GitLab users'
           optional :prevent_secrets, type: Boolean, desc: 'GitLab will reject any files that are likely to contain secrets'
           optional :commit_message_regex, type: String, desc: 'All commit messages must match this'
+          optional :commit_message_negative_regex, type: String, desc: 'No commit message is allowed to match this'
           optional :branch_name_regex, type: String, desc: 'All branches names must match this'
           optional :author_email_regex, type: String, desc: 'All commit author emails must match this'
           optional :file_name_regex, type: String, desc: 'All commited filenames must not match this'
           optional :max_file_size, type: Integer, desc: 'Maximum file size (MB)'
           at_least_one_of :deny_delete_tag, :member_check, :prevent_secrets,
-                          :commit_message_regex, :branch_name_regex, :author_email_regex,
+                          :commit_message_regex, :commit_message_negative_regex, :branch_name_regex, :author_email_regex,
                           :file_name_regex, :max_file_size
         end
       end

ee/lib/ee/api/entities.rb

@@ -125,7 +125,7 @@ module EE
       ########################
       class ProjectPushRule < Grape::Entity
         expose :id, :project_id, :created_at
-        expose :commit_message_regex, :branch_name_regex, :deny_delete_tag
+        expose :commit_message_regex, :commit_message_negative_regex, :branch_name_regex, :deny_delete_tag
         expose :member_check, :prevent_secrets, :author_email_regex
         expose :file_name_regex, :max_file_size
       end

ee/lib/ee/gitlab/checks/change_access.rb

@@ -119,6 +119,10 @@ module EE
             return "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'"
           end
 
+          if push_rule.commit_message_blocked?(commit.safe_message)
+            return "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'"
+          end
+
           unless push_rule.author_email_allowed?(commit.committer_email)
             return "Committer's email '#{commit.committer_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
           end

ee/spec/lib/gitlab/checks/change_access_spec.rb

@@ -71,15 +71,29 @@ describe Gitlab::Checks::ChangeAccess do
 
         it_behaves_like 'check ignored when push rule unlicensed'
 
-        it 'returns an error if the rule fails' do
+        it 'returns an error if the rule fails due to missing required characters' 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 rule fails due to forbidden characters' do
+          push_rule.commit_message_regex = nil
+          push_rule.commit_message_negative_regex = '.*'
+
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_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
+
+        it 'returns an error if the negative regex is invalid' do
+          push_rule.commit_message_regex = nil
+          push_rule.commit_message_negative_regex = '+'
+
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /\ARegular expression '\+' is invalid/)
+        end
       end
 
       context 'author email rules' do

ee/spec/lib/gitlab/git_access_spec.rb

@@ -55,12 +55,18 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
       end
 
-      it 'returns false' do
+      it 'returns false when a commit message is missing required matches (positive regex match)' do
         project.create_push_rule(commit_message_regex: "@only.com")
 
         expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
       end
 
+      it 'returns false when a commit message contains forbidden characters (negative regex match)' do
+        project.create_push_rule(commit_message_negative_regex: "@gmail.com")
+
+        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+      end
+
       it 'returns true for tags' do
         project.create_push_rule(commit_message_regex: "@only.com")
 

ee/spec/models/push_rule_spec.rb

@@ -82,6 +82,16 @@ describe PushRule do
     end
   end
 
+  describe '#commit_message_blocked?' do
+    subject(:push_rule) { create(:push_rule, commit_message_negative_regex: 'commit')}
+
+    it 'uses multiline regex' do
+      commit_message = "Some git commit feature\n\nSigned-off-by: Someone"
+
+      expect(subject.commit_message_blocked?(commit_message)).to be true
+    end
+  end
+
   describe '#commit_validation?' do
     let(:settings_with_global_default) { %i(reject_unsigned_commits) }
 
@@ -118,6 +128,7 @@ describe PushRule do
 
   methods_and_regexes = {
     commit_message_allowed?: :commit_message_regex,
+    commit_message_blocked?: :commit_message_negative_regex,
     branch_name_allowed?: :branch_name_regex,
     author_email_allowed?: :author_email_regex,
     filename_blacklisted?: :file_name_regex

ee/spec/services/merge_requests/merge_service_spec.rb

@@ -44,7 +44,7 @@ describe MergeRequests::MergeService do
       expect(service.hooks_validation_pass?(merge_request)).to be_truthy
     end
 
-    context 'commit message validation' do
+    context 'commit message validation for required characters' do
       before do
         allow(project).to receive(:push_rule) { build(:push_rule, commit_message_regex: 'unmatched pattern .*') }
       end
@@ -57,6 +57,19 @@ describe MergeRequests::MergeService do
       end
     end
 
+    context 'commit message validation for forbidden characters' do
+      before do
+        allow(project).to receive(:push_rule) { build(:push_rule, commit_message_negative_regex: '.*') }
+      end
+
+      it_behaves_like 'hook validations are skipped when push rules unlicensed'
+
+      it 'returns false and saves error when invalid' do
+        expect(service.hooks_validation_pass?(merge_request)).to be_falsey
+        expect(merge_request.merge_error).not_to be_empty
+      end
+    end
+
     context 'authors email validation' do
       before do
         allow(project).to receive(:push_rule) { build(:push_rule, author_email_regex: '.*@unmatchedemaildomain.com') }