use safer .hooks_for instead of .public_send

with .public_send we can't make sure that the scope on the model
actually exists.

4 files changed, 26 insertions, 2 deletions

diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb
index 6e02275d552..42f40c52d29 100644
--- a/app/models/concerns/triggerable_hooks.rb
+++ b/app/models/concerns/triggerable_hooks.rb
@@ -17,6 +17,12 @@ module TriggerableHooks
   class_methods do
     attr_reader :triggerable_hooks
 
+    def hooks_for(trigger)
+      return none unless self::TRIGGERS.keys.include?(trigger)
+
+      public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend
+    end
+
     private
 
     def triggerable_hooks(hooks)
diff --git a/app/models/project.rb b/app/models/project.rb
index a8c634200ce..602c6a2795f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -970,7 +970,7 @@ class Project < ActiveRecord::Base
 
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do
-      hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
+      hooks.hooks_for(hooks_scope).each do |hook|
         hook.async_execute(data, hooks_scope.to_s)
       end
     end
diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb
index 690918b4a00..68c4597a133 100644
--- a/app/services/system_hooks_service.rb
+++ b/app/services/system_hooks_service.rb
@@ -8,7 +8,7 @@ class SystemHooksService
   end
 
   def execute_hooks(data, hooks_scope = :all)
-    SystemHook.public_send(hooks_scope).find_each do |hook| # rubocop:disable GitlabSecurity/PublicSend
+    SystemHook.hooks_for(hooks_scope).find_each do |hook|
       hook.async_execute(data, 'system_hooks')
     end
   end
diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb
index 695df5e10aa..3ddf477e41e 100644
--- a/spec/models/concerns/triggerable_hooks_spec.rb
+++ b/spec/models/concerns/triggerable_hooks_spec.rb
@@ -14,4 +14,22 @@ RSpec.describe TriggerableHooks do
       expect(TestableHook).not_to respond_to :tag_push_hooks
     end
   end
+
+  describe '.hooks_for' do
+    context 'the model has the required trigger scope' do
+      it 'returns the record' do
+        hook = TestableHook.create!(url: 'http://example.com', push_events: true)
+
+        expect(TestableHook.hooks_for(:push_hooks)).to eq [hook]
+      end
+    end
+
+    context 'the model does not have the required trigger scope' do
+      it 'returns an empty relation' do
+        TestableHook.create!(url: 'http://example.com', push_events: true)
+
+        expect(TestableHook.hooks_for(:tag_push_hooks)).to eq []
+      end
+    end
+  end
 end