Desktop view: Add rate limiting

app/graphql/gql/concerns/handles_throttling.rb

@@ -0,0 +1,15 @@
+# Copyright (C) 2012-2023 Zammad Foundation, https://zammad-foundation.org/
+
+module Gql::Concerns::HandlesThrottling
+  extend ActiveSupport::Concern
+
+  included do
+    def throttle!(limit:, period:, by_identifier: nil)
+      ip = context[:controller].request.remote_ip
+
+      OperationsRateLimiter
+        .new(limit:, period:, operation: self.class.name)
+        .ensure_within_limits!(by_ip: ip, by_identifier: by_identifier)
+    end
+  end
+end

app/graphql/gql/mutations/admin_password_auth_send.rb

@@ -2,6 +2,8 @@
 
 module Gql::Mutations
   class AdminPasswordAuthSend < BaseMutation
+    include Gql::Concerns::HandlesThrottling
+
     description 'Sends a email with a token to login via password.'
 
     argument :login, String, 'Login information that is used to create a token.'
@@ -12,6 +14,10 @@ module Gql::Mutations
       true
     end
 
+    def ready?(login:)
+      throttle!(limit: 3, period: 1.minute, by_identifier: login)
+    end
+
     def resolve(login:)
       send = Service::Auth::SendAdminToken.new(login: login)
       succeeded = send.execute

i18n/zammad.pot

@@ -11878,6 +11878,10 @@ msgstr ""
 msgid "The request could not be processed."
 msgstr ""
 
+#: lib/operations_rate_limiter.rb
+msgid "The request limit for this operation was exceeded."
+msgstr ""
+
 #: lib/validations/verify_perform_rules_validator.rb
 msgid "The required '%{attribute}' value for %{key}, %{inner} is missing!"
 msgstr ""

lib/operations_rate_limiter.rb

@@ -0,0 +1,66 @@
+# Copyright (C) 2012-2023 Zammad Foundation, https://zammad-foundation.org/
+
+class OperationsRateLimiter
+  class ThrottleLimitExceeded < Exceptions::Forbidden; end
+
+  def initialize(limit:, period:, operation:)
+    @limit = limit
+    @period = period
+    @operation = operation
+  end
+
+  def ensure_within_limits!(by_ip:, by_identifier: nil)
+    ensure_within_identifier_limit(by_identifier) if by_identifier.present?
+    ensure_within_ip_limit(by_ip)
+  end
+
+  private
+
+  def ensure_within_identifier_limit(value)
+    value       = value.downcase.gsub(%r{\s+}, '')
+    fingerprint = Digest::MD5.hexdigest(value)
+
+    ensure_within :identifier, fingerprint
+  end
+
+  def ensure_within_ip_limit(ip_addr)
+    ensure_within :ip, ip_addr
+  end
+
+  def ensure_within(key, value)
+    period_identifier, lapsed_time = Time.now.to_i.divmod(@period.to_i)
+
+    cache_key  = cache_key(key, value, period_identifier)
+    expires_in = cache_expires_in(lapsed_time)
+    value      = increment(cache_key, expires_in)
+
+    return true if value <= @limit
+
+    raise ThrottleLimitExceeded, __('The request limit for this operation was exceeded.')
+  end
+
+  def cache_key(key, value, period_identifier)
+    [
+      self.class.name,
+      @operation,
+      key,
+      value,
+      period_identifier
+    ].join('::')
+  end
+
+  def cache_expires_in(lapsed_time)
+    @period.to_i - lapsed_time + 1.minute # make sure there's no race condition with cache expiring during processing
+  end
+
+  def increment(cache_key, expires_in)
+    # Rails.cache.increment has surpising behaviours/bugs in Rails 7.0, so we don't use it.
+    # This may be working better in 7.1 and could be cleaned up after upgrading to Rails 7.1
+    #
+    # https://github.com/rails/rails/commit/f48bf3975f62e875a1cf4264b18ce3735915684d
+    new_value = (Rails.cache.read(cache_key) || 0) + 1
+    new_value.tap do
+      Rails.cache.write(cache_key, new_value, expires_in:)
+    end
+  end
+end

spec/graphql/gql/mutations/admin_password_auth_send_spec.rb

@@ -64,6 +64,27 @@ RSpec.describe Gql::Mutations::AdminPasswordAuthSend, type: :graphql do
 
           expect(message).to include "<a href=\"http://zammad.example.com/desktop/login?token=#{Token.last.token}\">"
         end
+
+        context 'when user requests admin auth more than throttle allows', :rack_attack do
+
+          let(:static_ipv4) { Faker::Internet.ip_v4_address }
+
+          it 'blocks due to username throttling (multiple IPs)' do
+            4.times do
+              gql.execute(query, variables: variables, context: { REMOTE_IP: Faker::Internet.ip_v4_address })
+            end
+
+            expect(gql.result.error_message).to eq 'The request limit for this operation was exceeded.'
+          end
+
+          it 'blocks due to source IP address throttling (multiple usernames)' do
+            4.times do
+              gql.execute(query, variables: variables.merge(username: create(:admin).login), context: { REMOTE_IP: static_ipv4 })
+            end
+
+            expect(gql.result.error_message).to eq 'The request limit for this operation was exceeded.'
+          end
+        end
       end
     end
   end

spec/lib/operations_rate_limiter_spec.rb

@@ -0,0 +1,83 @@
+# Copyright (C) 2012-2023 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe OperationsRateLimiter do
+  before { freeze_time }
+
+  describe '#ensure_within_limits!' do
+    context 'with an IP address' do
+      it 'passes' do
+        expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4') }
+          .not_to raise_error
+      end
+
+      context 'with many operations' do
+        before do
+          5.times do
+            described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4')
+          end
+        end
+
+        it 'raises error due to too many operations from same IP in short time' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4') }
+            .to raise_error(described_class::ThrottleLimitExceeded)
+        end
+
+        it 'passes after period is over' do
+          travel 2.hours
+
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4') }
+            .not_to raise_error
+        end
+
+        it 'passes with a different IP address' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7') }
+            .not_to raise_error
+        end
+      end
+    end
+
+    context 'with a field' do
+      it 'passes' do
+        expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4', by_identifier: 'bar') }
+          .not_to raise_error
+      end
+
+      context 'with many operations' do
+        before do
+          5.times do
+            described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '1.2.3.4', by_identifier: 'bar')
+          end
+        end
+
+        it 'raises error due too many operations with the same value from different IPs' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7', by_identifier: 'bar') }
+            .to raise_error(described_class::ThrottleLimitExceeded)
+        end
+
+        it 'raises error due too many operations with upercase value' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7', by_identifier: 'BAR') }
+            .to raise_error(described_class::ThrottleLimitExceeded)
+        end
+
+        it 'raises error due too many operations with trailing space' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7', by_identifier: 'bar ') }
+            .to raise_error(described_class::ThrottleLimitExceeded)
+        end
+
+        it 'passes after period is over' do
+          travel 2.hours
+
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7', by_identifier: 'bar') }
+            .not_to raise_error
+        end
+
+        it 'passes with a different value' do
+          expect { described_class.new(limit: 5, period: 1.hour, operation: 'test').ensure_within_limits!(by_ip: '4.5.6.7', by_identifier: 'baz') }
+            .not_to raise_error
+        end
+      end
+    end
+  end
+end

spec/support/graphql.rb

@@ -201,6 +201,12 @@ module ZammadSpecSupportGraphql
     #   gql.result
     #
     def execute(query, variables: {}, context: {})
+      context[:controller] ||= GraphqlController.new
+                                 .tap do |controller|
+                                   controller.request = ActionDispatch::Request.new({})
+                                   controller.request.remote_ip = context[:REMOTE_IP] || '127.0.0.1'
+                                 end
+
       context[:current_user] ||= @graphql_current_user
       if @graphql_current_user
         # TODO: we only fake a SID for now, create a real session?