Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions apps/api/plane/api/views/issue.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
9 changes: 8 additions & 1 deletion apps/api/plane/api/views/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
96 changes: 96 additions & 0 deletions apps/api/plane/tests/contract/api/test_issues.py
Original file line number Diff line number Diff line change
@@ -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}"
)
45 changes: 45 additions & 0 deletions apps/api/plane/tests/contract/api/test_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
132 changes: 132 additions & 0 deletions apps/api/plane/tests/unit/utils/test_order_by_sanitize.py
Original file line number Diff line number Diff line change
@@ -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/<slug>/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/<slug>/projects/<project_id>/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
)
Loading