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

Ratelimits for 1/h appear to allow 2, but no more #76

Closed
jvc26 opened this issue Aug 11, 2015 · 4 comments
Closed

Ratelimits for 1/h appear to allow 2, but no more #76

jvc26 opened this issue Aug 11, 2015 · 4 comments

Comments

@jvc26
Copy link

jvc26 commented Aug 11, 2015

Using the helper as I don't want to increment all requests, only ones where a successful submission has taken place:

class RateLimitedFormView(FormView):
    ratelimit_key = 'ip'
    ratelimit_block = True
    ratelimit_rate = '1/h'
    ratelimit_group = None

    def dispatch(self, *args, **kwargs):
        ratelimited = is_ratelimited(request=self.request,
                                     group=self.ratelimit_group,
                                     key=self.ratelimit_key,
                                     rate=self.ratelimit_rate,
                                     increment=False)
        if ratelimited and self.ratelimit_block:
            raise Ratelimited()
        return super(RateLimitedFormView, self).dispatch(*args, **kwargs)


class RegistrationView(RateLimitedFormView):
    template_name = 'accounts/register.html'
    form_class = EmailUserCreationForm
    ratelimit_group = 'registration'

    def form_valid(self, form):
        # Saves the form and does login here ... [ snip ]
        # Calls is_ratelimited here to increment the counter
        is_ratelimited(request=self.request, group=self.ratelimit_group, key=self.ratelimit_key,
                       rate=self.ratelimit_rate, increment=True)
        return super(RegistrationView, self).form_valid(form)

One would expect the above to increment the counter by 1 for every form_valid call, and therefore get ratelimited at the second dispatch(). However, this does not happen:

{'': {}}
{'': {u':1:rl:bd2e7d391ec4f6cc09024ab9b8f38395': '\x80\x02K\x01.'}}
{'': {u':1:rl:bd2e7d391ec4f6cc09024ab9b8f38395': '\x80\x02K\x02.'}}
{'': {u':1:rl:bd2e7d391ec4f6cc09024ab9b8f38395': '\x80\x02K\x02.'}}

The above are the four lines of output from a test, printing the contents of the cache on each subsequent request to the RegistrationView. At round one, it adds the cache key, but at round two, the dispatch override returns False from is_ratelimited, even though the rate is 1/h, and then allows a second request to go through, before ratelimiting on the third.

Why is this?

@jsocol
Copy link
Owner

jsocol commented Aug 11, 2015

What version of django-ratelimit are you using? Can you include the line that's dumping out the contents of the cache, as well as the cache backend?

@jsocol
Copy link
Owner

jsocol commented Aug 11, 2015

Also... because it does a > check, which ISTR is for a reason, though I'm not 100% sure what that reason was.

@jsocol
Copy link
Owner

jsocol commented Aug 11, 2015

Also... because it does a > check, which ISTR is for a reason, though I'm not 100% sure what that reason was.

Ah, it's coming back to me a little—because this is an atypical way to use ratelimit.

When you use the decorator or the mixin, it pre-increments, so if you say rate='5/m' then 5 requests per minute are allowed. When you post-increment, you're allowing the action to happen (twice) before counting it. So when you check the ratelimit in dispatch(), you have not yet surpassed the ratelimit.

Ratelimit may not actually be the right tool here. Remember that it relies on the cache, which is not always reliable, and has staggered windows. If you only want each IP to have one submission per clock hour, just checking to see whether or not there is a saved submission since datetime.now().replace(minute=0) might be more what you're looking to achieve.

@jsocol jsocol closed this as completed Aug 11, 2015
@jvc26
Copy link
Author

jvc26 commented Aug 11, 2015

Great thanks for the clarification - I thought I might be missing something - really helpful.

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