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
12 changes: 11 additions & 1 deletion apps/api/plane/app/views/cycle/issue.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,17 @@ def create(self, request, slug, project_id, cycle_id):
)

# Get all CycleIssues already created
cycle_issues = list(CycleIssue.objects.filter(~Q(cycle_id=cycle_id), issue_id__in=issues))
# Scope to workspace+project to prevent cross-tenant IDOR: without this
# scope, foreign-tenant CycleIssue rows matched by issue_id would be
# reassigned to the caller's cycle (GHSA-4w5x-wc9w-f47x).
cycle_issues = list(
CycleIssue.objects.filter(
~Q(cycle_id=cycle_id),
issue_id__in=issues,
workspace__slug=slug,
project_id=project_id,
)
)
existing_issues = [str(cycle_issue.issue_id) for cycle_issue in cycle_issues]
new_issues = list(set(issues) - set(existing_issues))

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
176 changes: 176 additions & 0 deletions apps/api/plane/tests/contract/app/test_cycle_issue_app.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
# Copyright (c) 2023-present Plane Software, Inc. and contributors
# SPDX-License-Identifier: AGPL-3.0-only
# See the LICENSE file for details.

"""Regression test for GHSA-4w5x-wc9w-f47x.

CycleIssueViewSet.create reassigned any CycleIssue row matched by issue_id to
the caller's cycle without scoping the lookup to the caller's
workspace/project. An ADMIN/MEMBER of their own project could therefore pass a
work-item UUID from a *different* tenant and silently evict the victim's work
item from the victim's cycle (cross-tenant write / BOLA).
"""

from uuid import uuid4

import pytest
from rest_framework import status

from plane.db.models import (
Cycle,
CycleIssue,
Issue,
Project,
ProjectMember,
State,
User,
Workspace,
WorkspaceMember,
)


@pytest.fixture
def attacker_project(db, workspace, create_user):
"""Project + cycle in the attacker's own workspace; attacker is admin."""
project = Project.objects.create(
name="Attacker Project",
identifier="ATK",
workspace=workspace,
created_by=create_user,
)
ProjectMember.objects.create(project=project, member=create_user, role=20, is_active=True)
return project


@pytest.fixture
def attacker_cycle(db, workspace, attacker_project, create_user):
return Cycle.objects.create(
name="Attacker Cycle",
project=attacker_project,
workspace=workspace,
owned_by=create_user,
)


@pytest.fixture
def victim_tenant(db):
"""A completely separate workspace/project/cycle owning a work item that is
already assigned to the victim's own cycle."""
uid = uuid4().hex[:8]
victim_user = User.objects.create(
email=f"victim-{uid}@plane.so",
username=f"victim_{uid}",
first_name="Victim",
last_name="User",
)
victim_ws = Workspace.objects.create(name="Victim WS", owner=victim_user, slug=f"victim-{uid}")
WorkspaceMember.objects.create(workspace=victim_ws, member=victim_user, role=20)
victim_project = Project.objects.create(
name="Victim Project",
identifier="VIC",
workspace=victim_ws,
created_by=victim_user,
)
ProjectMember.objects.create(project=victim_project, member=victim_user, role=20, is_active=True)
state = State.objects.create(
name="Todo", project=victim_project, workspace=victim_ws, group="backlog", default=True
)
victim_issue = Issue.objects.create(
name="Victim Issue",
workspace=victim_ws,
project=victim_project,
state=state,
created_by=victim_user,
)
victim_cycle = Cycle.objects.create(
name="Victim Cycle", project=victim_project, workspace=victim_ws, owned_by=victim_user
)
cycle_issue = CycleIssue.objects.create(
issue=victim_issue,
cycle=victim_cycle,
project=victim_project,
workspace=victim_ws,
created_by=victim_user,
)
return {
"issue": victim_issue,
"cycle": victim_cycle,
"cycle_issue": cycle_issue,
}


@pytest.mark.contract
class TestCycleIssueCrossTenantBOLA:
def get_url(self, workspace_slug, project_id, cycle_id):
return f"/api/workspaces/{workspace_slug}/projects/{project_id}/cycles/{cycle_id}/cycle-issues/"

@pytest.mark.django_db
def test_foreign_tenant_cycle_issue_not_reassigned(
self, session_client, workspace, attacker_project, attacker_cycle, victim_tenant
):
"""The attacker adds a foreign-tenant work-item UUID to their own cycle.

Before the fix the victim's CycleIssue row was reassigned to the
attacker's cycle (cycle_id flipped). After the fix the foreign row is
excluded from the lookup, so it stays in the victim's cycle.
"""
victim_issue = victim_tenant["issue"]
victim_cycle = victim_tenant["cycle"]
victim_cycle_issue = victim_tenant["cycle_issue"]

url = self.get_url(workspace.slug, attacker_project.id, attacker_cycle.id)
response = session_client.post(url, {"issues": [str(victim_issue.id)]}, format="json")

# The endpoint reports success regardless; the security property is that
# the foreign row is untouched.
assert response.status_code in (status.HTTP_201_CREATED, status.HTTP_200_OK), (
f"Got {response.status_code}: {response.data!r}"
)

victim_cycle_issue.refresh_from_db()
assert victim_cycle_issue.cycle_id == victim_cycle.id, (
"Cross-tenant reassignment: victim's CycleIssue was moved to the attacker's cycle"
)
# No CycleIssue for the victim's issue should exist under the attacker's cycle.
assert not CycleIssue.objects.filter(
cycle_id=attacker_cycle.id, issue_id=victim_issue.id
).exists()

@pytest.mark.django_db
def test_same_tenant_reassignment_still_works(
self, session_client, workspace, attacker_project, attacker_cycle, create_user
):
"""A legitimate reassignment within the caller's own project must still
move the issue into the target cycle — the scope guard must not break
the normal flow."""
state = State.objects.create(
name="Todo", project=attacker_project, workspace=workspace, group="backlog", default=True
)
own_issue = Issue.objects.create(
name="Own Issue",
workspace=workspace,
project=attacker_project,
state=state,
created_by=create_user,
)
old_cycle = Cycle.objects.create(
name="Old Cycle", project=attacker_project, workspace=workspace, owned_by=create_user
)
own_cycle_issue = CycleIssue.objects.create(
issue=own_issue,
cycle=old_cycle,
project=attacker_project,
workspace=workspace,
created_by=create_user,
)

url = self.get_url(workspace.slug, attacker_project.id, attacker_cycle.id)
response = session_client.post(url, {"issues": [str(own_issue.id)]}, format="json")

assert response.status_code in (status.HTTP_201_CREATED, status.HTTP_200_OK), (
f"Got {response.status_code}: {response.data!r}"
)
own_cycle_issue.refresh_from_db()
assert own_cycle_issue.cycle_id == attacker_cycle.id, (
"Legitimate same-project reassignment must still move the issue to the target cycle"
)
Loading
Loading