Merge branch '329743-block-recursive-webhooks' into 'master'

Block recursive web hooks

See merge request gitlab-org/gitlab!79085

app/services/web_hook_service.rb

@@ -49,7 +49,10 @@ class WebHookService
   def execute
     return { status: :error, message: 'Hook disabled' } unless hook.executable?
 
-    log_recursion_limit if recursion_blocked?
+    if recursion_blocked?
+      log_recursion_blocked
+      return { status: :error, message: 'Recursive webhook blocked' }
+    end
 
     Gitlab::WebHooks::RecursionDetection.register!(hook)
 
@@ -96,9 +99,8 @@ class WebHookService
 
   def async_execute
     Gitlab::ApplicationContext.with_context(hook.application_context) do
-      break log_rate_limit if rate_limited?
-
-      log_recursion_limit if recursion_blocked?
+      break log_rate_limited if rate_limited?
+      break log_recursion_blocked if recursion_blocked?
 
       data[:_gitlab_recursion_detection_request_uuid] = Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
 
@@ -202,7 +204,7 @@ class WebHookService
     @rate_limit ||= hook.rate_limit
   end
 
-  def log_rate_limit
+  def log_rate_limited
     Gitlab::AuthLogger.error(
       message: 'Webhook rate limit exceeded',
       hook_id: hook.id,
@@ -212,9 +214,9 @@ class WebHookService
     )
   end
 
-  def log_recursion_limit
+  def log_recursion_blocked
     Gitlab::AuthLogger.error(
-      message: 'Webhook recursion detected and will be blocked in future',
+      message: 'Recursive webhook blocked from executing',
       hook_id: hook.id,
       hook_type: hook.type,
       hook_name: hook_name,

doc/administration/instance_limits.md

@@ -228,7 +228,11 @@ There is a limit when embedding metrics in GitLab Flavored Markdown (GFM) for pe
 
 - **Max limit**: 100 embeds.
 
-## Number of webhooks
+## Webhook limits
+
+Also see [Webhook rate limits](#webhook-rate-limit).
+
+### Number of webhooks
 
 To set the maximum number of group or project webhooks for a self-managed installation,
 run the following in the [GitLab Rails console](operations/rails_console.md#starting-a-rails-console-session):
@@ -246,11 +250,33 @@ Plan.default.actual_limits.update!(group_hooks: 100)
 
 Set the limit to `0` to disable it.
 
-- **Default maximum number of webhooks**: `100` per project, `50` per group
-- **Maximum payload size**: 25 MB
+The default maximum number of webhooks is `100` per project, `50` per group.
 
 For GitLab.com, see the [webhook limits for GitLab.com](../user/gitlab_com/index.md#webhooks).
 
+### Webhook payload size
+
+The maximum webhook payload size is 25 MB.
+
+### Recursive webhooks
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/329743) in GitLab 14.8.
+
+GitLab detects and blocks webhooks that are recursive or that exceed the limit
+of webhooks that can be triggered from other webhooks. This enables GitLab to
+continue to support workflows that use webhooks to call the API non-recursively, or that
+do not trigger an unreasonable number of other webhooks.
+
+Recursion can happen when a webhook is configured to make a call
+to its own GitLab instance (for example, the API). The call then triggers the same
+webhook and creates an infinite loop.
+
+The maximum number of requests to an instance made by a series of webhooks that
+trigger other webhooks is 100. When the limit is reached, GitLab blocks any further
+webhooks that would be triggered by the series.
+
+Blocked recursive webhook calls are logged in `auth.log` with the message `"Recursive webhook blocked from executing"`.
+
 ## Pull Mirroring Interval
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/237891) in GitLab 13.7.

spec/requests/recursive_webhook_detection_spec.rb

@@ -11,6 +11,11 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
   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) }
 
+  let(:stubbed_project_hook_hostname) { stubbed_hostname(project_hook.url, hostname: stubbed_project_hook_ip_address) }
+  let(:stubbed_system_hook_hostname) { stubbed_hostname(system_hook.url, hostname: stubbed_system_hook_ip_address) }
+  let(:stubbed_project_hook_ip_address) { '8.8.8.8' }
+  let(:stubbed_system_hook_ip_address) { '8.8.8.9' }
+
   # Trigger a change to the merge request to fire the webhooks.
   def trigger_web_hooks
     params = { merge_request: { description: FFaker::Lorem.sentence } }
@@ -18,8 +23,8 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
   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')
+    stub_full_request(project_hook.url, method: :post, ip_address: stubbed_project_hook_ip_address)
+    stub_full_request(system_hook.url, method: :post, ip_address: stubbed_system_hook_ip_address)
   end
 
   before do
@@ -37,10 +42,10 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
 
       trigger_web_hooks
 
-      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
+      expect(WebMock).to have_requested(:post, stubbed_project_hook_hostname)
         .with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
         .once
-      expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
+      expect(WebMock).to have_requested(:post, stubbed_system_hook_hostname)
         .with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
         .once
     end
@@ -54,24 +59,24 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
         Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
       end
 
-      it 'executes all webhooks and logs an error for the recursive hook', :aggregate_failures do
+      it 'blocks and logs an error for the recursive webhook, but execute the non-recursive webhook', :aggregate_failures do
         stub_requests
 
         expect(Gitlab::AuthLogger).to receive(:error).with(
           include(
-            message: 'Webhook recursion detected and will be blocked in future',
+            message: 'Recursive webhook blocked from executing',
             hook_id: project_hook.id,
             recursion_detection: {
               uuid: uuid,
               ids: [project_hook.id]
             }
           )
-        ).twice # Twice: once in `#async_execute`, and again in `#execute`.
+        ).once
 
         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
+        expect(WebMock).not_to have_requested(:post, stubbed_project_hook_hostname)
+        expect(WebMock).to have_requested(:post, stubbed_system_hook_hostname).once
       end
     end
 
@@ -87,35 +92,35 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
         Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
       end
 
-      it 'executes and logs errors for all hooks', :aggregate_failures do
+      it 'blocks 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',
+            message: 'Recursive webhook blocked from executing',
             hook_id: project_hook.id,
             recursion_detection: {
               uuid: uuid,
               ids: include(*previous_hook_ids)
             }
           )
-        ).twice
+        ).once
         expect(Gitlab::AuthLogger).to receive(:error).with(
           include(
-            message: 'Webhook recursion detected and will be blocked in future',
+            message: 'Recursive webhook blocked from executing',
             hook_id: system_hook.id,
             recursion_detection: {
               uuid: uuid,
               ids: include(*previous_hook_ids)
             }
           )
-        ).twice
+        ).once
 
         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
+        expect(WebMock).not_to have_requested(:post, stubbed_project_hook_hostname)
+        expect(WebMock).not_to have_requested(:post, stubbed_system_hook_hostname)
       end
     end
   end
@@ -156,10 +161,10 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
 
       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))
+      expect(WebMock).to have_requested(:post, stubbed_project_hook_hostname)
         .with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
         .once
-      expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
+      expect(WebMock).to have_requested(:post, stubbed_system_hook_hostname)
         .with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
         .once
     end
@@ -175,8 +180,8 @@ RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_red
       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
+      expect(WebMock).to have_requested(:post, stubbed_project_hook_hostname).twice
+      expect(WebMock).to have_requested(:post, stubbed_system_hook_hostname).twice
     end
   end
 end

spec/services/web_hook_service_spec.rb

@@ -149,13 +149,13 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
         .once
     end
 
-    it 'executes and logs if a recursive web hook is detected', :aggregate_failures do
+    it 'blocks 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',
+          message: 'Recursive webhook blocked from executing',
           hook_id: project_hook.id,
           hook_type: 'ProjectHook',
           hook_name: 'push_hooks',
@@ -166,12 +166,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
 
       service_instance.execute
 
-      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
-        .with(headers: headers)
-        .once
+      expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url))
     end
 
-    it 'executes and logs if the recursion count limit would be exceeded', :aggregate_failures do
+    it 'blocks 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)
@@ -179,7 +177,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
 
       expect(Gitlab::AuthLogger).to receive(:error).with(
         include(
-          message: 'Webhook recursion detected and will be blocked in future',
+          message: 'Recursive webhook blocked from executing',
           hook_id: project_hook.id,
           hook_type: 'ProjectHook',
           hook_name: 'push_hooks',
@@ -190,9 +188,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
 
       service_instance.execute
 
-      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
-        .with(headers: headers)
-        .once
+      expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url))
     end
 
     it 'handles exceptions' do
@@ -496,15 +492,15 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
         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
+      it 'does not queue 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(WebHookWorker).not_to receive(:perform_async)
         expect(Gitlab::AuthLogger).to receive(:error).with(
           include(
-            message: 'Webhook recursion detected and will be blocked in future',
+            message: 'Recursive webhook blocked from executing',
             hook_id: project_hook.id,
             hook_type: 'ProjectHook',
             hook_name: 'push_hooks',
@@ -519,13 +515,13 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
         service_instance.async_execute
       end
 
-      it 'queues a worker and logs an error if a recursive call chain is detected' do
+      it 'does not queue 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(WebHookWorker).not_to receive(:perform_async)
         expect(Gitlab::AuthLogger).to receive(:error).with(
           include(
-            message: 'Webhook recursion detected and will be blocked in future',
+            message: 'Recursive webhook blocked from executing',
             hook_id: project_hook.id,
             hook_type: 'ProjectHook',
             hook_name: 'push_hooks',