mirror of
				https://github.com/zulip/zulip.git
				synced 2025-10-31 20:13:46 +00:00 
			
		
		
		
	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.
This commit is contained in:
		
				
					committed by
					
						 Tim Abbott
						Tim Abbott
					
				
			
			
				
	
			
			
			
						parent
						
							5f0897e6f7
						
					
				
				
					commit
					0cfb156545
				
			| @@ -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: | ||||
|   | ||||
| @@ -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( | ||||
|   | ||||
| @@ -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, | ||||
|   | ||||
| @@ -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": [], | ||||
| } | ||||
|  | ||||
|   | ||||
		Reference in New Issue
	
	Block a user