Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions apps/api/plane/app/serializers/workspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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",
]
Comment thread
mguptahub marked this conversation as resolved.
Outdated


class WorkspaceMemberMeSerializer(BaseSerializer):
Expand All @@ -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):
Expand All @@ -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):
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Comment thread
mguptahub marked this conversation as resolved.
Outdated
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
Loading