From 47ab37cadee2554eb24531f265f3dff744a75014 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 28 Jul 2026 14:49:12 +0530 Subject: [PATCH 1/6] [SECUR-236] fix: harden pagination bounds and auth brute-force rate limiting AppScan DAST remediation, ported from the plane-ee fix (#8591) and re-verified against a live plane-ce instance. - paginator: reject non-positive per_page (per_page=0 -> ZeroDivisionError -> HTTP 500) and bound the client-supplied cursor value/offset. The grouped paginators use cursor.value as the per-group page size: a negative value slices the queryset with a negative stop (ValueError -> HTTP 500) and a huge value fetches far more than max_per_page rows per group (cap bypass / DoS). One central guard in BasePaginator.paginate(); regression tests added. - auth: sign-in/sign-up (app + space) were plain Views with no rate limiting. Add the IP-based AuthenticationThrottle check plus a per-account throttle keyed on the normalized email (AuthenticationAccountThrottle). The IP key is bypassable by spoofing X-Forwarded-For (NUM_PROXIES unset); the per-account limiter caps credential guessing against a single account regardless of IP. - project serializer: mark created_by/updated_by read-only (fields="__all__" left them client-writable, allowing project ownership/attribution forgery). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/plane/app/serializers/project.py | 7 +- apps/api/plane/authentication/rate_limit.py | 27 +++++- .../plane/authentication/views/app/email.py | 35 ++++++++ .../plane/authentication/views/space/email.py | 35 ++++++++ .../plane/tests/unit/utils/test_paginator.py | 86 +++++++++++++++++++ apps/api/plane/utils/paginator.py | 18 ++++ 6 files changed, 206 insertions(+), 2 deletions(-) diff --git a/apps/api/plane/app/serializers/project.py b/apps/api/plane/app/serializers/project.py index aef296bc6c2..13e6e7e2e40 100644 --- a/apps/api/plane/app/serializers/project.py +++ b/apps/api/plane/app/serializers/project.py @@ -34,7 +34,12 @@ class ProjectSerializer(BaseSerializer): class Meta: model = Project fields = "__all__" - read_only_fields = ["workspace", "deleted_at"] + # created_by/updated_by are audit fields set server-side by BaseModel.save() + # from the request user; with fields="__all__" they are otherwise client-writable, + # letting a caller forge project ownership/attribution. save() only backfills + # created_by when it is None, so a supplied value would survive — mark them + # read-only so the client value is ignored. + read_only_fields = ["workspace", "deleted_at", "created_by", "updated_by"] def validate_name(self, name): project_id = self.instance.id if self.instance else None diff --git a/apps/api/plane/authentication/rate_limit.py b/apps/api/plane/authentication/rate_limit.py index bfadf82b702..86982968d9e 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, UserRateThrottle +from rest_framework.throttling import AnonRateThrottle, SimpleRateThrottle, UserRateThrottle from rest_framework import status from rest_framework.response import Response @@ -49,6 +49,31 @@ def authentication_throttle_allows(request): return throttle.allow_request(request, None) +class AuthenticationAccountThrottle(SimpleRateThrottle): + """Per-account (submitted email) authentication throttle. + + AuthenticationThrottle keys on DRF get_ident, which honors X-Forwarded-For when + NUM_PROXIES is unset — an attacker can rotate that header to get a fresh bucket per + request and brute-force credentials unthrottled. Bucketing a second limiter by the + normalized email caps guesses against any single account regardless of source IP. + 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. + """ + + scope = "authentication_account" + rate = os.environ.get("AUTHENTICATION_ACCOUNT_RATE_LIMIT", "5/minute") + + def get_cache_key(self, request, view=None): + email = (request.POST.get("email") or "").strip().lower() + ident = f"email:{email}" if email else f"ip:{self.get_ident(request)}" + 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 3d1954875c4..7b0aa00a5d4 100644 --- a/apps/api/plane/authentication/views/app/email.py +++ b/apps/api/plane/authentication/views/app/email.py @@ -10,6 +10,10 @@ # 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 @@ -26,6 +30,22 @@ 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: @@ -135,6 +155,21 @@ def post(self, request): 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 827348cef23..a2afe87f871 100644 --- a/apps/api/plane/authentication/views/space/email.py +++ b/apps/api/plane/authentication/views/space/email.py @@ -11,6 +11,10 @@ # 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 @@ -25,6 +29,22 @@ 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: @@ -110,6 +130,21 @@ def post(self, request): 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/utils/test_paginator.py b/apps/api/plane/tests/unit/utils/test_paginator.py index b249f4d184b..00ec8ccfca6 100644 --- a/apps/api/plane/tests/unit/utils/test_paginator.py +++ b/apps/api/plane/tests/unit/utils/test_paginator.py @@ -121,3 +121,89 @@ def test_no_group_by_is_unaffected(self): paginator_cls=_StubGroupedPaginator, ) 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.""" + + def __init__(self, **kwargs): + raise AssertionError("paginator_cls must not be constructed for an invalid cursor") + + +@pytest.mark.unit +class TestCursorBounds: + """paginate() must reject an out-of-bounds client cursor before it drives slicing. + + The grouped paginators use cursor.value as the per-group page size + (stop = offset + (cursor.value or limit) + 1). A negative value slices the queryset + with a negative stop -> ValueError('Negative indexing is not supported') -> HTTP 500; + a huge value fetches far more than max_per_page rows per group (cap bypass / DoS). + cursor.offset must be non-negative.""" + + @pytest.mark.parametrize("cursor", ["-1:0:0", "1000000:0:0", "20:-1:0"]) + def test_out_of_bounds_cursor_rejected_before_paginator(self, cursor): + request = _make_request(cursor=cursor) + with pytest.raises(ParseError): + BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_ExplodingPaginator, + default_per_page=20, + max_per_page=1000, + ) + + def test_valid_cursor_passes_the_guard(self): + request = _make_request(cursor="20:0:0") + response = BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + default_per_page=20, + max_per_page=1000, + ) + assert response.data["results"] == [] + + def test_cursor_value_at_max_is_allowed(self): + request = _make_request(cursor="1000:0:0") + response = BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + default_per_page=20, + max_per_page=1000, + ) + assert response.data["results"] == [] diff --git a/apps/api/plane/utils/paginator.py b/apps/api/plane/utils/paginator.py index 2082041f1ac..a10a69978bc 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -646,6 +646,13 @@ def get_per_page(self, request, default_per_page=1000, max_per_page=1000): 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}.") @@ -680,6 +687,17 @@ def paginate( except ValueError: raise ParseError(detail="Invalid cursor parameter.") + # Bound the client-supplied cursor before it drives any slicing. The grouped + # paginators use cursor.value as the per-group page size + # (stop = offset + (cursor.value or limit) + 1). Left unbounded, a negative value + # slices the queryset with a negative stop -> "Negative indexing is not supported" + # (HTTP 500), and a huge value fetches far more than max_per_page rows per group + # (max_per_page cap bypass / resource-exhaustion DoS). cursor.offset is the page + # index and must be non-negative. + effective_max_per_page = max(max_per_page, default_per_page) + if not (0 <= input_cursor.value <= effective_max_per_page) or input_cursor.offset < 0: + raise ParseError(detail="Invalid cursor parameter.") + if not paginator: if group_by_field_name: # Validate against the allowlist before the field name reaches From 3257ee9305fc14f893a55325c0c4d36615d82c11 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 28 Jul 2026 15:12:13 +0530 Subject: [PATCH 2/6] =?UTF-8?q?[SECUR-236]=20fix:=20address=20CodeRabbit?= =?UTF-8?q?=20=E2=80=94=20composite=20account=20throttle=20key=20+=20rate?= =?UTF-8?q?=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AuthenticationAccountThrottle keyed on (email + client IP) instead of email alone. An email-only key let anyone lock a victim out of their own account by spamming their address from other IPs (self-inflicted lockout DoS). Combining with the client IP prevents that while still capping single-source guessing. - Guard the AUTHENTICATION_ACCOUNT_RATE_LIMIT env value: a malformed rate would raise in DRF parse_rate() on every auth POST (throttle is instantiated per request), taking authentication down instance-wide. Falls back to the default. - Add regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/plane/authentication/rate_limit.py | 42 +++++++++++---- .../tests/unit/authentication/__init__.py | 0 .../unit/authentication/test_rate_limit.py | 54 +++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 apps/api/plane/tests/unit/authentication/__init__.py create mode 100644 apps/api/plane/tests/unit/authentication/test_rate_limit.py diff --git a/apps/api/plane/authentication/rate_limit.py b/apps/api/plane/authentication/rate_limit.py index 86982968d9e..a35a074525d 100644 --- a/apps/api/plane/authentication/rate_limit.py +++ b/apps/api/plane/authentication/rate_limit.py @@ -49,23 +49,45 @@ 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 (submitted email) authentication throttle. - - AuthenticationThrottle keys on DRF get_ident, which honors X-Forwarded-For when - NUM_PROXIES is unset — an attacker can rotate that header to get a fresh bucket per - request and brute-force credentials unthrottled. Bucketing a second limiter by the - normalized email caps guesses against any single account regardless of source IP. - 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. + """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 = os.environ.get("AUTHENTICATION_ACCOUNT_RATE_LIMIT", "5/minute") + 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}" if email else f"ip:{self.get_ident(request)}" + ident = f"email:{email}|ip:{ip}" if email else f"ip:{ip}" return self.cache_format % {"scope": self.scope, "ident": ident} diff --git a/apps/api/plane/tests/unit/authentication/__init__.py b/apps/api/plane/tests/unit/authentication/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/apps/api/plane/tests/unit/authentication/test_rate_limit.py b/apps/api/plane/tests/unit/authentication/test_rate_limit.py new file mode 100644 index 00000000000..7b62242146a --- /dev/null +++ b/apps/api/plane/tests/unit/authentication/test_rate_limit.py @@ -0,0 +1,54 @@ +# 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 From 687483ea79c4100bb85e13cd661c45ae3c955cb5 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 28 Jul 2026 15:31:40 +0530 Subject: [PATCH 3/6] chore: add copyright header to authentication test package __init__ Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/plane/tests/unit/authentication/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/api/plane/tests/unit/authentication/__init__.py b/apps/api/plane/tests/unit/authentication/__init__.py index e69de29bb2d..fcc34a703d7 100644 --- a/apps/api/plane/tests/unit/authentication/__init__.py +++ b/apps/api/plane/tests/unit/authentication/__init__.py @@ -0,0 +1,3 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. From 6bc666abdb8a79ca8428580fb71e9f59df56e893 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 28 Jul 2026 18:05:30 +0530 Subject: [PATCH 4/6] [SECUR-236] chore: drop overlapping per_page + auth fixes, defer to #9429 / #9335 Two parts of this PR fully overlapped existing open PRs against preview, so per the "drop the most recent on full overlap" call they are removed here: - per_page non-positive guard (get_per_page) -> covered identically by #9429. - password sign-in/sign-up rate limiting -> covered (more cleanly, via a decorator) by #9335 (GHSA-349j-pjw5-67q4). Reverted rate_limit.py and email.py (app + space) and removed the auth unit tests. This PR now carries only its unique, non-overlapping fixes: - grouped-paginator cursor bound in paginate() (cap-bypass DoS + negative-slice 500 that #9429 does not address), with TestCursorBounds. - Project created_by/updated_by read-only (mass-assignment). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/plane/authentication/rate_limit.py | 49 +---------------- .../plane/authentication/views/app/email.py | 35 ------------ .../plane/authentication/views/space/email.py | 35 ------------ .../tests/unit/authentication/__init__.py | 3 -- .../unit/authentication/test_rate_limit.py | 54 ------------------- .../plane/tests/unit/utils/test_paginator.py | 34 ------------ apps/api/plane/utils/paginator.py | 7 --- 7 files changed, 1 insertion(+), 216 deletions(-) delete mode 100644 apps/api/plane/tests/unit/authentication/__init__.py delete mode 100644 apps/api/plane/tests/unit/authentication/test_rate_limit.py diff --git a/apps/api/plane/authentication/rate_limit.py b/apps/api/plane/authentication/rate_limit.py index a35a074525d..bfadf82b702 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 7b0aa00a5d4..3d1954875c4 100644 --- a/apps/api/plane/authentication/views/app/email.py +++ b/apps/api/plane/authentication/views/app/email.py @@ -10,10 +10,6 @@ # 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 @@ 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 @@ def post(self, request): 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 a2afe87f871..827348cef23 100644 --- a/apps/api/plane/authentication/views/space/email.py +++ b/apps/api/plane/authentication/views/space/email.py @@ -11,10 +11,6 @@ # 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 @@ 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 @@ def post(self, request): 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 fcc34a703d7..00000000000 --- 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 7b62242146a..00000000000 --- 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 00ec8ccfca6..2e2b6a60ec8 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 @@ def test_no_group_by_is_unaffected(self): 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 a10a69978bc..3ff29213834 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -646,13 +646,6 @@ def get_per_page(self, request, default_per_page=1000, max_per_page=1000): 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}.") From 33977a3bf8f49b8b9167ec12859edf1c0888afc0 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 27 Aug 2026 10:34:27 +0530 Subject: [PATCH 5/6] [SECUR-236] fix: close remaining per_page crash and view/cycle mass-assignment gaps Code review on this PR found its own vulnerability classes left open on adjacent code it didn't touch: - get_per_page() bounded per_page against max_per_page but never rejected a negative value, so it still reached OffsetPaginator.get_result() and produced a negative slice bound -- the same unhandled Django negative-index crash (HTTP 500) the cursor-bound fix in this PR closes, just via a different input. Reject negative per_page the same way an over-large one is already rejected, and dedupe the now-twice-computed effective max_per_page. - ProjectSerializer was fixed to mark created_by/updated_by read-only so a client can't forge attribution via PATCH (BaseModel.save() never re-stamps created_by on update). IssueViewSerializer and CycleWriteSerializer had the identical gap and are fixed the same way. - Corrected the ProjectSerializer comment: save() stamps created_by unconditionally on create, not just when it was previously None -- the real gap is that update() never touches it at all. Co-authored-by: Plane AI --- apps/api/plane/app/serializers/cycle.py | 14 ++- apps/api/plane/app/serializers/project.py | 7 +- apps/api/plane/app/serializers/view.py | 7 ++ .../unit/serializers/test_mass_assignment.py | 109 ++++++++++++++++++ .../plane/tests/unit/utils/test_paginator.py | 37 ++++++ apps/api/plane/utils/paginator.py | 15 ++- 6 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 apps/api/plane/tests/unit/serializers/test_mass_assignment.py diff --git a/apps/api/plane/app/serializers/cycle.py b/apps/api/plane/app/serializers/cycle.py index afdc58116bf..61c7e6149be 100644 --- a/apps/api/plane/app/serializers/cycle.py +++ b/apps/api/plane/app/serializers/cycle.py @@ -40,7 +40,19 @@ def validate(self, data): class Meta: model = Cycle fields = "__all__" - read_only_fields = ["workspace", "project", "owned_by", "archived_at"] + # created_by/updated_by are audit fields set server-side by BaseModel.save() + # from the request user; with fields="__all__" they are otherwise + # client-writable, letting a caller forge attribution on the cycle. save() + # never re-stamps them on update, so a supplied value would survive — mark + # them read-only so the client value is ignored. + read_only_fields = [ + "workspace", + "project", + "owned_by", + "archived_at", + "created_by", + "updated_by", + ] class CycleSerializer(BaseSerializer): diff --git a/apps/api/plane/app/serializers/project.py b/apps/api/plane/app/serializers/project.py index 13e6e7e2e40..939e6724cda 100644 --- a/apps/api/plane/app/serializers/project.py +++ b/apps/api/plane/app/serializers/project.py @@ -36,9 +36,10 @@ class Meta: fields = "__all__" # created_by/updated_by are audit fields set server-side by BaseModel.save() # from the request user; with fields="__all__" they are otherwise client-writable, - # letting a caller forge project ownership/attribution. save() only backfills - # created_by when it is None, so a supplied value would survive — mark them - # read-only so the client value is ignored. + # letting a caller forge project ownership/attribution. save() stamps created_by + # unconditionally on create, but on update it only stamps updated_by and never + # touches created_by — so a client-supplied created_by on a PATCH would survive + # untouched. Mark them read-only so the client value is ignored. read_only_fields = ["workspace", "deleted_at", "created_by", "updated_by"] def validate_name(self, name): diff --git a/apps/api/plane/app/serializers/view.py b/apps/api/plane/app/serializers/view.py index 72f72ff71b2..5331c50acd2 100644 --- a/apps/api/plane/app/serializers/view.py +++ b/apps/api/plane/app/serializers/view.py @@ -59,6 +59,11 @@ class IssueViewSerializer(DynamicBaseSerializer): class Meta: model = IssueView fields = "__all__" + # created_by/updated_by are audit fields set server-side by BaseModel.save() + # from the request user; with fields="__all__" they are otherwise + # client-writable, letting a caller forge attribution on the view. save() + # never re-stamps them on update, so a supplied value would survive — mark + # them read-only so the client value is ignored. read_only_fields = [ "workspace", "project", @@ -66,6 +71,8 @@ class Meta: "owned_by", "access", "is_locked", + "created_by", + "updated_by", ] def create(self, validated_data): diff --git a/apps/api/plane/tests/unit/serializers/test_mass_assignment.py b/apps/api/plane/tests/unit/serializers/test_mass_assignment.py new file mode 100644 index 00000000000..5481dc291ee --- /dev/null +++ b/apps/api/plane/tests/unit/serializers/test_mass_assignment.py @@ -0,0 +1,109 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Regression tests: created_by/updated_by must not be client-forgeable. + +BaseModel.save() stamps created_by/updated_by from the current (crum) request +user; on update it only ever re-stamps updated_by, never created_by. With +fields="__all__" and no read_only_fields entry, created_by (and, on update, +updated_by) are ordinary client-writable serializer fields, so a PATCH payload +can forge who a view/cycle is attributed to. These tests mirror the same gap +ProjectSerializer was already fixed for, applied to IssueViewSerializer and +CycleWriteSerializer. +""" + +import pytest +from crum import set_current_user + +from plane.app.serializers.cycle import CycleWriteSerializer +from plane.app.serializers.view import IssueViewSerializer +from plane.db.models import Cycle, IssueView, Project, User + + +@pytest.fixture +def current_user(create_user): + """Simulate an authenticated request by populating crum's thread-local + current user for the duration of the test — BaseModel.save() reads this + to decide who to stamp as updated_by.""" + set_current_user(create_user) + yield create_user + set_current_user(None) + + +@pytest.mark.unit +class TestIssueViewSerializerMassAssignment: + """created_by/updated_by must be read-only on IssueViewSerializer.""" + + @pytest.mark.django_db + def test_created_by_is_not_forgeable_via_update(self, db, workspace, current_user): + project = Project.objects.create(name="Test Project", identifier="TESTV", workspace=workspace) + attacker = User.objects.create(email="attacker-view@plane.so", username="attacker_view") + + view = IssueView.objects.create( + name="Original View", + query={}, + project=project, + workspace=workspace, + owned_by=current_user, + ) + # BaseModel.save() stamped created_by from the crum current user on + # create; pin it explicitly so the assertion below doesn't depend on + # that behaviour. + IssueView.objects.filter(pk=view.pk).update(created_by=current_user) + view.refresh_from_db() + + serializer = IssueViewSerializer( + instance=view, + data={"name": "Renamed by attacker", "created_by": attacker.id, "updated_by": attacker.id}, + partial=True, + ) + assert serializer.is_valid(), serializer.errors + assert "created_by" not in serializer.validated_data + assert "updated_by" not in serializer.validated_data + + saved = serializer.save() + saved.refresh_from_db() + + assert saved.name == "Renamed by attacker" + assert saved.created_by_id == current_user.id + assert saved.created_by_id != attacker.id + # updated_by is legitimately stamped from the request user, but must + # not be forced to the attacker-supplied value either. + assert saved.updated_by_id != attacker.id + + +@pytest.mark.unit +class TestCycleWriteSerializerMassAssignment: + """created_by/updated_by must be read-only on CycleWriteSerializer.""" + + @pytest.mark.django_db + def test_created_by_is_not_forgeable_via_update(self, db, workspace, current_user): + project = Project.objects.create(name="Test Project", identifier="TESTC", workspace=workspace) + attacker = User.objects.create(email="attacker-cycle@plane.so", username="attacker_cycle") + + cycle = Cycle.objects.create( + name="Original Cycle", + project=project, + workspace=workspace, + owned_by=current_user, + ) + Cycle.objects.filter(pk=cycle.pk).update(created_by=current_user) + cycle.refresh_from_db() + + serializer = CycleWriteSerializer( + instance=cycle, + data={"name": "Renamed by attacker", "created_by": attacker.id, "updated_by": attacker.id}, + partial=True, + ) + assert serializer.is_valid(), serializer.errors + assert "created_by" not in serializer.validated_data + assert "updated_by" not in serializer.validated_data + + saved = serializer.save() + saved.refresh_from_db() + + assert saved.name == "Renamed by attacker" + assert saved.created_by_id == current_user.id + assert saved.created_by_id != attacker.id + assert saved.updated_by_id != attacker.id diff --git a/apps/api/plane/tests/unit/utils/test_paginator.py b/apps/api/plane/tests/unit/utils/test_paginator.py index 2e2b6a60ec8..623a71f893a 100644 --- a/apps/api/plane/tests/unit/utils/test_paginator.py +++ b/apps/api/plane/tests/unit/utils/test_paginator.py @@ -173,3 +173,40 @@ def test_cursor_value_at_max_is_allowed(self): max_per_page=1000, ) assert response.data["results"] == [] + + +@pytest.mark.unit +class TestGetPerPageNegativeRejected: + """A negative per_page must be rejected, not just a too-large one. + + OffsetPaginator.get_result() slices the queryset with + queryset[offset : offset + limit]; get_per_page() previously only checked + per_page against max_per_page (an upper bound), so a negative per_page sailed + through untouched, reached the slice as a negative stop, and raised Django's + unhandled "Negative indexing is not supported" -> HTTP 500. This is the same + crash class TestCursorBounds closes for the cursor-driven grouped paginators, + reachable here on every non-grouped paginated endpoint via a plain query + param.""" + + @pytest.mark.parametrize("per_page", [-1, -50, -1000]) + def test_negative_per_page_rejected_by_get_per_page(self, per_page): + request = _make_request(per_page=str(per_page)) + with pytest.raises(ParseError): + BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) + + def test_negative_per_page_rejected_before_paginator_runs(self): + request = _make_request(per_page="-1") + with pytest.raises(ParseError): + BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_ExplodingPaginator, + default_per_page=20, + max_per_page=1000, + ) + + def test_zero_per_page_is_still_allowed(self): + # 0 is a valid (if degenerate) page size, not a negative one — must not + # be rejected by this guard. + request = _make_request(per_page="0") + assert BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) == 0 diff --git a/apps/api/plane/utils/paginator.py b/apps/api/plane/utils/paginator.py index 3ff29213834..63ae874d7a9 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -646,12 +646,23 @@ def get_per_page(self, request, default_per_page=1000, max_per_page=1000): except ValueError: raise ParseError(detail="Invalid per_page parameter.") - max_per_page = max(max_per_page, default_per_page) + max_per_page = self._effective_max_per_page(max_per_page, default_per_page) + # A negative per_page reaches OffsetPaginator.get_result() unmodified and + # produces a negative slice bound (queryset[offset : offset + per_page]), + # which raises Django's unhandled "Negative indexing is not supported" + # (HTTP 500) — the same crash class the cursor-bound check below closes, + # just reachable on every paginated endpoint via a plain query param. + if per_page < 0: + raise ParseError(detail="Invalid per_page value. Cannot be negative.") if per_page > max_per_page: raise ParseError(detail=f"Invalid per_page value. Cannot exceed {max_per_page}.") return per_page + @staticmethod + def _effective_max_per_page(max_per_page, default_per_page): + return max(max_per_page, default_per_page) + def paginate( self, request, @@ -687,7 +698,7 @@ def paginate( # (HTTP 500), and a huge value fetches far more than max_per_page rows per group # (max_per_page cap bypass / resource-exhaustion DoS). cursor.offset is the page # index and must be non-negative. - effective_max_per_page = max(max_per_page, default_per_page) + effective_max_per_page = self._effective_max_per_page(max_per_page, default_per_page) if not (0 <= input_cursor.value <= effective_max_per_page) or input_cursor.offset < 0: raise ParseError(detail="Invalid cursor parameter.") From 26db9ea57b2261c2b5c4dcb4ea37507ad31b6dba Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 27 Aug 2026 10:48:38 +0530 Subject: [PATCH 6/6] [SECUR-236] fix: reject per_page=0 in get_per_page, not just negatives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The negative-per_page guard added earlier in this PR let per_page=0 through unchanged. A zero value still reaches OffsetPaginator.get_result(), where math.ceil(count / limit) divides by the limit and raises an unhandled ZeroDivisionError (HTTP 500) — the same unhandled-crash class this PR closes for negative values, just with a different trigger. Tighten the guard to per_page <= 0 and correct the regression test that previously asserted 0 was accepted. Co-authored-by: Plane AI --- .../plane/tests/unit/utils/test_paginator.py | 26 +++++++++---------- apps/api/plane/utils/paginator.py | 17 +++++++----- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/apps/api/plane/tests/unit/utils/test_paginator.py b/apps/api/plane/tests/unit/utils/test_paginator.py index 623a71f893a..cb67b07c60b 100644 --- a/apps/api/plane/tests/unit/utils/test_paginator.py +++ b/apps/api/plane/tests/unit/utils/test_paginator.py @@ -176,8 +176,9 @@ def test_cursor_value_at_max_is_allowed(self): @pytest.mark.unit -class TestGetPerPageNegativeRejected: - """A negative per_page must be rejected, not just a too-large one. +class TestGetPerPageNonPositiveRejected: + """A non-positive per_page (negative or zero) must be rejected, not just a + too-large one. OffsetPaginator.get_result() slices the queryset with queryset[offset : offset + limit]; get_per_page() previously only checked @@ -186,16 +187,21 @@ class TestGetPerPageNegativeRejected: unhandled "Negative indexing is not supported" -> HTTP 500. This is the same crash class TestCursorBounds closes for the cursor-driven grouped paginators, reachable here on every non-grouped paginated endpoint via a plain query - param.""" + param. - @pytest.mark.parametrize("per_page", [-1, -50, -1000]) - def test_negative_per_page_rejected_by_get_per_page(self, per_page): + A per_page of exactly 0 has the same failure mode: it reaches + OffsetPaginator.get_result() with limit=0, where math.ceil(count / limit) + raises an unhandled ZeroDivisionError -> HTTP 500.""" + + @pytest.mark.parametrize("per_page", [-1, -50, -1000, 0]) + def test_non_positive_per_page_rejected_by_get_per_page(self, per_page): request = _make_request(per_page=str(per_page)) with pytest.raises(ParseError): BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) - def test_negative_per_page_rejected_before_paginator_runs(self): - request = _make_request(per_page="-1") + @pytest.mark.parametrize("per_page", ["-1", "0"]) + def test_non_positive_per_page_rejected_before_paginator_runs(self, per_page): + request = _make_request(per_page=per_page) with pytest.raises(ParseError): BasePaginator().paginate( request=request, @@ -204,9 +210,3 @@ def test_negative_per_page_rejected_before_paginator_runs(self): default_per_page=20, max_per_page=1000, ) - - def test_zero_per_page_is_still_allowed(self): - # 0 is a valid (if degenerate) page size, not a negative one — must not - # be rejected by this guard. - request = _make_request(per_page="0") - assert BasePaginator().get_per_page(request, default_per_page=20, max_per_page=1000) == 0 diff --git a/apps/api/plane/utils/paginator.py b/apps/api/plane/utils/paginator.py index 63ae874d7a9..21bb43f1a5e 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -647,13 +647,16 @@ def get_per_page(self, request, default_per_page=1000, max_per_page=1000): raise ParseError(detail="Invalid per_page parameter.") max_per_page = self._effective_max_per_page(max_per_page, default_per_page) - # A negative per_page reaches OffsetPaginator.get_result() unmodified and - # produces a negative slice bound (queryset[offset : offset + per_page]), - # which raises Django's unhandled "Negative indexing is not supported" - # (HTTP 500) — the same crash class the cursor-bound check below closes, - # just reachable on every paginated endpoint via a plain query param. - if per_page < 0: - raise ParseError(detail="Invalid per_page value. Cannot be negative.") + # A non-positive per_page reaches OffsetPaginator.get_result() unmodified. + # A negative value produces a negative slice bound + # (queryset[offset : offset + per_page]), which raises Django's unhandled + # "Negative indexing is not supported" (HTTP 500) — the same crash class + # the cursor-bound check below closes, just reachable on every paginated + # endpoint via a plain query param. A zero value reaches + # math.ceil(count / limit) with limit=0 and raises an unhandled + # ZeroDivisionError (HTTP 500) instead. + if per_page <= 0: + raise ParseError(detail="Invalid per_page value. Must be greater than zero.") if per_page > max_per_page: raise ParseError(detail=f"Invalid per_page value. Cannot exceed {max_per_page}.")