mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
bd25c20105
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 by5829f0feb(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 at880960390a: 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>