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

Intermittent CI fail with ratelimit #64

Closed
samuelcolvin opened this issue Apr 13, 2015 · 3 comments
Closed

Intermittent CI fail with ratelimit #64

samuelcolvin opened this issue Apr 13, 2015 · 3 comments

Comments

@samuelcolvin
Copy link

We're using ratelimit for our login page:

@ratelimit(key='ip', rate=settings.LOGIN_LIMIT)
@ratelimit(key='post:username', rate=settings.LOGIN_LIMIT)
def login(request):
    ...

(in settings LOGIN_LIMIT = '4/m')

And test the rate limit is working on tests:

    @override_settings(RATELIMIT_ENABLE=True)
    def test_login_form_captcha(self):
        """
        Test repeated login attempts yield a captcha.
        """
        # check that captcha is not here initially
        response = self.client.get(self.login_url)
        self.assertEqual(response.status_code, 200)
        self.assertNotIn('captcha', response.content)
        # try to login many times
        wrong_credentials = {'username': 'testing@example.com', 'password': 'wrong'}
        for _ in range(3):
            response = self.client.post(self.login_url, wrong_credentials)
            self.assertEqual(response.status_code, 200)
            self.assertFalse(response.context['login_form'].is_valid())
            self.assertNotContains(response, 'captcha')
        # one time more and captcha appears
        response = self.client.post(self.login_url, wrong_credentials)
        self.assertEqual(response.status_code, 200)
        self.assertFalse(response.context['login_form'].is_valid())
        self.assertContains(response, 'captcha')

This always works locally and works about 95% of the time on CI but just occationally fails with

AssertionError: Couldn't find 'captcha' in response

I'm aware there's a good chance this could be a weird error unique to our system but I thought it was worth asking whether there's any pointer as to what's causing ratelimit not to kick in?

@jsocol
Copy link
Owner

jsocol commented Apr 13, 2015

Ratelimits are calculated on staggered, fixed windows, i.e., for a given set of group/path/methods/key-value/period, ratelimit will calculate some value that will be the same every time it's calculated for period seconds (it uses the value to stagger them so, e.g. not all hour ratelimits expire at the top of the hour).

So, imagine the limit is 1/min, the second attempt (&) will be ratelimited, but the 3rd won't be, even though it's within 1 minute of the first:

period:       |             |
attempts:            ^  &    ^

The reason for this is that if you reset the period every time (what ratelimit used to do) then you can end up blocking even good actors basically forever (e.g. they try every 59 seconds, continually pushing the limit out and never being allowed).

So what's probably happening is that every once in a while, you cross the boundary during that loop. The odds are bad—and your flakiness rate is low—but it's entirely possible.

You could probably simplify this by mocking ratelimit.utils.is_ratelimited to always return True during this test, instead of trying to actually trip the limit. Alternatively, set the number of attempts to twice the limit, to make sure you hit it even if you cross the boundary (but you'll have to drop some of the assertions within the loop).

@jsocol jsocol closed this as completed Apr 13, 2015
@samuelcolvin
Copy link
Author

Thanks a lot, makes complete sense, this had occurred to me but I assumed the limit was reset every time.

Thanks for your help.

@samuelcolvin
Copy link
Author

For anyone else having trouble with ratelimt and CI, my solution is:

    wrong_credentials = {'username': 'testing@example.com', 'password': 'wrong'}
    found_captcha = False
    for _ in range(7):
        response = self.client.post(self.login_url, wrong_credentials)
        self.assertEqual(response.status_code, 200)
        self.assertFalse(response.context['login_form'].is_valid())
        if 'captcha' in response.content:
            found_captcha = True
            break
    self.assertTrue(found_captcha, 'captcha not found on login page after 7 failed login attempts')

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

No branches or pull requests

2 participants