mirror of
https://github.com/makeplane/plane.git
synced 2026-09-01 19:48:42 +02:00
fix(security): keep archived-issue relations readable within their own project
The list() project-scope guard added for the cross-project relation-list IDOR used Issue.issue_objects to look up the parent issue, but that manager excludes archived issues — so viewing relations on an archived issue started 404ing even for members of its own project. The issue detail page fetches relations unconditionally on load, including for archived issues, and renders the panel read-only rather than hiding it there, so this closed off a legitimate read path along with the hole. Give issue_in_project() an include_archived flag and use it only in list(), following the same Issue.objects (archive-inclusive) pattern IssueViewSet.retrieve() already uses for reads. The write paths (create/remove_relation) keep the narrower issue_objects manager unchanged, and the project/workspace scoping check itself is untouched in all three, so the original cross-project fix still holds. Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -34,15 +34,23 @@ from plane.utils.issue_relation_mapper import get_actual_relation
|
||||
from plane.utils.host import base_host
|
||||
|
||||
|
||||
def issue_in_project(issue_id, slug, project_id):
|
||||
def issue_in_project(issue_id, slug, project_id, *, include_archived=False):
|
||||
"""Whether ``issue_id`` belongs to the URL workspace + project.
|
||||
|
||||
``ProjectEntityPermission`` only checks that the caller is a member of the URL's
|
||||
``project_id`` — it never binds the sibling ``issue_id`` path parameter to that
|
||||
project. Every handler taking both must therefore check this itself, or it is
|
||||
reachable cross-project/cross-tenant.
|
||||
|
||||
``Issue.issue_objects`` (the default) additionally excludes archived/draft/triage
|
||||
issues, which is correct for the write paths below — but a read path like
|
||||
``list()`` must still find archived issues in the URL's own project, the same way
|
||||
``IssueViewSet.retrieve()`` does with ``Issue.objects``. Pass
|
||||
``include_archived=True`` for that case; the project/workspace binding itself is
|
||||
unchanged either way.
|
||||
"""
|
||||
return Issue.issue_objects.filter(pk=issue_id, workspace__slug=slug, project_id=project_id).exists()
|
||||
manager = Issue.objects if include_archived else Issue.issue_objects
|
||||
return manager.filter(pk=issue_id, workspace__slug=slug, project_id=project_id).exists()
|
||||
|
||||
|
||||
class IssueRelationViewSet(BaseViewSet):
|
||||
@@ -55,7 +63,12 @@ class IssueRelationViewSet(BaseViewSet):
|
||||
# it a member of project A could list the relations of an issue in project B of
|
||||
# the same workspace and receive that issue's name, priority, assignees and
|
||||
# labels in the response.
|
||||
if not issue_in_project(issue_id, slug, project_id):
|
||||
#
|
||||
# include_archived=True: this is a read, not a write, and the issue detail page
|
||||
# fetches relations unconditionally for archived issues too. Scoping this lookup
|
||||
# with the write-path's issue_objects (which excludes archived issues) would 404
|
||||
# a legitimate same-project read instead of only closing the cross-project hole.
|
||||
if not issue_in_project(issue_id, slug, project_id, include_archived=True):
|
||||
return Response({"error": "Issue not found"}, status=status.HTTP_404_NOT_FOUND)
|
||||
|
||||
issue_relations = (
|
||||
|
||||
@@ -386,15 +386,24 @@ class TestIssueRelationListScope:
|
||||
|
||||
@pytest.mark.contract
|
||||
class TestIssueManagerBoundary:
|
||||
"""Pin the one axis on which the two fixes deliberately differ.
|
||||
"""Pin the axes on which the fixes deliberately differ.
|
||||
|
||||
``comment.py`` scopes with ``Issue.objects`` and ``relation.py`` with
|
||||
``Issue.issue_objects``. ``IssueManager`` additionally excludes triage-state,
|
||||
archived and draft issues, so the choice is behavioural, not cosmetic: intake
|
||||
(triage) and archived issues must stay commentable, while the relation endpoints
|
||||
follow the ``issue_objects`` convention already used for the body ``issues`` list
|
||||
and by ``SubIssuesEndpoint``. Without these tests, "tidying" the two to match
|
||||
would silently break commenting on intake items.
|
||||
``comment.py`` scopes with ``Issue.objects`` and the relation *write* paths
|
||||
(``create`` / ``remove_relation``) with ``Issue.issue_objects``. ``IssueManager``
|
||||
additionally excludes triage-state, archived and draft issues, so the choice is
|
||||
behavioural, not cosmetic: intake (triage) and archived issues must stay
|
||||
commentable, while the relation write endpoints follow the ``issue_objects``
|
||||
convention already used for the body ``issues`` list and by ``SubIssuesEndpoint``.
|
||||
|
||||
``IssueRelationViewSet.list()`` is the odd one out: it is a read, and the issue
|
||||
detail page fetches relations unconditionally even for archived issues, rendering
|
||||
them read-only rather than hiding the panel. So ``list()`` scopes with
|
||||
``Issue.objects`` (``include_archived=True``) like ``IssueViewSet.retrieve()``
|
||||
does — archived issues in the URL's own project stay listable, while an archived
|
||||
issue in a *different* project is still refused, same as everywhere else.
|
||||
|
||||
Without these tests, "tidying" the managers to match would either silently break
|
||||
commenting/listing on intake or archived items, or reopen the cross-project hole.
|
||||
"""
|
||||
|
||||
@pytest.mark.django_db
|
||||
@@ -465,16 +474,35 @@ class TestIssueManagerBoundary:
|
||||
)
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_relation_list_excludes_archived_url_issue(
|
||||
def test_relation_list_allows_archived_issue_in_own_project(
|
||||
self, attacker_client, attacker_workspace, attacker_project, attacker_issue
|
||||
):
|
||||
"""And on the read path, so all three handlers are pinned to the same manager."""
|
||||
"""Unlike the write paths, ``list()`` must keep working on an archived issue
|
||||
that belongs to the URL's own project — the issue detail page reads relations
|
||||
for archived issues too, and the panel there is read-only, not hidden."""
|
||||
attacker_issue.archived_at = timezone.now().date()
|
||||
attacker_issue.save(update_fields=["archived_at"])
|
||||
|
||||
url = relation_url(attacker_workspace.slug, attacker_project.id, attacker_issue.id)
|
||||
response = attacker_client.get(url)
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Archived issue in the URL's own project must stay listable, "
|
||||
f"got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_relation_list_rejects_archived_issue_in_other_project(
|
||||
self, attacker_client, attacker_workspace, attacker_project, sibling_project_issue
|
||||
):
|
||||
"""The archive-inclusive lookup must not reopen the cross-project hole: an
|
||||
archived issue in a *sibling* project is still refused."""
|
||||
sibling_project_issue.archived_at = timezone.now().date()
|
||||
sibling_project_issue.save(update_fields=["archived_at"])
|
||||
|
||||
url = relation_url(attacker_workspace.slug, attacker_project.id, sibling_project_issue.id)
|
||||
response = attacker_client.get(url)
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user