From a7d9f1f257e4db193aa5c25235fc40009dae21d0 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 11:16:23 +0530 Subject: [PATCH 1/3] [INFRA-774] fix(security): stop trusting body-supplied workspace/member on WorkSpaceMemberSerializer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkSpaceMemberSerializer declared fields = "__all__" with no read_only_fields, so DRF auto-generated a writable workspace FK. WorkSpaceMemberViewSet.partial_update passes raw request.data straight into the serializer with no scrubbing, so a workspace ADMIN could PATCH any other active member's row with a workspace field pointing at a foreign workspace's UUID — moving that row (with whatever role was also in the body) into the foreign workspace with no invitation, no consent from its owner, and no audit trail. Add workspace/member (plus the usual created_by/updated_by/created_at/ updated_at) to read_only_fields on WorkSpaceMemberSerializer and its siblings WorkspaceMemberMeSerializer/WorkspaceMemberAdminSerializer — same model, same footgun shape, even though the latter two are only ever instantiated read-only today. Co-authored-by: Plane AI --- apps/api/plane/app/serializers/workspace.py | 40 ++++++- ...kspace_member_cross_tenant_reassignment.py | 105 ++++++++++++++++++ 2 files changed, 142 insertions(+), 3 deletions(-) create mode 100644 apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py diff --git a/apps/api/plane/app/serializers/workspace.py b/apps/api/plane/app/serializers/workspace.py index 63d4ea99d12..79838bfa2db 100644 --- a/apps/api/plane/app/serializers/workspace.py +++ b/apps/api/plane/app/serializers/workspace.py @@ -53,9 +53,7 @@ def validate_name(self, value): # digit. Mirrors the frontend HAS_ALPHANUMERIC_REGEX check so the rule # cannot be bypassed via a direct API call. if not has_alphanumeric(value): - raise serializers.ValidationError( - "Name must contain at least one letter or number" - ) + raise serializers.ValidationError("Name must contain at least one letter or number") return value def validate_slug(self, value): @@ -96,6 +94,20 @@ class WorkSpaceMemberSerializer(DynamicBaseSerializer): class Meta: model = WorkspaceMember fields = "__all__" + # workspace/member must stay read-only: WorkSpaceMemberViewSet.partial_update + # passes request.data straight into this serializer with no scrubbing, so a + # writable `workspace` FK let any workspace admin PATCH a member's row into an + # arbitrary foreign workspace (with whatever role was also in the body) — + # instant cross-tenant admin takeover, no invite, no consent, no audit trail. + read_only_fields = [ + "id", + "workspace", + "member", + "created_by", + "updated_by", + "created_at", + "updated_at", + ] class WorkspaceMemberMeSerializer(BaseSerializer): @@ -104,6 +116,17 @@ class WorkspaceMemberMeSerializer(BaseSerializer): class Meta: model = WorkspaceMember fields = "__all__" + # See WorkSpaceMemberSerializer above — same model, same fix, applied here + # even though this serializer is only ever used read-only today. + read_only_fields = [ + "id", + "workspace", + "member", + "created_by", + "updated_by", + "created_at", + "updated_at", + ] class WorkspaceMemberAdminSerializer(DynamicBaseSerializer): @@ -112,6 +135,17 @@ class WorkspaceMemberAdminSerializer(DynamicBaseSerializer): class Meta: model = WorkspaceMember fields = "__all__" + # See WorkSpaceMemberSerializer above — same model, same fix, applied here + # even though this serializer is only ever used read-only today. + read_only_fields = [ + "id", + "workspace", + "member", + "created_by", + "updated_by", + "created_at", + "updated_at", + ] class WorkSpaceMemberInviteSerializer(BaseSerializer): diff --git a/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py b/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py new file mode 100644 index 00000000000..9300706043e --- /dev/null +++ b/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py @@ -0,0 +1,105 @@ +# 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 cross-workspace privilege escalation via +WorkSpaceMemberSerializer.partial_update. + +Root cause: WorkSpaceMemberSerializer (and its read-only-in-practice siblings +WorkspaceMemberMeSerializer / WorkspaceMemberAdminSerializer) declared +fields = "__all__" with no read_only_fields, so DRF auto-generated a writable +`workspace` FK field. WorkSpaceMemberViewSet.partial_update passes raw +request.data straight into the serializer with no scrubbing, so a workspace +ADMIN could PATCH any other active member's WorkspaceMember row with a +`workspace` field pointing at a foreign workspace's UUID — moving that row +(and whatever role was also in the body) into the foreign workspace with no +invitation, no consent from its owner, and no audit trail. + +Fixed by adding workspace/member (plus the usual created_by/updated_by/ +created_at/updated_at) to read_only_fields on all three serializers. +""" + +import uuid + +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from plane.db.models import User, Workspace, WorkspaceMember + +pytestmark = pytest.mark.contract + + +def _member_detail_url(slug: str, pk: uuid.UUID) -> str: + return f"/api/workspaces/{slug}/members/{pk}/" + + +def _make_user(email: str) -> User: + local_part = email.split("@")[0] + user = User.objects.create(email=email, username=local_part, first_name=local_part) + user.set_password("test-password") + user.save() + return user + + +@pytest.fixture +def foreign_workspace(db): + """Workspace B — the attacker's escalation target, unrelated to `workspace` (A).""" + owner = _make_user(f"foreign-owner-{uuid.uuid4().hex[:8]}@plane.so") + return Workspace.objects.create(name="Foreign Workspace", owner=owner, slug=f"foreign-ws-{uuid.uuid4().hex[:8]}") + + +@pytest.fixture +def victim_member(db, workspace): + """A second, active, non-bot member of workspace A — the row being attacked.""" + victim = _make_user(f"victim-{uuid.uuid4().hex[:8]}@plane.so") + return WorkspaceMember.objects.create(workspace=workspace, member=victim, role=15, is_active=True) + + +@pytest.mark.django_db +class TestWorkspaceMemberCrossTenantReassignment: + def test_admin_cannot_move_member_into_foreign_workspace( + self, workspace, foreign_workspace, victim_member, create_user + ): + """create_user is workspace A's admin (via the `workspace` fixture). They + must not be able to move victim_member's row into foreign_workspace by + PATCHing `workspace` in the body, even though they're a legitimate admin + of A and the request also carries an otherwise-valid `role`.""" + client = APIClient() + client.force_authenticate(user=create_user) + + response = client.patch( + _member_detail_url(workspace.slug, victim_member.id), + {"workspace": str(foreign_workspace.id), "role": 20}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, f"got {response.status_code}: {response.data!r}" + + victim_member.refresh_from_db() + assert victim_member.workspace_id == workspace.id, ( + f"workspace must stay A regardless of what the body requested — got moved to {victim_member.workspace_id!r}" + ) + foreign_row_exists = WorkspaceMember.objects.filter( + workspace=foreign_workspace, member_id=victim_member.member_id + ).exists() + assert not foreign_row_exists, "no row should have been created/moved in the foreign workspace" + # The legitimate part of the same request (role) must still apply — + # proves this isn't just silently rejecting the whole PATCH. + assert victim_member.role == 20 + + def test_admin_can_still_change_role_without_workspace_in_body(self, workspace, victim_member, create_user): + """Positive control: the fix must not break the legitimate role-only PATCH.""" + client = APIClient() + client.force_authenticate(user=create_user) + + response = client.patch( + _member_detail_url(workspace.slug, victim_member.id), + {"role": 5}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, f"got {response.status_code}: {response.data!r}" + victim_member.refresh_from_db() + assert victim_member.role == 5 + assert victim_member.workspace_id == workspace.id From 926e84c4ce45947ea30a1e31381f53066e57f5b7 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 11:23:07 +0530 Subject: [PATCH 2/3] [INFRA-774] dedupe read_only_fields into a shared constant Address /code-review finding on PR #9705: the identical 7-entry read_only_fields list was duplicated verbatim across all three WorkspaceMember-backed serializers. Extracted to WORKSPACE_MEMBER_READ_ONLY_FIELDS so a future field addition/removal can't silently drift between them and reopen the same write-scoping gap. Co-authored-by: Plane AI --- apps/api/plane/app/serializers/workspace.py | 59 ++++++++------------- 1 file changed, 23 insertions(+), 36 deletions(-) diff --git a/apps/api/plane/app/serializers/workspace.py b/apps/api/plane/app/serializers/workspace.py index 79838bfa2db..6e286231787 100644 --- a/apps/api/plane/app/serializers/workspace.py +++ b/apps/api/plane/app/serializers/workspace.py @@ -88,26 +88,31 @@ class Meta: read_only_fields = fields +# workspace/member must stay read-only on every WorkspaceMember-backed serializer +# below: WorkSpaceMemberViewSet.partial_update passes request.data straight into +# whichever of these is in play with no scrubbing, so a writable `workspace` FK let +# any workspace admin PATCH a member's row into an arbitrary foreign workspace (with +# whatever role was also in the body) — instant cross-tenant admin takeover, no +# invite, no consent, no audit trail. Shared as one constant so a future field +# addition (or removal) can't drift between these three and reopen the same gap. +WORKSPACE_MEMBER_READ_ONLY_FIELDS = [ + "id", + "workspace", + "member", + "created_by", + "updated_by", + "created_at", + "updated_at", +] + + class WorkSpaceMemberSerializer(DynamicBaseSerializer): member = UserLiteSerializer(read_only=True) class Meta: model = WorkspaceMember fields = "__all__" - # workspace/member must stay read-only: WorkSpaceMemberViewSet.partial_update - # passes request.data straight into this serializer with no scrubbing, so a - # writable `workspace` FK let any workspace admin PATCH a member's row into an - # arbitrary foreign workspace (with whatever role was also in the body) — - # instant cross-tenant admin takeover, no invite, no consent, no audit trail. - read_only_fields = [ - "id", - "workspace", - "member", - "created_by", - "updated_by", - "created_at", - "updated_at", - ] + read_only_fields = WORKSPACE_MEMBER_READ_ONLY_FIELDS class WorkspaceMemberMeSerializer(BaseSerializer): @@ -116,17 +121,8 @@ class WorkspaceMemberMeSerializer(BaseSerializer): class Meta: model = WorkspaceMember fields = "__all__" - # See WorkSpaceMemberSerializer above — same model, same fix, applied here - # even though this serializer is only ever used read-only today. - read_only_fields = [ - "id", - "workspace", - "member", - "created_by", - "updated_by", - "created_at", - "updated_at", - ] + # Only ever instantiated read-only today — see WORKSPACE_MEMBER_READ_ONLY_FIELDS above. + read_only_fields = WORKSPACE_MEMBER_READ_ONLY_FIELDS class WorkspaceMemberAdminSerializer(DynamicBaseSerializer): @@ -135,17 +131,8 @@ class WorkspaceMemberAdminSerializer(DynamicBaseSerializer): class Meta: model = WorkspaceMember fields = "__all__" - # See WorkSpaceMemberSerializer above — same model, same fix, applied here - # even though this serializer is only ever used read-only today. - read_only_fields = [ - "id", - "workspace", - "member", - "created_by", - "updated_by", - "created_at", - "updated_at", - ] + # Only ever instantiated read-only today — see WORKSPACE_MEMBER_READ_ONLY_FIELDS above. + read_only_fields = WORKSPACE_MEMBER_READ_ONLY_FIELDS class WorkSpaceMemberInviteSerializer(BaseSerializer): From 07739f27c24de9cc30a6b7fe15908e426c71b21c Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 11:44:01 +0530 Subject: [PATCH 3/3] [INFRA-774] also lock down is_active/deleted_at on WorkspaceMember serializers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review findings on PR #9705: - WorkSpaceMemberSerializer (and its siblings) also exposed is_active and deleted_at as writable via fields = "__all__". Every legitimate place that flips these fields (WorkSpaceMemberViewSet.destroy/.leave, invite acceptance) does so via direct model-field assignment, never through this serializer — so this was a side channel letting an admin PATCH around destroy()'s own safety checks (self-removal, role-outranking, last-project-admin orphaning) and its ProjectMember deactivation cascade. deleted_at is worse: the default manager filters on it, so setting it directly silently vanishes the row from every normal queryset with a forgeable timestamp and no audit trail. - Added both fields to WORKSPACE_MEMBER_READ_ONLY_FIELDS. Two new regression tests, fail-before verified (both fail against the pre-this-commit code: is_active flips, and the deleted_at row genuinely vanishes from WorkspaceMember.objects). - Fixed a docstring inaccuracy: partial_update lives on WorkSpaceMemberViewSet, not on the serializer. Co-authored-by: Plane AI --- apps/api/plane/app/serializers/workspace.py | 13 ++++ ...kspace_member_cross_tenant_reassignment.py | 76 ++++++++++++++++--- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/apps/api/plane/app/serializers/workspace.py b/apps/api/plane/app/serializers/workspace.py index 6e286231787..286ccb63f82 100644 --- a/apps/api/plane/app/serializers/workspace.py +++ b/apps/api/plane/app/serializers/workspace.py @@ -95,10 +95,23 @@ class Meta: # whatever role was also in the body) — instant cross-tenant admin takeover, no # invite, no consent, no audit trail. Shared as one constant so a future field # addition (or removal) can't drift between these three and reopen the same gap. +# +# is_active/deleted_at are read-only for the same reason: every legitimate place +# that flips them (WorkSpaceMemberViewSet.destroy, .leave, invite acceptance) does +# so via direct model-field assignment, never through this serializer — so exposing +# them here only ever gave an admin a side-channel PATCH that skips destroy()'s own +# checks (can't remove yourself, can't remove someone outranking you, can't orphan a +# project's last admin) and its ProjectMember deactivation cascade. deleted_at is +# worse: WorkspaceMember's default manager filters on it, so setting it directly +# would silently vanish the row from every normal queryset with no trace, and an +# admin could set it to an arbitrary timestamp — the same audit-trail-forgery shape +# as the created_by/created_at class fixed elsewhere in this security pass. WORKSPACE_MEMBER_READ_ONLY_FIELDS = [ "id", "workspace", "member", + "is_active", + "deleted_at", "created_by", "updated_by", "created_at", diff --git a/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py b/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py index 9300706043e..ac63dee06e7 100644 --- a/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py +++ b/apps/api/plane/tests/contract/app/test_workspace_member_cross_tenant_reassignment.py @@ -2,26 +2,36 @@ # SPDX-License-Identifier: AGPL-3.0-only # See the LICENSE file for details. -"""Regression test for cross-workspace privilege escalation via -WorkSpaceMemberSerializer.partial_update. +"""Regression tests for cross-workspace privilege escalation and +authorization-bypass forgery via WorkSpaceMemberViewSet.partial_update. Root cause: WorkSpaceMemberSerializer (and its read-only-in-practice siblings WorkspaceMemberMeSerializer / WorkspaceMemberAdminSerializer) declared -fields = "__all__" with no read_only_fields, so DRF auto-generated a writable -`workspace` FK field. WorkSpaceMemberViewSet.partial_update passes raw -request.data straight into the serializer with no scrubbing, so a workspace -ADMIN could PATCH any other active member's WorkspaceMember row with a -`workspace` field pointing at a foreign workspace's UUID — moving that row -(and whatever role was also in the body) into the foreign workspace with no -invitation, no consent from its owner, and no audit trail. - -Fixed by adding workspace/member (plus the usual created_by/updated_by/ -created_at/updated_at) to read_only_fields on all three serializers. +fields = "__all__" with no read_only_fields, so DRF auto-generated writable +fields for everything on the model. WorkSpaceMemberViewSet.partial_update +passes raw request.data straight into the serializer with no scrubbing, so a +workspace ADMIN could: + +- PATCH `workspace` on any other active member's row to a foreign workspace's + UUID — moving that row (and whatever role was also in the body) into the + foreign workspace with no invitation, no consent from its owner, and no + audit trail. +- PATCH `is_active`/`deleted_at` directly, as a side channel around + WorkSpaceMemberViewSet.destroy()'s own checks (can't remove yourself, can't + remove someone outranking you, can't orphan a project's last admin) and its + ProjectMember deactivation cascade — every legitimate place that flips + these fields does so via direct model-field assignment, never through this + serializer. + +Fixed by adding workspace/member/is_active/deleted_at (plus the usual +created_by/updated_by/created_at/updated_at) to read_only_fields on all +three serializers. """ import uuid import pytest +from django.utils import timezone from rest_framework import status from rest_framework.test import APIClient @@ -103,3 +113,45 @@ def test_admin_can_still_change_role_without_workspace_in_body(self, workspace, victim_member.refresh_from_db() assert victim_member.role == 5 assert victim_member.workspace_id == workspace.id + + +@pytest.mark.django_db +class TestWorkspaceMemberIsActiveDeletedAtBypass: + """is_active/deleted_at must not be settable through this PATCH — that would + let an admin route around WorkSpaceMemberViewSet.destroy()'s own safety + checks (self-removal, role-outranking, last-project-admin orphaning) and + skip its ProjectMember deactivation cascade entirely.""" + + def test_admin_cannot_deactivate_member_via_patch(self, workspace, victim_member, create_user): + client = APIClient() + client.force_authenticate(user=create_user) + + response = client.patch( + _member_detail_url(workspace.slug, victim_member.id), + {"is_active": False}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, f"got {response.status_code}: {response.data!r}" + victim_member.refresh_from_db() + assert victim_member.is_active is True, "is_active must not be settable through this PATCH at all" + + def test_admin_cannot_soft_delete_member_via_patch(self, workspace, victim_member, create_user): + """deleted_at is worse than is_active: the default manager filters on it, + so setting it directly would silently vanish the row from every normal + queryset, forgeable to an arbitrary timestamp with no audit trail.""" + client = APIClient() + client.force_authenticate(user=create_user) + + response = client.patch( + _member_detail_url(workspace.slug, victim_member.id), + {"deleted_at": timezone.now().isoformat()}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, f"got {response.status_code}: {response.data!r}" + assert WorkspaceMember.objects.filter(pk=victim_member.id).exists(), ( + "the row must still be visible through the default (non-deleted) manager" + ) + victim_member.refresh_from_db() + assert victim_member.deleted_at is None, "deleted_at must not be settable through this PATCH at all"