mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
[WEB-8012] fix: prevent ORM group_by/sub_group_by injection in issue endpoints (#9347)
Add ISSUE_GROUP_BY_ALLOWLIST and validate group_by_field_name/ sub_group_by_field_name in BasePaginator.paginate() — the single chokepoint all GroupedOffsetPaginator/SubGroupedOffsetPaginator callers funnel through (the unauthenticated public deploy-board endpoint plus 5 GUEST-reachable authenticated endpoints). Invalid fields now raise ParseError (HTTP 400) instead of reaching F()/.values()/.order_by()/Window partition_by as a raw ORM field name, which previously let an anonymous caller crash the endpoint or force a blind relational-traversal oracle (GHSA-wwgj-929g-42cm). Same field-name-injection class as the order_by fix (GHSA-2r95/GHSA-w45q, WEB-7813), which never extended to group_by/sub_group_by. Closes WEB-8012 Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
65
apps/api/plane/tests/unit/utils/test_order_queryset.py
Normal file
65
apps/api/plane/tests/unit/utils/test_order_queryset.py
Normal file
@@ -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"
|
||||
123
apps/api/plane/tests/unit/utils/test_paginator.py
Normal file
123
apps/api/plane/tests/unit/utils/test_paginator.py
Normal file
@@ -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
|
||||
@@ -76,6 +76,27 @@ NOTIFICATION_ORDER_BY_ALLOWLIST = frozenset({
|
||||
"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*.
|
||||
|
||||
@@ -16,6 +16,7 @@ from rest_framework.exceptions import ParseError
|
||||
from rest_framework.response import Response
|
||||
|
||||
# Module imports
|
||||
from plane.utils.order_queryset import ISSUE_GROUP_BY_ALLOWLIST
|
||||
|
||||
|
||||
class Cursor:
|
||||
@@ -681,11 +682,22 @@ class BasePaginator:
|
||||
|
||||
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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user