From 0cfb156545a2232c1b9bb99b29b943d6fbfb8a01 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 3 Nov 2021 13:40:28 -0700 Subject: [PATCH] rate_limit: Merge two IP rate limits domains that send emails. Both `create_realm_by_ip` and `find_account_by_ip` send emails to arbitrary email addresses, and as such can be used to spam users. Lump their IP rate limits into the same bucket; most legitimate users will likely not be using both of these endpoints at similar times. The rate is set at 5 in 30 minutes, the more quickly-restrictive of the two previous rates. --- zerver/tests/test_external.py | 21 ++++++++++++++++++--- zerver/views/registration.py | 4 ++-- zproject/computed_settings.py | 9 +++------ zproject/test_extra_settings.py | 3 +-- 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/zerver/tests/test_external.py b/zerver/tests/test_external.py index e81bcaa952..51b17dff2d 100644 --- a/zerver/tests/test_external.py +++ b/zerver/tests/test_external.py @@ -203,7 +203,7 @@ class RateLimitTests(ZulipTestCase): resp = self.send_unauthed_api_request(REMOTE_ADDR="127.0.0.2") self.assertNotEqual(resp.status_code, 429) - @rate_limit_rule(1, 5, domain="create_realm_by_ip") + @rate_limit_rule(1, 5, domain="sends_email_by_ip") def test_create_realm_rate_limiting(self) -> None: with self.settings(OPEN_REALM_CREATION=True): self.do_test_hit_ratelimits( @@ -211,7 +211,7 @@ class RateLimitTests(ZulipTestCase): is_json=False, ) - @rate_limit_rule(1, 5, domain="find_account_by_ip") + @rate_limit_rule(1, 5, domain="sends_email_by_ip") def test_find_account_rate_limiting(self) -> None: self.do_test_hit_ratelimits( lambda: self.client_post("/accounts/find/", {"emails": "new@zulip.com"}), @@ -221,13 +221,28 @@ class RateLimitTests(ZulipTestCase): # 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. - @rate_limit_rule(1, 10, domain="find_account_by_ip") + @rate_limit_rule(1, 10, domain="sends_email_by_ip") def test_find_account_rate_limiting_multiple(self) -> None: self.do_test_hit_ratelimits( lambda: self.client_post("/accounts/find/", {"emails": "new@zulip.com,new2@zulip.com"}), is_json=False, ) + @rate_limit_rule(1, 5, domain="sends_email_by_ip") + def test_combined_ip_limits(self) -> None: + # Alternate requests to /new/ and /accounts/find/ + request_count = 0 + + def alternate_requests() -> HttpResponse: + nonlocal request_count + request_count += 1 + if request_count % 2 == 1: + return self.client_post("/new/", {"email": "new@zulip.com"}) + else: + return self.client_post("/accounts/find/", {"emails": "new@zulip.com"}) + + self.do_test_hit_ratelimits(alternate_requests, is_json=False) + @skipUnless(settings.ZILENCER_ENABLED, "requires zilencer") @rate_limit_rule(1, 5, domain="api_by_remote_server") def test_hit_ratelimits_as_remote_server(self) -> None: diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 335de8f676..0bd5b67ab7 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -605,7 +605,7 @@ def create_realm(request: HttpRequest, creation_key: Optional[str] = None) -> Ht form = RealmCreationForm(request.POST) if form.is_valid(): try: - rate_limit_request_by_ip(request, domain="create_realm_by_ip") + rate_limit_request_by_ip(request, domain="sends_email_by_ip") except RateLimited as e: assert e.secs_to_freedom is not None return render( @@ -725,7 +725,7 @@ def find_account( emails = form.cleaned_data["emails"] for i in range(len(emails)): try: - rate_limit_request_by_ip(request, domain="find_account_by_ip") + rate_limit_request_by_ip(request, domain="sends_email_by_ip") except RateLimited as e: assert e.secs_to_freedom is not None return render( diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 470471a6d5..ac2dd6c6bf 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -385,16 +385,13 @@ RATE_LIMITING_RULES = { "authenticate_by_username": [ (1800, 5), # 5 login attempts within 30 minutes ], - "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 ], + "sends_email_by_ip": [ + (1800, 5), + ], } # List of domains that, when applied to a request in a Tornado process, diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index e1382816bb..448045a60b 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -266,8 +266,7 @@ RATE_LIMITING_RULES: Dict[str, List[Tuple[int, int]]] = { "api_by_ip": [], "api_by_remote_server": [], "authenticate_by_username": [], - "create_realm_by_ip": [], - "find_account_by_ip": [], + "sends_email_by_ip": [], "password_reset_form_by_email": [], }