From 4376c5a267a993117d80433c9e96372d3df28a76 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 3 Jul 2026 16:01:15 +0530 Subject: [PATCH] [WEB-8017] fix(security): sanitize order_by on external REST API list endpoints Close a partial bypass of WEB-7813 (GHSA-2r95 / GHSA-w45q): the external REST API project-list and work-item-list endpoints passed a raw order_by query parameter to Django's .order_by(). Because Django resolves __-separated relational paths, an attacker could order by sensitive columns on related tables (created_by__password / token / email) to build a blind ordering oracle, or crash the endpoint (HTTP 500) with an unknown field. Route both endpoints through the existing sanitize_order_by() helper with the appropriate allowlist (PROJECT_ORDER_BY_ALLOWLIST, default sort_order; ISSUE_ORDER_BY_ALLOWLIST, default -created_at), mirroring how order_issue_queryset() already sanitizes. Non-allowlisted values collapse to the safe default; legitimate orderings are unchanged. Adds unit tests (allowlist neutralisation + passthrough) and contract tests asserting both endpoints return 200 (not 500) for injected fields; fail-before verified via git stash. Advisory: GHSA-p885-6jpg-cr2p Co-authored-by: Plane AI --- apps/api/plane/api/views/issue.py | 15 +- apps/api/plane/api/views/project.py | 9 +- .../plane/tests/contract/api/test_issues.py | 96 +++++++++++++ .../plane/tests/contract/api/test_projects.py | 45 ++++++ .../unit/utils/test_order_by_sanitize.py | 132 ++++++++++++++++++ 5 files changed, 294 insertions(+), 3 deletions(-) create mode 100644 apps/api/plane/tests/contract/api/test_issues.py create mode 100644 apps/api/plane/tests/unit/utils/test_order_by_sanitize.py diff --git a/apps/api/plane/api/views/issue.py b/apps/api/plane/api/views/issue.py index 02fcfce6f94..60a8d8b479b 100644 --- a/apps/api/plane/api/views/issue.py +++ b/apps/api/plane/api/views/issue.py @@ -80,7 +80,11 @@ ) from plane.settings.storage import S3Storage from plane.utils.path_validator import sanitize_filename -from plane.utils.order_queryset import ACTIVITY_ORDER_BY_ALLOWLIST, sanitize_order_by +from plane.utils.order_queryset import ( + ACTIVITY_ORDER_BY_ALLOWLIST, + ISSUE_ORDER_BY_ALLOWLIST, + sanitize_order_by, +) from plane.bgtasks.storage_metadata_task import get_asset_object_metadata from .base import BaseAPIView from plane.utils.host import base_host @@ -329,7 +333,14 @@ def get(self, request, slug, project_id): priority_order = ["urgent", "high", "medium", "low", "none"] state_order = ["backlog", "unstarted", "started", "completed", "cancelled"] - order_by_param = request.GET.get("order_by", "-created_at") + # Reject any field not in the allowlist before it reaches .order_by(). + # An unrecognised value is replaced with the safe default, preventing + # ORM order_by injection via relational traversal (GHSA-p885-6jpg-cr2p). + order_by_param = sanitize_order_by( + request.GET.get("order_by", "-created_at"), + ISSUE_ORDER_BY_ALLOWLIST, + default="-created_at", + ) issue_queryset = ( self.get_queryset() diff --git a/apps/api/plane/api/views/project.py b/apps/api/plane/api/views/project.py index bc478d1bb10..28ac2f32b81 100644 --- a/apps/api/plane/api/views/project.py +++ b/apps/api/plane/api/views/project.py @@ -41,6 +41,7 @@ from plane.utils.exception_logger import log_exception from .base import BaseAPIView from plane.utils.host import base_host +from plane.utils.order_queryset import PROJECT_ORDER_BY_ALLOWLIST, sanitize_order_by from plane.api.serializers import ( ProjectSerializer, ProjectCreateSerializer, @@ -184,7 +185,13 @@ def get(self, request, slug): ), ) ) - .order_by(request.GET.get("order_by", "sort_order")) + .order_by( + sanitize_order_by( + request.GET.get("order_by", "sort_order"), + PROJECT_ORDER_BY_ALLOWLIST, + default="sort_order", + ) + ) ) return self.paginate( request=request, diff --git a/apps/api/plane/tests/contract/api/test_issues.py b/apps/api/plane/tests/contract/api/test_issues.py new file mode 100644 index 00000000000..cda57b64800 --- /dev/null +++ b/apps/api/plane/tests/contract/api/test_issues.py @@ -0,0 +1,96 @@ +# 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 rest_framework import status + +from plane.db.models import Issue, Project, ProjectMember, State + + +@pytest.fixture +def project(db, workspace, create_user): + """Create a test project with the requesting user as an active member.""" + project = Project.objects.create( + name="Test Project", + identifier="TP", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create( + project=project, + member=create_user, + role=20, # Admin + is_active=True, + ) + return project + + +@pytest.fixture +def state(db, workspace, project): + return State.objects.create( + name="Todo", + project=project, + workspace=workspace, + group="backlog", + default=True, + ) + + +@pytest.fixture +def issue(db, workspace, project, state, create_user): + return Issue.objects.create( + name="Test Issue", + workspace=workspace, + project=project, + state=state, + created_by=create_user, + ) + + +@pytest.mark.contract +class TestIssueListOrderByInjection: + """Regression tests for GHSA-p885-6jpg-cr2p on the work-item list + endpoint: GET /api/v1/workspaces/{slug}/projects/{project_id}/issues/. + + The raw ``order_by`` query parameter fell through the endpoint's hardcoded + branch logic to ``issue_queryset.order_by(order_by_param)``, letting an + attacker order by sensitive related columns (blind oracle) or crash the + endpoint with an unknown field (HTTP 500). The fix sanitizes the parameter + against ISSUE_ORDER_BY_ALLOWLIST before the branch logic runs. + """ + + def get_url(self, workspace_slug, project_id): + return f"/api/v1/workspaces/{workspace_slug}/projects/{project_id}/issues/" + + @pytest.mark.django_db + def test_invalid_order_by_does_not_500(self, api_key_client, workspace, project, issue): + """Unknown field used to raise FieldError → HTTP 500; now sanitized to + the safe default and returns 200 (DoS half of the advisory).""" + url = self.get_url(workspace.slug, project.id) + response = api_key_client.get(url, {"order_by": "not_a_field"}) + + assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}" + + @pytest.mark.django_db + def test_relational_order_by_injection_does_not_500(self, api_key_client, workspace, project, issue): + """Ordering by a related-table column (``created_by__password``) used to + reach ``.order_by()`` raw, forming a blind ordering oracle. It is now + neutralized to the safe default. (Deterministic neutralization is + asserted in tests/unit/utils/test_order_by_sanitize.py.)""" + url = self.get_url(workspace.slug, project.id) + response = api_key_client.get(url, {"order_by": "created_by__password"}) + + assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}" + + @pytest.mark.django_db + def test_legitimate_order_by_still_works(self, api_key_client, workspace, project, issue): + """A valid, allowlisted ordering value continues to return 200 — + the sanitizer must not break legitimate ordering.""" + url = self.get_url(workspace.slug, project.id) + + for value in ["-created_at", "priority", "state__group", "sequence_id"]: + response = api_key_client.get(url, {"order_by": value}) + assert response.status_code == status.HTTP_200_OK, ( + f"order_by={value!r} got {response.status_code}: {response.data!r}" + ) diff --git a/apps/api/plane/tests/contract/api/test_projects.py b/apps/api/plane/tests/contract/api/test_projects.py index 329097e60cd..dd32877118b 100644 --- a/apps/api/plane/tests/contract/api/test_projects.py +++ b/apps/api/plane/tests/contract/api/test_projects.py @@ -183,6 +183,51 @@ def test_model_activity_not_called_on_rollback(self, api_key_client, workspace, # transaction.on_commit() callbacks only fire on a successful commit. mocked_activity.delay.assert_not_called() + @pytest.mark.django_db + def test_list_invalid_order_by_does_not_500(self, api_key_client, workspace, create_user): + """Regression for GHSA-p885-6jpg-cr2p (DoS half). + + An unknown ``order_by`` field used to reach Django's ``.order_by()`` + raw and raise a ``FieldError`` → HTTP 500. After the fix the value is + sanitized to the safe default and the endpoint returns 200. + """ + Project.objects.create( + name="Ordered Project", + identifier="OP", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create(project=Project.objects.get(identifier="OP"), member=create_user, role=20) + + url = self.get_url(workspace.slug) + response = api_key_client.get(url, {"order_by": "not_a_field"}) + + assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}" + + @pytest.mark.django_db + def test_list_relational_order_by_injection_does_not_500(self, api_key_client, workspace, create_user): + """Regression for GHSA-p885-6jpg-cr2p (relational-traversal leak half). + + Ordering by a related-table column (``created_by__password``) used to + reach ``.order_by()`` raw, forming a blind ordering oracle. After the + fix the value is not in ``PROJECT_ORDER_BY_ALLOWLIST`` and is replaced + with the safe default, so it can no longer influence SQL ordering. + (That the payload maps to the default is asserted deterministically in + tests/unit/utils/test_order_by_sanitize.py.) + """ + project = Project.objects.create( + name="Oracle Project", + identifier="OR", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create(project=project, member=create_user, role=20) + + url = self.get_url(workspace.slug) + response = api_key_client.get(url, {"order_by": "created_by__password"}) + + assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}" + @pytest.mark.django_db(transaction=True) def test_response_still_201_when_broker_dispatch_fails(self, api_key_client, workspace, create_user): """If model_activity.delay raises *after* the atomic block has diff --git a/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py b/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py new file mode 100644 index 00000000000..12fde9376bf --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py @@ -0,0 +1,132 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Regression tests for order_by injection on the external REST API. + +Covers GHSA-p885-6jpg-cr2p: the external-API project list and work-item +list endpoints passed a raw ``order_by`` query parameter to Django's +``.order_by()``. Because Django resolves ``__``-separated relational paths, +an attacker could order by sensitive columns on related tables +(``created_by__password``, ``created_by__token``, ``created_by__email``, +``workspace__owner__password`` ...) to build a blind ordering oracle, or +crash the endpoint (HTTP 500) with an unknown field. + +The fix routes both endpoints through ``sanitize_order_by()`` with the +appropriate allowlist. These tests assert that the two allowlists used by +those endpoints neutralise the disclosed attack strings and preserve every +legitimate ordering value. +""" + +import pytest + +from plane.utils.order_queryset import ( + ISSUE_ORDER_BY_ALLOWLIST, + PROJECT_ORDER_BY_ALLOWLIST, + sanitize_order_by, +) + +# Relational-traversal payloads from the advisory PoC plus common variants. +INJECTION_PAYLOADS = [ + "created_by__password", + "created_by__token", + "created_by__email", + "-created_by__password", + "workspace__owner__password", + "updated_by__password", + "not_a_field", + "id; drop table", + "--created_at", # malformed double-dash prefix +] + + +@pytest.mark.unit +class TestProjectOrderBySanitization: + """order_by sanitization for GET /api/v1/workspaces//projects/.""" + + DEFAULT = "sort_order" + + @pytest.mark.parametrize("payload", INJECTION_PAYLOADS) + def test_injection_payload_falls_back_to_default(self, payload): + """Any non-allowlisted / relational value is replaced with the + endpoint's safe default instead of reaching .order_by().""" + assert ( + sanitize_order_by(payload, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == self.DEFAULT + ) + + @pytest.mark.parametrize( + "value", + ["created_at", "updated_at", "name", "network", "sort_order"], + ) + def test_legitimate_ascending_values_pass_through(self, value): + assert ( + sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == value + ) + + @pytest.mark.parametrize( + "value", + ["-created_at", "-updated_at", "-name", "-network", "-sort_order"], + ) + def test_legitimate_descending_values_pass_through(self, value): + assert ( + sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == value + ) + + def test_empty_value_uses_default(self): + assert ( + sanitize_order_by("", PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == self.DEFAULT + ) + + +@pytest.mark.unit +class TestIssueOrderBySanitization: + """order_by sanitization for + GET /api/v1/workspaces//projects//issues/.""" + + DEFAULT = "-created_at" + + @pytest.mark.parametrize("payload", INJECTION_PAYLOADS) + def test_injection_payload_falls_back_to_default(self, payload): + assert ( + sanitize_order_by(payload, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == self.DEFAULT + ) + + @pytest.mark.parametrize( + "value", + [ + "created_at", + "updated_at", + "sequence_id", + "sort_order", + "target_date", + "start_date", + "priority", + "state__name", + "state__group", + "assignees__first_name", + "labels__name", + ], + ) + def test_legitimate_values_pass_through(self, value): + """Every value the endpoint's branch logic special-cases must survive + sanitization, otherwise legitimate ordering would silently break.""" + assert ( + sanitize_order_by(value, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == value + ) + # Descending variant is equally valid. + assert ( + sanitize_order_by(f"-{value}", ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == f"-{value}" + ) + + def test_default_is_preserved_for_missing_param(self): + assert ( + sanitize_order_by(None, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) + == self.DEFAULT + )