Merge branch 'security-223-webhook-dos-attack' into 'master'

Rate limit on webhooks testing feature

Closes #223

See merge request gitlab-org/security/gitlab!826

app/controllers/concerns/hooks_execution.rb

@@ -17,4 +17,16 @@ module HooksExecution
       flash[:alert] = "Hook execution failed: #{message}"
     end
   end
+
+  def create_rate_limit(key, scope)
+    if rate_limiter.throttled?(key, scope: [scope, current_user])
+      rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
+
+      render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests
+    end
+  end
+
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
+  end
 end

app/controllers/projects/hooks_controller.rb

@@ -6,6 +6,7 @@ class Projects::HooksController < Projects::ApplicationController
   # Authorize
   before_action :authorize_admin_project!
   before_action :hook_logs, only: :edit
+  before_action -> { create_rate_limit(:project_testing_hook, @project) }, only: :test
 
   respond_to :html
 

changelogs/unreleased/security-223-webhook-dos-attack.yml

+---
+title: Add rate limit on webhooks testing feature
+merge_request:
+author:
+type: security

ee/app/controllers/groups/hooks_controller.rb

@@ -8,6 +8,7 @@ class Groups::HooksController < Groups::ApplicationController
   before_action :authorize_admin_group!
   before_action :check_group_webhooks_available!
   before_action :set_hook, only: [:edit, :update, :test, :destroy]
+  before_action -> { create_rate_limit(:group_testing_hook, @group) }, only: :test
 
   respond_to :html
 

ee/spec/controllers/groups/hooks_controller_spec.rb

@@ -154,6 +154,24 @@ RSpec.describe Groups::HooksController do
             expect(flash[:notice]).to eq('Hook executed successfully: HTTP 200')
           end
         end
+
+        context 'when the endpoint receives requests above the limit' do
+          before do
+            allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits)
+              .and_return(group_testing_hook: { threshold: 1, interval: 1.minute })
+          end
+
+          it 'prevents making test requests' do
+            expect_next_instance_of(TestHooks::ProjectService) do |service|
+              expect(service).to receive(:execute).and_return(http_status: 200)
+            end
+
+            2.times { post :test, params: { group_id: group.to_param, id: hook } }
+
+            expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
+            expect(response).to have_gitlab_http_status(:too_many_requests)
+          end
+        end
       end
     end
   end

lib/gitlab/application_rate_limiter.rb

@@ -25,11 +25,13 @@ module Gitlab
           project_repositories_archive: { threshold: 5, interval: 1.minute },
           project_generate_new_export:  { threshold: -> { application_settings.project_export_limit }, interval: 1.minute },
           project_import:               { threshold: -> { application_settings.project_import_limit }, interval: 1.minute },
+          project_testing_hook:         { threshold: 5, interval: 1.minute },
           play_pipeline_schedule:       { threshold: 1, interval: 1.minute },
           show_raw_controller:          { threshold: -> { application_settings.raw_blob_request_limit }, interval: 1.minute },
           group_export:                 { threshold: -> { application_settings.group_export_limit }, interval: 1.minute },
           group_download_export:        { threshold: -> { application_settings.group_download_export_limit }, interval: 1.minute },
-          group_import:                 { threshold: -> { application_settings.group_import_limit }, interval: 1.minute }
+          group_import:                 { threshold: -> { application_settings.group_import_limit }, interval: 1.minute },
+          group_testing_hook:           { threshold: 5, interval: 1.minute }
         }.freeze
       end
 

spec/controllers/projects/hooks_controller_spec.rb

@@ -47,4 +47,26 @@ RSpec.describe Projects::HooksController do
       expect(ProjectHook.first).to have_attributes(hook_params)
     end
   end
+
+  describe '#test' do
+    let(:hook) { create(:project_hook, project: project) }
+
+    context 'when the endpoint receives requests above the limit' do
+      before do
+        allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits)
+          .and_return(project_testing_hook: { threshold: 1, interval: 1.minute })
+      end
+
+      it 'prevents making test requests' do
+        expect_next_instance_of(TestHooks::ProjectService) do |service|
+          expect(service).to receive(:execute).and_return(http_status: 200)
+        end
+
+        2.times { post :test, params: { namespace_id: project.namespace, project_id: project, id: hook } }
+
+        expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
+        expect(response).to have_gitlab_http_status(:too_many_requests)
+      end
+    end
+  end
 end