Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-08-29 01:58:36 +02:00
Code Issues Packages Projects Releases Wiki Activity

[INFRA-502] fix(security): bind external-API work item attachments to the URL work item

Browse Source 
IssueAttachmentDetailAPIEndpoint evaluated its permission against the Issue named
in the URL while the object acted on was a FileAsset that had never been bound to
it. delete, get and patch all resolved on pk + slug + project_id, so a caller
named a work item they were entitled to act on and reached any asset in the
project. With no entity_type filter, page and comment inline images resolved
through the work-item attachment route as well and could be read or destroyed.

The app copy was fixed by 5829f0feb (PR #9315); this is the port, plus the
entity_type binding that fix did not include. The sibling list handler here
already filters on entity_type, so the detail route now resolves exactly what the
list route returns.

patch also set created_by = request.user, so confirming an upload transferred
recorded ownership. That matters beyond the audit trail: delete grants the creator
a delete right, so the reassignment handed over the ability to destroy someone
else's attachment. Line dropped, as the app copy dropped it.

delete's allowed_roles listed every role, which made the filter a no-op and
reduced the check to any active project member, contradicting both the handler's
own comment and the app decorator. Narrowing it to ADMIN alone would have been
wrong in both directions: user_has_issue_permission's allow_creator branch
compares against issue.created_by_id, the work item's author, not the
attachment's uploader. That would have granted delete to an author who never
touched the file and denied it to the member who uploaded it, breaking a
legitimate path. So the asset is resolved first and authorization is then
admin-or-uploader against the asset, matching @allow_permission([ROLE.ADMIN],
creator=True, model=FileAsset).

Contract tests cover cross-work-item delete and read, a page image through the
attachment route, a member deleting another member's upload, and the ownership
transfer, with controls for an admin deleting a correctly bound attachment and an
uploader deleting their own. Fail-before at 880960390a: 5 of 13 fail, the 8
passing being this PR's existing tests plus both controls.

Note for the test fixtures: BaseModel.save() resolves the current user and sets
created_by to None when no request is in scope, so ownership has to be seeded
with a queryset update rather than objects.create(created_by=...).

Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
Manish Gupta
2026-08-21 17:23:19 +05:30
parent 880960390a
commit bd25c20105
2 changed files with 211 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

59
apps/api/plane/api/views/issue.py
View File

@@ -2023,10 +2023,9 @@ 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.
# Bare IsAuthenticated here, and API tokens authenticate globally rather
# than per workspace, so enforce project membership as post() does --
# otherwise any token holder reads any issue's attachment metadata.
issue = Issue.objects.get(pk=issue_id, workspace__slug=slug, project_id=project_id)
if not user_has_issue_permission(
request.user.id,
@@ -2077,20 +2076,34 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView):
Records deletion activity and triggers metadata cleanup.
"""
issue = Issue.objects.get(pk=issue_id, workspace__slug=slug, project_id=project_id)
# if the request user is creator or admin then delete the attachment
if not user_has_issue_permission(
# Bind the asset to the URL work item and to the attachment entity type,
# matching the sibling list handler. Resolved before authorizing, so the
# check below applies to the object actually being acted on.
issue_attachment = FileAsset.objects.get(
pk=pk,
workspace__slug=slug,
project_id=project_id,
issue_id=issue_id,
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
)
# Admin or the uploader, as the app surface allows. allow_creator is off
# because it compares against the work item's author, not this file's
# uploader, so the ownership test is done explicitly below.
is_project_admin = 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,
):
allowed_roles=[ROLE.ADMIN.value],
allow_creator=False,
)
if not (is_project_admin or issue_attachment.created_by_id == request.user.id):
return Response(
{"error": "You are not allowed to delete this attachment"},
status=status.HTTP_403_FORBIDDEN,
)
issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id)
issue_attachment.is_deleted = True
issue_attachment.deleted_at = timezone.now()
issue_attachment.save()
@@ -2163,8 +2176,16 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView):
status=status.HTTP_403_FORBIDDEN,
)
# Get the asset
asset = FileAsset.objects.get(id=pk, workspace__slug=slug, project_id=project_id)
# Bind to the URL work item and entity type: scoped to the project alone,
# this route issued a presigned URL for any asset in it, page and comment
# images included.
asset = FileAsset.objects.get(
id=pk,
workspace__slug=slug,
project_id=project_id,
issue_id=issue_id,
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
)
# Check if the asset is uploaded
if not asset.is_uploaded:
@@ -2228,7 +2249,14 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView):
status=status.HTTP_403_FORBIDDEN,
)
issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id)
# Bound as in the delete and retrieve handlers above.
issue_attachment = FileAsset.objects.get(
pk=pk,
workspace__slug=slug,
project_id=project_id,
issue_id=issue_id,
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
)
serializer = IssueAttachmentSerializer(issue_attachment)
# Send this activity only if the attachment is not uploaded before
@@ -2245,9 +2273,10 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView):
origin=base_host(request=request, is_app=True),
)
# Update the attachment
# created_by is deliberately not reassigned: confirming an upload is
# not authorship, and rewriting it would hand the caller the
# creator-based delete right on someone else's attachment.
issue_attachment.is_uploaded = True
issue_attachment.created_by = request.user
# Get the storage metadata
if not issue_attachment.storage_metadata:

167
apps/api/plane/tests/contract/api/test_external_api_issue_authz.py
View File

@@ -182,3 +182,170 @@ class TestExternalApiAttachmentAuthz:
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
assert len(response.data) == 1
# --- attachment object binding + ownership transfer ---------------------------
ATTACH_DETAIL_URL = "/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/issue-attachments/{pk}/"
@pytest.fixture
def other_issue(db, workspace, project, create_user):
"""A second work item in the same project, which the attachment does NOT belong to."""
issue = Issue(name="Other Issue", project=project, workspace=workspace)
issue.save(created_by_id=create_user.id)
return issue
@pytest.fixture
def page_image(db, workspace, project, create_user):
"""A page inline image: same project, no issue, a different entity type."""
return FileAsset.objects.create(
attributes={"name": "diagram.png", "type": "image/png", "size": 100},
asset=f"{workspace.id}/{uuid4().hex}-diagram.png",
size=100,
workspace=workspace,
project=project,
created_by=create_user,
entity_type=FileAsset.EntityTypeContext.PAGE_DESCRIPTION,
is_uploaded=True,
)
@pytest.mark.contract
@pytest.mark.django_db
class TestExternalApiAttachmentObjectBinding:
"""The permission is checked against the URL work item; the object must match it.
Resolved by project alone, any asset in it came back -- page and comment
images included -- so naming a work item you may act on reached all of them.
"""
def test_admin_cannot_delete_attachment_through_a_different_work_item(
self, workspace, project, issue, other_issue, attachment, create_user
):
# Admin, so the 404 is attributable to the binding, not the role check.
response = _token_client(create_user).delete(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=other_issue.id, pk=attachment.id
)
)
assert response.status_code == status.HTTP_404_NOT_FOUND, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
attachment.refresh_from_db()
assert attachment.is_deleted is False, "an attachment of another work item was destroyed"
def test_member_cannot_read_attachment_through_a_different_work_item(
self, workspace, project, issue, other_issue, attachment
):
# Retrieve is open to any project member, so a member is the right caller.
member = _project_member(workspace, project, role=15)
response = _token_client(member).get(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=other_issue.id, pk=attachment.id
)
)
assert response.status_code == status.HTTP_404_NOT_FOUND, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
def test_page_image_is_not_reachable_through_the_attachment_route(
self, workspace, project, issue, page_image, create_user
):
"""The cross-entity half: a page's inline image is not a work item attachment."""
# Admin again, so the refusal is the entity_type binding and not the role.
response = _token_client(create_user).delete(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=page_image.id
)
)
assert response.status_code == status.HTTP_404_NOT_FOUND, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
page_image.refresh_from_db()
assert page_image.is_deleted is False, "a page image was destroyed through the attachment route"
def test_admin_can_delete_a_correctly_bound_attachment(
self, workspace, project, issue, attachment, create_user
):
"""Positive control: the binding must not break the legitimate path."""
response = _token_client(create_user).delete(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id
)
)
assert response.status_code == status.HTTP_204_NO_CONTENT, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
attachment.refresh_from_db()
assert attachment.is_deleted is True
@pytest.mark.contract
@pytest.mark.django_db
class TestExternalApiAttachmentDeleteRole:
"""Delete is admin-or-uploader, matching the app surface.
Listing every role made the filter a no-op, reducing it to any active project
member while the handler's own comment claimed "creator or admin".
"""
def test_member_who_did_not_upload_cannot_delete(self, workspace, project, issue, attachment):
member = _project_member(workspace, project, role=15)
response = _token_client(member).delete(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id
)
)
assert response.status_code == status.HTTP_403_FORBIDDEN, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
attachment.refresh_from_db()
assert attachment.is_deleted is False
def test_uploader_can_delete_own_attachment(self, workspace, project, issue, attachment):
uploader = _project_member(workspace, project, role=15)
# A queryset update, not save(): BaseModel.save() nulls created_by when no
# request is in scope, which is the case in a test.
FileAsset.objects.filter(pk=attachment.id).update(created_by=uploader)
response = _token_client(uploader).delete(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id
)
)
assert response.status_code == status.HTTP_204_NO_CONTENT, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
@pytest.mark.contract
@pytest.mark.django_db
class TestExternalApiAttachmentOwnershipTransfer:
"""Confirming an upload must not rewrite created_by.
Whoever confirmed became the recorded uploader, and delete grants the creator
a delete right -- so the reassignment handed over someone else's attachment.
"""
def test_patch_does_not_transfer_ownership(self, workspace, project, issue, attachment, create_user):
# See above: set ownership through the queryset so save() cannot clear it.
FileAsset.objects.filter(pk=attachment.id).update(is_uploaded=False, created_by=create_user)
attachment.refresh_from_db()
member = _project_member(workspace, project, role=15)
response = _token_client(member).patch(
ATTACH_DETAIL_URL.format(
slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id
),
{"is_uploaded": True},
format="json",
)
assert response.status_code == status.HTTP_204_NO_CONTENT, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
attachment.refresh_from_db()
assert attachment.is_uploaded is True, "the upload confirmation itself must still work"
assert attachment.created_by_id == create_user.id, "created_by was transferred to the caller"