Merge pull request #428 from grzuy/style

Enable more code style checks

.rubocop.yml

@@ -1,3 +1,6 @@
+require:
+  - rubocop-performance
+
 inherit_mode:
   merge:
     - Exclude
@@ -26,6 +29,9 @@ Naming:
   Exclude:
     - "lib/rack/attack/path_normalizer.rb"
 
+Metrics/LineLength:
+  Max: 120
+
 Performance:
   Enabled: true
 
@@ -34,13 +40,25 @@ Security:
 
 Style/BlockDelimiters:
   Enabled: true
+  IgnoredMethods: [] # Workaround rubocop bug: https://github.com/rubocop-hq/rubocop/issues/6179
 
 Style/BracesAroundHashParameters:
   Enabled: true
 
+Style/ClassAndModuleChildren:
+  Enabled: true
+  Exclude:
+    - "spec/**/*"
+
+Style/ConditionalAssignment:
+  Enabled: true
+
 Style/Encoding:
   Enabled: true
 
+Style/ExpandPathArguments:
+  Enabled: true
+
 Style/EmptyMethod:
   Enabled: true
 
@@ -53,6 +71,9 @@ Style/HashSyntax:
 Style/OptionalArguments:
   Enabled: true
 
+Style/ParallelAssignment:
+  Enabled: true
+
 Style/RaiseArgs:
   Enabled: true
 
@@ -70,3 +91,9 @@ Style/Semicolon:
 
 Style/SingleLineMethods:
   Enabled: true
+
+Style/SpecialGlobalVars:
+  Enabled: true
+
+Style/UnneededPercentQ:
+  Enabled: true

lib/rack/attack.rb

@@ -6,159 +6,164 @@ require 'rack/attack/path_normalizer'
 require 'rack/attack/request'
 require "ipaddr"
 
-class Rack::Attack
+module Rack
-  class MisconfiguredStoreError < StandardError; end
+  class Attack
-  class MissingStoreError < StandardError; end
+    class MisconfiguredStoreError < StandardError; end
+    class MissingStoreError < StandardError; end
 
-  autoload :Cache,                'rack/attack/cache'
+    autoload :Cache,                'rack/attack/cache'
-  autoload :Check,                'rack/attack/check'
+    autoload :Check,                'rack/attack/check'
-  autoload :Throttle,             'rack/attack/throttle'
+    autoload :Throttle,             'rack/attack/throttle'
-  autoload :Safelist,             'rack/attack/safelist'
+    autoload :Safelist,             'rack/attack/safelist'
-  autoload :Blocklist,            'rack/attack/blocklist'
+    autoload :Blocklist,            'rack/attack/blocklist'
-  autoload :Track,                'rack/attack/track'
+    autoload :Track,                'rack/attack/track'
-  autoload :StoreProxy,           'rack/attack/store_proxy'
+    autoload :StoreProxy,           'rack/attack/store_proxy'
-  autoload :DalliProxy,           'rack/attack/store_proxy/dalli_proxy'
+    autoload :DalliProxy,           'rack/attack/store_proxy/dalli_proxy'
-  autoload :MemCacheStoreProxy,   'rack/attack/store_proxy/mem_cache_store_proxy'
+    autoload :MemCacheStoreProxy,   'rack/attack/store_proxy/mem_cache_store_proxy'
-  autoload :RedisProxy,           'rack/attack/store_proxy/redis_proxy'
+    autoload :RedisProxy,           'rack/attack/store_proxy/redis_proxy'
-  autoload :RedisStoreProxy,      'rack/attack/store_proxy/redis_store_proxy'
+    autoload :RedisStoreProxy,      'rack/attack/store_proxy/redis_store_proxy'
-  autoload :RedisCacheStoreProxy, 'rack/attack/store_proxy/redis_cache_store_proxy'
+    autoload :RedisCacheStoreProxy, 'rack/attack/store_proxy/redis_cache_store_proxy'
-  autoload :ActiveSupportRedisStoreProxy, 'rack/attack/store_proxy/active_support_redis_store_proxy'
+    autoload :ActiveSupportRedisStoreProxy, 'rack/attack/store_proxy/active_support_redis_store_proxy'
-  autoload :Fail2Ban,             'rack/attack/fail2ban'
+    autoload :Fail2Ban,             'rack/attack/fail2ban'
-  autoload :Allow2Ban,            'rack/attack/allow2ban'
+    autoload :Allow2Ban,            'rack/attack/allow2ban'
 
-  class << self
+    class << self
-    attr_accessor :notifier, :blocklisted_response, :throttled_response, :anonymous_blocklists, :anonymous_safelists
+      attr_accessor :notifier, :blocklisted_response, :throttled_response, :anonymous_blocklists, :anonymous_safelists
 
-    def safelist(name = nil, &block)
+      def safelist(name = nil, &block)
-      safelist = Safelist.new(name, &block)
+        safelist = Safelist.new(name, &block)
 
-      if name
+        if name
-        safelists[name] = safelist
+          safelists[name] = safelist
+        else
+          anonymous_safelists << safelist
+        end
+      end
+
+      def blocklist(name = nil, &block)
+        blocklist = Blocklist.new(name, &block)
+
+        if name
+          blocklists[name] = blocklist
+        else
+          anonymous_blocklists << blocklist
+        end
+      end
+
+      def blocklist_ip(ip_address)
+        anonymous_blocklists << Blocklist.new { |request| IPAddr.new(ip_address).include?(IPAddr.new(request.ip)) }
+      end
+
+      def safelist_ip(ip_address)
+        anonymous_safelists << Safelist.new { |request| IPAddr.new(ip_address).include?(IPAddr.new(request.ip)) }
+      end
+
+      def throttle(name, options, &block)
+        throttles[name] = Throttle.new(name, options, &block)
+      end
+
+      def track(name, options = {}, &block)
+        tracks[name] = Track.new(name, options, &block)
+      end
+
+      def safelists
+        @safelists ||= {}
+      end
+
+      def blocklists
+        @blocklists ||= {}
+      end
+
+      def throttles
+        @throttles ||= {}
+      end
+
+      def tracks
+        @tracks ||= {}
+      end
+
+      def safelisted?(request)
+        anonymous_safelists.any? { |safelist| safelist.matched_by?(request) } ||
+          safelists.any? { |_name, safelist| safelist.matched_by?(request) }
+      end
+
+      def blocklisted?(request)
+        anonymous_blocklists.any? { |blocklist| blocklist.matched_by?(request) } ||
+          blocklists.any? { |_name, blocklist| blocklist.matched_by?(request) }
+      end
+
+      def throttled?(request)
+        throttles.any? do |_name, throttle|
+          throttle.matched_by?(request)
+        end
+      end
+
+      def tracked?(request)
+        tracks.each_value do |track|
+          track.matched_by?(request)
+        end
+      end
+
+      def instrument(request)
+        if notifier
+          event_type = request.env["rack.attack.match_type"]
+          notifier.instrument("#{event_type}.rack_attack", request: request)
+
+          # Deprecated: Keeping just for backwards compatibility
+          notifier.instrument("rack.attack", request: request)
+        end
+      end
+
+      def cache
+        @cache ||= Cache.new
+      end
+
+      def clear_configuration
+        @safelists = {}
+        @blocklists = {}
+        @throttles = {}
+        @tracks = {}
+        self.anonymous_blocklists = []
+        self.anonymous_safelists = []
+      end
+
+      def clear!
+        warn "[DEPRECATION] Rack::Attack.clear! is deprecated. Please use Rack::Attack.clear_configuration instead"
+        clear_configuration
+      end
+    end
+
+    # Set defaults
+    @anonymous_blocklists = []
+    @anonymous_safelists = []
+    @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
+    @blocklisted_response = lambda { |_env| [403, { 'Content-Type' => 'text/plain' }, ["Forbidden\n"]] }
+    @throttled_response   = lambda do |env|
+      retry_after = (env['rack.attack.match_data'] || {})[:period]
+      [429, { 'Content-Type' => 'text/plain', 'Retry-After' => retry_after.to_s }, ["Retry later\n"]]
+    end
+
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
+      request = Rack::Attack::Request.new(env)
+
+      if safelisted?(request)
+        @app.call(env)
+      elsif blocklisted?(request)
+        self.class.blocklisted_response.call(env)
+      elsif throttled?(request)
+        self.class.throttled_response.call(env)
       else
-        anonymous_safelists << safelist
+        tracked?(request)
+        @app.call(env)
       end
     end
 
-    def blocklist(name = nil, &block)
+    extend Forwardable
-      blocklist = Blocklist.new(name, &block)
+    def_delegators self, :safelisted?, :blocklisted?, :throttled?, :tracked?
-
-      if name
-        blocklists[name] = blocklist
-      else
-        anonymous_blocklists << blocklist
-      end
-    end
-
-    def blocklist_ip(ip_address)
-      anonymous_blocklists << Blocklist.new { |request| IPAddr.new(ip_address).include?(IPAddr.new(request.ip)) }
-    end
-
-    def safelist_ip(ip_address)
-      anonymous_safelists << Safelist.new { |request| IPAddr.new(ip_address).include?(IPAddr.new(request.ip)) }
-    end
-
-    def throttle(name, options, &block)
-      throttles[name] = Throttle.new(name, options, &block)
-    end
-
-    def track(name, options = {}, &block)
-      tracks[name] = Track.new(name, options, &block)
-    end
-
-    def safelists
-      @safelists  ||= {}
-    end
-
-    def blocklists
-      @blocklists ||= {}
-    end
-
-    def throttles
-      @throttles  ||= {}
-    end
-
-    def tracks
-      @tracks     ||= {}
-    end
-
-    def safelisted?(request)
-      anonymous_safelists.any? { |safelist| safelist.matched_by?(request) } ||
-        safelists.any? { |_name, safelist| safelist.matched_by?(request) }
-    end
-
-    def blocklisted?(request)
-      anonymous_blocklists.any? { |blocklist| blocklist.matched_by?(request) } ||
-        blocklists.any? { |_name, blocklist| blocklist.matched_by?(request) }
-    end
-
-    def throttled?(request)
-      throttles.any? do |_name, throttle|
-        throttle.matched_by?(request)
-      end
-    end
-
-    def tracked?(request)
-      tracks.each_value do |track|
-        track.matched_by?(request)
-      end
-    end
-
-    def instrument(request)
-      if notifier
-        event_type = request.env["rack.attack.match_type"]
-        notifier.instrument("#{event_type}.rack_attack", request: request)
-
-        # Deprecated: Keeping just for backwards compatibility
-        notifier.instrument("rack.attack", request: request)
-      end
-    end
-
-    def cache
-      @cache ||= Cache.new
-    end
-
-    def clear_configuration
-      @safelists, @blocklists, @throttles, @tracks = {}, {}, {}, {}
-      self.anonymous_blocklists = []
-      self.anonymous_safelists = []
-    end
-
-    def clear!
-      warn "[DEPRECATION] Rack::Attack.clear! is deprecated. Please use Rack::Attack.clear_configuration instead"
-      clear_configuration
-    end
   end
-
-  # Set defaults
-  @anonymous_blocklists = []
-  @anonymous_safelists = []
-  @notifier             = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
-  @blocklisted_response = lambda { |_env| [403, { 'Content-Type' => 'text/plain' }, ["Forbidden\n"]] }
-  @throttled_response   = lambda { |env|
-    retry_after = (env['rack.attack.match_data'] || {})[:period]
-    [429, { 'Content-Type' => 'text/plain', 'Retry-After' => retry_after.to_s }, ["Retry later\n"]]
-  }
-
-  def initialize(app)
-    @app = app
-  end
-
-  def call(env)
-    env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
-    request = Rack::Attack::Request.new(env)
-
-    if safelisted?(request)
-      @app.call(env)
-    elsif blocklisted?(request)
-      self.class.blocklisted_response.call(env)
-    elsif throttled?(request)
-      self.class.throttled_response.call(env)
-    else
-      tracked?(request)
-      @app.call(env)
-    end
-  end
-
-  extend Forwardable
-  def_delegators self, :safelisted?, :blocklisted?, :throttled?, :tracked?
 end

lib/rack/attack/cache.rb

@@ -73,7 +73,10 @@ module Rack
 
       def enforce_store_method_presence!(method_name)
         if !store.respond_to?(method_name)
-          raise Rack::Attack::MisconfiguredStoreError, "Configured store #{store.class.name} doesn't respond to ##{method_name} method"
+          raise(
+            Rack::Attack::MisconfiguredStoreError,
+            "Configured store #{store.class.name} doesn't respond to ##{method_name} method"
+          )
         end
       end
     end

lib/rack/attack/check.rb

@@ -5,7 +5,8 @@ module Rack
     class Check
       attr_reader :name, :block, :type
       def initialize(name, options = {}, &block)
-        @name, @block = name, block
+        @name = name
+        @block = block
         @type = options.fetch(:type, nil)
       end
 

lib/rack/attack/path_normalizer.rb

@@ -1,24 +1,26 @@
 # frozen_string_literal: true
 
-class Rack::Attack
+module Rack
-  # When using Rack::Attack with a Rails app, developers expect the request path
+  class Attack
-  # to be normalized. In particular, trailing slashes are stripped.
+    # When using Rack::Attack with a Rails app, developers expect the request path
-  # (See https://git.io/v0rrR for implementation.)
+    # to be normalized. In particular, trailing slashes are stripped.
-  #
+    # (See https://git.io/v0rrR for implementation.)
-  # Look for an ActionDispatch utility class that Rails folks would expect
+    #
-  # to normalize request paths. If unavailable, use a fallback class that
+    # Look for an ActionDispatch utility class that Rails folks would expect
-  # doesn't normalize the path (as a non-Rails rack app developer expects).
+    # to normalize request paths. If unavailable, use a fallback class that
+    # doesn't normalize the path (as a non-Rails rack app developer expects).
 
-  module FallbackPathNormalizer
+    module FallbackPathNormalizer
-    def self.normalize_path(path)
+      def self.normalize_path(path)
-      path
+        path
+      end
     end
-  end
 
-  PathNormalizer = if defined?(::ActionDispatch::Journey::Router::Utils)
+    PathNormalizer = if defined?(::ActionDispatch::Journey::Router::Utils)
-                     # For Rails apps
+                       # For Rails apps
-                     ::ActionDispatch::Journey::Router::Utils
+                       ::ActionDispatch::Journey::Router::Utils
-                   else
+                     else
-                     FallbackPathNormalizer
+                       FallbackPathNormalizer
-                   end
+                     end
+  end
 end

lib/rack/attack/store_proxy/active_support_redis_store_proxy.rb

@@ -7,7 +7,9 @@ module Rack
     module StoreProxy
       class ActiveSupportRedisStoreProxy < SimpleDelegator
         def self.handle?(store)
-          defined?(::Redis) && defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore)
+          defined?(::Redis) &&
+            defined?(::ActiveSupport::Cache::RedisStore) &&
+            store.is_a?(::ActiveSupport::Cache::RedisStore)
         end
 
         def increment(name, amount = 1, options = {})

lib/rack/attack/store_proxy/mem_cache_store_proxy.rb

@@ -7,7 +7,9 @@ module Rack
     module StoreProxy
       class MemCacheStoreProxy < SimpleDelegator
         def self.handle?(store)
-          defined?(::Dalli) && defined?(::ActiveSupport::Cache::MemCacheStore) && store.is_a?(::ActiveSupport::Cache::MemCacheStore)
+          defined?(::Dalli) &&
+            defined?(::ActiveSupport::Cache::MemCacheStore) &&
+            store.is_a?(::ActiveSupport::Cache::MemCacheStore)
         end
 
         def write(name, value, options = {})

lib/rack/attack/throttle.rb

@@ -7,11 +7,12 @@ module Rack
 
       attr_reader :name, :limit, :period, :block, :type
       def initialize(name, options, &block)
-        @name, @block = name, block
+        @name = name
+        @block = block
         MANDATORY_OPTIONS.each do |opt|
           raise ArgumentError, "Must pass #{opt.inspect} option" unless options[opt]
         end
-        @limit  = options[:limit]
+        @limit = options[:limit]
         @period = options[:period].respond_to?(:call) ? options[:period] : options[:period].to_i
         @type   = options.fetch(:type, :throttle)
       end

lib/rack/attack/track.rb

@@ -8,11 +8,12 @@ module Rack
       def initialize(name, options = {}, &block)
         options[:type] = :track
 
-        if options[:limit] && options[:period]
+        @filter =
-          @filter = Throttle.new(name, options, &block)
+          if options[:limit] && options[:period]
-        else
+            Throttle.new(name, options, &block)
-          @filter = Check.new(name, options, &block)
+          else
-        end
+            Check.new(name, options, &block)
+          end
       end
 
       def matched_by?(request)

rack-attack.gemspec

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
-lib = File.expand_path('../lib/', __FILE__)
+lib = File.expand_path('lib', __dir__)
-$:.unshift lib unless $:.include?(lib)
+$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
 
 require 'rack/attack/version'
 
@@ -18,7 +18,7 @@ Gem::Specification.new do |s|
   s.homepage = 'https://github.com/kickstarter/rack-attack'
   s.rdoc_options = ["--charset=UTF-8"]
   s.require_paths = ["lib"]
-  s.summary = %q{Block & throttle abusive requests}
+  s.summary = 'Block & throttle abusive requests'
   s.test_files = Dir.glob("spec/**/*")
 
   s.metadata = {
@@ -37,7 +37,8 @@ Gem::Specification.new do |s|
   s.add_development_dependency "minitest-stub-const", "~> 0.6"
   s.add_development_dependency 'rack-test', "~> 1.0"
   s.add_development_dependency 'rake', "~> 12.3"
-  s.add_development_dependency "rubocop", "0.67.2"
+  s.add_development_dependency "rubocop", "0.74.0"
+  s.add_development_dependency "rubocop-performance", "~> 1.4.1"
   s.add_development_dependency "timecop", "~> 0.9.1"
 
   # byebug only works with MRI

spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb

@@ -15,8 +15,6 @@ if defined?(::ConnectionPool) && defined?(::Dalli)
       Rack::Attack.cache.store.clear
     end
 
-    it_works_for_cache_backed_features(fetch_from_store: ->(key) {
+    it_works_for_cache_backed_features(fetch_from_store: ->(key) { Rack::Attack.cache.store.read(key) })
-      Rack::Attack.cache.store.read(key)
-    })
   end
 end

spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb

@@ -2,7 +2,13 @@
 
 require_relative "../../spec_helper"
 
-if defined?(::ConnectionPool) && defined?(::Redis) && Gem::Version.new(::Redis::VERSION) >= Gem::Version.new("4") && defined?(::ActiveSupport::Cache::RedisCacheStore)
+should_run =
+  defined?(::ConnectionPool) &&
+  defined?(::Redis) &&
+  Gem::Version.new(::Redis::VERSION) >= Gem::Version.new("4") &&
+  defined?(::ActiveSupport::Cache::RedisCacheStore)
+
+if should_run
   require_relative "../../support/cache_store_helper"
   require "timecop"
 

spec/acceptance/stores/active_support_redis_cache_store_spec.rb

@@ -2,7 +2,12 @@
 
 require_relative "../../spec_helper"
 
-if defined?(::Redis) && Gem::Version.new(::Redis::VERSION) >= Gem::Version.new("4") && defined?(::ActiveSupport::Cache::RedisCacheStore)
+should_run =
+  defined?(::Redis) &&
+  Gem::Version.new(::Redis::VERSION) >= Gem::Version.new("4") &&
+  defined?(::ActiveSupport::Cache::RedisCacheStore)
+
+if should_run
   require_relative "../../support/cache_store_helper"
   require "timecop"
 

spec/acceptance/stores/connection_pool_dalli_client_spec.rb

@@ -17,8 +17,8 @@ if defined?(::Dalli) && defined?(::ConnectionPool)
       Rack::Attack.cache.store.with { |client| client.flush_all }
     end
 
-    it_works_for_cache_backed_features(fetch_from_store: ->(key) {
+    it_works_for_cache_backed_features(
-      Rack::Attack.cache.store.with { |client| client.fetch(key) }
+      fetch_from_store: ->(key) { Rack::Attack.cache.store.with { |client| client.fetch(key) } }
-    })
+    )
   end
 end

spec/rack_attack_spec.rb

@@ -22,9 +22,9 @@ describe 'Rack::Attack' do
       Rack::Attack.blocklist("ip #{@bad_ip}") { |req| req.ip == @bad_ip }
     end
 
-    it('has a blocklist') {
+    it 'has a blocklist' do
       Rack::Attack.blocklists.key?("ip #{@bad_ip}").must_equal true
-    }
+    end
 
     describe "a bad request" do
       before { get '/', {}, 'REMOTE_ADDR' => @bad_ip }