mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 01:58:36 +02:00
[INFRA-777] fix(security): scope ProjectArchiveUnarchiveAPIEndpoint to the URL project
ProjectArchiveUnarchiveAPIEndpoint used ProjectBasePermission, whose POST branch is written for project creation and checks only workspace-level role (ADMIN/MEMBER) with no project_id binding at all. Since archive is POST but is not creation, any active workspace member could archive (and delete every UserFavorite row on) any project in the workspace, including a fully private one they have no ProjectMember row on and cannot otherwise read. DELETE (unarchive) was never affected by this — it falls through to ProjectBasePermission's non-POST branch, which does check project membership, so this was an asymmetry between the two verbs of the same endpoint (and that branch was itself stricter than intended: it required project ADMIN or workspace-ADMIN-plus-project-membership, not just an active project MEMBER). Give the endpoint a dedicated ProjectArchiveUnarchivePermission, scoped to project_id, mirroring the app-layer twin's gate (active ProjectMember with role ADMIN or MEMBER, or any active ProjectMember plus a workspace ADMIN) on both post and delete — instead of touching ProjectBasePermission itself, which is correctly relied on by the actual project-create endpoint and by ProjectDetailAPIEndpoint's patch/delete. 3 new tests, fail-before verified. Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -49,7 +49,11 @@ from plane.api.serializers import (
|
||||
ProjectCreateSerializer,
|
||||
ProjectUpdateSerializer,
|
||||
)
|
||||
from plane.app.permissions import ProjectBasePermission, WorkSpaceAdminPermission
|
||||
from plane.app.permissions import (
|
||||
ProjectArchiveUnarchivePermission,
|
||||
ProjectBasePermission,
|
||||
WorkSpaceAdminPermission,
|
||||
)
|
||||
from plane.utils.openapi import (
|
||||
project_docs,
|
||||
PROJECT_ID_PARAMETER,
|
||||
@@ -649,7 +653,10 @@ class ProjectDetailAPIEndpoint(BaseAPIView):
|
||||
class ProjectArchiveUnarchiveAPIEndpoint(BaseAPIView):
|
||||
"""Project Archive and Unarchive Endpoint"""
|
||||
|
||||
permission_classes = [ProjectBasePermission]
|
||||
# Not ProjectBasePermission: its POST branch is written for project
|
||||
# creation (workspace-role check only, no project_id binding) and archive
|
||||
# is not creation — see ProjectArchiveUnarchivePermission's docstring.
|
||||
permission_classes = [ProjectArchiveUnarchivePermission]
|
||||
|
||||
@project_docs(
|
||||
operation_id="archive_project",
|
||||
|
||||
@@ -13,6 +13,7 @@ from .workspace import (
|
||||
)
|
||||
from .project import (
|
||||
ProjectBasePermission,
|
||||
ProjectArchiveUnarchivePermission,
|
||||
ProjectEntityPermission,
|
||||
ProjectMemberPermission,
|
||||
ProjectLitePermission,
|
||||
|
||||
@@ -53,6 +53,50 @@ class ProjectBasePermission(BasePermission):
|
||||
)
|
||||
|
||||
|
||||
class ProjectArchiveUnarchivePermission(BasePermission):
|
||||
"""Archive/unarchive always target an existing project_id — never project
|
||||
creation — so this must not share ProjectBasePermission's POST branch,
|
||||
which assumes POST means "create a project" and checks only
|
||||
workspace-level role with no project_id binding at all. A workspace
|
||||
ADMIN/MEMBER with no ProjectMember row on the target project could
|
||||
otherwise archive (and delete every UserFavorite row on) a project they
|
||||
cannot even read.
|
||||
|
||||
Mirrors the app-layer twin's gate (``allow_permission([ROLE.ADMIN,
|
||||
ROLE.MEMBER])``, ``level="PROJECT"`` default) on both post and delete so
|
||||
the two verbs agree — unarchive already got this right by falling
|
||||
through to ProjectBasePermission's non-POST branch; archive didn't.
|
||||
"""
|
||||
|
||||
def has_permission(self, request, view):
|
||||
if request.user.is_anonymous:
|
||||
return False
|
||||
|
||||
if ProjectMember.objects.filter(
|
||||
workspace__slug=view.workspace_slug,
|
||||
member=request.user,
|
||||
project_id=view.project_id,
|
||||
role__in=[ROLE.ADMIN.value, ROLE.MEMBER.value],
|
||||
is_active=True,
|
||||
).exists():
|
||||
return True
|
||||
|
||||
return (
|
||||
ProjectMember.objects.filter(
|
||||
workspace__slug=view.workspace_slug,
|
||||
member=request.user,
|
||||
project_id=view.project_id,
|
||||
is_active=True,
|
||||
).exists()
|
||||
and WorkspaceMember.objects.filter(
|
||||
member=request.user,
|
||||
workspace__slug=view.workspace_slug,
|
||||
role=ROLE.ADMIN.value,
|
||||
is_active=True,
|
||||
).exists()
|
||||
)
|
||||
|
||||
|
||||
class ProjectMemberPermission(BasePermission):
|
||||
def has_permission(self, request, view):
|
||||
if request.user.is_anonymous:
|
||||
|
||||
125
apps/api/plane/tests/contract/api/test_project_archive_scope.py
Normal file
125
apps/api/plane/tests/contract/api/test_project_archive_scope.py
Normal file
@@ -0,0 +1,125 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""Regression test for project-archive authorization bypass on the external API.
|
||||
|
||||
Root cause: ProjectArchiveUnarchiveAPIEndpoint used ProjectBasePermission,
|
||||
whose POST branch is written for project creation — it checks only
|
||||
workspace-level role (ADMIN/MEMBER) with no project_id binding at all. Since
|
||||
archive is POST but is not creation, any active workspace member could
|
||||
archive (and delete every UserFavorite row on) any project in the workspace,
|
||||
including a fully private one they have no ProjectMember row on and cannot
|
||||
otherwise read.
|
||||
|
||||
DELETE (unarchive) was never affected — it falls through to
|
||||
ProjectBasePermission's non-POST branch, which does check project membership.
|
||||
So this was specifically an asymmetry between the two verbs of the same
|
||||
endpoint.
|
||||
|
||||
Fixed by giving the endpoint its own ProjectArchiveUnarchivePermission,
|
||||
scoped to project_id, mirroring the app-layer twin's gate (active
|
||||
ProjectMember with role ADMIN or MEMBER, or any active ProjectMember plus a
|
||||
workspace ADMIN role) — applied to both post and delete.
|
||||
"""
|
||||
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from rest_framework import status
|
||||
from rest_framework.test import APIClient
|
||||
|
||||
from plane.db.models import APIToken, Project, ProjectMember, User, WorkspaceMember
|
||||
|
||||
pytestmark = pytest.mark.contract
|
||||
|
||||
|
||||
def _make_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 _client_for(user):
|
||||
token = APIToken.objects.create(user=user, label="Test Token", token=f"token-{uuid4().hex}")
|
||||
client = APIClient()
|
||||
client.credentials(HTTP_X_API_KEY=token.token)
|
||||
return client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def private_project(db, workspace, create_user):
|
||||
"""A fully private (network=0) project owned by create_user, the
|
||||
workspace admin (via the `workspace` fixture)."""
|
||||
project = Project.objects.create(
|
||||
name="Confidential",
|
||||
identifier="CONF",
|
||||
workspace=workspace,
|
||||
created_by=create_user,
|
||||
network=0,
|
||||
)
|
||||
ProjectMember.objects.create(project=project, workspace=workspace, member=create_user, role=20, is_active=True)
|
||||
return project
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def attacker_member(db, workspace):
|
||||
"""An active workspace MEMBER (role 15) with no ProjectMember row on
|
||||
private_project — the attacker in this advisory."""
|
||||
user = _make_user("attacker")
|
||||
WorkspaceMember.objects.create(workspace=workspace, member=user, role=15, is_active=True)
|
||||
return user
|
||||
|
||||
|
||||
def _archive_url(slug, project_id):
|
||||
return f"/api/v1/workspaces/{slug}/projects/{project_id}/archive/"
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
class TestProjectArchiveScope:
|
||||
def test_workspace_member_without_project_membership_cannot_archive(
|
||||
self, workspace, private_project, attacker_member
|
||||
):
|
||||
client = _client_for(attacker_member)
|
||||
|
||||
response = client.post(_archive_url(workspace.slug, private_project.id))
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, f"got {response.status_code}: {response.data!r}"
|
||||
private_project.refresh_from_db()
|
||||
assert private_project.archived_at is None, "the project must not have been archived"
|
||||
|
||||
def test_workspace_member_without_project_membership_cannot_unarchive(
|
||||
self, workspace, private_project, attacker_member
|
||||
):
|
||||
"""Symmetry check: unarchive was already correctly gated before this
|
||||
fix — must still be, now that archive shares the same permission
|
||||
class."""
|
||||
client = _client_for(attacker_member)
|
||||
|
||||
response = client.delete(_archive_url(workspace.slug, private_project.id))
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, f"got {response.status_code}: {response.data!r}"
|
||||
|
||||
def test_project_member_can_archive_and_unarchive_their_own_project(self, workspace, private_project):
|
||||
"""Positive control: an active project MEMBER (not just admin) must
|
||||
still be able to archive/unarchive — the fix must not overtighten
|
||||
beyond the app-layer twin's ADMIN-or-MEMBER gate."""
|
||||
member_user = _make_user("member")
|
||||
WorkspaceMember.objects.create(workspace=workspace, member=member_user, role=15, is_active=True)
|
||||
ProjectMember.objects.create(
|
||||
project=private_project, workspace=workspace, member=member_user, role=15, is_active=True
|
||||
)
|
||||
client = _client_for(member_user)
|
||||
url = _archive_url(workspace.slug, private_project.id)
|
||||
|
||||
response = client.post(url)
|
||||
assert response.status_code == status.HTTP_204_NO_CONTENT, f"got {response.status_code}: {response.data!r}"
|
||||
private_project.refresh_from_db()
|
||||
assert private_project.archived_at is not None
|
||||
|
||||
response = client.delete(url)
|
||||
assert response.status_code == status.HTTP_204_NO_CONTENT, f"got {response.status_code}: {response.data!r}"
|
||||
private_project.refresh_from_db()
|
||||
assert private_project.archived_at is None
|
||||
Reference in New Issue
Block a user