diff --git a/apps/api/plane/tests/unit/utils/test_order_queryset.py b/apps/api/plane/tests/unit/utils/test_order_queryset.py new file mode 100644 index 00000000000..493fbb8e3d2 --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_order_queryset.py @@ -0,0 +1,65 @@ +# 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 plane.utils.order_queryset import ISSUE_GROUP_BY_ALLOWLIST, sanitize_order_by + + +@pytest.mark.unit +class TestIssueGroupByAllowlist: + """Regression tests for GHSA-wwgj-929g-42cm. + + ISSUE_GROUP_BY_ALLOWLIST must contain exactly the field names that + `issue_group_values()` (plane/utils/grouper.py and + plane/space/utils/grouper.py) knows how to resolve safely. Anything + outside this set must never reach the ORM as a raw field name via + GroupedOffsetPaginator/SubGroupedOffsetPaginator. + """ + + def test_allowlist_matches_known_safe_group_fields(self): + expected = { + "state_id", + "state__group", + "priority", + "labels__id", + "assignees__id", + "issue_module__module_id", + "cycle_id", + "project_id", + "created_by", + "target_date", + "start_date", + } + assert ISSUE_GROUP_BY_ALLOWLIST == frozenset(expected) + + def test_allowlist_rejects_injection_style_fields(self): + # These are exactly the kind of values GHSA-wwgj-929g-42cm's PoC used + # to trigger a 500 / force a blind relational-traversal oracle. + dangerous_values = [ + "not_a_field", + "created_by__password", + "workspace__secret_key", + "assignees__password", + "id", + "", + None, + ] + for value in dangerous_values: + assert value not in ISSUE_GROUP_BY_ALLOWLIST + + +@pytest.mark.unit +class TestSanitizeOrderByStillWorks: + """Sanity check that the pre-existing order_by sanitizer (GHSA-2r95 / + GHSA-w45q) is untouched by this change.""" + + def test_valid_field_passes_through(self): + allowed = frozenset({"created_at", "priority"}) + assert sanitize_order_by("priority", allowed) == "priority" + assert sanitize_order_by("-created_at", allowed) == "-created_at" + + def test_invalid_field_falls_back_to_default(self): + allowed = frozenset({"created_at"}) + assert sanitize_order_by("not_a_field", allowed, default="-created_at") == "-created_at" diff --git a/apps/api/plane/tests/unit/utils/test_paginator.py b/apps/api/plane/tests/unit/utils/test_paginator.py new file mode 100644 index 00000000000..b249f4d184b --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_paginator.py @@ -0,0 +1,123 @@ +# 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 rest_framework.exceptions import ParseError +from rest_framework.request import Request + +from plane.utils.paginator import BasePaginator, Cursor, CursorResult + + +class _StubGroupedPaginator: + """Stand-in for GroupedOffsetPaginator/SubGroupedOffsetPaginator that + records its constructor kwargs without touching the DB/ORM. The security + property under test lives entirely in BasePaginator.paginate() — by the + time a real paginator class would run a query, the field name has + already been validated (or rejected).""" + + def __init__(self, **kwargs): + self.kwargs = kwargs + + def get_result(self, limit, cursor): + return CursorResult( + results=[], + next=Cursor(limit, 1, False, False), + prev=Cursor(limit, -1, True, False), + hits=0, + max_hits=0, + ) + + def process_results(self, results): + return results + + +def _make_request(**params): + django_request = RequestFactory().get("/fake-url/", data=params) + return Request(django_request) + + +@pytest.mark.unit +class TestPaginateGroupByValidation: + """Regression tests for GHSA-wwgj-929g-42cm. + + BasePaginator.paginate() is the single chokepoint all group_by/sub_group_by + call sites (public Spaces endpoint + 5 authenticated issue/cycle/module/ + workspace endpoints) funnel through. It must reject any field name outside + ISSUE_GROUP_BY_ALLOWLIST with a 400 (ParseError), before the value ever + reaches a real paginator's F()/.values()/.order_by()/Window partition_by. + """ + + def test_invalid_group_by_raises_parse_error(self): + request = _make_request(group_by="created_by__password") + with pytest.raises(ParseError): + BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + group_by_field_name="created_by__password", + group_by_fields=[], + count_filter=None, + ) + + def test_invalid_sub_group_by_raises_parse_error(self): + # A valid group_by paired with an invalid sub_group_by must still be + # rejected — the PoC in the advisory used exactly this combination + # (group_by=state_id&sub_group_by=created_by__password). + request = _make_request(group_by="priority", sub_group_by="workspace__secret_key") + with pytest.raises(ParseError): + BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + group_by_field_name="priority", + group_by_fields=[], + sub_group_by_field_name="workspace__secret_key", + sub_group_by_fields=[], + count_filter=None, + ) + + def test_unrecognised_field_never_reaches_paginator_constructor(self): + # Belt-and-braces: assert the stub paginator is never even + # instantiated for a rejected field name. + request = _make_request(group_by="not_a_field") + + class _ExplodingPaginator: + def __init__(self, **kwargs): + raise AssertionError("paginator_cls must not be constructed for an invalid group_by field") + + with pytest.raises(ParseError): + BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_ExplodingPaginator, + group_by_field_name="not_a_field", + group_by_fields=[], + count_filter=None, + ) + + def test_valid_group_by_and_sub_group_by_pass_through(self): + request = _make_request(group_by="priority", sub_group_by="state_id") + response = BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + group_by_field_name="priority", + group_by_fields=[], + sub_group_by_field_name="state_id", + sub_group_by_fields=[], + count_filter=None, + ) + assert response.data["grouped_by"] == "priority" + assert response.data["sub_grouped_by"] == "state_id" + + def test_no_group_by_is_unaffected(self): + # Plain (non-grouped) pagination must not be touched by this fix. + request = _make_request() + response = BasePaginator().paginate( + request=request, + queryset=None, + paginator_cls=_StubGroupedPaginator, + ) + assert response.data["grouped_by"] is None diff --git a/apps/api/plane/utils/order_queryset.py b/apps/api/plane/utils/order_queryset.py index 1ef2b056894..8816fc4c6db 100644 --- a/apps/api/plane/utils/order_queryset.py +++ b/apps/api/plane/utils/order_queryset.py @@ -76,6 +76,27 @@ "updated_at", }) +# --------------------------------------------------------------------------- +# group_by / sub_group_by allowlist for Issue querysets — used by +# GroupedOffsetPaginator / SubGroupedOffsetPaginator (plane/utils/paginator.py), +# which pass the field name straight into F(), .values(), .order_by(), and +# Window partition_by. Prevents unauthenticated ORM field-name injection via +# user-supplied query params (GHSA-wwgj-929g-42cm). +# --------------------------------------------------------------------------- +ISSUE_GROUP_BY_ALLOWLIST = frozenset({ + "state_id", + "state__group", + "priority", + "labels__id", + "assignees__id", + "issue_module__module_id", + "cycle_id", + "project_id", + "created_by", + "target_date", + "start_date", +}) + def sanitize_order_by(value, allowed_fields, default="-created_at"): """Return a safe ordering string derived from *value*. diff --git a/apps/api/plane/utils/paginator.py b/apps/api/plane/utils/paginator.py index 5ae4d38150c..2082041f1ac 100644 --- a/apps/api/plane/utils/paginator.py +++ b/apps/api/plane/utils/paginator.py @@ -16,6 +16,7 @@ from rest_framework.response import Response # Module imports +from plane.utils.order_queryset import ISSUE_GROUP_BY_ALLOWLIST class Cursor: @@ -681,11 +682,22 @@ def paginate( if not paginator: if group_by_field_name: + # Validate against the allowlist before the field name reaches + # F()/.values()/.order_by()/Window partition_by in the grouped + # paginators below — prevents unauthenticated ORM field-name + # injection via user-supplied group_by/sub_group_by query params + # (GHSA-wwgj-929g-42cm). + if group_by_field_name not in ISSUE_GROUP_BY_ALLOWLIST: + raise ParseError(detail=f"Invalid group_by field: {group_by_field_name}") + paginator_kwargs["group_by_field_name"] = group_by_field_name paginator_kwargs["group_by_fields"] = group_by_fields paginator_kwargs["count_filter"] = count_filter if sub_group_by_field_name: + if sub_group_by_field_name not in ISSUE_GROUP_BY_ALLOWLIST: + raise ParseError(detail=f"Invalid sub_group_by field: {sub_group_by_field_name}") + paginator_kwargs["sub_group_by_field_name"] = sub_group_by_field_name paginator_kwargs["sub_group_by_fields"] = sub_group_by_fields