Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
15 changes: 15 additions & 0 deletions apps/api/plane/app/views/project/member.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,14 @@ def get_queryset(self, slug, project_id, member_id):

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
def patch(self, request, slug, project_id, member_id):
# Preferences are personal: a member may only read/modify their OWN
# preferences. member_id is a URL param, so reject any mismatch to prevent
# cross-member IDOR (GHSA-gx67-r6wp-3357).
if str(member_id) != str(request.user.id):
return Response(
{"error": "You cannot access another member's preferences."},
status=status.HTTP_403_FORBIDDEN,
)
project_member = self.get_queryset(slug, project_id, member_id)

serializer = ProjectMemberPreferenceSerializer(project_member, {"preferences": request.data}, partial=True)
Expand All @@ -401,6 +409,13 @@ def patch(self, request, slug, project_id, member_id):

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
def get(self, request, slug, project_id, member_id):
# Preferences are personal: a member may only read their OWN preferences.
# member_id is a URL param, so reject any mismatch (GHSA-gx67-r6wp-3357).
if str(member_id) != str(request.user.id):
return Response(
{"error": "You cannot access another member's preferences."},
status=status.HTTP_403_FORBIDDEN,
)
project_member = self.get_queryset(slug, project_id, member_id)

serializer = ProjectMemberPreferenceSerializer(project_member)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

"""Contract tests for ProjectMemberPreferenceEndpoint ownership scoping.

Regression coverage for GHSA-gx67-r6wp-3357. The endpoint takes a ``member_id``
URL parameter and loaded the ``ProjectMember`` by ``(project_id, member_id,
workspace__slug)`` with no check that ``member_id`` is the caller — so any project
member (including a Guest) could read and modify any other member's per-project
preferences.

The fix rejects any request where ``member_id != request.user.id`` (403);
preferences are personal.
"""

from uuid import uuid4

import pytest
from rest_framework import status
from rest_framework.test import APIClient

from plane.db.models import Project, ProjectMember, User, WorkspaceMember

PREF_URL = "/api/workspaces/{slug}/projects/{project_id}/preferences/member/{member_id}/"


def _member(workspace, project, *, role):
unique = uuid4().hex[:8]
user = User.objects.create(email=f"pref-{role}-{unique}@plane.so", username=f"pref_{role}_{unique}")
user.set_password("test-password")
user.save()
WorkspaceMember.objects.create(workspace=workspace, member=user, role=role, is_active=True)
ProjectMember.objects.create(project=project, member=user, workspace=workspace, role=role, is_active=True)
return user


def _client(user):
client = APIClient()
client.force_authenticate(user=user)
return client


@pytest.fixture
def project(db, workspace, create_user):
project = Project.objects.create(
name="Pref Project", identifier="PR", workspace=workspace, created_by=create_user
)
ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20, is_active=True)
return project


@pytest.mark.contract
@pytest.mark.django_db
class TestMemberPreferenceScope:
"""A member may only read/modify their OWN project preferences."""

def test_member_cannot_read_others_preferences(self, workspace, project, create_user):
attacker = _member(workspace, project, role=15)
# attacker requests the admin (create_user)'s preferences
response = _client(attacker).get(
PREF_URL.format(slug=workspace.slug, project_id=project.id, member_id=create_user.id)
)
assert response.status_code == status.HTTP_403_FORBIDDEN, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)

def test_member_cannot_modify_others_preferences(self, workspace, project, create_user):
attacker = _member(workspace, project, role=15)
victim_member = ProjectMember.objects.get(project=project, member=create_user)
original = victim_member.preferences

response = _client(attacker).patch(
PREF_URL.format(slug=workspace.slug, project_id=project.id, member_id=create_user.id),
{"pinned": ["hacked"]},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
victim_member.refresh_from_db()
assert victim_member.preferences == original, "Another member's preferences were modified"

def test_member_can_read_own_preferences(self, workspace, project):
member = _member(workspace, project, role=15)
member_record = ProjectMember.objects.get(project=project, member=member)
member_record.preferences = {"pinned": ["existing"]}
member_record.save(update_fields=["preferences"])

response = _client(member).get(
PREF_URL.format(slug=workspace.slug, project_id=project.id, member_id=member.id)
)
assert response.status_code == status.HTTP_200_OK, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
# Assert the seeded value is actually served, not just that the route 200s —
# a queryset regression that returned the wrong member would still pass on
# status alone.
assert response.data["preferences"] == {"pinned": ["existing"]}
assert str(response.data["member_id"]) == str(member.id)
Comment on lines +86 to +100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Cover the Guest role in these contract tests.

Every test member in this class uses role=15. The repository maps 15 to ROLE.MEMBER and 5 to ROLE.GUEST, so the new assertions do not exercise the Guest path described in the PR objective. Parameterize the cross-member and self-service read/write tests for both roles. (raw.githubusercontent.com)

Also applies to: 104-106

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/tests/contract/app/test_member_preference_scope_app.py` around
lines 86 - 100, The contract tests currently cover only role 15 (member);
parameterize the cross-member and self-service read/write tests in this class to
run for both role 15 (ROLE.MEMBER) and role 5 (ROLE.GUEST). Update each affected
test’s setup and test identifiers to use the parameterized role while preserving
the existing assertions and behavior checks.

Source: MCP tools


def test_member_can_modify_own_preferences(self, workspace, project):
member = _member(workspace, project, role=15)
member_record = ProjectMember.objects.get(project=project, member=member)
original = dict(member_record.preferences)

response = _client(member).patch(
PREF_URL.format(slug=workspace.slug, project_id=project.id, member_id=member.id),
{"pinned": ["my-view"]},
format="json",
)
assert response.status_code == status.HTTP_200_OK, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
assert response.data["preferences"]["pinned"] == ["my-view"]

# The write must actually persist — a no-op PATCH would still return 200.
member_record.refresh_from_db()
assert member_record.preferences["pinned"] == ["my-view"]

# ProjectMemberPreferenceSerializer.validate_preferences merges into the
# existing dict rather than replacing it, so the untouched default keys
# must survive. Pins that semantic against a wholesale-replace regression.
for key, value in original.items():
assert member_record.preferences[key] == value
Loading