mirror of
https://github.com/makeplane/plane.git
synced 2026-09-01 19:48:42 +02:00
[WEB-8382][WEB-8383] fix(security): external-API issue comment + attachment authz (GHSA-h4p4-mwfg-qh82, GHSA-xvc5-m5jf-gvpj)
Two authorization gaps in the external API (api/views/issue.py): - GHSA-h4p4: IssueCommentDetailAPIEndpoint uses ProjectLitePermission (any active project member) and edited/deleted comments by id with no author/admin check — a Guest could tamper with or delete anyone's comments. patch/delete now require the comment author OR a project admin (403 otherwise), matching the app. - GHSA-xvc5: IssueAttachmentListCreateAPIEndpoint.get had no permission_classes (bare IsAuthenticated) and no membership check, while post calls user_has_issue_permission. Since API tokens authenticate globally, any token holder could list any issue's attachment metadata cross-tenant. get now enforces project membership on the issue, mirroring post. Adds 6 contract tests (guest comment patch/delete 403, author/admin allowed; outsider attachment list 403, member allowed); fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1602,6 +1602,17 @@ class IssueCommentDetailAPIEndpoint(BaseAPIView):
|
||||
Validates external ID uniqueness if provided.
|
||||
"""
|
||||
issue_comment = IssueComment.objects.get(workspace__slug=slug, project_id=project_id, issue_id=issue_id, pk=pk)
|
||||
# Only the comment author or a project admin may modify a comment.
|
||||
# ProjectLitePermission alone lets any active member (incl. Guest) reach
|
||||
# here, so enforce the same author/admin rule the app applies
|
||||
# (GHSA-h4p4-mwfg-qh82).
|
||||
if issue_comment.created_by_id != request.user.id and not ProjectMember.objects.filter(
|
||||
project_id=project_id, member_id=request.user.id, role=ROLE.ADMIN.value, is_active=True
|
||||
).exists():
|
||||
return Response(
|
||||
{"error": "Only the comment author or a project admin can modify this comment."},
|
||||
status=status.HTTP_403_FORBIDDEN,
|
||||
)
|
||||
requested_data = json.dumps(self.request.data, cls=DjangoJSONEncoder)
|
||||
current_instance = json.dumps(IssueCommentSerializer(issue_comment).data, cls=DjangoJSONEncoder)
|
||||
|
||||
@@ -1671,6 +1682,15 @@ class IssueCommentDetailAPIEndpoint(BaseAPIView):
|
||||
Records deletion activity for audit purposes.
|
||||
"""
|
||||
issue_comment = IssueComment.objects.get(workspace__slug=slug, project_id=project_id, issue_id=issue_id, pk=pk)
|
||||
# Only the comment author or a project admin may delete a comment
|
||||
# (GHSA-h4p4-mwfg-qh82).
|
||||
if issue_comment.created_by_id != request.user.id and not ProjectMember.objects.filter(
|
||||
project_id=project_id, member_id=request.user.id, role=ROLE.ADMIN.value, is_active=True
|
||||
).exists():
|
||||
return Response(
|
||||
{"error": "Only the comment author or a project admin can delete this comment."},
|
||||
status=status.HTTP_403_FORBIDDEN,
|
||||
)
|
||||
current_instance = json.dumps(IssueCommentSerializer(issue_comment).data, cls=DjangoJSONEncoder)
|
||||
issue_comment.delete()
|
||||
issue_activity.delay(
|
||||
@@ -2002,6 +2022,23 @@ class IssueAttachmentListCreateAPIEndpoint(BaseAPIView):
|
||||
|
||||
List all attachments for an issue.
|
||||
"""
|
||||
# This endpoint has no permission_classes beyond IsAuthenticated, and API
|
||||
# tokens authenticate globally (not workspace-scoped), so enforce project
|
||||
# membership on the issue the same way post() does — otherwise any token
|
||||
# holder could read any issue's attachment metadata cross-tenant
|
||||
# (GHSA-xvc5-m5jf-gvpj).
|
||||
issue = Issue.objects.get(pk=issue_id, workspace__slug=slug, project_id=project_id)
|
||||
if not user_has_issue_permission(
|
||||
request.user.id,
|
||||
project_id=project_id,
|
||||
issue=issue,
|
||||
allowed_roles=[ROLE.ADMIN.value, ROLE.MEMBER.value, ROLE.GUEST.value],
|
||||
allow_creator=True,
|
||||
):
|
||||
return Response(
|
||||
{"error": "You are not allowed to view these attachments"},
|
||||
status=status.HTTP_403_FORBIDDEN,
|
||||
)
|
||||
# Get all the attachments
|
||||
issue_attachments = FileAsset.objects.filter(
|
||||
issue_id=issue_id,
|
||||
|
||||
@@ -0,0 +1,184 @@
|
||||
# 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 external-API issue comment + attachment authorization.
|
||||
|
||||
Regression coverage for:
|
||||
|
||||
* GHSA-h4p4-mwfg-qh82 — ``IssueCommentDetailAPIEndpoint`` (``ProjectLitePermission``,
|
||||
any active member) edited/deleted comments by id with no author/admin check, so
|
||||
a Guest could tamper with anyone's comments.
|
||||
* GHSA-xvc5-m5jf-gvpj — ``IssueAttachmentListCreateAPIEndpoint.get`` had no
|
||||
``permission_classes`` (bare ``IsAuthenticated``) and no membership check, so any
|
||||
API-token holder could list any issue's attachment metadata cross-tenant.
|
||||
|
||||
The fixes: comment patch/delete require the comment author or a project admin;
|
||||
the attachment GET enforces project membership on the issue (mirroring ``post``).
|
||||
"""
|
||||
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from rest_framework import status
|
||||
from rest_framework.test import APIClient
|
||||
|
||||
from plane.db.models import (
|
||||
APIToken,
|
||||
FileAsset,
|
||||
Issue,
|
||||
IssueComment,
|
||||
Project,
|
||||
ProjectMember,
|
||||
User,
|
||||
WorkspaceMember,
|
||||
)
|
||||
|
||||
COMMENT_URL = "/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/comments/{pk}/"
|
||||
ATTACH_URL = "/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/issue-attachments/"
|
||||
|
||||
|
||||
def _user(prefix):
|
||||
unique = uuid4().hex[:8]
|
||||
user = User.objects.create(email=f"{prefix}-{unique}@plane.so", username=f"{prefix}_{unique}")
|
||||
user.set_password("test-password")
|
||||
user.save()
|
||||
return user
|
||||
|
||||
|
||||
def _token_client(user):
|
||||
token = APIToken.objects.create(user=user, label=f"tok-{uuid4().hex[:6]}", token=f"tok-{uuid4().hex}")
|
||||
client = APIClient()
|
||||
client.credentials(HTTP_X_API_KEY=token.token)
|
||||
return client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def project(db, workspace, create_user):
|
||||
project = Project.objects.create(
|
||||
name="Ext API Project", identifier="EA", workspace=workspace, created_by=create_user
|
||||
)
|
||||
# create_user is the project ADMIN
|
||||
ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20, is_active=True)
|
||||
return project
|
||||
|
||||
|
||||
def _project_member(workspace, project, *, role):
|
||||
user = _user(f"role{role}")
|
||||
WorkspaceMember.objects.create(workspace=workspace, member=user, role=role, is_active=True)
|
||||
ProjectMember.objects.create(project=project, member=user, workspace=workspace, role=role, is_active=True)
|
||||
return user
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def issue(db, workspace, project, create_user):
|
||||
issue = Issue(name="Issue", project=project, workspace=workspace)
|
||||
issue.save(created_by_id=create_user.id)
|
||||
return issue
|
||||
|
||||
|
||||
# --- h4p4: comment tamper -----------------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def author(db, workspace, project):
|
||||
return _project_member(workspace, project, role=15)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def comment(db, workspace, project, issue, author):
|
||||
c = IssueComment(
|
||||
workspace=workspace, project=project, issue=issue, comment_html="<p>original</p>", actor=author
|
||||
)
|
||||
c.save(created_by_id=author.id)
|
||||
return c
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
@pytest.mark.django_db
|
||||
class TestExternalApiCommentAuthz:
|
||||
"""A Guest / non-author must not edit or delete another user's comment."""
|
||||
|
||||
def test_guest_cannot_patch_comment(self, workspace, project, issue, comment):
|
||||
guest = _project_member(workspace, project, role=5)
|
||||
response = _token_client(guest).patch(
|
||||
COMMENT_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=comment.id),
|
||||
{"comment_html": "<p>tampered</p>"},
|
||||
format="json",
|
||||
)
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
comment.refresh_from_db()
|
||||
assert comment.comment_html == "<p>original</p>"
|
||||
|
||||
def test_guest_cannot_delete_comment(self, workspace, project, issue, comment):
|
||||
guest = _project_member(workspace, project, role=5)
|
||||
response = _token_client(guest).delete(
|
||||
COMMENT_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=comment.id)
|
||||
)
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert IssueComment.objects.filter(pk=comment.id).exists()
|
||||
|
||||
def test_author_can_patch_own_comment(self, workspace, project, issue, comment, author):
|
||||
response = _token_client(author).patch(
|
||||
COMMENT_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=comment.id),
|
||||
{"comment_html": "<p>edited by author</p>"},
|
||||
format="json",
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
|
||||
def test_admin_can_delete_comment(self, workspace, project, issue, comment, create_user):
|
||||
"""Positive control: a project admin may delete another user's comment."""
|
||||
response = _token_client(create_user).delete(
|
||||
COMMENT_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=comment.id)
|
||||
)
|
||||
assert response.status_code == status.HTTP_204_NO_CONTENT, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
|
||||
|
||||
# --- xvc5: attachment metadata disclosure -------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def attachment(db, workspace, project, issue):
|
||||
return FileAsset.objects.create(
|
||||
attributes={"name": "secret.pdf", "type": "application/pdf", "size": 100},
|
||||
asset=f"{workspace.id}/{uuid4().hex}-secret.pdf",
|
||||
size=100,
|
||||
workspace=workspace,
|
||||
project=project,
|
||||
issue_id=issue.id,
|
||||
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
|
||||
is_uploaded=True,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
@pytest.mark.django_db
|
||||
class TestExternalApiAttachmentAuthz:
|
||||
"""A token holder with no project membership must not read attachment metadata."""
|
||||
|
||||
def test_outsider_cannot_list_attachments(self, workspace, project, issue, attachment):
|
||||
outsider = _user("outsider") # valid token, NO membership anywhere
|
||||
response = _token_client(outsider).get(
|
||||
ATTACH_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id)
|
||||
)
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
|
||||
def test_project_member_can_list_attachments(self, workspace, project, issue, attachment):
|
||||
member = _project_member(workspace, project, role=15)
|
||||
response = _token_client(member).get(
|
||||
ATTACH_URL.format(slug=workspace.slug, project_id=project.id, issue_id=issue.id)
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert len(response.data) == 1
|
||||
Reference in New Issue
Block a user