diff --git a/apps/api/plane/authentication/rate_limit.py b/apps/api/plane/authentication/rate_limit.py index a35a074525..bfadf82b70 100644 --- a/apps/api/plane/authentication/rate_limit.py +++ b/apps/api/plane/authentication/rate_limit.py @@ -6,7 +6,7 @@ import os # Third party imports -from rest_framework.throttling import AnonRateThrottle, SimpleRateThrottle, UserRateThrottle +from rest_framework.throttling import AnonRateThrottle, UserRateThrottle from rest_framework import status from rest_framework.response import Response @@ -49,53 +49,6 @@ def authentication_throttle_allows(request): return throttle.allow_request(request, None) -def _valid_rate_or_default(value, default): - """Return `value` only if it is a well-formed DRF throttle rate ("/"), - else `default`. SimpleRateThrottle.parse_rate() raises on a malformed rate, and the - throttle is instantiated on every auth POST — an unvalidated env value would take - authentication down instance-wide. Falling back to the default keeps auth up. - """ - try: - num, period = value.split("/") - int(num) - if period[:1] not in ("s", "m", "h", "d"): - raise ValueError - except (ValueError, AttributeError): - return default - return value - - -class AuthenticationAccountThrottle(SimpleRateThrottle): - """Per-(account, client-IP) authentication throttle. - - Supplements the IP-only AuthenticationThrottle by also bucketing on the normalized - submitted email, capping rapid credential guessing against a single account from a - given source. The key combines email AND client IP on purpose: keying on email alone - would let anyone lock a victim out of their own account by spamming their address from - other IPs (self-inflicted account-lockout DoS). Email is normalized (strip + lower) to - match the login lookup so casing tricks cannot multiply the allowance; requests without - an email fall back to the client identity only. - - NOTE: this does not by itself stop a spoofed-source distributed brute force — that - requires a trustworthy client IP (configure NUM_PROXIES / the proxy so X-Forwarded-For - cannot be forged). It is defense-in-depth alongside that deployment control. - """ - - scope = "authentication_account" - rate = _valid_rate_or_default(os.environ.get("AUTHENTICATION_ACCOUNT_RATE_LIMIT", "5/minute"), "5/minute") - - def get_cache_key(self, request, view=None): - ip = self.get_ident(request) - email = (request.POST.get("email") or "").strip().lower() - ident = f"email:{email}|ip:{ip}" if email else f"ip:{ip}" - return self.cache_format % {"scope": self.scope, "ident": ident} - - -def authentication_account_throttle_allows(request): - """Per-account counterpart to authentication_throttle_allows (see above).""" - return AuthenticationAccountThrottle().allow_request(request, None) - - class EmailVerificationThrottle(UserRateThrottle): """ Throttle for email verification code generation. diff --git a/apps/api/plane/authentication/views/app/email.py b/apps/api/plane/authentication/views/app/email.py index 7b0aa00a5d..3d1954875c 100644 --- a/apps/api/plane/authentication/views/app/email.py +++ b/apps/api/plane/authentication/views/app/email.py @@ -10,10 +10,6 @@ from django.views import View # Module imports from plane.authentication.provider.credentials.email import EmailProvider -from plane.authentication.rate_limit import ( - authentication_throttle_allows, - authentication_account_throttle_allows, -) from plane.authentication.utils.login import user_login from plane.license.models import Instance from plane.authentication.utils.host import base_host @@ -30,22 +26,6 @@ from plane.utils.path_validator import get_safe_redirect_url class SignInAuthEndpoint(View): def post(self, request): next_path = request.POST.get("next_path") - - # Rate-limit password sign-in per IP to prevent credential brute-force. - # This is a plain django View, so DRF throttle_classes do not apply and - # the throttle must be invoked manually (as in the magic-link endpoints). - if not authentication_throttle_allows(request) or not authentication_account_throttle_allows(request): - exc = AuthenticationException( - error_code=AUTHENTICATION_ERROR_CODES["RATE_LIMIT_EXCEEDED"], - error_message="RATE_LIMIT_EXCEEDED", - ) - url = get_safe_redirect_url( - base_url=base_host(request=request, is_app=True), - next_path=next_path, - params=exc.get_error_dict(), - ) - return HttpResponseRedirect(url) - # Check instance configuration instance = Instance.objects.first() if instance is None or not instance.is_setup_done: @@ -155,21 +135,6 @@ class SignInAuthEndpoint(View): class SignUpAuthEndpoint(View): def post(self, request): next_path = request.POST.get("next_path") - - # Rate-limit password sign-up per IP to prevent automated abuse, - # mirroring the sign-in path and the magic-link endpoints. - if not authentication_throttle_allows(request) or not authentication_account_throttle_allows(request): - exc = AuthenticationException( - error_code=AUTHENTICATION_ERROR_CODES["RATE_LIMIT_EXCEEDED"], - error_message="RATE_LIMIT_EXCEEDED", - ) - url = get_safe_redirect_url( - base_url=base_host(request=request, is_app=True), - next_path=next_path, - params=exc.get_error_dict(), - ) - return HttpResponseRedirect(url) - # Check instance configuration instance = Instance.objects.first() if instance is None or not instance.is_setup_done: diff --git a/apps/api/plane/authentication/views/space/email.py b/apps/api/plane/authentication/views/space/email.py index a2afe87f87..827348cef2 100644 --- a/apps/api/plane/authentication/views/space/email.py +++ b/apps/api/plane/authentication/views/space/email.py @@ -11,10 +11,6 @@ from django.utils.http import url_has_allowed_host_and_scheme # Module imports from plane.authentication.provider.credentials.email import EmailProvider -from plane.authentication.rate_limit import ( - authentication_throttle_allows, - authentication_account_throttle_allows, -) from plane.authentication.utils.login import user_login from plane.license.models import Instance from plane.authentication.utils.host import base_host @@ -29,22 +25,6 @@ from plane.utils.path_validator import get_safe_redirect_url, validate_next_path class SignInAuthSpaceEndpoint(View): def post(self, request): next_path = request.POST.get("next_path") - - # Rate-limit password sign-in per IP to prevent credential brute-force. - # Plain django View → DRF throttle_classes do not apply, so invoke the - # throttle manually (as in the magic-link endpoints). - if not authentication_throttle_allows(request) or not authentication_account_throttle_allows(request): - exc = AuthenticationException( - error_code=AUTHENTICATION_ERROR_CODES["RATE_LIMIT_EXCEEDED"], - error_message="RATE_LIMIT_EXCEEDED", - ) - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), - next_path=next_path, - params=exc.get_error_dict(), - ) - return HttpResponseRedirect(url) - # Check instance configuration instance = Instance.objects.first() if instance is None or not instance.is_setup_done: @@ -130,21 +110,6 @@ class SignInAuthSpaceEndpoint(View): class SignUpAuthSpaceEndpoint(View): def post(self, request): next_path = request.POST.get("next_path") - - # Rate-limit password sign-up per IP to prevent automated abuse, - # mirroring the sign-in path and the magic-link endpoints. - if not authentication_throttle_allows(request) or not authentication_account_throttle_allows(request): - exc = AuthenticationException( - error_code=AUTHENTICATION_ERROR_CODES["RATE_LIMIT_EXCEEDED"], - error_message="RATE_LIMIT_EXCEEDED", - ) - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), - next_path=next_path, - params=exc.get_error_dict(), - ) - return HttpResponseRedirect(url) - # Check instance configuration instance = Instance.objects.first() if instance is None or not instance.is_setup_done: diff --git a/apps/api/plane/tests/unit/authentication/__init__.py b/apps/api/plane/tests/unit/authentication/__init__.py deleted file mode 100644 index fcc34a703d..0000000000 --- a/apps/api/plane/tests/unit/authentication/__init__.py +++ /dev/null @@ -1,3 +0,0 @@ -# Copyright (c) 2023-present Plane Software, Inc. and contributors -# SPDX-License-Identifier: AGPL-3.0-only -# See the LICENSE file for details. diff --git a/apps/api/plane/tests/unit/authentication/test_rate_limit.py b/apps/api/plane/tests/unit/authentication/test_rate_limit.py deleted file mode 100644 index 7b62242146..0000000000 --- a/apps/api/plane/tests/unit/authentication/test_rate_limit.py +++ /dev/null @@ -1,54 +0,0 @@ -# Copyright (c) 2023-present Plane Software, Inc. and contributors -# SPDX-License-Identifier: AGPL-3.0-only -# See the LICENSE file for details. - -import pytest -from django.test import RequestFactory - -from plane.authentication.rate_limit import ( - AuthenticationAccountThrottle, - _valid_rate_or_default, -) - - -@pytest.mark.unit -class TestValidRateOrDefault: - """A malformed AUTHENTICATION_ACCOUNT_RATE_LIMIT must not crash auth: the throttle is - built on every sign-in POST, and DRF parse_rate() raises on a bad rate. Guard falls - back to the default so authentication stays up.""" - - @pytest.mark.parametrize("bad", ["", "bad//x", "10/xyz", "abc/m", "5", "5/", None]) - def test_malformed_falls_back_to_default(self, bad): - assert _valid_rate_or_default(bad, "5/minute") == "5/minute" - - @pytest.mark.parametrize("good", ["3/m", "5/minute", "10/h", "1/s", "100/d"]) - def test_valid_rate_passes_through(self, good): - assert _valid_rate_or_default(good, "5/minute") == good - - -@pytest.mark.unit -class TestAccountThrottleCacheKey: - """The per-account throttle keys on email AND client IP. Keying on email alone would - let anyone lock a victim out of their own account by spamming their address from other - IPs; combining with the client IP prevents that self-inflicted lockout DoS.""" - - def _request(self, remote_addr, **post): - request = RequestFactory().post("/auth/sign-in/", data=post) - request.META["REMOTE_ADDR"] = remote_addr - return request - - def test_key_combines_normalized_email_and_ip(self): - key = AuthenticationAccountThrottle().get_cache_key(self._request("10.0.0.1", email="Victim@Example.COM ")) - assert "email:victim@example.com" in key # normalized (strip + lower) - assert "ip:10.0.0.1" in key - - def test_same_email_different_ip_yields_different_buckets(self): - throttle = AuthenticationAccountThrottle() - k1 = throttle.get_cache_key(self._request("1.1.1.1", email="v@example.com")) - k2 = throttle.get_cache_key(self._request("2.2.2.2", email="v@example.com")) - assert k1 != k2 # an attacker on another IP cannot consume the victim's bucket - - def test_no_email_falls_back_to_ip_only(self): - key = AuthenticationAccountThrottle().get_cache_key(self._request("9.9.9.9")) - assert "ip:9.9.9.9" in key - assert "email:" not in key diff --git a/apps/api/plane/tests/unit/utils/test_paginator.py b/apps/api/plane/tests/unit/utils/test_paginator.py index 00ec8ccfca..2e2b6a60ec 100644 --- a/apps/api/plane/tests/unit/utils/test_paginator.py +++ b/apps/api/plane/tests/unit/utils/test_paginator.py @@ -123,40 +123,6 @@ class TestPaginateGroupByValidation: assert response.data["grouped_by"] is None -@pytest.mark.unit -class TestGetPerPageBounds: - """get_per_page() must reject non-positive per_page before it reaches the - paginator. A per_page of 0 divides by zero in math.ceil(count / limit) and - a negative per_page slices the queryset with garbage bounds — both would - otherwise surface as an unhandled HTTP 500 (flagged by AppScan as - "Integer Overflow" on the stickies per_page parameter).""" - - @pytest.mark.parametrize("per_page", ["0", "-1", "-1000"]) - def test_non_positive_per_page_raises_parse_error(self, per_page): - request = _make_request(per_page=per_page) - with pytest.raises(ParseError): - BasePaginator().get_per_page(request) - - def test_over_max_per_page_still_rejected(self): - request = _make_request(per_page="5000") - with pytest.raises(ParseError): - BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) - - def test_non_integer_per_page_raises_parse_error(self): - request = _make_request(per_page="abc") - with pytest.raises(ParseError): - BasePaginator().get_per_page(request) - - def test_valid_per_page_passes_through(self): - request = _make_request(per_page="30") - assert BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) == 30 - - def test_per_page_of_one_is_allowed(self): - # The exact lower boundary must be accepted. - request = _make_request(per_page="1") - assert BasePaginator().get_per_page(request) == 1 - - class _ExplodingPaginator: """Fails if constructed — proves the cursor guard rejects BEFORE any paginator runs.""" diff --git a/apps/api/plane/utils/paginator.py b/apps/api/plane/utils/paginator.py index a10a69978b..3ff2921383 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -646,13 +646,6 @@ class BasePaginator: except ValueError: raise ParseError(detail="Invalid per_page parameter.") - # Reject non-positive values before they reach the paginator, where a - # zero limit divides by zero in math.ceil(count / limit) and a negative - # limit slices the queryset with garbage bounds — both surface as an - # unhandled HTTP 500 instead of a clean client error. - if per_page < 1: - raise ParseError(detail="Invalid per_page value. Must be at least 1.") - max_per_page = max(max_per_page, default_per_page) if per_page > max_per_page: raise ParseError(detail=f"Invalid per_page value. Cannot exceed {max_per_page}.")