Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specs failing at random intervals #69

Closed
vanso-waqar opened this issue May 27, 2014 · 4 comments · Fixed by #85
Closed

Specs failing at random intervals #69

vanso-waqar opened this issue May 27, 2014 · 4 comments · Fixed by #85

Comments

@vanso-waqar
Copy link

Hi,

I have written somes test specs for my throttling configs.

initializer file

class Rack::Attack
  throttle('req/ip', :limit => 300, :period => 5.minutes) do |req|
      req.ip
    end
  throttle('logins/ip', :limit => 5, :period => 20.seconds) do |req|
      if req.path == '/users/sign_in' && req.post?
        req.ip
      end
    end
  throttle("logins/email", :limit => 5, :period => 20.seconds) do |req|
      if req.path == '/users/sign_in' && req.post?
        req.params['email'].presence # return the email if present, nil otherwise
      end
    end
  end

I tested the first and second throttles and it does not seems to work consistantly.
Sometimes it blocks the 301st request and sometimes it allows it. Same thing happens with 'logins/ip' it allows the 6th request.

My specs file is as follows

require 'spec_helper'

  describe 'Rack::Attack.throttle', type: :rack_attack do
    before do
      @period = 5.minutes # Use a long period; failures due to cache key rotation less likely
      Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
      # Rack::Attack.throttle('req/ip', :limit => 300, :period => @period) { |req| req.ip }
    end

    it('should have all throttles') { Rack::Attack.throttles.keys.should =~ ['req/ip', 'logins/ip', 'logins/email'] }

    it "must allow ok requests" do
      get '/', {}, 'REMOTE_ADDR' => '127.0.0.1'
      response.status.should == 200
    end

    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).to_i}:req/ip:1.2.3.4"
        Rack::Attack.cache.store.read(key).should == 1
      end

      it 'should populate throttle data' do
        data = { :count => 1, :limit => 300, :period => @period.to_i }
        request.env['rack.attack.throttle_data']['req/ip'].should == data
      end
    end

    describe "with 301 requests to home page" do
      before do
        (@period+1).times { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' }
      end

      it 'should block the last request' do
        response.status.should == 429
      end

      it 'should tag the env' do
        request.env['rack.attack.matched'].should == 'req/ip'
        request.env['rack.attack.match_type'].should == :throttle
        request.env['rack.attack.match_data'].should == {:count => 301, :limit => 300, :period => @period.to_i }
        request.env["REMOTE_ADDR"].should == "1.2.3.4"
      end

      it 'should set a Retry-After header' do
        response.headers['Retry-After'].should == @period.to_s
      end
    end

    describe "with 6 requests to sign_in page" do
      before do
        6.times { post(new_user_session_path, user: {username: 'bogus', password: 'random123'}) }
      end

      it 'should block the last request' do
        response.status.should == 429
      end

      it 'should tag the env' do
        request.env['rack.attack.matched'].should == 'logins/ip'
        request.env['rack.attack.match_type'].should == :throttle
        request.env['rack.attack.match_data'].should == {:count => 6, :limit => 5, :period => 20 }
        request.env["REMOTE_ADDR"].should == "127.0.0.1"
      end
    end

    after(:all) do
      Rack::Attack.clear!
    end
end

Can please help me out.

@ktheory
Copy link
Collaborator

ktheory commented May 27, 2014

Very curious. Does it happen with other cache stores?

Many thanks if you can track down the source of this bug.

@vanso-waqar
Copy link
Author

Hi,

Thanks for replying back.
I switched to RedisStore and debugged it.
It is incrementing the count to 301("with 301 requests to home page") and 6("with 6 requests to sign_in page"), which is correct.
But it is still not throttling the request.
I get a 200 status for my last request.

ktheory added a commit that referenced this issue Aug 31, 2014
Fixes #69.

There was a race condition when `Time.now.to_i` changes between when
`epoch_time` is computed in line 18, and the cache request is made (and
the `key`) is expired.

I.e., a throttle check starts at t0, but doesn’t reach the cache until
t1, the cache will have expired the throttle count. The request will
likely be allowed, even if the request exceeded the limit.

This has the effect of keeping keys in cache about 1 second longer than
strictly necessary. But the extra cache space seems like a good
trade-off for correct throttling.
ktheory added a commit that referenced this issue Aug 31, 2014
Fixes #69.

There was a race condition when `Time.now.to_i` changes between when
`epoch_time` is computed in line 18, and the cache request is made (and
the `key` is expired).

I.e., a throttle check starts at t0, but doesn’t reach the cache until
t1, the cache will have expired the throttle count. The request will
likely be allowed, even if the request exceeded the limit.

This has the effect of keeping keys in cache about 1 second longer than
strictly necessary. But the extra cache space seems like a good
trade-off for correct throttling.
@ktheory
Copy link
Collaborator

ktheory commented Aug 31, 2014

Pretty sure I finally tracked this down in #85. At least it was a simple fix. 😄

ktheory added a commit that referenced this issue Sep 2, 2014
Fixes #69.

There was a race condition when `Time.now.to_i` changes between when
`epoch_time` is computed in line 18, and the cache request is made (and
the `key` is expired).

I.e., a throttle check starts at t0, but doesn’t reach the cache until
t1, the cache will have expired the throttle count. The request will
likely be allowed, even if the request exceeded the limit.

This has the effect of keeping keys in cache about 1 second longer than
strictly necessary. But the extra cache space seems like a good
trade-off for correct throttling.
@vanso-waqar
Copy link
Author

👍 thanks
I'll take a look a it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants