Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-09-02 20:19:49 +02:00
Code Issues Packages Projects Releases Wiki Activity

[SECUR-236] chore: drop overlapping per_page + auth fixes, defer to #9429 / #9335

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Manish Gupta
2026-07-28 18:05:30 +05:30
parent 687483ea79
commit 6bc666abdb
7 changed files with 1 additions and 216 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
apps/api/plane/authentication/rate_limit.py
View File

@@ -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 ("<num>/<period>"),
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.

35
apps/api/plane/authentication/views/app/email.py
View File

@@ -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:

35
apps/api/plane/authentication/views/space/email.py
View File

@@ -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:

3
apps/api/plane/tests/unit/authentication/__init__.py
View File

@@ -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.

54
apps/api/plane/tests/unit/authentication/test_rate_limit.py
View File

@@ -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

34
apps/api/plane/tests/unit/utils/test_paginator.py
View File

@@ -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."""

7
apps/api/plane/utils/paginator.py
View File

@@ -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}.")