mirror of
https://github.com/makeplane/plane.git
synced 2026-09-02 12:09:14 +02:00
[WEB-8332] fix(security): block workspace-member mass-assignment (GHSA-f739-39g5-jj49)
WorkSpaceMemberViewSet.partial_update passed request.data verbatim into
WorkSpaceMemberSerializer (fields="__all__"), making workspace, role, and
is_active mass-assignable. A workspace admin could PATCH a member row setting
workspace=<victim UUID> and role=20, relocating a controlled account into the
victim workspace as admin — full cross-tenant takeover.
The endpoint's only legitimate mutation is `role`, so restrict the writable
payload to {"role": ...}. Note: passing fields=("id","member","role") does NOT
work — DynamicBaseSerializer discards the fields= kwarg (base.py) — so the
allowlist is enforced in the view instead. Preserves the self-role-update guard
and the guest role-cascade. is_active is only changed via destroy(), not here.
Adds 4 contract tests; fail-before verified (2 attack tests failed unpatched → 4 pass).
Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -88,7 +88,18 @@ class WorkSpaceMemberViewSet(BaseViewSet):
|
||||
if "role" in request.data and int(request.data.get("role")) == 5:
|
||||
ProjectMember.objects.filter(workspace__slug=slug, member_id=workspace_member.member_id).update(role=5)
|
||||
|
||||
serializer = WorkSpaceMemberSerializer(workspace_member, data=request.data, partial=True)
|
||||
# SECURITY: The only field this endpoint is allowed to mutate is ``role``.
|
||||
# ``WorkSpaceMemberSerializer`` is declared with ``fields = "__all__"`` and
|
||||
# ``DynamicBaseSerializer`` ignores the ``fields=`` kwarg for writes, so
|
||||
# passing ``request.data`` verbatim would let a workspace admin mass-assign
|
||||
# ``workspace`` (relocating a controlled member row into a victim workspace as
|
||||
# an admin — full cross-tenant takeover), ``is_active``, and other columns.
|
||||
# Restrict the writable payload to ``role`` only. See GHSA-f739-39g5-jj49.
|
||||
allowed_data = {}
|
||||
if "role" in request.data:
|
||||
allowed_data["role"] = request.data.get("role")
|
||||
|
||||
serializer = WorkSpaceMemberSerializer(workspace_member, data=allowed_data, partial=True)
|
||||
|
||||
if serializer.is_valid():
|
||||
serializer.save()
|
||||
|
||||
@@ -0,0 +1,160 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""
|
||||
Regression tests for GHSA-f739-39g5-jj49 (WEB-8332).
|
||||
|
||||
``WorkSpaceMemberViewSet.partial_update`` is only meant to update a member's
|
||||
``role``. Before the fix it passed ``request.data`` verbatim to
|
||||
``WorkSpaceMemberSerializer`` (declared ``fields = "__all__"`` with only the
|
||||
nested ``member`` read-only). ``DynamicBaseSerializer`` ignores the ``fields=``
|
||||
kwarg for writes, so ``workspace`` (FK), ``is_active`` and other columns were
|
||||
mass-assignable.
|
||||
|
||||
An admin of an attacker-owned workspace could therefore PATCH a controlled
|
||||
member row setting ``workspace`` = victim-workspace UUID and ``role`` = 20,
|
||||
relocating a controlled account into the victim workspace as an admin with no
|
||||
invitation — a full cross-tenant takeover. The ``@allow_permission(level=
|
||||
"WORKSPACE")`` decorator authorizes against the URL slug and does NOT prevent
|
||||
the write from moving the row to a different workspace.
|
||||
|
||||
The fix restricts the writable payload to ``role`` only.
|
||||
"""
|
||||
|
||||
import uuid
|
||||
|
||||
import pytest
|
||||
from rest_framework import status
|
||||
from rest_framework.test import APIClient
|
||||
|
||||
from plane.db.models import User, Workspace, WorkspaceMember
|
||||
|
||||
|
||||
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
|
||||
|
||||
|
||||
def _make_workspace(name: str, slug: str, owner: User) -> Workspace:
|
||||
workspace = Workspace.objects.create(name=name, slug=slug, owner=owner)
|
||||
WorkspaceMember.objects.create(workspace=workspace, member=owner, role=20, is_active=True)
|
||||
return workspace
|
||||
|
||||
|
||||
def _add_member(workspace: Workspace, user: User, *, role: int) -> WorkspaceMember:
|
||||
return WorkspaceMember.objects.create(
|
||||
workspace=workspace, member=user, role=role, is_active=True
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def attacker_workspace(db):
|
||||
"""A workspace fully controlled by the attacker (they are the admin/owner)."""
|
||||
attacker = _make_user("ws-attacker@plane.so")
|
||||
workspace = _make_workspace("Attacker Workspace", "attacker-ws", attacker)
|
||||
return workspace, attacker
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def victim_workspace(db):
|
||||
"""An unrelated workspace the attacker has no membership in."""
|
||||
victim_owner = _make_user("ws-victim-owner@plane.so")
|
||||
workspace = _make_workspace("Victim Workspace", "victim-ws", victim_owner)
|
||||
return workspace, victim_owner
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
@pytest.mark.django_db
|
||||
class TestWorkspaceMemberMassAssignment:
|
||||
def test_admin_cannot_move_member_to_another_workspace(self, attacker_workspace, victim_workspace):
|
||||
"""
|
||||
Core takeover vector: a workspace admin must not be able to relocate a
|
||||
member row into another workspace via the ``workspace`` FK.
|
||||
"""
|
||||
attacker_ws, attacker = attacker_workspace
|
||||
victim_ws, _ = victim_workspace
|
||||
|
||||
# A puppet account the attacker controls, sitting in the attacker workspace.
|
||||
puppet = _make_user("puppet@plane.so")
|
||||
puppet_member = _add_member(attacker_ws, puppet, role=15)
|
||||
|
||||
victim_members_before = WorkspaceMember.objects.filter(workspace=victim_ws).count()
|
||||
|
||||
client = APIClient()
|
||||
client.force_authenticate(user=attacker)
|
||||
response = client.patch(
|
||||
_member_detail_url(attacker_ws.slug, puppet_member.id),
|
||||
{"workspace": str(victim_ws.id), "role": 20},
|
||||
format="json",
|
||||
)
|
||||
|
||||
# The request itself succeeds (admin updating a role is legitimate) ...
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
# ... but the member row must NOT have moved to the victim workspace.
|
||||
puppet_member.refresh_from_db()
|
||||
assert puppet_member.workspace_id == attacker_ws.id
|
||||
assert puppet_member.workspace_id != victim_ws.id
|
||||
# And no new member appeared in the victim workspace.
|
||||
assert WorkspaceMember.objects.filter(workspace=victim_ws).count() == victim_members_before
|
||||
assert not WorkspaceMember.objects.filter(workspace=victim_ws, member=puppet).exists()
|
||||
|
||||
def test_patch_is_active_false_does_not_deactivate(self, attacker_workspace):
|
||||
"""``is_active`` must not be assignable through this endpoint."""
|
||||
attacker_ws, attacker = attacker_workspace
|
||||
target = _make_user("active-target@plane.so")
|
||||
target_member = _add_member(attacker_ws, target, role=15)
|
||||
|
||||
client = APIClient()
|
||||
client.force_authenticate(user=attacker)
|
||||
response = client.patch(
|
||||
_member_detail_url(attacker_ws.slug, target_member.id),
|
||||
{"is_active": False},
|
||||
format="json",
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
target_member.refresh_from_db()
|
||||
assert target_member.is_active is True
|
||||
|
||||
def test_legitimate_role_update_still_works(self, attacker_workspace):
|
||||
"""Positive control: the intended ``role`` update still functions."""
|
||||
attacker_ws, attacker = attacker_workspace
|
||||
target = _make_user("role-target@plane.so")
|
||||
target_member = _add_member(attacker_ws, target, role=15)
|
||||
|
||||
client = APIClient()
|
||||
client.force_authenticate(user=attacker)
|
||||
response = client.patch(
|
||||
_member_detail_url(attacker_ws.slug, target_member.id),
|
||||
{"role": 5},
|
||||
format="json",
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
target_member.refresh_from_db()
|
||||
assert target_member.role == 5
|
||||
|
||||
def test_self_role_update_still_forbidden(self, attacker_workspace):
|
||||
"""Positive control: the self-role-update guard is preserved."""
|
||||
attacker_ws, attacker = attacker_workspace
|
||||
own_member = WorkspaceMember.objects.get(workspace=attacker_ws, member=attacker)
|
||||
|
||||
client = APIClient()
|
||||
client.force_authenticate(user=attacker)
|
||||
response = client.patch(
|
||||
_member_detail_url(attacker_ws.slug, own_member.id),
|
||||
{"role": 5},
|
||||
format="json",
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
own_member.refresh_from_db()
|
||||
assert own_member.role == 20
|
||||
Reference in New Issue
Block a user