From 3cec6c872e126e375ae8671aa97ab16e5c023a76 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 20 Aug 2026 17:49:02 +0530 Subject: [PATCH 1/2] fix(security): require workspace membership to read a global view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkspaceViewViewSet.retrieve was the only action on the class with no authorization check, and its queryset supplied none either. It filters on workspace__slug and then `Q(owned_by=request.user) | Q(access=1)`. That second clause reads as a visibility predicate but is vacuous: `access` sits in IssueViewSerializer.read_only_fields so the API never sets it, and the model defaults it to 1 (Public), so every row matches. Any authenticated account holding a view id could therefore read any global view in any workspace, including one it had no membership in. Requires workspace membership, matching the role set on list(). Also adds regression coverage for IssueDetailIdentifierEndpoint. That endpoint was reported as missing the guest restriction; it is not — the membership check at the top of get() is followed, after the issue is fetched, by an explicit role-5 / guest_view_all_features / created_by check. It had no test, so a guard preventing a guest from walking PROJ-1..PROJ-N and reading every work item's description_html was one refactor from being lost silently. Verified non-vacuous: neutering that check makes the test fail with 200 and the foreign work item's full payload. Co-authored-by: Plane AI --- apps/api/plane/app/views/view/base.py | 10 + .../app/test_undecorated_action_authz_app.py | 263 ++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py diff --git a/apps/api/plane/app/views/view/base.py b/apps/api/plane/app/views/view/base.py index bc464c4e6aa..c56ea7657c7 100644 --- a/apps/api/plane/app/views/view/base.py +++ b/apps/api/plane/app/views/view/base.py @@ -117,7 +117,17 @@ def partial_update(self, request, slug, pk): return Response(serializer.data, status=status.HTTP_200_OK) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) + @allow_permission(allowed_roles=[ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE") def retrieve(self, request, slug, pk): + # Requires workspace membership, matching list() above. This was the only + # action on this class with no check at all, and get_queryset() does not + # supply one either: it filters on workspace__slug and then + # `Q(owned_by=request.user) | Q(access=1)`. That second clause looks like + # a visibility predicate but is vacuous — `access` is in the + # serializer's read_only_fields so the API never sets it, and the model + # defaults it to 1 (Public), so every row matches. A caller with no + # membership in the workspace could therefore read any global view in it + # by id. issue_view = self.get_queryset().filter(pk=pk).first() serializer = IssueViewSerializer(issue_view) recent_visited_task.delay( diff --git a/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py new file mode 100644 index 00000000000..8f5b4495dec --- /dev/null +++ b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py @@ -0,0 +1,263 @@ +# 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 viewset actions whose authorization lives outside a decorator. + +These actions are *defined* on their viewset, so a scan for routes falling +through to a DRF generic mixin cannot see them, and neither carries an +``@allow_permission`` decorator. That makes them easy to misread in both +directions — one of the two turned out to be guarded after all. + +1. **Fix.** ``WorkspaceViewViewSet.retrieve`` + (``/workspaces//views//``) had no check at all, and its queryset + supplied none either: it filters on ``workspace__slug`` and then + ``Q(owned_by=request.user) | Q(access=1)``. That second clause reads like a + visibility predicate but is vacuous — ``access`` sits in the serializer's + ``read_only_fields`` so the API never sets it, and the model defaults it to + ``1`` (Public), so every row matches. A user with no membership in the + workspace could read any global view in it by id. It now requires workspace + membership, matching ``list``. + +2. **Regression coverage only, no fix.** ``IssueDetailIdentifierEndpoint`` + (``/workspaces//work-items/-/``) resolves a work item by its + human-readable sequence number, which makes it an enumeration surface. It was + reported as missing the guest restriction. **It is not.** The membership check + near the top of ``get`` is followed, after the issue is fetched, by an + explicit role-5 / ``guest_view_all_features`` / ``created_by`` check that + returns 403. That guard had no test, so the disclosure it prevents was one + refactor away from being lost silently. These tests pin it. +""" + +from uuid import uuid4 + +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from plane.db.models import ( + Issue, + IssueView, + Project, + ProjectMember, + User, + Workspace, + WorkspaceMember, +) + +WORK_ITEM_BY_IDENTIFIER_URL = "/api/workspaces/{slug}/work-items/{project_identifier}-{sequence_id}/" +WORKSPACE_VIEW_DETAIL_URL = "/api/workspaces/{slug}/views/{pk}/" + + +def _make_user(prefix): + """A saved, active user with a unique email.""" + unique_id = uuid4().hex[:8] + user = User.objects.create( + email=f"{prefix}-{unique_id}@plane.so", + username=f"{prefix}_{unique_id}", + first_name=prefix.title(), + last_name="User", + ) + user.set_password("test-password") + user.save() + return user + + +def _client_for(user): + client = APIClient() + client.force_authenticate(user=user) + return client + + +@pytest.fixture +def project(db, workspace, create_user): + """A project with guest_view_all_features at its default of False.""" + project = Project.objects.create( + name="Scoped Project", + identifier="SP", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20) + return project + + +@pytest.fixture +def guest(db, workspace, project): + """An active project GUEST (role=5).""" + user = _make_user("guest") + WorkspaceMember.objects.create(workspace=workspace, member=user, role=5) + ProjectMember.objects.create(project=project, member=user, workspace=workspace, role=5) + return user + + +@pytest.fixture +def guest_client(guest): + return _client_for(guest) + + +def _make_issue(name, project, workspace, author): + """Create an issue with a deterministic ``created_by``. + + ``BaseModel.save`` sets ``created_by`` from the current request user, which + is anonymous under tests, so a ``created_by=`` kwarg to ``create`` is + discarded. Passing ``created_by_id`` to ``save`` sets it explicitly. + """ + issue = Issue(name=name, project=project, workspace=workspace) + issue.save(created_by_id=author.id) + return issue + + +@pytest.fixture +def own_issue(db, workspace, project, guest): + return _make_issue("Guest's own work item", project, workspace, guest) + + +@pytest.fixture +def foreign_issue(db, workspace, project, create_user): + return _make_issue("Someone else's work item", project, workspace, create_user) + + +@pytest.mark.contract +class TestWorkItemByIdentifierGuestScope: + """A restricted guest must not resolve work items they did not author. + + Pins an existing guard rather than covering a new fix. This endpoint resolves + by sequence number, so losing the guard would let a guest walk + ``PROJ-1..PROJ-N`` and read every work item's full detail — including + ``description_html`` — plus the project and issue UUIDs that every other + endpoint keys on. + """ + + @pytest.mark.django_db + def test_guest_cannot_read_foreign_work_item(self, guest_client, workspace, project, foreign_issue): + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_403_FORBIDDEN, ( + f"Restricted guest resolved a foreign work item: {response.status_code} {getattr(response, 'data', None)!r}" + ) + + @pytest.mark.django_db + def test_guest_can_read_their_own_work_item(self, guest_client, workspace, project, own_issue): + """Positive control: the guard must not lock a guest out of their own item.""" + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=own_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(own_issue.id) + + @pytest.mark.django_db + def test_full_member_can_read_any_work_item(self, session_client, workspace, project, foreign_issue): + """Positive control: a full member is unaffected.""" + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = session_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(foreign_issue.id) + + @pytest.mark.django_db + def test_guest_with_view_all_features_can_read_any_work_item(self, guest_client, workspace, project, foreign_issue): + """Positive control: the guard is conditional on the project setting.""" + project.guest_view_all_features = True + project.save(update_fields=["guest_view_all_features"]) + + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + @pytest.mark.django_db + def test_non_member_is_refused(self, workspace, project, foreign_issue): + """A user with no project membership must still be refused outright.""" + outsider = _make_user("outsider") + WorkspaceMember.objects.create(workspace=workspace, member=outsider, role=15) + + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = _client_for(outsider).get(url) + + assert response.status_code == status.HTTP_403_FORBIDDEN, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + +@pytest.fixture +def workspace_view(db, workspace, create_user): + """A global (project-less) workspace view owned by the workspace owner.""" + view = IssueView(name="Global view", workspace=workspace, owned_by=create_user, access=1) + view.save() + return view + + +@pytest.mark.contract +class TestWorkspaceViewRetrieveRequiresMembership: + """Reading a global view must require membership of its workspace.""" + + @pytest.mark.django_db + def test_outsider_cannot_read_a_global_view(self, workspace, workspace_view): + """A user in a different workspace entirely, holding only the view id.""" + outsider = _make_user("outsider") + other_workspace = Workspace.objects.create( + name="Outsider Workspace", + owner=outsider, + slug=f"outsider-{uuid4().hex[:8]}", + ) + WorkspaceMember.objects.create(workspace=other_workspace, member=outsider, role=20) + + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = _client_for(outsider).get(url) + + assert response.status_code in ( + status.HTTP_403_FORBIDDEN, + status.HTTP_404_NOT_FOUND, + ), f"Non-member read a global view: {response.status_code} {getattr(response, 'data', None)!r}" + + @pytest.mark.django_db + def test_workspace_member_can_read_a_global_view(self, session_client, workspace, workspace_view): + """Positive control: a member of the workspace still reads it.""" + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = session_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(workspace_view.id) + + @pytest.mark.django_db + def test_workspace_guest_can_read_a_global_view(self, workspace, workspace_view): + """Positive control: the role set matches list(), which permits guests.""" + guest_user = _make_user("wsguest") + WorkspaceMember.objects.create(workspace=workspace, member=guest_user, role=5) + + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = _client_for(guest_user).get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) From 6f065b76212e8b8bc6e69819d0f0d8abb78b63a1 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 20 Aug 2026 18:11:23 +0530 Subject: [PATCH 2/2] fix: 404 instead of a hollow 200 when a global view does not exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch, verified before fixing. WorkspaceViewViewSet.retrieve resolves the view with .first() and serialized the result unconditionally, so a member asking for an id that does not exist got 200 with every field null or empty (`{"name": "", "description": "", "filters": null, ...}`) and a recent-visit enqueued for a nonexistent entity. Because get_queryset() is scoped to the URL workspace, the same happened for a real view id belonging to a different workspace. Returns 404, matching the other retrieve endpoints. Noted while confirming this, not fixed here: the project-level sibling IssueViewViewSet.retrieve has the same .first() pattern and then dereferences `issue_view.owned_by`, which raises AttributeError on None rather than answering 404 — a 500 instead of a hollow 200. Different method, so it gets its own ticket rather than widening this one. Co-authored-by: Plane AI --- apps/api/plane/app/views/view/base.py | 12 ++++++++++++ .../app/test_undecorated_action_authz_app.py | 14 ++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/apps/api/plane/app/views/view/base.py b/apps/api/plane/app/views/view/base.py index c56ea7657c7..863b165eb09 100644 --- a/apps/api/plane/app/views/view/base.py +++ b/apps/api/plane/app/views/view/base.py @@ -129,6 +129,18 @@ def retrieve(self, request, slug, pk): # membership in the workspace could therefore read any global view in it # by id. issue_view = self.get_queryset().filter(pk=pk).first() + + # get_queryset() is scoped to the URL workspace, so None means either no + # such view or one belonging to a workspace this URL does not name. + # Serializing None yields a hollow object — every field null or empty — + # returned as 200, and enqueues a recent-visit for an entity that does + # not exist. Answer 404, as the other retrieve endpoints do. + if issue_view is None: + return Response( + {"error": "The required object does not exist."}, + status=status.HTTP_404_NOT_FOUND, + ) + serializer = IssueViewSerializer(issue_view) recent_visited_task.delay( slug=slug, diff --git a/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py index 8f5b4495dec..3f409327073 100644 --- a/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py +++ b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py @@ -249,6 +249,20 @@ def test_workspace_member_can_read_a_global_view(self, session_client, workspace ) assert str(response.data["id"]) == str(workspace_view.id) + @pytest.mark.django_db + def test_missing_view_is_a_404_not_an_empty_200(self, session_client, workspace): + """A member asking for a view id that does not exist must get 404. + + get_queryset() is scoped to the URL workspace, so this also covers a real + view id belonging to a different workspace. + """ + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=uuid4()) + response = session_client.get(url) + + assert response.status_code == status.HTTP_404_NOT_FOUND, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + @pytest.mark.django_db def test_workspace_guest_can_read_a_global_view(self, workspace, workspace_view): """Positive control: the role set matches list(), which permits guests."""