rack / rack-attack

Rack middleware for blocking & throttling
MIT License
5.56k stars 337 forks source link

Rack::Attack.clear_configuration breaks subsequent tests #666

Closed jarosluv closed 1 month ago

jarosluv commented 2 months ago

Using Rack::Attack.clear_configuration breaks subsequent tests.

Expected Behavior

Second consecutive request is throttled when Rack Attack throttling is set to limit = 1.

Actual Behavior

Second consecutive request returns status code 200 when I use Rack::Attack.clear_configuration in the previous test.

Steps to Reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  gem "rack-attack"
  gem 'rspec-rails', '~> 6.1.0'
end

require "action_controller/railtie"
require "rack/attack"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  config.middleware.use Rack::Attack

  routes.draw do
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    render plain: "Home"
  end
end

require "rspec/autorun"
require "rack/test"

class Rack::Attack
  Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new

  throttle("request/ip", limit: 1, period: 5.minutes) do |request|
    request.ip
  end
end

RSpec.describe "inline Bundler and autorun RSpec" do
  include Rack::Test::Methods

  let(:app) { Rails.application }

  it "responses with 200 status code for 1 request" do
    get "/"
    expect(last_response.status).to eq 200
    Rack::Attack.clear_configuration
  end

  describe "GET" do
    it "responses with 200 status code even if throttling limit is exceeded" do
      get "/"
      expect(last_response.status).to eq 200
      get "/"
      expect(last_response.status).to eq 429 # Still returns 200 status code
    end
  end
end
jarosluv commented 2 months ago

As usual, right after I posted the bug report, I realized the issue. I'll leave this note here in case someone else faces a similar situation.

From the documentation, I understood that between tests, you should use Rack::Attack.clear_configuration to isolate test examples. However, this command actually resets all throttling, blocking, etc. settings and configurations made in the app initializer.

In reality, to isolate test scenarios, you should use Rack::Attack.cache.store.clear (or Rails.cache.clear). This way, the configuration is preserved, but the locks are cleared.

So, my question is: Did I misunderstand the documentation in README.md, or is this aspect described ambiguously?

santib commented 1 month ago

Hi @jarosluv. First of all, thanks for the detailed report.

From the README I understand that between test cases you should use Rack::Attack.reset!, and only use Rack::Attack.clear_configuration to unset blocklist and safelist configurations.

That said, if you want to open a PR improving the README, I'm open to review it.