mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
[WEB-8352] fix(security): scope SubIssuesEndpoint to the URL project (#9466)
* [WEB-8352] fix(security): scope SubIssuesEndpoint to the URL project (GHSA-gxhv-fw9x-2pg3) SubIssuesEndpoint is guarded only by ProjectEntityPermission, which verifies the caller belongs to the URL project_id but not that the path issue_id lives in that project. Both handlers then resolved issues without a project scope: - GET filtered sub-issues by parent_id + workspace__slug only, leaking the names/priorities/assignees/dates of another project's sub-issues (read IDOR). - POST loaded the parent by bare pk (no workspace/project scope) and filtered the moved sub-issues by workspace__slug only, letting any project member re-parent issues from other projects/workspaces (write IDOR). Scope the parent lookup and both sub-issue querysets to the URL project_id (and bind the parent to the workspace), returning 404 when the parent is not in the caller's project. Adds 5 contract tests (3 security, 2 positive controls); fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [WEB-8352] fix: dispatch sub-issue activity only for project-scoped issues (CodeRabbit/Copilot #9466) The DB update + response were scoped to the URL project, but the activity loop still iterated the raw caller-supplied sub_issue_ids. A cross-project id (excluded from the re-parent) would still fire issue_activity.delay, whose task does an unscoped Issue.objects.get and bumps updated_at — touching a foreign issue and creating a bogus activity row. Dispatch from the project-scoped sub_issues (scoped_sub_issue_ids) instead. Strengthened the test to assert the foreign issue is absent from the response body (sub_issues / state_distribution) and that no activity is dispatched for it (mock). Fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(security): drop advisory identifiers from code comments Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -35,8 +35,14 @@ class SubIssuesEndpoint(BaseAPIView):
|
||||
|
||||
@method_decorator(gzip_page)
|
||||
def get(self, request, slug, project_id, issue_id):
|
||||
# SECURITY: scope the parent lookup to the URL project. ProjectEntityPermission
|
||||
# only checks that the caller belongs to `project_id`, not that `issue_id` lives
|
||||
# in it, so an unscoped filter leaks sub-issue metadata across projects in the
|
||||
# same workspace.
|
||||
sub_issues = (
|
||||
Issue.issue_objects.filter(parent_id=issue_id, workspace__slug=slug)
|
||||
Issue.issue_objects.filter(
|
||||
parent_id=issue_id, workspace__slug=slug, project_id=project_id
|
||||
)
|
||||
.annotate(
|
||||
cycle_id=Subquery(
|
||||
CycleIssue.objects.filter(issue=OuterRef("id"), deleted_at__isnull=True).values("cycle_id")[:1]
|
||||
@@ -202,7 +208,17 @@ class SubIssuesEndpoint(BaseAPIView):
|
||||
|
||||
# Assign multiple sub issues
|
||||
def post(self, request, slug, project_id, issue_id):
|
||||
parent_issue = Issue.issue_objects.get(pk=issue_id)
|
||||
# SECURITY: bind the parent issue to the URL workspace + project. A bare
|
||||
# pk lookup let any project member re-parent issues under a parent in a
|
||||
# different project/workspace.
|
||||
parent_issue = Issue.issue_objects.filter(
|
||||
pk=issue_id, workspace__slug=slug, project_id=project_id
|
||||
).first()
|
||||
if parent_issue is None:
|
||||
return Response(
|
||||
{"error": "Parent issue not found"},
|
||||
status=status.HTTP_404_NOT_FOUND,
|
||||
)
|
||||
sub_issue_ids = request.data.get("sub_issue_ids", [])
|
||||
|
||||
if not len(sub_issue_ids):
|
||||
@@ -211,15 +227,25 @@ class SubIssuesEndpoint(BaseAPIView):
|
||||
status=status.HTTP_400_BAD_REQUEST,
|
||||
)
|
||||
|
||||
# Scope to workspace to prevent cross-tenant IDOR
|
||||
sub_issues = Issue.issue_objects.filter(id__in=sub_issue_ids, workspace__slug=slug)
|
||||
# Scope to workspace + project to prevent cross-project/cross-tenant IDOR
|
||||
sub_issues = Issue.issue_objects.filter(
|
||||
id__in=sub_issue_ids, workspace__slug=slug, project_id=project_id
|
||||
)
|
||||
|
||||
for sub_issue in sub_issues:
|
||||
sub_issue.parent = parent_issue
|
||||
|
||||
_ = Issue.objects.bulk_update(sub_issues, ["parent"], batch_size=10)
|
||||
|
||||
updated_sub_issues = Issue.issue_objects.filter(id__in=sub_issue_ids).annotate(state_group=F("state__group"))
|
||||
# Only the issues that were actually re-parented — i.e. the project-scoped
|
||||
# `sub_issues`, not the raw caller-supplied ids. Otherwise a cross-project id
|
||||
# (excluded from the update above) would still fire issue_activity, whose task
|
||||
# does an unscoped Issue.objects.get and bumps updated_at on a foreign issue.
|
||||
scoped_sub_issue_ids = [str(sub_issue.id) for sub_issue in sub_issues]
|
||||
|
||||
updated_sub_issues = Issue.issue_objects.filter(
|
||||
id__in=scoped_sub_issue_ids, workspace__slug=slug, project_id=project_id
|
||||
).annotate(state_group=F("state__group"))
|
||||
|
||||
# Track the issue
|
||||
_ = [
|
||||
@@ -227,14 +253,14 @@ class SubIssuesEndpoint(BaseAPIView):
|
||||
type="issue.activity.updated",
|
||||
requested_data=json.dumps({"parent": str(issue_id)}),
|
||||
actor_id=str(request.user.id),
|
||||
issue_id=str(sub_issue_id),
|
||||
issue_id=sub_issue_id,
|
||||
project_id=str(project_id),
|
||||
current_instance=json.dumps({"parent": str(sub_issue_id)}),
|
||||
current_instance=json.dumps({"parent": sub_issue_id}),
|
||||
epoch=int(timezone.now().timestamp()),
|
||||
notification=True,
|
||||
origin=base_host(request=request, is_app=True),
|
||||
)
|
||||
for sub_issue_id in sub_issue_ids
|
||||
for sub_issue_id in scoped_sub_issue_ids
|
||||
]
|
||||
|
||||
# create's a dict with state group name with their respective issue id's
|
||||
|
||||
@@ -0,0 +1,224 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""Contract tests for ``SubIssuesEndpoint`` cross-project scoping.
|
||||
|
||||
``SubIssuesEndpoint``
|
||||
(``/workspaces/<slug>/projects/<project_id>/issues/<issue_id>/sub-issues/``) is
|
||||
guarded only by ``ProjectEntityPermission``, which checks that the caller is a
|
||||
member of the URL ``project_id`` — not that ``issue_id`` belongs to it. Both
|
||||
handlers then resolved issues without scoping to the URL project:
|
||||
|
||||
* GET filtered sub-issues by ``parent_id`` + ``workspace__slug`` only, leaking
|
||||
the titles/metadata of another project's sub-issues (read IDOR).
|
||||
* POST loaded the parent by bare ``pk`` (no workspace/project scope) and filtered
|
||||
the moved sub-issues by ``workspace__slug`` only, letting a member re-parent
|
||||
issues from other projects/workspaces (write IDOR).
|
||||
|
||||
The fix scopes every lookup to the URL ``project_id`` (and binds the parent to
|
||||
the workspace), so a caller can only ever touch sub-issues of the project they
|
||||
are actually a member of.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
from rest_framework import status
|
||||
|
||||
from plane.db.models import (
|
||||
Issue,
|
||||
Project,
|
||||
ProjectMember,
|
||||
)
|
||||
|
||||
SUB_ISSUES_URL = (
|
||||
"/api/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/sub-issues/"
|
||||
)
|
||||
|
||||
|
||||
def _make_issue(name, project, workspace, author, parent=None):
|
||||
"""Create an issue with a deterministic ``created_by``.
|
||||
|
||||
``BaseModel.save`` auto-sets ``created_by`` from the current request user
|
||||
(None/anonymous under tests), so a ``created_by=`` kwarg to ``create`` is
|
||||
overwritten. Passing ``created_by_id`` to ``save`` sets it explicitly.
|
||||
"""
|
||||
issue = Issue(name=name, project=project, workspace=workspace, parent=parent)
|
||||
issue.save(created_by_id=author.id)
|
||||
return issue
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def project_a(db, workspace, create_user):
|
||||
"""The project the caller is a member of (the URL project)."""
|
||||
project = Project.objects.create(
|
||||
name="Project A",
|
||||
identifier="PA",
|
||||
workspace=workspace,
|
||||
created_by=create_user,
|
||||
)
|
||||
ProjectMember.objects.create(
|
||||
project=project, member=create_user, workspace=workspace, role=20
|
||||
)
|
||||
return project
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def project_b(db, workspace, create_user):
|
||||
"""A sibling project in the same workspace the caller is NOT a member of."""
|
||||
return Project.objects.create(
|
||||
name="Project B",
|
||||
identifier="PB",
|
||||
workspace=workspace,
|
||||
created_by=create_user,
|
||||
)
|
||||
|
||||
|
||||
# --- Project B (victim) issues -------------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def parent_b(db, workspace, project_b, create_user):
|
||||
return _make_issue("B parent", project_b, workspace, create_user)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def sub_b(db, workspace, project_b, parent_b, create_user):
|
||||
return _make_issue("B sub-issue", project_b, workspace, create_user, parent=parent_b)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def orphan_b(db, workspace, project_b, create_user):
|
||||
"""A standalone (unparented) issue in project B — target of a write IDOR."""
|
||||
return _make_issue("B orphan", project_b, workspace, create_user)
|
||||
|
||||
|
||||
# --- Project A (caller's) issues ----------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def parent_a(db, workspace, project_a, create_user):
|
||||
return _make_issue("A parent", project_a, workspace, create_user)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def sub_a(db, workspace, project_a, parent_a, create_user):
|
||||
return _make_issue("A sub-issue", project_a, workspace, create_user, parent=parent_a)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def orphan_a(db, workspace, project_a, create_user):
|
||||
return _make_issue("A orphan", project_a, workspace, create_user)
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
class TestSubIssuesCrossProjectScope:
|
||||
"""A project member must not read or write another project's sub-issue graph."""
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_read_cross_project_sub_issues_hidden(
|
||||
self, session_client, workspace, project_a, parent_b, sub_b
|
||||
):
|
||||
"""GET with a parent that lives in a project the caller isn't in leaks nothing.
|
||||
|
||||
The URL project is A (caller is a member); the parent issue lives in B.
|
||||
Before the fix the endpoint returned B's sub-issues; now the project scope
|
||||
excludes them.
|
||||
"""
|
||||
url = SUB_ISSUES_URL.format(
|
||||
slug=workspace.slug, project_id=project_a.id, issue_id=parent_b.id
|
||||
)
|
||||
response = session_client.get(url)
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
returned_ids = {str(row["id"]) for row in response.data["sub_issues"]}
|
||||
assert str(sub_b.id) not in returned_ids, (
|
||||
f"Leaked cross-project sub-issue: {response.data!r}"
|
||||
)
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_write_cross_project_reparent_blocked(
|
||||
self, session_client, workspace, project_a, parent_b, orphan_b
|
||||
):
|
||||
"""POST cannot re-parent an issue onto a parent outside the URL project.
|
||||
|
||||
Parent B is not in project A, so the scoped lookup 404s and no issue is
|
||||
moved. Before the fix the parent resolved by bare pk and the orphan was
|
||||
re-parented.
|
||||
"""
|
||||
url = SUB_ISSUES_URL.format(
|
||||
slug=workspace.slug, project_id=project_a.id, issue_id=parent_b.id
|
||||
)
|
||||
response = session_client.post(url, {"sub_issue_ids": [str(orphan_b.id)]}, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND, (
|
||||
f"Expected 404, got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
orphan_b.refresh_from_db()
|
||||
assert orphan_b.parent_id is None, "Orphan issue was re-parented across projects"
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_write_cross_project_sub_issue_ids_ignored(
|
||||
self, session_client, workspace, project_a, parent_a, orphan_b, mocker
|
||||
):
|
||||
"""Even with an in-project parent, sub_issue_ids from another project are ignored.
|
||||
|
||||
Parent A is valid for the URL project, but ``orphan_b`` lives in project B,
|
||||
so the project-scoped ``sub_issue_ids`` filter must exclude it and leave its
|
||||
parent untouched — and no activity event may be enqueued for it (the activity
|
||||
task does an unscoped lookup + updated_at bump on whatever id it receives).
|
||||
"""
|
||||
mock_activity = mocker.patch(
|
||||
"plane.app.views.issue.sub_issue.issue_activity.delay"
|
||||
)
|
||||
url = SUB_ISSUES_URL.format(
|
||||
slug=workspace.slug, project_id=project_a.id, issue_id=parent_a.id
|
||||
)
|
||||
response = session_client.post(url, {"sub_issue_ids": [str(orphan_b.id)]}, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
# The foreign issue must not appear in the response body either.
|
||||
assert response.data["sub_issues"] == [], f"Leaked cross-project issue: {response.data!r}"
|
||||
assert response.data["state_distribution"] == {}, f"Leaked state_distribution: {response.data!r}"
|
||||
orphan_b.refresh_from_db()
|
||||
assert orphan_b.parent_id is None, "Cross-project issue was re-parented"
|
||||
# No activity may be dispatched for the excluded cross-project issue.
|
||||
dispatched_ids = {call.kwargs.get("issue_id") for call in mock_activity.call_args_list}
|
||||
assert str(orphan_b.id) not in dispatched_ids, (
|
||||
f"Activity dispatched for a cross-project issue: {mock_activity.call_args_list!r}"
|
||||
)
|
||||
|
||||
# --- Positive controls: legitimate same-project use still works -----------
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_read_same_project_sub_issues_visible(
|
||||
self, session_client, workspace, project_a, parent_a, sub_a
|
||||
):
|
||||
url = SUB_ISSUES_URL.format(
|
||||
slug=workspace.slug, project_id=project_a.id, issue_id=parent_a.id
|
||||
)
|
||||
response = session_client.get(url)
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
returned_ids = {str(row["id"]) for row in response.data["sub_issues"]}
|
||||
assert str(sub_a.id) in returned_ids, (
|
||||
f"Expected sub-issue {sub_a.id} in {response.data!r}"
|
||||
)
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_write_same_project_reparent_allowed(
|
||||
self, session_client, workspace, project_a, parent_a, orphan_a
|
||||
):
|
||||
url = SUB_ISSUES_URL.format(
|
||||
slug=workspace.slug, project_id=project_a.id, issue_id=parent_a.id
|
||||
)
|
||||
response = session_client.post(url, {"sub_issue_ids": [str(orphan_a.id)]}, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
orphan_a.refresh_from_db()
|
||||
assert str(orphan_a.parent_id) == str(parent_a.id), "Same-project re-parent did not persist"
|
||||
Reference in New Issue
Block a user