Merge branch 'georgekoltsov/add-rate-limit-to-exports' into 'master'

Project Export rate limits

See merge request gitlab-org/gitlab!20962

app/controllers/projects/pipeline_schedules_controller.rb

@@ -83,12 +83,14 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   def play_rate_limit
     return unless current_user
 
-    limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule)
-
-    return unless limiter.throttled?([current_user, schedule], 1)
+    if rate_limiter.throttled?(:play_pipeline_schedule, scope: [current_user, schedule])
+      flash[:alert] = _('You cannot play this scheduled pipeline at the moment. Please wait a minute.')
+      redirect_to pipeline_schedules_path(@project)
+    end
+  end
 
-    flash[:alert] = _('You cannot play this scheduled pipeline at the moment. Please wait a minute.')
-    redirect_to pipeline_schedules_path(@project)
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
   end
 
   def schedule

app/controllers/projects/raw_controller.rb

@@ -19,14 +19,16 @@ class Projects::RawController < Projects::ApplicationController
   private
 
   def show_rate_limit
-    limiter = ::Gitlab::ActionRateLimiter.new(action: :show_raw_controller)
+    if rate_limiter.throttled?(:show_raw_controller, scope: [@project, @commit, @path], threshold: raw_blob_request_limit)
+      rate_limiter.log_request(request, :raw_blob_request_limit, current_user)
 
-    return unless limiter.throttled?([@project, @commit, @path], raw_blob_request_limit)
-
-    limiter.log_request(request, :raw_blob_request_limit, current_user)
+      flash[:alert] = _('You cannot access the raw file. Please wait a minute.')
+      redirect_to project_blob_path(@project, File.join(@ref, @path)), status: :too_many_requests
+    end
+  end
 
-    flash[:alert] = _('You cannot access the raw file. Please wait a minute.')
-    redirect_to project_blob_path(@project, File.join(@ref, @path)), status: :too_many_requests
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
   end
 
   def raw_blob_request_limit

app/controllers/projects_controller.rb

@@ -32,6 +32,9 @@ class ProjectsController < Projects::ApplicationController
   before_action :authorize_archive_project!, only: [:archive, :unarchive]
   before_action :event_filter, only: [:show, :activity]
 
+  # Project Export Rate Limit
+  before_action :export_rate_limit, only: [:export, :download_export, :generate_new_export]
+
   layout :determine_layout
 
   def index
@@ -465,6 +468,21 @@ class ProjectsController < Projects::ApplicationController
   def present_project
     @project = @project.present(current_user: current_user)
   end
+
+  def export_rate_limit
+    prefixed_action = "project_#{params[:action]}".to_sym
+
+    if rate_limiter.throttled?(prefixed_action, scope: [current_user, prefixed_action, @project])
+      rate_limiter.log_request(request, "#{prefixed_action}_request_limit".to_sym, current_user)
+
+      flash[:alert] = _('This endpoint has been requested too many times. Try again later.')
+      redirect_to edit_project_path(@project)
+    end
+  end
+
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
+  end
 end
 
 ProjectsController.prepend_if_ee('EE::ProjectsController')

changelogs/unreleased/georgekoltsov-add-rate-limit-to-exports.yml

+---
+title: Add Project Export request/download rate limits
+merge_request: 20962
+author:
+type: other

lib/api/project_export.rb

@@ -2,6 +2,15 @@
 
 module API
   class ProjectExport < Grape::API
+    helpers do
+      def throttled?(action)
+        rate_limiter.throttled?(action, scope: [current_user, action, user_project])
+      end
+
+      def rate_limiter
+        ::Gitlab::ApplicationRateLimiter
+      end
+    end
     before do
       not_found! unless Gitlab::CurrentSettings.project_export_enabled?
       authorize_admin_project
@@ -23,6 +32,10 @@ module API
         detail 'This feature was introduced in GitLab 10.6.'
       end
       get ':id/export/download' do
+        if throttled?(:project_download_export)
+          render_api_error!({ error: 'This endpoint has been requested too many times. Try again later.' }, 429)
+        end
+
         if user_project.export_file_exists?
           present_carrierwave_file!(user_project.export_file)
         else
@@ -41,6 +54,10 @@ module API
         end
       end
       post ':id/export' do
+        if throttled?(:project_export)
+          render_api_error!({ error: 'This endpoint has been requested too many times. Try again later.' }, 429)
+        end
+
         project_export_params = declared_params(include_missing: false)
         after_export_params = project_export_params.delete(:upload) || {}
 

lib/gitlab/action_rate_limiter.rb

-# frozen_string_literal: true
-
-module Gitlab
-  # This class implements a simple rate limiter that can be used to throttle
-  # certain actions. Unlike Rack Attack and Rack::Throttle, which operate at
-  # the middleware level, this can be used at the controller level.
-  class ActionRateLimiter
-    TIME_TO_EXPIRE = 60 # 1 min
-
-    attr_accessor :action, :expiry_time
-
-    def initialize(action:, expiry_time: TIME_TO_EXPIRE)
-      @action = action
-      @expiry_time = expiry_time
-    end
-
-    # Increments the given cache key and increments the value by 1 with the
-    # given expiration time. Returns the incremented value.
-    #
-    # key - An array of ActiveRecord instances
-    def increment(key)
-      value = 0
-
-      Gitlab::Redis::Cache.with do |redis|
-        cache_key = action_key(key)
-        value = redis.incr(cache_key)
-        redis.expire(cache_key, expiry_time) if value == 1
-      end
-
-      value
-    end
-
-    # Increments the given key and returns true if the action should
-    # be throttled.
-    #
-    # key - An array of ActiveRecord instances or strings
-    # threshold_value - The maximum number of times this action should occur in the given time interval. If number is zero is considered disabled.
-    def throttled?(key, threshold_value)
-      threshold_value > 0 &&
-        self.increment(key) > threshold_value
-    end
-
-    # Logs request into auth.log
-    #
-    # request - Web request to be logged
-    # type - A symbol key that represents the request.
-    # current_user - Current user of the request, it can be nil.
-    def log_request(request, type, current_user)
-      request_information = {
-        message: 'Action_Rate_Limiter_Request',
-        env: type,
-        remote_ip: request.ip,
-        request_method: request.request_method,
-        path: request.fullpath
-      }
-
-      if current_user
-        request_information.merge!({
-          user_id: current_user.id,
-          username: current_user.username
-        })
-      end
-
-      Gitlab::AuthLogger.error(request_information)
-    end
-
-    private
-
-    def action_key(key)
-      serialized = key.map do |obj|
-        if obj.is_a?(String)
-          "#{obj}"
-        else
-          "#{obj.class.model_name.to_s.underscore}:#{obj.id}"
-        end
-      end.join(":")
-
-      "action_rate_limiter:#{action}:#{serialized}"
-    end
-  end
-end

lib/gitlab/application_rate_limiter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  # This class implements a simple rate limiter that can be used to throttle
+  # certain actions. Unlike Rack Attack and Rack::Throttle, which operate at
+  # the middleware level, this can be used at the controller or API level.
+  #
+  # @example
+  #  if Gitlab::ApplicationRateLimiter.throttled?(:project_export, scope: [@project, @current_user])
+  #   flash[:alert] = 'error!'
+  #   redirect_to(edit_project_path(@project), status: :too_many_requests)
+  # end
+  class ApplicationRateLimiter
+    class << self
+      # Application rate limits
+      #
+      # Threshold value can be either an Integer or a Proc
+      # in order to not evaluate it's value every time this method is called
+      # and only do that when it's needed.
+      def rate_limits
+        {
+          project_export:              { threshold: 1, interval: 5.minutes },
+          project_download_export:     { threshold: 10, interval: 10.minutes },
+          project_generate_new_export: { threshold: 1, interval: 5.minutes },
+          play_pipeline_schedule:      { threshold: 1, interval: 1.minute },
+          show_raw_controller:         { threshold: -> { Gitlab::CurrentSettings.current_application_settings.raw_blob_request_limit }, interval: 1.minute }
+        }.freeze
+      end
+
+      # Increments the given key and returns true if the action should
+      # be throttled.
+      #
+      # @param key [Symbol] Key attribute registered in `.rate_limits`
+      # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
+      # @option threshold [Integer] Optional threshold value to override default one registered in `.rate_limits`
+      # @option interval [Integer] Optional interval value to override default one registered in `.rate_limits`
+      #
+      # @return [Boolean] Whether or not a request should be throttled
+      def throttled?(key, scope: nil, interval: nil, threshold: nil)
+        return unless rate_limits[key]
+
+        threshold_value = threshold || threshold(key)
+
+        threshold_value > 0 &&
+          increment(key, scope, interval) > threshold_value
+      end
+
+      # Increments the given cache key and increments the value by 1 with the
+      # expiration interval defined in `.rate_limits`.
+      #
+      # @param key [Symbol] Key attribute registered in `.rate_limits`
+      # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
+      # @option interval [Integer] Optional interval value to override default one registered in `.rate_limits`
+      #
+      # @return [Integer] incremented value
+      def increment(key, scope, interval = nil)
+        value = 0
+        interval_value = interval || interval(key)
+
+        Gitlab::Redis::Cache.with do |redis|
+          cache_key = action_key(key, scope)
+          value     = redis.incr(cache_key)
+          redis.expire(cache_key, interval_value) if value == 1
+        end
+
+        value
+      end
+
+      # Logs request using provided logger
+      #
+      # @param request [Http::Request] - Web request to be logged
+      # @param type [Symbol] A symbol key that represents the request
+      # @param current_user [User] Current user of the request, it can be nil
+      # @param logger [Logger] Logger to log request to a specific log file. Defaults to Gitlab::AuthLogger
+      def log_request(request, type, current_user, logger = Gitlab::AuthLogger)
+        request_information = {
+          message:        'Application_Rate_Limiter_Request',
+          env:            type,
+          remote_ip:      request.ip,
+          request_method: request.request_method,
+          path:           request.fullpath
+        }
+
+        if current_user
+          request_information.merge!({
+                                       user_id:  current_user.id,
+                                       username: current_user.username
+                                     })
+        end
+
+        logger.error(request_information)
+      end
+
+      private
+
+      def threshold(key)
+        value = rate_limit_value_by_key(key, :threshold)
+
+        return value.call if value.is_a?(Proc)
+
+        value.to_i
+      end
+
+      def interval(key)
+        rate_limit_value_by_key(key, :interval).to_i
+      end
+
+      def rate_limit_value_by_key(key, setting)
+        action = rate_limits[key]
+
+        action[setting] if action
+      end
+
+      def action_key(key, scope)
+        composed_key = [key, scope].flatten.compact
+
+        serialized = composed_key.map do |obj|
+          if obj.is_a?(String) || obj.is_a?(Symbol)
+            "#{obj}"
+          else
+            "#{obj.class.model_name.to_s.underscore}:#{obj.id}"
+          end
+        end.join(":")
+
+        "application_rate_limiter:#{serialized}"
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -17898,6 +17898,9 @@ msgstr ""
 msgid "This domain is not verified. You will need to verify ownership before access is enabled."
 msgstr ""
 
+msgid "This endpoint has been requested too many times. Try again later."
+msgstr ""
+
 msgid "This environment has no deployments yet."
 msgstr ""
 

spec/controllers/projects/raw_controller_spec.rb

@@ -65,7 +65,7 @@ describe Projects::RawController do
 
       it 'logs the event on auth.log' do
         attributes = {
-          message: 'Action_Rate_Limiter_Request',
+          message: 'Application_Rate_Limiter_Request',
           env: :raw_blob_request_limit,
           remote_ip: '0.0.0.0',
           request_method: 'GET',

spec/controllers/projects_controller_spec.rb

@@ -1055,45 +1055,34 @@ describe ProjectsController do
     end
   end
 
-  describe '#export' do
+  describe 'project export' do
     before do
       sign_in(user)
 
       project.add_maintainer(user)
     end
 
-    context 'when project export is enabled' do
-      it 'returns 302' do
-        get :export, params: { namespace_id: project.namespace, id: project }
-
-        expect(response).to have_gitlab_http_status(302)
-      end
-    end
-
-    context 'when project export is disabled' do
+    shared_examples 'rate limits project export endpoint' do
       before do
-        stub_application_setting(project_export_enabled?: false)
+        allow(::Gitlab::ApplicationRateLimiter)
+          .to receive(:throttled?)
+          .and_return(true)
       end
 
-      it 'returns 404' do
-        get :export, params: { namespace_id: project.namespace, id: project }
+      it 'prevents requesting project export' do
+        get action, params: { namespace_id: project.namespace, id: project }
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(flash[:alert]).to eq('This endpoint has been requested too many times. Try again later.')
+        expect(response).to have_gitlab_http_status(302)
       end
     end
-  end
 
-  describe '#download_export' do
-    before do
-      sign_in(user)
+    describe '#export' do
+      let(:action) { :export }
 
-      project.add_maintainer(user)
-    end
-
-    context 'object storage enabled' do
       context 'when project export is enabled' do
         it 'returns 302' do
-          get :download_export, params: { namespace_id: project.namespace, id: project }
+          get action, params: { namespace_id: project.namespace, id: project }
 
           expect(response).to have_gitlab_http_status(302)
         end
@@ -1105,66 +1094,96 @@ describe ProjectsController do
         end
 
         it 'returns 404' do
-          get :download_export, params: { namespace_id: project.namespace, id: project }
+          get action, params: { namespace_id: project.namespace, id: project }
 
           expect(response).to have_gitlab_http_status(404)
         end
       end
+
+      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+        include_examples 'rate limits project export endpoint'
+      end
     end
-  end
 
-  describe '#remove_export' do
-    before do
-      sign_in(user)
+    describe '#download_export' do
+      let(:action) { :download_export }
 
-      project.add_maintainer(user)
-    end
+      context 'object storage enabled' do
+        context 'when project export is enabled' do
+          it 'returns 302' do
+            get action, params: { namespace_id: project.namespace, id: project }
 
-    context 'when project export is enabled' do
-      it 'returns 302' do
-        post :remove_export, params: { namespace_id: project.namespace, id: project }
+            expect(response).to have_gitlab_http_status(302)
+          end
+        end
 
-        expect(response).to have_gitlab_http_status(302)
-      end
-    end
+        context 'when project export is disabled' do
+          before do
+            stub_application_setting(project_export_enabled?: false)
+          end
 
-    context 'when project export is disabled' do
-      before do
-        stub_application_setting(project_export_enabled?: false)
-      end
+          it 'returns 404' do
+            get action, params: { namespace_id: project.namespace, id: project }
 
-      it 'returns 404' do
-        post :remove_export, params: { namespace_id: project.namespace, id: project }
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
 
-        expect(response).to have_gitlab_http_status(404)
+        context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+          include_examples 'rate limits project export endpoint'
+        end
       end
     end
-  end
 
-  describe '#generate_new_export' do
-    before do
-      sign_in(user)
+    describe '#remove_export' do
+      let(:action) { :remove_export }
 
-      project.add_maintainer(user)
-    end
+      context 'when project export is enabled' do
+        it 'returns 302' do
+          post action, params: { namespace_id: project.namespace, id: project }
 
-    context 'when project export is enabled' do
-      it 'returns 302' do
-        post :generate_new_export, params: { namespace_id: project.namespace, id: project }
+          expect(response).to have_gitlab_http_status(302)
+        end
+      end
 
-        expect(response).to have_gitlab_http_status(302)
+      context 'when project export is disabled' do
+        before do
+          stub_application_setting(project_export_enabled?: false)
+        end
+
+        it 'returns 404' do
+          post action, params: { namespace_id: project.namespace, id: project }
+
+          expect(response).to have_gitlab_http_status(404)
+        end
       end
     end
 
-    context 'when project export is disabled' do
-      before do
-        stub_application_setting(project_export_enabled?: false)
+    describe '#generate_new_export' do
+      let(:action) { :generate_new_export }
+
+      context 'when project export is enabled' do
+        it 'returns 302' do
+          post action, params: { namespace_id: project.namespace, id: project }
+
+          expect(response).to have_gitlab_http_status(302)
+        end
       end
 
-      it 'returns 404' do
-        post :generate_new_export, params: { namespace_id: project.namespace, id: project }
+      context 'when project export is disabled' do
+        before do
+          stub_application_setting(project_export_enabled?: false)
+        end
 
-        expect(response).to have_gitlab_http_status(404)
+        it 'returns 404' do
+          post action, params: { namespace_id: project.namespace, id: project }
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
+      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+        include_examples 'rate limits project export endpoint'
       end
     end
   end

spec/lib/gitlab/action_rate_limiter_spec.rb → spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -2,30 +2,40 @@
 
 require 'spec_helper'
 
-describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
+describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
   let(:redis) { double('redis') }
   let(:user) { create(:user) }
   let(:project) { create(:project) }
+  let(:rate_limits) do
+    {
+      test_action: {
+        threshold: 1,
+        interval: 2.minutes
+      }
+    }
+  end
+  let(:key) { rate_limits.keys[0] }
 
-  subject { described_class.new(action: :test_action, expiry_time: 100) }
+  subject { described_class }
 
   before do
     allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis)
+    allow(described_class).to receive(:rate_limits).and_return(rate_limits)
   end
 
   shared_examples 'action rate limiter' do
     it 'increases the throttle count and sets the expiration time' do
       expect(redis).to receive(:incr).with(cache_key).and_return(1)
-      expect(redis).to receive(:expire).with(cache_key, 100)
+      expect(redis).to receive(:expire).with(cache_key, 120)
 
-      expect(subject.throttled?(key, 1)).to be_falsy
+      expect(subject.throttled?(key, scope: scope)).to be_falsy
     end
 
     it 'returns true if the key is throttled' do
       expect(redis).to receive(:incr).with(cache_key).and_return(2)
       expect(redis).not_to receive(:expire)
 
-      expect(subject.throttled?(key, 1)).to be_truthy
+      expect(subject.throttled?(key, scope: scope)).to be_truthy
     end
 
     context 'when throttling is disabled' do
@@ -33,16 +43,16 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
         expect(redis).not_to receive(:incr)
         expect(redis).not_to receive(:expire)
 
-        expect(subject.throttled?(key, 0)).to be_falsy
+        expect(subject.throttled?(key, scope: scope, threshold: 0)).to be_falsy
       end
     end
   end
 
   context 'when the key is an array of only ActiveRecord models' do
-    let(:key) { [user, project] }
+    let(:scope) { [user, project] }
 
     let(:cache_key) do
-      "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
+      "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
     end
 
     it_behaves_like 'action rate limiter'
@@ -52,10 +62,10 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
     let(:project) { create(:project, :public, :repository) }
     let(:commit) { project.repository.commit }
     let(:path) { 'app/controllers/groups_controller.rb' }
-    let(:key) { [project, commit, path] }
+    let(:scope) { [project, commit, path] }
 
     let(:cache_key) do
-      "action_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
+      "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
     end
 
     it_behaves_like 'action rate limiter'
@@ -72,7 +82,7 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
 
     let(:base_attributes) do
       {
-        message: 'Action_Rate_Limiter_Request',
+        message: 'Application_Rate_Limiter_Request',
         env: type,
         remote_ip: '127.0.0.1',
         request_method: 'GET',

spec/requests/api/project_export_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe API::ProjectExport do
+describe API::ProjectExport, :clean_gitlab_redis_cache do
   set(:project) { create(:project) }
   set(:project_none) { create(:project) }
   set(:project_started) { create(:project) }
@@ -47,6 +47,19 @@ describe API::ProjectExport do
     it_behaves_like '404 response'
   end
 
+  shared_examples_for 'when rate limit is exceeded' do
+    before do
+      allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
+    end
+
+    it 'prevents requesting project export' do
+      request
+
+      expect(response).to have_gitlab_http_status(429)
+      expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.')
+    end
+  end
+
   describe 'GET /projects/:project_id/export' do
     shared_examples_for 'get project export status not found' do
       it_behaves_like '404 response' do
@@ -219,6 +232,12 @@ describe API::ProjectExport do
         let(:user) { admin }
 
         it_behaves_like 'get project download by strategy'
+
+        context 'when rate limit is exceeded' do
+          let(:request) { get api(download_path, admin) }
+
+          include_examples 'when rate limit is exceeded'
+        end
       end
 
       context 'when user is a maintainer' do
@@ -329,6 +348,12 @@ describe API::ProjectExport do
         let(:user) { admin }
 
         it_behaves_like 'post project export start'
+
+        context 'when rate limit is exceeded' do
+          let(:request) { post api(path, admin) }
+
+          include_examples 'when rate limit is exceeded'
+        end
       end
 
       context 'when user is a maintainer' do