diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..718572bf --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,7 @@ +version: 2 + +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c7950f0f..8d7e3b37 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -15,8 +15,10 @@ jobs: ports: - 11211:11211 strategy: + fail-fast: false matrix: ruby: + - '3.3' - '3.2' - '3.1' - '3.0' @@ -26,7 +28,6 @@ jobs: gemfile: - rack_3 - rack_2 - - rack_1 - rails_7_1 - rails_7_0 - rails_6_1 @@ -47,8 +48,14 @@ jobs: - active_support_5_redis_cache_store_pooled - redis_store exclude: - - gemfile: rack_1 - ruby: '3.2' + - gemfile: rails_5_2 + ruby: '3.3' + - gemfile: active_support_5_redis_cache_store + ruby: '3.3' + - gemfile: active_support_5_redis_cache_store_pooled + ruby: '3.3' + - gemfile: dalli2 + ruby: '3.3' - gemfile: rails_5_2 ruby: '3.2' - gemfile: active_support_5_redis_cache_store @@ -57,8 +64,6 @@ jobs: ruby: '3.2' - gemfile: dalli2 ruby: '3.2' - - gemfile: rack_1 - ruby: '3.1' - gemfile: rails_5_2 ruby: '3.1' - gemfile: active_support_5_redis_cache_store @@ -67,8 +72,6 @@ jobs: ruby: '3.1' - gemfile: dalli2 ruby: '3.1' - - gemfile: rack_1 - ruby: '3.0' - gemfile: rails_5_2 ruby: '3.0' - gemfile: active_support_5_redis_cache_store @@ -77,8 +80,6 @@ jobs: ruby: '3.0' - gemfile: dalli2 ruby: '3.0' - - gemfile: rack_1 - ruby: '2.7' - gemfile: rails_7_0 ruby: '2.6' - gemfile: rails_7_0 @@ -106,7 +107,7 @@ jobs: env: BUNDLE_GEMFILE: gemfiles/${{ matrix.gemfile }}.gemfile steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - uses: ruby/setup-ruby@v1 with: ruby-version: ${{ matrix.ruby }} diff --git a/Appraisals b/Appraisals index f68e8b7f..6250f3aa 100644 --- a/Appraisals +++ b/Appraisals @@ -8,19 +8,6 @@ appraise "rack_2" do gem "rack", "~> 2.0" end -appraise "rack_1" do - # Override activesupport and actionpack version constraints by making - # it more loose so it's compatible with rack 1.6.x - gem "actionpack", ">= 4.2" - gem "activesupport", ">= 4.2" - - gem "rack", "~> 1.6" - - # Override rack-test version constraint by making it more loose - # so it's compatible with actionpack 4.2.x - gem "rack-test", ">= 0.6" -end - appraise 'rails_7-1' do gem 'railties', '~> 7.1.0' end diff --git a/README.md b/README.md index 545003cc..199021f6 100644 --- a/README.md +++ b/README.md @@ -291,7 +291,7 @@ Rack::Attack.track("special_agent", limit: 6, period: 60) do |req| end # Track it using ActiveSupport::Notification -ActiveSupport::Notifications.subscribe("track.rack_attack") do |name, start, finish, request_id, payload| +ActiveSupport::Notifications.subscribe("track.rack_attack") do |name, start, finish, instrumenter_id, payload| req = payload[:request] if req.env['rack.attack.matched'] == "special_agent" Rails.logger.info "special_agent: #{req.path}" @@ -383,7 +383,7 @@ To get notified about specific type of events, subscribe to the event name follo E.g. for throttles use: ```ruby -ActiveSupport::Notifications.subscribe("throttle.rack_attack") do |name, start, finish, request_id, payload| +ActiveSupport::Notifications.subscribe("throttle.rack_attack") do |name, start, finish, instrumenter_id, payload| # request object available in payload[:request] # Your code here @@ -393,7 +393,7 @@ end If you want to subscribe to every `rack_attack` event, use: ```ruby -ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, request_id, payload| +ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, instrumenter_id, payload| # request object available in payload[:request] # Your code here diff --git a/gemfiles/rack_1.gemfile b/gemfiles/rack_1.gemfile deleted file mode 100644 index 36b2f91b..00000000 --- a/gemfiles/rack_1.gemfile +++ /dev/null @@ -1,15 +0,0 @@ -# This file was generated by Appraisal - -source "https://rubygems.org" - -gem "actionpack", ">= 4.2" -gem "activesupport", ">= 4.2" -gem "rack", "~> 1.6" -gem "rack-test", ">= 0.6" - -group :maintenance, optional: true do - gem "bake" - gem "bake-gem" -end - -gemspec path: "../" diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb index 00670f06..4fe42002 100644 --- a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -10,17 +10,19 @@ def self.handle?(store) store.class.name == "ActiveSupport::Cache::RedisCacheStore" end - def increment(name, amount = 1, **options) - # RedisCacheStore#increment ignores options[:expires_in]. - # - # So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize - # the counter. After that we continue using the original RedisCacheStore#increment. - if options[:expires_in] && !read(name) - write(name, amount, options) + if defined?(::ActiveSupport) && ::ActiveSupport::VERSION::MAJOR < 6 + def increment(name, amount = 1, **options) + # RedisCacheStore#increment ignores options[:expires_in] in versions prior to 6. + # + # So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize + # the counter. After that we continue using the original RedisCacheStore#increment. + if options[:expires_in] && !read(name) + write(name, amount, options) - amount - else - super + amount + else + super + end end end diff --git a/spec/acceptance/blocking_ip_spec.rb b/spec/acceptance/blocking_ip_spec.rb index 4d08042f..4e41f29b 100644 --- a/spec/acceptance/blocking_ip_spec.rb +++ b/spec/acceptance/blocking_ip_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "Blocking an IP" do + let(:notifications) { [] } + before do Rack::Attack.blocklist_ip("1.2.3.4") end @@ -26,21 +28,18 @@ end it "notifies when the request is blocked" do - notified = false - notification_type = nil - ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload| - notified = true - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - refute notified + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert notified - assert_equal :blocklist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/blocking_spec.rb b/spec/acceptance/blocking_spec.rb index 4a57c7d8..2ea9f67a 100644 --- a/spec/acceptance/blocking_spec.rb +++ b/spec/acceptance/blocking_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "#blocklist" do + let(:notifications) { [] } + before do Rack::Attack.blocklist do |request| request.ip == "1.2.3.4" @@ -22,27 +24,26 @@ end it "notifies when the request is blocked" do - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_nil notification_matched - assert_equal :blocklist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_nil notification[:request].env["rack.attack.matched"] + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] end end describe "#blocklist with name" do + let(:notifications) { [] } + before do Rack::Attack.blocklist("block 1.2.3.4") do |request| request.ip == "1.2.3.4" @@ -62,22 +63,19 @@ end it "notifies when the request is blocked" do - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal "block 1.2.3.4", notification_matched - assert_equal :blocklist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal "block 1.2.3.4", notification[:request].env["rack.attack.matched"] + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/blocking_subnet_spec.rb b/spec/acceptance/blocking_subnet_spec.rb index d6083809..9fe30598 100644 --- a/spec/acceptance/blocking_subnet_spec.rb +++ b/spec/acceptance/blocking_subnet_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "Blocking an IP subnet" do + let(:notifications) { [] } + before do Rack::Attack.blocklist_ip("1.2.3.4/31") end @@ -26,21 +28,18 @@ end it "notifies when the request is blocked" do - notified = false - notification_type = nil - ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload| - notified = true - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - refute notified + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert notified - assert_equal :blocklist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/cache_store_config_for_allow2ban_spec.rb b/spec/acceptance/cache_store_config_for_allow2ban_spec.rb index ce994993..45f8f5c3 100644 --- a/spec/acceptance/cache_store_config_for_allow2ban_spec.rb +++ b/spec/acceptance/cache_store_config_for_allow2ban_spec.rb @@ -12,9 +12,11 @@ end end - it "gives semantic error if no store was configured" do - assert_raises(Rack::Attack::MissingStoreError) do - get "/scarce-resource" + unless defined?(Rails) + it "gives semantic error if no store was configured" do + assert_raises(Rack::Attack::MissingStoreError) do + get "/scarce-resource" + end end end diff --git a/spec/acceptance/cache_store_config_for_fail2ban_spec.rb b/spec/acceptance/cache_store_config_for_fail2ban_spec.rb index 6fd79807..b46f9fe5 100644 --- a/spec/acceptance/cache_store_config_for_fail2ban_spec.rb +++ b/spec/acceptance/cache_store_config_for_fail2ban_spec.rb @@ -12,9 +12,11 @@ end end - it "gives semantic error if no store was configured" do - assert_raises(Rack::Attack::MissingStoreError) do - get "/private-place" + unless defined?(Rails) + it "gives semantic error if no store was configured" do + assert_raises(Rack::Attack::MissingStoreError) do + get "/private-place" + end end end diff --git a/spec/acceptance/cache_store_config_for_throttle_spec.rb b/spec/acceptance/cache_store_config_for_throttle_spec.rb index 9be6e59a..a89b8272 100644 --- a/spec/acceptance/cache_store_config_for_throttle_spec.rb +++ b/spec/acceptance/cache_store_config_for_throttle_spec.rb @@ -9,9 +9,11 @@ end end - it "gives semantic error if no store was configured" do - assert_raises(Rack::Attack::MissingStoreError) do - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + unless defined?(Rails) + it "gives semantic error if no store was configured" do + assert_raises(Rack::Attack::MissingStoreError) do + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + end end end diff --git a/spec/acceptance/cache_store_config_with_rails_spec.rb b/spec/acceptance/cache_store_config_with_rails_spec.rb index 3d9ac22f..66bb76a8 100644 --- a/spec/acceptance/cache_store_config_with_rails_spec.rb +++ b/spec/acceptance/cache_store_config_with_rails_spec.rb @@ -11,10 +11,12 @@ end end - it "fails when Rails.cache is not set" do - Object.stub_const(:Rails, OpenStruct.new(cache: nil)) do - assert_raises(Rack::Attack::MissingStoreError) do - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + unless defined?(Rails) + it "fails when Rails.cache is not set" do + Object.stub_const(:Rails, OpenStruct.new(cache: nil)) do + assert_raises(Rack::Attack::MissingStoreError) do + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + end end end end diff --git a/spec/acceptance/fail2ban_spec.rb b/spec/acceptance/fail2ban_spec.rb index fde67f9e..74c01f57 100644 --- a/spec/acceptance/fail2ban_spec.rb +++ b/spec/acceptance/fail2ban_spec.rb @@ -4,6 +4,8 @@ require "timecop" describe "fail2ban" do + let(:notifications) { [] } + before do Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new @@ -75,4 +77,44 @@ assert_equal 200, last_response.status end end + + it "notifies when the request is blocked" do + ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload| + notifications.push(payload) + end + + get "/" + + assert_equal 200, last_response.status + assert notifications.empty? + + get "/private-place" + + assert_equal 403, last_response.status + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal 'fail2ban pentesters', notification[:request].env["rack.attack.matched"] + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] + + get "/" + + assert_equal 200, last_response.status + assert notifications.empty? + + get "/private-place" + + assert_equal 403, last_response.status + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal 'fail2ban pentesters', notification[:request].env["rack.attack.matched"] + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] + + get "/" + + assert_equal 403, last_response.status + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal 'fail2ban pentesters', notification[:request].env["rack.attack.matched"] + assert_equal :blocklist, notification[:request].env["rack.attack.match_type"] + end end diff --git a/spec/acceptance/safelisting_ip_spec.rb b/spec/acceptance/safelisting_ip_spec.rb index fd80dc1b..e04ca6e5 100644 --- a/spec/acceptance/safelisting_ip_spec.rb +++ b/spec/acceptance/safelisting_ip_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "Safelist an IP" do + let(:notifications) { [] } + before do Rack::Attack.blocklist("admin") do |request| request.path == "/admin" @@ -42,15 +44,15 @@ end it "notifies when the request is safe" do - notification_type = nil - ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload| - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/admin", {}, "REMOTE_ADDR" => "5.6.7.8" assert_equal 200, last_response.status - assert_equal :safelist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal :safelist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/safelisting_spec.rb b/spec/acceptance/safelisting_spec.rb index 74a3c8e4..f00fada7 100644 --- a/spec/acceptance/safelisting_spec.rb +++ b/spec/acceptance/safelisting_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "#safelist" do + let(:notifications) { [] } + before do Rack::Attack.blocklist do |request| request.ip == "1.2.3.4" @@ -38,23 +40,23 @@ end it "notifies when the request is safe" do - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/safe_space", {}, "REMOTE_ADDR" => "1.2.3.4" assert_equal 200, last_response.status - assert_nil notification_matched - assert_equal :safelist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_nil notification[:request].env["rack.attack.matched"] + assert_equal :safelist, notification[:request].env["rack.attack.match_type"] end end describe "#safelist with name" do + let(:notifications) { [] } + before do Rack::Attack.blocklist("block 1.2.3.4") do |request| request.ip == "1.2.3.4" @@ -90,18 +92,16 @@ end it "notifies when the request is safe" do - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/safe_space", {}, "REMOTE_ADDR" => "1.2.3.4" assert_equal 200, last_response.status - assert_equal "safe path", notification_matched - assert_equal :safelist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal "safe path", notification[:request].env["rack.attack.matched"] + assert_equal :safelist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/safelisting_subnet_spec.rb b/spec/acceptance/safelisting_subnet_spec.rb index 45c5bf9f..baeb7e46 100644 --- a/spec/acceptance/safelisting_subnet_spec.rb +++ b/spec/acceptance/safelisting_subnet_spec.rb @@ -3,6 +3,8 @@ require_relative "../spec_helper" describe "Safelisting an IP subnet" do + let(:notifications) { [] } + before do Rack::Attack.blocklist("admin") do |request| request.path == "/admin" @@ -36,15 +38,15 @@ end it "notifies when the request is safe" do - notification_type = nil - ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload| - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/admin", {}, "REMOTE_ADDR" => "5.6.0.0" assert_equal 200, last_response.status - assert_equal :safelist, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal :safelist, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/stores/active_support_dalli_store_spec.rb b/spec/acceptance/stores/active_support_dalli_store_spec.rb index 70355161..4aded231 100644 --- a/spec/acceptance/stores/active_support_dalli_store_spec.rb +++ b/spec/acceptance/stores/active_support_dalli_store_spec.rb @@ -9,7 +9,6 @@ if should_run require_relative "../../support/cache_store_helper" require "active_support/cache/dalli_store" - require "timecop" describe "ActiveSupport::Cache::DalliStore as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb b/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb index 8344b6e4..a04cedab 100644 --- a/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb +++ b/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb @@ -4,7 +4,6 @@ if defined?(::ConnectionPool) && defined?(::Dalli) require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::MemCacheStore (pooled) as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_mem_cache_store_spec.rb b/spec/acceptance/stores/active_support_mem_cache_store_spec.rb index 65abe7d7..09f63517 100644 --- a/spec/acceptance/stores/active_support_mem_cache_store_spec.rb +++ b/spec/acceptance/stores/active_support_mem_cache_store_spec.rb @@ -4,7 +4,6 @@ if defined?(::Dalli) require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::MemCacheStore as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_memory_store_spec.rb b/spec/acceptance/stores/active_support_memory_store_spec.rb index e047b444..4ed81e7f 100644 --- a/spec/acceptance/stores/active_support_memory_store_spec.rb +++ b/spec/acceptance/stores/active_support_memory_store_spec.rb @@ -3,8 +3,6 @@ require_relative "../../spec_helper" require_relative "../../support/cache_store_helper" -require "timecop" - describe "ActiveSupport::Cache::MemoryStore as a cache backend" do before do Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new diff --git a/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb b/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb index fe951074..3da658f6 100644 --- a/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb +++ b/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb @@ -10,7 +10,6 @@ if should_run require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::RedisCacheStore (pooled) as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_redis_cache_store_spec.rb b/spec/acceptance/stores/active_support_redis_cache_store_spec.rb index a824edea..99701a3e 100644 --- a/spec/acceptance/stores/active_support_redis_cache_store_spec.rb +++ b/spec/acceptance/stores/active_support_redis_cache_store_spec.rb @@ -9,7 +9,6 @@ if should_run require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::RedisCacheStore as a cache backend" do before do diff --git a/spec/acceptance/stores/connection_pool_dalli_client_spec.rb b/spec/acceptance/stores/connection_pool_dalli_client_spec.rb index d532a29b..9658480d 100644 --- a/spec/acceptance/stores/connection_pool_dalli_client_spec.rb +++ b/spec/acceptance/stores/connection_pool_dalli_client_spec.rb @@ -6,7 +6,6 @@ require_relative "../../support/cache_store_helper" require "connection_pool" require "dalli" - require "timecop" describe "ConnectionPool with Dalli::Client as a cache backend" do before do diff --git a/spec/acceptance/stores/dalli_client_spec.rb b/spec/acceptance/stores/dalli_client_spec.rb index 08038c2f..f6841e4a 100644 --- a/spec/acceptance/stores/dalli_client_spec.rb +++ b/spec/acceptance/stores/dalli_client_spec.rb @@ -5,7 +5,6 @@ if defined?(::Dalli) require_relative "../../support/cache_store_helper" require "dalli" - require "timecop" describe "Dalli::Client as a cache backend" do before do diff --git a/spec/acceptance/stores/redis_spec.rb b/spec/acceptance/stores/redis_spec.rb index 4361566c..bf68bc23 100644 --- a/spec/acceptance/stores/redis_spec.rb +++ b/spec/acceptance/stores/redis_spec.rb @@ -4,7 +4,6 @@ if defined?(::Redis) require_relative "../../support/cache_store_helper" - require "timecop" describe "Plain redis as a cache backend" do before do diff --git a/spec/acceptance/stores/redis_store_spec.rb b/spec/acceptance/stores/redis_store_spec.rb index dee35bcf..83d0e659 100644 --- a/spec/acceptance/stores/redis_store_spec.rb +++ b/spec/acceptance/stores/redis_store_spec.rb @@ -4,8 +4,6 @@ require_relative "../../support/cache_store_helper" if defined?(::Redis::Store) - require "timecop" - describe "Redis::Store as a cache backend" do before do Rack::Attack.cache.store = ::Redis::Store.new diff --git a/spec/acceptance/throttling_spec.rb b/spec/acceptance/throttling_spec.rb index 0db89dd6..ca81d1c5 100644 --- a/spec/acceptance/throttling_spec.rb +++ b/spec/acceptance/throttling_spec.rb @@ -4,6 +4,8 @@ require "timecop" describe "#throttle" do + let(:notifications) { [] } + before do Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new end @@ -138,42 +140,31 @@ request.ip end - notification_matched = nil - notification_type = nil - notification_data = nil - notification_discriminator = nil - ActiveSupport::Notifications.subscribe("throttle.rack_attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] - notification_data = payload[:request].env['rack.attack.match_data'] - notification_discriminator = payload[:request].env['rack.attack.match_discriminator'] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" assert_equal 200, last_response.status - assert_nil notification_matched - assert_nil notification_type - assert_nil notification_data - assert_nil notification_discriminator + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" assert_equal 200, last_response.status - assert_nil notification_matched - assert_nil notification_type - assert_nil notification_data - assert_nil notification_discriminator + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" assert_equal 429, last_response.status - assert_equal "by ip", notification_matched - assert_equal :throttle, notification_type - assert_equal 60, notification_data[:period] - assert_equal 1, notification_data[:limit] - assert_equal 2, notification_data[:count] - assert_equal "1.2.3.4", notification_discriminator + + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal "by ip", notification[:request].env["rack.attack.matched"] + assert_equal :throttle, notification[:request].env["rack.attack.match_type"] + assert_equal 60, notification[:request].env["rack.attack.match_data"][:period] + assert_equal 1, notification[:request].env["rack.attack.match_data"][:limit] + assert_equal 2, notification[:request].env["rack.attack.match_data"][:count] + assert_equal "1.2.3.4", notification[:request].env["rack.attack.match_discriminator"] end end diff --git a/spec/acceptance/track_spec.rb b/spec/acceptance/track_spec.rb index cec3000d..62d1551a 100644 --- a/spec/acceptance/track_spec.rb +++ b/spec/acceptance/track_spec.rb @@ -3,27 +3,26 @@ require_relative "../spec_helper" describe "#track" do + let(:notifications) { [] } + it "notifies when track block returns true" do Rack::Attack.track("ip 1.2.3.4") do |request| request.ip == "1.2.3.4" end - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal "ip 1.2.3.4", notification_matched - assert_equal :track, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal "ip 1.2.3.4", notification[:request].env["rack.attack.matched"] + assert_equal :track, notification[:request].env["rack.attack.match_type"] end end diff --git a/spec/acceptance/track_throttle_spec.rb b/spec/acceptance/track_throttle_spec.rb index bc035555..e6aa553a 100644 --- a/spec/acceptance/track_throttle_spec.rb +++ b/spec/acceptance/track_throttle_spec.rb @@ -4,6 +4,8 @@ require "timecop" describe "#track with throttle-ish options" do + let(:notifications) { [] } + it "notifies when throttle goes over the limit without actually throttling requests" do Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new @@ -11,43 +13,35 @@ request.ip end - notification_matched = nil - notification_type = nil - ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload| - notification_matched = payload[:request].env["rack.attack.matched"] - notification_type = payload[:request].env["rack.attack.match_type"] + notifications.push(payload) end get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? assert_equal 200, last_response.status get "/", {}, "REMOTE_ADDR" => "5.6.7.8" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? assert_equal 200, last_response.status get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal "by ip", notification_matched - assert_equal :track, notification_type + assert_equal 1, notifications.size + notification = notifications.pop + assert_equal "by ip", notification[:request].env["rack.attack.matched"] + assert_equal :track, notification[:request].env["rack.attack.match_type"] assert_equal 200, last_response.status Timecop.travel(60) do - notification_matched = nil - notification_type = nil - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_nil notification_matched - assert_nil notification_type + assert notifications.empty? assert_equal 200, last_response.status end diff --git a/spec/rack_attack_throttle_spec.rb b/spec/rack_attack_throttle_spec.rb index 0b0d68ac..1bf7f32f 100644 --- a/spec/rack_attack_throttle_spec.rb +++ b/spec/rack_attack_throttle_spec.rb @@ -1,10 +1,11 @@ # frozen_string_literal: true require_relative 'spec_helper' +require_relative 'support/freeze_time_helper' describe 'Rack::Attack.throttle' do before do - @period = 60 # Use a long period; failures due to cache key rotation less likely + @period = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new Rack::Attack.throttle('ip/sec', limit: 1, period: @period) { |req| req.ip } end @@ -14,14 +15,18 @@ it_allows_ok_requests describe 'a single request' do - before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } - it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" - _(Rack::Attack.cache.store.read(key)).must_equal 1 + within_same_period do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" + _(Rack::Attack.cache.store.read(key)).must_equal 1 + end end it 'should populate throttle data' do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + data = { count: 1, limit: 1, @@ -36,7 +41,9 @@ describe "with 2 requests" do before do - 2.times { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } + within_same_period do + 2.times { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } + end end it 'should block the last request' do @@ -62,7 +69,7 @@ describe 'Rack::Attack.throttle with limit as proc' do before do - @period = 60 # Use a long period; failures due to cache key rotation less likely + @period = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new Rack::Attack.throttle('ip/sec', limit: lambda { |_req| 1 }, period: @period) { |req| req.ip } end @@ -70,14 +77,17 @@ it_allows_ok_requests describe 'a single request' do - before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } - it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" - _(Rack::Attack.cache.store.read(key)).must_equal 1 + within_same_period do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" + _(Rack::Attack.cache.store.read(key)).must_equal 1 + end end it 'should populate throttle data' do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' data = { count: 1, limit: 1, @@ -93,7 +103,7 @@ describe 'Rack::Attack.throttle with period as proc' do before do - @period = 60 # Use a long period; failures due to cache key rotation less likely + @period = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new Rack::Attack.throttle('ip/sec', limit: lambda { |_req| 1 }, period: lambda { |_req| @period }) { |req| req.ip } end @@ -101,14 +111,18 @@ it_allows_ok_requests describe 'a single request' do - before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } - it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" - _(Rack::Attack.cache.store.read(key)).must_equal 1 + within_same_period do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" + _(Rack::Attack.cache.store.read(key)).must_equal 1 + end end it 'should populate throttle data' do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + data = { count: 1, limit: 1, @@ -122,7 +136,7 @@ end end -describe 'Rack::Attack.throttle with block retuning nil' do +describe 'Rack::Attack.throttle with block returning nil' do before do @period = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new @@ -132,14 +146,17 @@ it_allows_ok_requests describe 'a single request' do - before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } - it 'should not set the counter' do - key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" - assert_nil Rack::Attack.cache.store.read(key) + within_same_period do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' + + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" + assert_nil Rack::Attack.cache.store.read(key) + end end it 'should not populate throttle data' do + get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' assert_nil last_request.env['rack.attack.throttle_data'] end end @@ -162,9 +179,11 @@ end it 'should not differentiate requests when throttle_discriminator_normalizer is enabled' do - post_logins - key = "rack::attack:#{Time.now.to_i / @period}:logins/email:person@example.com" - _(Rack::Attack.cache.store.read(key)).must_equal 3 + within_same_period do + post_logins + key = "rack::attack:#{Time.now.to_i / @period}:logins/email:person@example.com" + _(Rack::Attack.cache.store.read(key)).must_equal 3 + end end it 'should differentiate requests when throttle_discriminator_normalizer is disabled' do @@ -172,10 +191,12 @@ prev = Rack::Attack.throttle_discriminator_normalizer Rack::Attack.throttle_discriminator_normalizer = nil - post_logins - @emails.each do |email| - key = "rack::attack:#{Time.now.to_i / @period}:logins/email:#{email}" - _(Rack::Attack.cache.store.read(key)).must_equal 1 + within_same_period do + post_logins + @emails.each do |email| + key = "rack::attack:#{Time.now.to_i / @period}:logins/email:#{email}" + _(Rack::Attack.cache.store.read(key)).must_equal 1 + end end ensure Rack::Attack.throttle_discriminator_normalizer = prev diff --git a/spec/rack_attack_track_spec.rb b/spec/rack_attack_track_spec.rb index d8c53b8a..0013c15b 100644 --- a/spec/rack_attack_track_spec.rb +++ b/spec/rack_attack_track_spec.rb @@ -3,21 +3,7 @@ require_relative 'spec_helper' describe 'Rack::Attack.track' do - let(:counter_class) do - Class.new do - def self.incr - @counter += 1 - end - - def self.reset - @counter = 0 - end - - def self.check - @counter - end - end - end + let(:notifications) { [] } before do Rack::Attack.track("everything") { |_req| true } @@ -34,19 +20,18 @@ def self.check describe "with a notification subscriber and two tracks" do before do - counter_class.reset # A second track Rack::Attack.track("homepage") { |req| req.path == "/" } - ActiveSupport::Notifications.subscribe("track.rack_attack") do |*_args| - counter_class.incr + ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload| + notifications.push(payload) end get "/" end it "should notify twice" do - _(counter_class.check).must_equal 2 + _(notifications.size).must_equal 2 end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f529e6a1..48ed7738 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -20,6 +20,7 @@ def safe_require(name) safe_require "connection_pool" safe_require "dalli" +safe_require "rails" safe_require "redis" safe_require "redis-store" @@ -27,7 +28,7 @@ class Minitest::Spec include Rack::Test::Methods before do - if Object.const_defined?(:Rails) && Rails.respond_to?(:cache) + if Object.const_defined?(:Rails) && Rails.respond_to?(:cache) && Rails.cache.respond_to?(:clear) Rails.cache.clear end end diff --git a/spec/support/cache_store_helper.rb b/spec/support/cache_store_helper.rb index 5b8f04b3..8295ac00 100644 --- a/spec/support/cache_store_helper.rb +++ b/spec/support/cache_store_helper.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative 'freeze_time_helper' + class Minitest::Spec def self.it_works_for_cache_backed_features(options) fetch_from_store = options.fetch(:fetch_from_store) @@ -9,11 +11,13 @@ def self.it_works_for_cache_backed_features(options) request.ip end - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal 200, last_response.status + within_same_period do + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + assert_equal 200, last_response.status - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal 429, last_response.status + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + assert_equal 429, last_response.status + end end it "works for fail2ban" do @@ -23,17 +27,19 @@ def self.it_works_for_cache_backed_features(options) end end - get "/" - assert_equal 200, last_response.status + within_same_period do + get "/" + assert_equal 200, last_response.status - get "/private-place" - assert_equal 403, last_response.status + get "/private-place" + assert_equal 403, last_response.status - get "/private-place" - assert_equal 403, last_response.status + get "/private-place" + assert_equal 403, last_response.status - get "/" - assert_equal 403, last_response.status + get "/" + assert_equal 403, last_response.status + end end it "works for allow2ban" do @@ -43,20 +49,22 @@ def self.it_works_for_cache_backed_features(options) end end - get "/" - assert_equal 200, last_response.status + within_same_period do + get "/" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 200, last_response.status + get "/scarce-resource" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 200, last_response.status + get "/scarce-resource" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 403, last_response.status + get "/scarce-resource" + assert_equal 403, last_response.status - get "/" - assert_equal 403, last_response.status + get "/" + assert_equal 403, last_response.status + end end it "doesn't leak keys" do @@ -66,9 +74,7 @@ def self.it_works_for_cache_backed_features(options) key = nil - # Freeze time during these statement to be sure that the key used by rack attack is the same - # we pre-calculate in local variable `key` - Timecop.freeze do + within_same_period do key = "rack::attack:#{Time.now.to_i}:by ip:1.2.3.4" get "/", {}, "REMOTE_ADDR" => "1.2.3.4" diff --git a/spec/support/freeze_time_helper.rb b/spec/support/freeze_time_helper.rb new file mode 100644 index 00000000..462c877a --- /dev/null +++ b/spec/support/freeze_time_helper.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require "timecop" + +class Minitest::Spec + def within_same_period(&block) + Timecop.freeze(&block) + end +end