From ddcfd9e2eea989f8ea75cf1aeda49d2c953b8d9c Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Thu, 5 Aug 2021 11:13:22 +0200 Subject: [PATCH] rate_limit: Rate limit the /accounts/find/ endpoint. Closes #19287 This endpoint allows submitting multiple addresses so we need to "weigh" the rate limit more heavily the more emails are submitted. Clearly e.g. a request triggering emails to 2 addresses should weigh twice as much as a request doing that for just 1 address. --- zerver/tests/test_external.py | 30 ++++++++++++++++++++++++++++++ zerver/views/registration.py | 11 +++++++++++ zproject/computed_settings.py | 3 +++ zproject/test_extra_settings.py | 1 + 4 files changed, 45 insertions(+) diff --git a/zerver/tests/test_external.py b/zerver/tests/test_external.py index 3e8c522243..dffa7ede70 100644 --- a/zerver/tests/test_external.py +++ b/zerver/tests/test_external.py @@ -200,6 +200,36 @@ class RateLimitTests(ZulipTestCase): finally: remove_ratelimit_rule(1, 5, domain="create_realm_by_ip") + def test_find_account_rate_limiting(self) -> None: + def assert_func(result: HttpResponse) -> None: + self.assertEqual(result.status_code, 429) + self.assert_in_response("Rate limit exceeded.", result) + + add_ratelimit_rule(1, 5, domain="find_account_by_ip") + try: + RateLimitedIPAddr("127.0.0.1", domain="find_account_by_ip").clear_history() + self.do_test_hit_ratelimits( + lambda: self.client_post("/accounts/find/", {"emails": "new@zulip.com"}), + assert_func=assert_func, + ) + finally: + remove_ratelimit_rule(1, 5, domain="find_account_by_ip") + + # Now test whether submitting multiple emails is handled correctly. + # The limit is set to 10 per second, so 5 requests with 2 emails + # submitted in each should be allowed. + add_ratelimit_rule(1, 10, domain="find_account_by_ip") + try: + RateLimitedIPAddr("127.0.0.1", domain="find_account_by_ip").clear_history() + self.do_test_hit_ratelimits( + lambda: self.client_post( + "/accounts/find/", {"emails": "new@zulip.com,new2@zulip.com"} + ), + assert_func=assert_func, + ) + finally: + remove_ratelimit_rule(1, 10, domain="find_account_by_ip") + @skipUnless(settings.ZILENCER_ENABLED, "requires zilencer") def test_hit_ratelimits_as_remote_server(self) -> None: add_ratelimit_rule(1, 5, domain="api_by_remote_server") diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 821c35dac4..7ebc2ff488 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -711,6 +711,17 @@ def find_account(request: HttpRequest) -> HttpResponse: form = FindMyTeamForm(request.POST) if form.is_valid(): emails = form.cleaned_data["emails"] + for i in range(len(emails)): + try: + rate_limit_request_by_ip(request, domain="find_account_by_ip") + except RateLimited as e: + assert e.secs_to_freedom is not None + return render( + request, + "zerver/rate_limit_exceeded.html", + context={"retry_after": int(e.secs_to_freedom)}, + status=429, + ) # Django doesn't support __iexact__in lookup with EmailField, so we have # to use Qs to get around that without needing to do multiple queries. diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index d01b24614b..b8c9facb69 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -387,6 +387,9 @@ RATE_LIMITING_RULES = { "create_realm_by_ip": [ (1800, 5), ], + "find_account_by_ip": [ + (3600, 10), + ], "password_reset_form_by_email": [ (3600, 2), # 2 reset emails per hour (86400, 5), # 5 per day diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index aa5b2c926a..360e88f31c 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -265,6 +265,7 @@ RATE_LIMITING_RULES: Dict[str, List[Tuple[int, int]]] = { "api_by_remote_server": [], "authenticate_by_username": [], "create_realm_by_ip": [], + "find_account_by_ip": [], "password_reset_form_by_email": [], }