Merge branch '329743-web_hooks-recursion-limit' into 'master'

Add Webhook recursion detection (step 1: logging)

See merge request gitlab-org/gitlab!75821

app/models/hooks/project_hook.rb

@@ -41,6 +41,11 @@ class ProjectHook < WebHook
     super.merge(project: project)
   end
 
+  override :parent
+  def parent
+    project
+  end
+
   private
 
   override :web_hooks_disable_failed?

app/models/hooks/service_hook.rb

@@ -2,6 +2,7 @@
 
 class ServiceHook < WebHook
   include Presentable
+  extend ::Gitlab::Utils::Override
 
   belongs_to :integration, foreign_key: :service_id
   validates :integration, presence: true
@@ -9,4 +10,7 @@ class ServiceHook < WebHook
   def execute(data, hook_name = 'service_hook')
     super(data, hook_name)
   end
+
+  override :parent
+  delegate :parent, to: :integration
 end

app/models/hooks/web_hook.rb

@@ -122,6 +122,11 @@ class WebHook < ApplicationRecord
     nil
   end
 
+  # Returns the associated Project or Group for the WebHook if one exists.
+  # Overridden by inheriting classes.
+  def parent
+  end
+
   # Custom attributes to be included in the worker context.
   def application_context
     { related_class: type }

app/services/web_hook_service.rb

@@ -26,7 +26,6 @@ class WebHookService
   end
 
   REQUEST_BODY_SIZE_LIMIT = 25.megabytes
-  GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
 
   attr_accessor :hook, :data, :hook_name, :request_options
   attr_reader :uniqueness_token
@@ -50,6 +49,10 @@ class WebHookService
   def execute
     return { status: :error, message: 'Hook disabled' } unless hook.executable?
 
+    log_recursion_limit if recursion_blocked?
+
+    Gitlab::WebHooks::RecursionDetection.register!(hook)
+
     start_time = Gitlab::Metrics::System.monotonic_time
 
     response = if parsed_url.userinfo.blank?
@@ -95,6 +98,10 @@ class WebHookService
     Gitlab::ApplicationContext.with_context(hook.application_context) do
       break log_rate_limit if rate_limited?
 
+      log_recursion_limit if recursion_blocked?
+
+      data[:_gitlab_recursion_detection_request_uuid] = Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
+
       WebHookWorker.perform_async(hook.id, data, hook_name)
     end
   end
@@ -108,7 +115,7 @@ class WebHookService
   def make_request(url, basic_auth = false)
     Gitlab::HTTP.post(url,
       body: Gitlab::Json::LimitedEncoder.encode(data, limit: REQUEST_BODY_SIZE_LIMIT),
-      headers: build_headers(hook_name),
+      headers: build_headers,
       verify: hook.enable_ssl_verification,
       basic_auth: basic_auth,
       **request_options)
@@ -129,7 +136,7 @@ class WebHookService
       trigger: trigger,
       url: url,
       execution_duration: execution_duration,
-      request_headers: build_headers(hook_name),
+      request_headers: build_headers,
       request_data: request_data,
       response_headers: format_response_headers(response),
       response_body: safe_response_body(response),
@@ -151,15 +158,16 @@ class WebHookService
     end
   end
 
-  def build_headers(hook_name)
+  def build_headers
     @headers ||= begin
-      {
+      headers = {
         'Content-Type' => 'application/json',
         'User-Agent' => "GitLab/#{Gitlab::VERSION}",
-        GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name)
-      }.tap do |hash|
-        hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
-      end
+        Gitlab::WebHooks::GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name)
+      }
+
+      headers['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
+      headers.merge!(Gitlab::WebHooks::RecursionDetection.header(hook))
     end
   end
 
@@ -186,6 +194,10 @@ class WebHookService
     )
   end
 
+  def recursion_blocked?
+    Gitlab::WebHooks::RecursionDetection.block?(hook)
+  end
+
   def rate_limit
     @rate_limit ||= hook.rate_limit
   end
@@ -199,4 +211,15 @@ class WebHookService
       **Gitlab::ApplicationContext.current
     )
   end
+
+  def log_recursion_limit
+    Gitlab::AuthLogger.error(
+      message: 'Webhook recursion detected and will be blocked in future',
+      hook_id: hook.id,
+      hook_type: hook.type,
+      hook_name: hook_name,
+      recursion_detection: ::Gitlab::WebHooks::RecursionDetection.to_log(hook),
+      **Gitlab::ApplicationContext.current
+    )
+  end
 end

app/workers/web_hook_worker.rb

@@ -13,11 +13,21 @@ class WebHookWorker
 
   worker_has_external_dependencies!
 
-  def perform(hook_id, data, hook_name)
+  # Webhook recursion detection properties are passed through the `data` arg.
+  # This will be migrated to the `params` arg over the next few releases.
+  # See https://gitlab.com/gitlab-org/gitlab/-/issues/347389.
+  def perform(hook_id, data, hook_name, params = {})
     hook = WebHook.find_by_id(hook_id)
     return unless hook
 
     data = data.with_indifferent_access
+
+    # Before executing the hook, reapply any recursion detection UUID that was
+    # initially present in the request header so the hook can pass this same header
+    # value in its request.
+    recursion_detection_uuid = data.delete(:_gitlab_recursion_detection_request_uuid)
+    Gitlab::WebHooks::RecursionDetection.set_request_uuid(recursion_detection_uuid)
+
     WebHookService.new(hook, data, hook_name, jid).execute
   end
 end

config/feature_flags/development/webhook_recursion_detection.yml

+---
+name: webhook_recursion_detection
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75821
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349845
+milestone: '14.7'
+type: development
+group: group::integrations
+default_enabled: false

config/initializers/webhook_recursion_detection.rb

+# frozen_string_literal: true
+
+Rails.application.configure do |config|
+  config.middleware.insert_after RequestStore::Middleware, Gitlab::Middleware::WebhookRecursionDetection
+end

ee/app/models/hooks/group_hook.rb

@@ -44,6 +44,11 @@ class GroupHook < WebHook
     group.actual_limits.limit_for(:web_hook_calls)
   end
 
+  override :parent
+  def parent
+    group
+  end
+
   private
 
   override :web_hooks_disable_failed?

ee/spec/models/hooks/group_hook_spec.rb

@@ -30,6 +30,15 @@ RSpec.describe GroupHook do
     end
   end
 
+  describe '#parent' do
+    it 'returns the associated group' do
+      group = build(:group)
+      hook = build(:group_hook, group: group)
+
+      expect(hook.parent).to eq(group)
+    end
+  end
+
   describe '#application_context' do
     let_it_be(:hook) { build(:group_hook) }
 

lib/api/ci/triggers.rb

@@ -5,7 +5,7 @@ module API
     class Triggers < ::API::Base
       include PaginationParams
 
-      HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase
+      HTTP_GITLAB_EVENT_HEADER = "HTTP_#{::Gitlab::WebHooks::GITLAB_EVENT_HEADER}".underscore.upcase
 
       feature_category :continuous_integration
 

lib/gitlab/middleware/webhook_recursion_detection.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Middleware
+    class WebhookRecursionDetection
+      def initialize(app)
+        @app = app
+      end
+
+      def call(env)
+        headers = ActionDispatch::Request.new(env).headers
+
+        ::Gitlab::WebHooks::RecursionDetection.set_from_headers(headers)
+
+        @app.call(env)
+      end
+    end
+  end
+end

lib/gitlab/web_hooks.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module WebHooks
+    GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
+  end
+end

lib/gitlab/web_hooks/recursion_detection.rb

+# frozen_string_literal: true
+
+# This module detects and blocks recursive webhook requests.
+#
+# Recursion can happen when a webhook has been configured to make a call
+# to its own GitLab instance (i.e., its API), and during the execution of
+# the call the webhook is triggered again to create an infinite loop of
+# being triggered.
+#
+# Additionally the module blocks a webhook once the number of requests to
+# the instance made by a series of webhooks triggering other webhooks reaches
+# a limit.
+#
+# Blocking recursive webhooks allows GitLab to continue to support workflows
+# that use webhooks to call the API non-recursively, or do not go on to
+# trigger an unreasonable number of other webhooks.
+module Gitlab
+  module WebHooks
+    module RecursionDetection
+      COUNT_LIMIT = 100
+      TOUCH_CACHE_TTL = 30.minutes
+
+      class << self
+        def set_from_headers(headers)
+          uuid = headers[UUID::HEADER]
+
+          return unless uuid
+
+          set_request_uuid(uuid)
+        end
+
+        def set_request_uuid(uuid)
+          UUID.instance.request_uuid = uuid
+        end
+
+        # Before a webhook is executed, `.register!` should be called.
+        # Adds the webhook ID to a cache (see `#cache_key_for_hook` for
+        # details of the cache).
+        def register!(hook)
+          return if disabled?(hook)
+
+          cache_key = cache_key_for_hook(hook)
+
+          ::Gitlab::Redis::SharedState.with do |redis|
+            redis.multi do
+              redis.sadd(cache_key, hook.id)
+              redis.expire(cache_key, TOUCH_CACHE_TTL)
+            end
+          end
+        end
+
+        # Returns true if the webhook ID is present in the cache, or if the
+        # number of IDs in the cache exceeds the limit (see
+        # `#cache_key_for_hook` for details of the cache).
+        def block?(hook)
+          return false if disabled?(hook)
+
+          # If a request UUID has not been set then we know the request was not
+          # made by a webhook, and no recursion is possible.
+          return false unless UUID.instance.request_uuid
+
+          cache_key = cache_key_for_hook(hook)
+
+          ::Gitlab::Redis::SharedState.with do |redis|
+            redis.sismember(cache_key, hook.id) ||
+              redis.scard(cache_key) >= COUNT_LIMIT
+          end
+        end
+
+        def header(hook)
+          UUID.instance.header(hook)
+        end
+
+        def to_log(hook)
+          {
+            uuid: UUID.instance.uuid_for_hook(hook),
+            ids: ::Gitlab::Redis::SharedState.with { |redis| redis.smembers(cache_key_for_hook(hook)).map(&:to_i) }
+          }
+        end
+
+        private
+
+        def disabled?(hook)
+          Feature.disabled?(:webhook_recursion_detection, hook.parent)
+        end
+
+        # Returns a cache key scoped to a UUID.
+        #
+        # The particular UUID will be either:
+        #
+        #   - A UUID that was recycled from the request headers if the request was made by a webhook.
+        #   - a new UUID initialized for the webhook.
+        #
+        # This means that cycles of webhooks that are triggered from other webhooks
+        # will share the same cache, and other webhooks will use a new cache.
+        def cache_key_for_hook(hook)
+          [:webhook_recursion_detection, UUID.instance.uuid_for_hook(hook)].join(':')
+        end
+      end
+    end
+  end
+end

lib/gitlab/web_hooks/recursion_detection/uuid.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module WebHooks
+    module RecursionDetection
+      class UUID
+        HEADER = "#{::Gitlab::WebHooks::GITLAB_EVENT_HEADER}-UUID"
+
+        include Singleton
+
+        attr_accessor :request_uuid
+
+        def initialize
+          self.new_uuids_for_hooks = {}
+        end
+
+        class << self
+          # Back the Singleton with RequestStore so it is isolated to this request.
+          def instance
+            Gitlab::SafeRequestStore[:web_hook_recursion_detection_uuid] ||= new
+          end
+        end
+
+        # Returns a UUID, which will be either:
+        #
+        #   - The UUID that was recycled from the request headers if the request was made by a webhook.
+        #   - A new UUID initialized for the webhook.
+        def uuid_for_hook(hook)
+          request_uuid || new_uuid_for_hook(hook)
+        end
+
+        def header(hook)
+          { HEADER => uuid_for_hook(hook) }
+        end
+
+        private
+
+        attr_accessor :new_uuids_for_hooks
+
+        def new_uuid_for_hook(hook)
+          new_uuids_for_hooks[hook.id] ||= SecureRandom.uuid
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/middleware/webhook_recursion_detection_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'action_dispatch'
+require 'rack'
+require 'request_store'
+
+RSpec.describe Gitlab::Middleware::WebhookRecursionDetection do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:env) { Rack::MockRequest.env_for("/").merge(headers) }
+
+  around do |example|
+    Gitlab::WithRequestStore.with_request_store { example.run }
+  end
+
+  describe '#call' do
+    subject(:call) { described_class.new(app).call(env) }
+
+    context 'when the recursion detection header is present' do
+      let(:new_uuid) { SecureRandom.uuid }
+      let(:headers) { { 'HTTP_X_GITLAB_EVENT_UUID' => new_uuid } }
+
+      it 'sets the request UUID from the header' do
+        expect(app).to receive(:call)
+        expect { call }.to change { Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid }.to(new_uuid)
+      end
+    end
+
+    context 'when recursion headers are not present' do
+      let(:headers) { {} }
+
+      it 'works without errors' do
+        expect(app).to receive(:call)
+
+        call
+
+        expect(Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid).to be_nil
+      end
+    end
+  end
+end

spec/lib/gitlab/web_hooks/recursion_detection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::WebHooks::RecursionDetection, :clean_gitlab_redis_shared_state, :request_store do
+  let_it_be(:web_hook) { create(:project_hook) }
+
+  let!(:uuid_class) { described_class::UUID }
+
+  describe '.set_from_headers' do
+    let(:old_uuid) { SecureRandom.uuid }
+    let(:rack_headers) { Rack::MockRequest.env_for("/").merge(headers) }
+
+    subject(:set_from_headers) { described_class.set_from_headers(rack_headers) }
+
+    # Note, having a previous `request_uuid` value set before `.set_from_headers` is
+    # called is contrived and should not normally happen. However, testing with this scenario
+    # allows us to assert the ideal outcome if it ever were to happen.
+    before do
+      uuid_class.instance.request_uuid = old_uuid
+    end
+
+    context 'when the detection header is present' do
+      let(:new_uuid) { SecureRandom.uuid }
+
+      let(:headers) do
+        { uuid_class::HEADER => new_uuid }
+      end
+
+      it 'sets the request UUID value from the headers' do
+        set_from_headers
+
+        expect(uuid_class.instance.request_uuid).to eq(new_uuid)
+      end
+    end
+
+    context 'when detection header is not present' do
+      let(:headers) { {} }
+
+      it 'does not set the request UUID' do
+        set_from_headers
+
+        expect(uuid_class.instance.request_uuid).to eq(old_uuid)
+      end
+    end
+  end
+
+  describe '.set_request_uuid' do
+    it 'sets the request UUID value' do
+      new_uuid = SecureRandom.uuid
+
+      described_class.set_request_uuid(new_uuid)
+
+      expect(uuid_class.instance.request_uuid).to eq(new_uuid)
+    end
+  end
+
+  describe '.register!' do
+    let_it_be(:second_web_hook) { create(:project_hook) }
+    let_it_be(:third_web_hook) { create(:project_hook) }
+
+    def cache_key(hook)
+      described_class.send(:cache_key_for_hook, hook)
+    end
+
+    it 'stores IDs in the same cache when a request UUID is set, until the request UUID changes', :aggregate_failures do
+      # Register web_hook and second_web_hook against the same request UUID.
+      uuid_class.instance.request_uuid = SecureRandom.uuid
+      described_class.register!(web_hook)
+      described_class.register!(second_web_hook)
+      first_cache_key = cache_key(web_hook)
+      second_cache_key = cache_key(second_web_hook)
+
+      # Register third_web_hook against a new request UUID.
+      uuid_class.instance.request_uuid = SecureRandom.uuid
+      described_class.register!(third_web_hook)
+      third_cache_key = cache_key(third_web_hook)
+
+      expect(first_cache_key).to eq(second_cache_key)
+      expect(second_cache_key).not_to eq(third_cache_key)
+
+      ::Gitlab::Redis::SharedState.with do |redis|
+        members = redis.smembers(first_cache_key).map(&:to_i)
+        expect(members).to contain_exactly(web_hook.id, second_web_hook.id)
+
+        members = redis.smembers(third_cache_key).map(&:to_i)
+        expect(members).to contain_exactly(third_web_hook.id)
+      end
+    end
+
+    it 'stores IDs in unique caches when no request UUID is present', :aggregate_failures do
+      described_class.register!(web_hook)
+      described_class.register!(second_web_hook)
+      described_class.register!(third_web_hook)
+
+      first_cache_key = cache_key(web_hook)
+      second_cache_key = cache_key(second_web_hook)
+      third_cache_key = cache_key(third_web_hook)
+
+      expect([first_cache_key, second_cache_key, third_cache_key].compact.length).to eq(3)
+
+      ::Gitlab::Redis::SharedState.with do |redis|
+        members = redis.smembers(first_cache_key).map(&:to_i)
+        expect(members).to contain_exactly(web_hook.id)
+
+        members = redis.smembers(second_cache_key).map(&:to_i)
+        expect(members).to contain_exactly(second_web_hook.id)
+
+        members = redis.smembers(third_cache_key).map(&:to_i)
+        expect(members).to contain_exactly(third_web_hook.id)
+      end
+    end
+
+    it 'touches the storage ttl each time it is called', :aggregate_failures do
+      freeze_time do
+        described_class.register!(web_hook)
+
+        ::Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.ttl(cache_key(web_hook))).to eq(described_class::TOUCH_CACHE_TTL.to_i)
+        end
+      end
+
+      travel_to(1.minute.from_now) do
+        described_class.register!(second_web_hook)
+
+        ::Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.ttl(cache_key(web_hook))).to eq(described_class::TOUCH_CACHE_TTL.to_i)
+        end
+      end
+    end
+
+    it 'does not store anything if the feature flag is disabled' do
+      stub_feature_flags(webhook_recursion_detection: false)
+
+      described_class.register!(web_hook)
+
+      ::Gitlab::Redis::SharedState.with do |redis|
+        expect(redis.exists(cache_key(web_hook))).to eq(false)
+      end
+    end
+  end
+
+  describe 'block?' do
+    let_it_be(:registered_web_hooks) { create_list(:project_hook, 2) }
+
+    subject(:block?) { described_class.block?(web_hook) }
+
+    before do
+      # Register some previous webhooks.
+      uuid_class.instance.request_uuid = SecureRandom.uuid
+
+      registered_web_hooks.each do |web_hook|
+        described_class.register!(web_hook)
+      end
+    end
+
+    it 'returns false if webhook should not be blocked' do
+      is_expected.to eq(false)
+    end
+
+    context 'when the webhook has previously fired' do
+      before do
+        described_class.register!(web_hook)
+      end
+
+      it 'returns true' do
+        is_expected.to eq(true)
+      end
+
+      it 'returns false if the feature flag is disabled' do
+        stub_feature_flags(webhook_recursion_detection: false)
+
+        is_expected.to eq(false)
+      end
+
+      context 'when the request UUID changes again' do
+        before do
+          uuid_class.instance.request_uuid = SecureRandom.uuid
+        end
+
+        it 'returns false' do
+          is_expected.to eq(false)
+        end
+      end
+    end
+
+    context 'when the count limit has been reached' do
+      let_it_be(:registered_web_hooks) { create_list(:project_hook, 2) }
+
+      before do
+        registered_web_hooks.each do |web_hook|
+          described_class.register!(web_hook)
+        end
+
+        stub_const("#{described_class.name}::COUNT_LIMIT", registered_web_hooks.size)
+      end
+
+      it 'returns true' do
+        is_expected.to eq(true)
+      end
+
+      it 'returns false if the feature flag is disabled' do
+        stub_feature_flags(webhook_recursion_detection: false)
+
+        is_expected.to eq(false)
+      end
+
+      context 'when the request UUID changes again' do
+        before do
+          uuid_class.instance.request_uuid = SecureRandom.uuid
+        end
+
+        it 'returns false' do
+          is_expected.to eq(false)
+        end
+      end
+    end
+  end
+
+  describe '.header' do
+    subject(:header) { described_class.header(web_hook) }
+
+    it 'returns a header with the UUID value' do
+      uuid = SecureRandom.uuid
+      allow(uuid_class.instance).to receive(:uuid_for_hook).and_return(uuid)
+
+      is_expected.to eq({ uuid_class::HEADER => uuid })
+    end
+  end
+
+  describe '.to_log' do
+    subject(:to_log) { described_class.to_log(web_hook) }
+
+    it 'returns the UUID value and all registered webhook IDs in a Hash' do
+      uuid = SecureRandom.uuid
+      allow(uuid_class.instance).to receive(:uuid_for_hook).and_return(uuid)
+      registered_web_hooks = create_list(:project_hook, 2)
+      registered_web_hooks.each { described_class.register!(_1) }
+
+      is_expected.to eq({ uuid: uuid, ids: registered_web_hooks.map(&:id) })
+    end
+  end
+end

spec/models/hooks/project_hook_spec.rb

@@ -40,6 +40,15 @@ RSpec.describe ProjectHook do
     end
   end
 
+  describe '#parent' do
+    it 'returns the associated project' do
+      project = build(:project)
+      hook = build(:project_hook, project: project)
+
+      expect(hook.parent).to eq(project)
+    end
+  end
+
   describe '#application_context' do
     let_it_be(:hook) { build(:project_hook) }
 

spec/models/hooks/service_hook_spec.rb

@@ -31,6 +31,36 @@ RSpec.describe ServiceHook do
     end
   end
 
+  describe '#parent' do
+    let(:hook) { build(:service_hook, integration: integration) }
+
+    context 'with a project-level integration' do
+      let(:project) { build(:project) }
+      let(:integration) { build(:integration, project: project) }
+
+      it 'returns the associated project' do
+        expect(hook.parent).to eq(project)
+      end
+    end
+
+    context 'with a group-level integration' do
+      let(:group) { build(:group) }
+      let(:integration) { build(:integration, :group, group: group) }
+
+      it 'returns the associated group' do
+        expect(hook.parent).to eq(group)
+      end
+    end
+
+    context 'with an instance-level integration' do
+      let(:integration) { build(:integration, :instance) }
+
+      it 'returns nil' do
+        expect(hook.parent).to be_nil
+      end
+    end
+  end
+
   describe '#application_context' do
     let(:hook) { build(:service_hook) }
 

spec/models/integrations/datadog_spec.rb

@@ -167,7 +167,7 @@ RSpec.describe Integrations::Datadog do
 
     context 'with pipeline data' do
       let(:data) { pipeline_data }
-      let(:expected_headers) { { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } }
+      let(:expected_headers) { { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Pipeline Hook' } }
       let(:expected_body) { data.with_retried_builds.to_json }
 
       it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made }
@@ -175,7 +175,7 @@ RSpec.describe Integrations::Datadog do
 
     context 'with job data' do
       let(:data) { build_data }
-      let(:expected_headers) { { WebHookService::GITLAB_EVENT_HEADER => 'Job Hook' } }
+      let(:expected_headers) { { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Job Hook' } }
       let(:expected_body) { data.to_json }
 
       it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made }

spec/requests/api/ci/triggers_spec.rb

@@ -162,7 +162,7 @@ RSpec.describe API::Ci::Triggers do
         expect do
           post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"),
             params: { ref: 'refs/heads/other-branch' },
-            headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' }
+            headers: { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Pipeline Hook' }
         end.not_to change(Ci::Pipeline, :count)
 
         expect(response).to have_gitlab_http_status(:forbidden)

spec/requests/recursive_webhook_detection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_redis_shared_state, :request_store do
+  include StubRequests
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, namespace: user.namespace, creator: user) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+  let_it_be(:project_hook) { create(:project_hook, project: project, merge_requests_events: true) }
+  let_it_be(:system_hook) { create(:system_hook, merge_requests_events: true) }
+
+  # Trigger a change to the merge request to fire the webhooks.
+  def trigger_web_hooks
+    params = { merge_request: { description: FFaker::Lorem.sentence } }
+    put project_merge_request_path(project, merge_request), params: params, headers: headers
+  end
+
+  def stub_requests
+    stub_full_request(project_hook.url, method: :post, ip_address: '8.8.8.8')
+    stub_full_request(system_hook.url, method: :post, ip_address: '8.8.8.9')
+  end
+
+  before do
+    login_as(user)
+  end
+
+  context 'when the request headers include the recursive webhook detection header' do
+    let(:uuid) { SecureRandom.uuid }
+    let(:headers) { { Gitlab::WebHooks::RecursionDetection::UUID::HEADER => uuid } }
+
+    it 'executes all webhooks, logs no errors, and the webhook requests contain the same UUID header', :aggregate_failures do
+      stub_requests
+
+      expect(Gitlab::AuthLogger).not_to receive(:error)
+
+      trigger_web_hooks
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+        .with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
+        .once
+      expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
+        .with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
+        .once
+    end
+
+    shared_examples 'when the feature flag is disabled' do
+      it 'executes and logs no errors' do
+        stub_feature_flags(webhook_recursion_detection: false)
+        stub_requests
+
+        expect(Gitlab::AuthLogger).not_to receive(:error)
+
+        trigger_web_hooks
+
+        expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
+        expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
+      end
+    end
+
+    context 'when one of the webhooks is recursive' do
+      before do
+        # Recreate the necessary state for the previous request to be
+        # considered made from the webhook.
+        Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
+        Gitlab::WebHooks::RecursionDetection.register!(project_hook)
+        Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
+      end
+
+      it 'executes all webhooks and logs an error for the recursive hook', :aggregate_failures do
+        stub_requests
+
+        expect(Gitlab::AuthLogger).to receive(:error).with(
+          include(
+            message: 'Webhook recursion detected and will be blocked in future',
+            hook_id: project_hook.id,
+            recursion_detection: {
+              uuid: uuid,
+              ids: [project_hook.id]
+            }
+          )
+        ).twice # Twice: once in `#async_execute`, and again in `#execute`.
+
+        trigger_web_hooks
+
+        expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
+        expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
+      end
+
+      include_examples 'when the feature flag is disabled'
+    end
+
+    context 'when the count limit has been reached' do
+      let_it_be(:previous_hooks) { create_list(:project_hook, 3) }
+
+      before do
+        stub_const('Gitlab::WebHooks::RecursionDetection::COUNT_LIMIT', 2)
+        # Recreate the necessary state for a number of previous webhooks to
+        # have been triggered previously.
+        Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
+        previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
+        Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
+      end
+
+      it 'executes and logs errors for all hooks', :aggregate_failures do
+        stub_requests
+        previous_hook_ids = previous_hooks.map(&:id)
+
+        expect(Gitlab::AuthLogger).to receive(:error).with(
+          include(
+            message: 'Webhook recursion detected and will be blocked in future',
+            hook_id: project_hook.id,
+            recursion_detection: {
+              uuid: uuid,
+              ids: include(*previous_hook_ids)
+            }
+          )
+        ).twice
+        expect(Gitlab::AuthLogger).to receive(:error).with(
+          include(
+            message: 'Webhook recursion detected and will be blocked in future',
+            hook_id: system_hook.id,
+            recursion_detection: {
+              uuid: uuid,
+              ids: include(*previous_hook_ids)
+            }
+          )
+        ).twice
+
+        trigger_web_hooks
+
+        expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
+        expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
+      end
+    end
+
+    include_examples 'when the feature flag is disabled'
+  end
+
+  context 'when the recursive webhook detection header is absent' do
+    let(:headers) { {} }
+
+    let(:uuid_header_spy) do
+      Class.new do
+        attr_reader :values
+
+        def initialize
+          @values = []
+        end
+
+        def to_proc
+          proc do |method, *args|
+            method.call(*args).tap do |headers|
+              @values << headers[Gitlab::WebHooks::RecursionDetection::UUID::HEADER]
+            end
+          end
+        end
+      end.new
+    end
+
+    before do
+      allow(Gitlab::WebHooks::RecursionDetection).to receive(:header).at_least(:once).and_wrap_original(&uuid_header_spy)
+    end
+
+    it 'executes all webhooks, logs no errors, and the webhook requests contain different UUID headers', :aggregate_failures do
+      stub_requests
+
+      expect(Gitlab::AuthLogger).not_to receive(:error)
+
+      trigger_web_hooks
+
+      uuid_headers = uuid_header_spy.values
+
+      expect(uuid_headers).to all(be_present)
+      expect(uuid_headers.uniq.length).to eq(2)
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+        .with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
+        .once
+      expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
+        .with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
+        .once
+    end
+
+    it 'uses new UUID values between requests' do
+      stub_requests
+
+      trigger_web_hooks
+      trigger_web_hooks
+
+      uuid_headers = uuid_header_spy.values
+
+      expect(uuid_headers).to all(be_present)
+      expect(uuid_headers.length).to eq(4)
+      expect(uuid_headers.uniq.length).to eq(4)
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).twice
+      expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).twice
+    end
+  end
+end

spec/services/web_hook_service_spec.rb

@@ -2,20 +2,12 @@
 
 require 'spec_helper'
 
-RSpec.describe WebHookService do
+RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state do
   include StubRequests
 
   let_it_be(:project) { create(:project) }
   let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) }
 
-  let(:headers) do
-    {
-      'Content-Type' => 'application/json',
-      'User-Agent' => "GitLab/#{Gitlab::VERSION}",
-      'X-Gitlab-Event' => 'Push Hook'
-    }
-  end
-
   let(:data) do
     { before: 'oldrev', after: 'newrev', ref: 'ref' }
   end
@@ -61,6 +53,21 @@ RSpec.describe WebHookService do
   end
 
   describe '#execute' do
+    let!(:uuid) { SecureRandom.uuid }
+    let(:headers) do
+      {
+        'Content-Type' => 'application/json',
+        'User-Agent' => "GitLab/#{Gitlab::VERSION}",
+        'X-Gitlab-Event' => 'Push Hook',
+        'X-Gitlab-Event-UUID' => uuid
+      }
+    end
+
+    before do
+      # Set a stable value for the `X-Gitlab-Event-UUID` header.
+      Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
+    end
+
     context 'when token is defined' do
       let_it_be(:project_hook) { create(:project_hook, :token) }
 
@@ -127,11 +134,74 @@ RSpec.describe WebHookService do
       expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' })
     end
 
+    it 'executes and registers the hook with the recursion detection', :aggregate_failures do
+      stub_full_request(project_hook.url, method: :post)
+      cache_key = Gitlab::WebHooks::RecursionDetection.send(:cache_key_for_hook, project_hook)
+
+      ::Gitlab::Redis::SharedState.with do |redis|
+        expect { service_instance.execute }.to change {
+          redis.sismember(cache_key, project_hook.id)
+        }.to(true)
+      end
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+        .with(headers: headers)
+        .once
+    end
+
+    it 'executes and logs if a recursive web hook is detected', :aggregate_failures do
+      stub_full_request(project_hook.url, method: :post)
+      Gitlab::WebHooks::RecursionDetection.register!(project_hook)
+
+      expect(Gitlab::AuthLogger).to receive(:error).with(
+        include(
+          message: 'Webhook recursion detected and will be blocked in future',
+          hook_id: project_hook.id,
+          hook_type: 'ProjectHook',
+          hook_name: 'push_hooks',
+          recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
+          'correlation_id' => kind_of(String)
+        )
+      )
+
+      service_instance.execute
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+        .with(headers: headers)
+        .once
+    end
+
+    it 'executes and logs if the recursion count limit would be exceeded', :aggregate_failures do
+      stub_full_request(project_hook.url, method: :post)
+      stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3)
+      previous_hooks = create_list(:project_hook, 3)
+      previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
+
+      expect(Gitlab::AuthLogger).to receive(:error).with(
+        include(
+          message: 'Webhook recursion detected and will be blocked in future',
+          hook_id: project_hook.id,
+          hook_type: 'ProjectHook',
+          hook_name: 'push_hooks',
+          recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
+          'correlation_id' => kind_of(String)
+        )
+      )
+
+      service_instance.execute
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+        .with(headers: headers)
+        .once
+    end
+
     it 'handles exceptions' do
       exceptions = Gitlab::HTTP::HTTP_ERRORS + [
         Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError
       ]
 
+      allow(Gitlab::WebHooks::RecursionDetection).to receive(:block?).and_return(false)
+
       exceptions.each do |exception_class|
         exception = exception_class.new('Exception message')
         project_hook.enable!
@@ -420,6 +490,57 @@ RSpec.describe WebHookService do
       end
     end
 
+    context 'recursion detection' do
+      before do
+        # Set a request UUID so `RecursionDetection.block?` will query redis.
+        Gitlab::WebHooks::RecursionDetection.set_request_uuid(SecureRandom.uuid)
+      end
+
+      it 'queues a worker and logs an error if the call chain limit would be exceeded' do
+        stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3)
+        previous_hooks = create_list(:project_hook, 3)
+        previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
+
+        expect(WebHookWorker).to receive(:perform_async)
+        expect(Gitlab::AuthLogger).to receive(:error).with(
+          include(
+            message: 'Webhook recursion detected and will be blocked in future',
+            hook_id: project_hook.id,
+            hook_type: 'ProjectHook',
+            hook_name: 'push_hooks',
+            recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
+            'correlation_id' => kind_of(String),
+            'meta.project' => project.full_path,
+            'meta.related_class' => 'ProjectHook',
+            'meta.root_namespace' => project.root_namespace.full_path
+          )
+        )
+
+        service_instance.async_execute
+      end
+
+      it 'queues a worker and logs an error if a recursive call chain is detected' do
+        Gitlab::WebHooks::RecursionDetection.register!(project_hook)
+
+        expect(WebHookWorker).to receive(:perform_async)
+        expect(Gitlab::AuthLogger).to receive(:error).with(
+          include(
+            message: 'Webhook recursion detected and will be blocked in future',
+            hook_id: project_hook.id,
+            hook_type: 'ProjectHook',
+            hook_name: 'push_hooks',
+            recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
+            'correlation_id' => kind_of(String),
+            'meta.project' => project.full_path,
+            'meta.related_class' => 'ProjectHook',
+            'meta.root_namespace' => project.root_namespace.full_path
+          )
+        )
+
+        service_instance.async_execute
+      end
+    end
+
     context 'when hook has custom context attributes' do
       it 'includes the attributes in the worker context' do
         expect(WebHookWorker).to receive(:perform_async) do

spec/workers/web_hook_worker_spec.rb

@@ -19,6 +19,15 @@ RSpec.describe WebHookWorker do
       expect { subject.perform(non_existing_record_id, data, hook_name) }.not_to raise_error
     end
 
+    it 'retrieves recursion detection data, reinstates it, and cleans it from payload', :request_store, :aggregate_failures do
+      uuid = SecureRandom.uuid
+      full_data = data.merge({ _gitlab_recursion_detection_request_uuid: uuid })
+
+      expect_next(WebHookService, project_hook, data.with_indifferent_access, hook_name, anything).to receive(:execute)
+      expect { subject.perform(project_hook.id, full_data, hook_name) }
+        .to change { Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid }.to(uuid)
+    end
+
     it_behaves_like 'worker with data consistency',
                   described_class,
                   data_consistency: :delayed