diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index e7a35a8ded489b01a0b0a2778ac75276cd8dba04..00cb6c311c0b4e37429a4e0cbeacfa2cf5ae5578 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -73,8 +73,15 @@ def execute return ServiceResponse.error(message: 'Hook disabled') if disabled? + if rate_limit! + log_rate_limited + create_broadcast_message("Webhook rate limit exceeded") + return ServiceResponse.error(message: 'Webhook rate limit exceeded') + end + if recursion_blocked? log_recursion_blocked + create_broadcast_message("Recursive webhook blocked") return ServiceResponse.error(message: 'Recursive webhook blocked') end @@ -117,8 +124,18 @@ def execute def async_execute Gitlab::ApplicationContext.with_context(hook.application_context) do break log_silent_mode_enabled if Gitlab::SilentMode.enabled? - break log_rate_limited if rate_limit! - break log_recursion_blocked if recursion_blocked? + + if rate_limit! + log_rate_limited + create_broadcast_message("Webhook rate limit exceeded") + break + end + + if recursion_blocked? + log_recursion_blocked + create_broadcast_message("Recursive webhook blocked") + break + end params = { "recursion_detection_request_uuid" => Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid, @@ -325,4 +342,47 @@ def render_custom_template(template, params) def raise_custom_webhook_template_error!(message) raise CustomWebHookTemplateError, "Error while parsing rendered custom webhook template: #{message}" end + + def create_broadcast_message(reason) + params = broadcast_message_params(reason) + + existing_messages = System::BroadcastMessage.current_banner_messages( + current_path: params[:target_path], + user_access_level: params[:target_access_levels].first + ).select { |current_message| current_message.message == params[:message] } + + return unless existing_messages.empty? + + System::BroadcastMessage.new(params).save! + rescue StandardError => e + Gitlab::AppLogger.error("Failed to save broadcast message for Webhook ID #{hook.id}, #{reason}") + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) + end + + def broadcast_message_params(reason) + hook_type = hook.type.gsub('Hook', '').downcase + message = format(s_("WebHooks|%{reason}. Update or delete the following ") + + hook_type + + s_(" hook: %{hook_name} (ID: %{hook_id})."), + reason: reason, + hook_name: hook.name, + hook_id: hook.id).squish + + base_params = { + message: message, + starts_at: Time.current, + ends_at: 3.months.from_now, + dismissable: true, + theme: "light-red" + } + + case hook + when SystemHook + base_params.merge(target_path: "/admin", target_access_levels: []) + when ProjectHook + base_params.merge(target_path: "/#{hook.project.full_path}", target_access_levels: [Gitlab::Access::OWNER]) + when defined?(GroupHook) && GroupHook + base_params.merge(target_path: "/#{hook.group.full_path}", target_access_levels: [Gitlab::Access::OWNER]) + end + end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4a7b416354ca7ddb84419c87094826d8da65091e..ef33dbc4df7dd00801644794f361c3b1bc4043a5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50,6 +50,9 @@ msgid_plural " except branches:" msgstr[0] "" msgstr[1] "" +msgid " hook: %{hook_name} (ID: %{hook_id})." +msgstr "" + msgid " or " msgstr "" @@ -73149,6 +73152,9 @@ msgstr "" msgid "WebAuthn only works with HTTPS-enabled websites. Contact your administrator for more details." msgstr "" +msgid "WebHooks|%{reason}. Update or delete the following " +msgstr "" + msgid "WebIDE|Quickly and easily edit multiple files in your project." msgstr "" diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index d8536acc573548001c3e044c7df356fea6117a2e..f1c669cd71558d4b4a65b47603659bbc70f1fe68 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -8,6 +8,12 @@ let(:uuid_regex) { /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ } let(:ellipsis) { '…' } let_it_be(:project) { create(:project) } + let(:recursion_message) do + "Recursive webhook blocked. " \ + "Update or delete the following project hook: " \ + "#{project_hook.name} (ID: #{project_hook.id})." + end + let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } let(:data) do @@ -222,6 +228,94 @@ end end + context 'when rate limited' do + let(:hook_type) { hook.type.gsub('Hook', '').downcase } + let(:message) do + "Webhook rate limit exceeded. " \ + "Update or delete the following #{hook_type} hook: " \ + "#{hook.name} (ID: #{hook.id})." + end + + subject(:service) { described_class.new(hook, {}, hook.name) } + + before do + allow_next_instance_of(Gitlab::WebHooks::RateLimiter) do |limiter| + allow(limiter).to receive(:rate_limit!).and_return(true) + end + end + + context 'with SystemHook' do + let(:hook) { create(:system_hook, name: 'Test System Hook') } + + it 'creates a broadcast message' do + expect(System::BroadcastMessage).to receive(:new).with( + hash_including( + message: message, + target_path: "/admin" + ) + ).and_call_original + + service.execute + end + end + + context 'with ProjectHook' do + let_it_be(:project) { create(:project) } + let(:hook) { create(:project_hook, project: project, name: 'Test Project Hook') } + + it 'creates a broadcast message' do + expect(System::BroadcastMessage).to receive(:new).with( + hash_including( + message: message, + target_path: "/#{project.full_path}" + ) + ).and_call_original + + service.execute + end + end + + if Gitlab.ee? + context 'with GroupHook' do + let(:group) { create(:group) } + let(:hook) { create(:group_hook, group: group, name: 'Test Group Hook') } + + it 'creates a broadcast message' do + expect(System::BroadcastMessage).to receive(:new).with( + hash_including( + message: message, + target_path: "/#{group.full_path}" + ) + ).and_call_original + + service.execute + end + end + end + + context 'when the broadcast message fails to save' do + let(:error) { StandardError.new("Test Error") } + let(:hook) { create(:system_hook, name: 'Test System Hook') } + let(:save_error_message) do + "Failed to save broadcast message for Webhook ID #{hook.id}, Webhook rate limit exceeded" + end + + before do + allow_next_instance_of(System::BroadcastMessage) do |message| + allow(message).to receive(:save!).and_raise(error) + end + end + + it 'logs an error' do + allow(Gitlab::AppLogger).to receive(:error) + expect(Gitlab::AppLogger).to receive(:error).with(save_error_message) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(error) + + service.execute + end + end + end + context 'with SystemHook' do let_it_be(:system_hook) { create(:system_hook) } let(:service_instance) { described_class.new(system_hook, data, :push_hooks) } @@ -321,7 +415,7 @@ .once end - it 'blocks and logs if a recursive web hook is detected', :aggregate_failures do + it 'blocks, logs, and creates broadcast message if a recursive web hook is detected', :aggregate_failures do stub_full_request(project_hook.url, method: :post) Gitlab::WebHooks::RecursionDetection.register!(project_hook) @@ -336,12 +430,17 @@ ) ) + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: recursion_message) + ).and_call_original + service_instance.execute expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url)) + expect(System::BroadcastMessage.last.message).to eq(recursion_message) end - it 'blocks and logs if the recursion count limit would be exceeded', :aggregate_failures do + it 'blocks, logs, and creates a broadcast message if the recursion count limit is 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) @@ -358,9 +457,14 @@ ) ) + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: recursion_message) + ).and_call_original + service_instance.execute expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url)) + expect(System::BroadcastMessage.last.message).to eq(recursion_message) end context 'when silent mode is enabled' do @@ -926,6 +1030,12 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) end context 'when rate limiting is configured' do + let(:rate_limit_message) do + "Webhook rate limit exceeded. " \ + "Update or delete the following project hook: " \ + "#{project_hook.name} (ID: #{project_hook.id})." + end + let_it_be(:threshold) { 3 } let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: threshold) } @@ -961,6 +1071,10 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) ) ) + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: rate_limit_message) + ).and_call_original + service_instance.async_execute end end @@ -972,9 +1086,13 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) threshold.times { service_instance.async_execute } end - it 'stops queueing workers and logs errors' do + it 'stops queueing workers, creates a single broadcast message, and logs errors' do expect(Gitlab::AuthLogger).to receive(:error).twice + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: rate_limit_message) + ).and_call_original.once + 2.times { service_instance.async_execute } end @@ -994,7 +1112,7 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) Gitlab::WebHooks::RecursionDetection.set_request_uuid(SecureRandom.uuid) end - it 'does not queue a worker and logs an error if the call chain limit would be exceeded' do + it 'does not queue worker, creates broadcast message, and logs error if call chain limit is 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) } @@ -1014,10 +1132,14 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) ) ) + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: recursion_message) + ).and_call_original + service_instance.async_execute end - it 'does not queue a worker and logs an error if a recursive call chain is detected' do + it 'does not queue worker, creates broadcast message, and logs error if recursive call chain is detected' do Gitlab::WebHooks::RecursionDetection.register!(project_hook) expect(WebHookWorker).not_to receive(:perform_async) @@ -1035,6 +1157,10 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) ) ) + expect(System::BroadcastMessage).to receive(:new).with( + hash_including(message: recursion_message) + ).and_call_original + service_instance.async_execute end end