diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 33e34ec41e2a4b4e689525afe04ab2cd2dd8a0e6..9bfcd9a9ec65210a8fd514280b2c546a563c53a9 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -49,7 +49,10 @@ def initialize(hook, data, hook_name, uniqueness_token = nil) 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 @@ def execute 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 @@ def rate_limit @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 @@ def log_rate_limit ) 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, diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 3bedb6b01bdf93f50c7127599f625671c3f5f79d..e985dfec339a78ef11166383f8390ca23f983a11 100644 --- a/doc/administration/instance_limits.md +++ b/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. diff --git a/spec/requests/recursive_webhook_detection_spec.rb b/spec/requests/recursive_webhook_detection_spec.rb index a3014bf1d73df1ec9ca14f34bce986fb1a72340e..fe27c90b6c8a74335dbf84060dd9da71e323d0a0 100644 --- a/spec/requests/recursive_webhook_detection_spec.rb +++ b/spec/requests/recursive_webhook_detection_spec.rb @@ -11,6 +11,11 @@ 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 @@ def trigger_web_hooks 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 @@ def stub_requests 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 @@ def stub_requests 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 @@ def stub_requests 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 @@ def to_proc 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 @@ def to_proc 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 diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 7d933ea9c5cd0b1e252e0aae3b46c0de6090dc62..14aa4e364a14ae753adef03fd866f075a31da75a 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -149,13 +149,13 @@ .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 @@ 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 @@ 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 @@ 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 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) 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 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) 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',