From e275acdec76bcb26c7743b67e9ed6a46364db2d1 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 20 Aug 2026 17:10:52 +0530 Subject: [PATCH 1/2] fix(security): refuse routed actions served by unauthorized DRF mixins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authorization in the app viewsets lives on the concrete method — an @allow_permission decorator or an inline role check. BaseViewSet subclasses DRF's ModelViewSet, which supplies list/retrieve/create/update/partial_update/ destroy for free, so when a URLconf maps a verb to an action the viewset does not implement, the request is served by the mixin with nothing but the bare default permission class. The caller is authenticated but not authorized at all, and the only thing between them and the object is whatever get_queryset() happens to filter on. Measured across the live URLconf: 225 routed actions, 27 of which resolved to a mixin under the bare default. The worst let any authenticated account with no membership in the target workspace rewrite a project it could not otherwise read — including its `workspace` field, since ProjectListSerializer declares fields="__all__" with no read_only_fields — re-parenting the project into the caller's own workspace. Others allowed overwriting or soft-deleting work items and comments with no activity record or webhook, reading project invitation tokens, and creating views in arbitrary workspaces by guessable slug. Guard it structurally in BaseViewSet.initial(): if the resolved action is one DRF's mixins provide and nothing in our own MRO implements it, refuse with 405 rather than letting the mixin operate. Three shapes are deliberately exempt — a custom @action, a perform_create override riding CreateModelMixin, and a viewset carrying a genuinely restrictive permission class. Permission classes are membership-tested rather than compared against the default, so a weaker declaration ([AllowAny], or an empty list) is not mistaken for a deliberate restrictive one. Point-fixing these one endpoint at a time is what produced two reports of the same class nine days apart, and it does not hold: of the routes that were not exploitable, most failed closed on an accident — a missing pk kwarg, or a decorator applied to a perform_create signature so it crashed before inserting — rather than on authorization. One lookup_url_kwarg change re-arms them. Also implements the five actions that clients do call and that were relying on a mixin, so they carry the same check as their siblings rather than being refused: issue and comment reaction list, project and workspace view create, workspace invitation list, and state retrieve. A contract test drives the real guard over Django's own resolver and asserts the refused set matches a reviewed manifest, in both directions, so a newly routed verb fails here instead of shipping unauthorized — and a fixed one cannot rot the list. A second manifest covers plane.api and plane.space, which define their own duplicated BaseViewSet and are not reached by this guard. Co-authored-by: Plane AI --- apps/api/plane/app/views/base.py | 102 ++++- apps/api/plane/app/views/issue/comment.py | 10 + apps/api/plane/app/views/issue/reaction.py | 10 + apps/api/plane/app/views/state/base.py | 10 + apps/api/plane/app/views/view/base.py | 23 + apps/api/plane/app/views/workspace/invite.py | 12 + .../views/test_routed_action_authorization.py | 422 ++++++++++++++++++ 7 files changed, 587 insertions(+), 2 deletions(-) create mode 100644 apps/api/plane/tests/unit/views/test_routed_action_authorization.py diff --git a/apps/api/plane/app/views/base.py b/apps/api/plane/app/views/base.py index 798845b22b3..2a2ed0ccc79 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -17,9 +17,9 @@ # Third part imports from rest_framework import status -from rest_framework.exceptions import APIException +from rest_framework.exceptions import APIException, MethodNotAllowed from rest_framework.filters import SearchFilter -from rest_framework.permissions import IsAuthenticated +from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView from rest_framework.viewsets import ModelViewSet @@ -45,6 +45,22 @@ def initial(self, request, *args, **kwargs): timezone.deactivate() +# Actions that DRF's generic mixins implement for us. Authorization in this +# codebase lives on the concrete method — either an @allow_permission decorator +# or an inline role check — so an action served straight from a mixin runs with +# nothing but `permission_classes`. Where that is the bare default, the request +# is authenticated but not authorized at all. +_MIXIN_PROVIDED_ACTIONS = frozenset({"list", "retrieve", "create", "update", "partial_update", "destroy"}) + +# Permission classes that establish identity but authorize nothing: they say +# who is calling, never what they may touch. A viewset carrying only these has +# delegated all of its authorization to per-method checks, so a mixin-served +# action has none. Tested for membership rather than comparing against the +# default, so that a weaker declaration than the default — `[AllowAny]`, or an +# empty list — is not mistaken for a deliberate, restrictive one. +_NON_AUTHORIZING_PERMISSIONS = frozenset({IsAuthenticated, AllowAny}) + + class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePaginator): model = None @@ -67,6 +83,88 @@ def get_queryset(self): log_exception(e) raise APIException("Please check the view", status.HTTP_400_BAD_REQUEST) + @classmethod + def _action_owner(cls, action): + """ + Return the class in the MRO that actually implements `action`. + + Used to tell "we wrote this method" from "DRF's generic mixin supplied + it". Inheriting the method from another class in this codebase counts as + ours: the authorization check travels with the implementation. + """ + for klass in cls.__mro__: + if action in vars(klass): + return klass + return None + + def _resolved_action_is_authorized(self): + """ + Whether the action this request resolved to carries any authorization. + + False only when a router mapped a verb to an action the viewset does not + implement, so the request is about to be served by a DRF mixin under the + bare default permission class. Every other shape is left alone. + """ + action = getattr(self, "action", None) + + # No action resolved (e.g. an OPTIONS probe) — DRF handles it. + if action is None: + return True + + # A custom @action is always explicitly written, so it carries whatever + # check its author put on it. + if action not in _MIXIN_PROVIDED_ACTIONS: + return True + + # A genuinely restrictive class-level permission class authorizes every + # action uniformly, including mixin-served ones. + if any(permission not in _NON_AUTHORIZING_PERMISSIONS for permission in self.permission_classes): + return True + + # Overriding perform_create is the documented way to ride + # CreateModelMixin.create while controlling what gets saved. Treat the + # override as the implementation, so this does not reject a deliberate + # pattern. NOTE: perform_create is a save hook, not an authorization + # hook — a viewset using it still needs its own permission check. + if action == "create" and self._action_owner("perform_create") is not None: + return True + + # DRF's partial_update delegates to self.update(), so a viewset that + # implements update() authorizes PATCH transitively even without its own + # partial_update. No viewset in the tree does this today, but the guard + # must not punish it if one appears. + if action == "partial_update": + update_owner = self._action_owner("update") + if update_owner is not None and not update_owner.__module__.startswith("rest_framework"): + return True + + owner = self._action_owner(action) + return owner is not None and not owner.__module__.startswith("rest_framework") + + def initial(self, request, *args, **kwargs): + # Runs after authentication and permission checks, so an anonymous + # caller still gets 401 rather than having the route's existence + # confirmed or denied first. + super().initial(request, *args, **kwargs) + + if not self._resolved_action_is_authorized(): + # The route exists but nothing authorizes it. Refuse rather than let + # the generic mixin operate on whatever get_queryset() happens to + # return. Reported as "method not allowed" because the correct state + # of the world is that this verb was never meant to be routed here. + # Logged as a warning without a traceback: the refusal is expected + # and carries no stack worth capturing, and anyone can trigger it by + # calling a dead route in a loop. A full logger.exception() here + # would hand them unbounded error-log amplification. + log_exception( + Exception( + f"Refused unauthorized mixin-served action: " + f"{type(self).__name__}.{self.action} via {request.method} {request.path}" + ), + warning=True, + ) + raise MethodNotAllowed(request.method) + def handle_exception(self, exc): """ Handle any exception that occurs, by returning an appropriate response, diff --git a/apps/api/plane/app/views/issue/comment.py b/apps/api/plane/app/views/issue/comment.py index 34fe0f9e4b9..e45d70bafc3 100644 --- a/apps/api/plane/app/views/issue/comment.py +++ b/apps/api/plane/app/views/issue/comment.py @@ -180,6 +180,16 @@ def get_queryset(self): .distinct() ) + @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) + def list(self, request, slug, project_id, comment_id): + # Declared explicitly so the routed GET carries the same project-role + # check as create() below. Without it the action is served by DRF's + # generic list mixin, which runs under the bare default permission + # class: authenticated but not authorized. get_queryset() already scopes + # to active project members, so this states that guarantee in the + # authorization layer instead of relying on a queryset filter to hold. + return super().list(request, slug=slug, project_id=project_id, comment_id=comment_id) + @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) def create(self, request, slug, project_id, comment_id): try: diff --git a/apps/api/plane/app/views/issue/reaction.py b/apps/api/plane/app/views/issue/reaction.py index c09e1e92442..fb78f77082c 100644 --- a/apps/api/plane/app/views/issue/reaction.py +++ b/apps/api/plane/app/views/issue/reaction.py @@ -42,6 +42,16 @@ def get_queryset(self): .distinct() ) + @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) + def list(self, request, slug, project_id, issue_id): + # Declared explicitly so the routed GET carries the same project-role + # check as create() below. Without it the action is served by DRF's + # generic list mixin, which runs under the bare default permission + # class: authenticated but not authorized. get_queryset() already scopes + # to active project members, so this states that guarantee in the + # authorization layer instead of relying on a queryset filter to hold. + return super().list(request, slug=slug, project_id=project_id, issue_id=issue_id) + @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) def create(self, request, slug, project_id, issue_id): serializer = IssueReactionSerializer(data=request.data) diff --git a/apps/api/plane/app/views/state/base.py b/apps/api/plane/app/views/state/base.py index 55c232fdf64..fc21a5aea42 100644 --- a/apps/api/plane/app/views/state/base.py +++ b/apps/api/plane/app/views/state/base.py @@ -42,6 +42,16 @@ def get_queryset(self): .distinct() ) + @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) + def retrieve(self, request, slug, project_id, pk): + # Declared explicitly so the routed GET carries the same project-role + # check as list() below. Without it the action is served by DRF's + # generic retrieve mixin under the bare default permission class: + # authenticated but not authorized. No client calls this today, but the + # route is live and a caller is already wired up one layer away, so it + # gets a real implementation rather than being left to fail closed. + return super().retrieve(request, slug=slug, project_id=project_id, pk=pk) + @invalidate_cache(path="workspaces/:slug/states/", url_params=True, user=False) @allow_permission([ROLE.ADMIN]) def create(self, request, slug, project_id): diff --git a/apps/api/plane/app/views/view/base.py b/apps/api/plane/app/views/view/base.py index 3aad55abcc6..bc464c4e6aa 100644 --- a/apps/api/plane/app/views/view/base.py +++ b/apps/api/plane/app/views/view/base.py @@ -53,6 +53,18 @@ class WorkspaceViewViewSet(BaseViewSet): serializer_class = IssueViewSerializer model = IssueView + @allow_permission(allowed_roles=[ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE") + def create(self, request, slug): + # Declared explicitly to put a workspace-membership check in front of + # the create. perform_create below resolves the workspace from the URL + # slug and saves, but it is a save hook, not an authorization hook: as + # an inherited generic create this ran under the bare default permission + # class, so any authenticated account could create a view in any + # workspace. Workspace slugs are human-readable, so no identifier had to + # be guessed. The role set matches list() — the defect is that + # non-members could write here at all, not which member roles may. + return super().create(request, slug=slug) + def perform_create(self, serializer): workspace = Workspace.objects.get(slug=self.kwargs.get("slug")) serializer.save(workspace_id=workspace.id, owned_by=self.request.user) @@ -263,6 +275,17 @@ class IssueViewViewSet(BaseViewSet): serializer_class = IssueViewSerializer model = IssueView + @allow_permission(allowed_roles=[ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) + def create(self, request, slug, project_id): + # Declared explicitly to put a project-membership check in front of the + # create. perform_create below trusts project_id straight from the URL, + # and as an inherited generic create it ran under the bare default + # permission class — so any authenticated account holding a project + # identifier could create a view inside that project. The role set + # matches list()/retrieve(); the defect is that non-members could write + # here at all, not which member roles may. + return super().create(request, slug=slug, project_id=project_id) + def perform_create(self, serializer): serializer.save(project_id=self.kwargs.get("project_id"), owned_by=self.request.user) diff --git a/apps/api/plane/app/views/workspace/invite.py b/apps/api/plane/app/views/workspace/invite.py index 0c61bce4419..edb69c21544 100644 --- a/apps/api/plane/app/views/workspace/invite.py +++ b/apps/api/plane/app/views/workspace/invite.py @@ -269,6 +269,18 @@ def get_queryset(self): super().get_queryset().filter(email=self.request.user.email).select_related("workspace") ) + def list(self, request): + # Declared explicitly rather than served by DRF's generic list mixin. + # This route is self-service and carries no `slug`, so the project/ + # workspace role decorators cannot apply here: the authorization + # boundary IS the `email=request.user.email` filter in get_queryset() + # above, exactly as it is for create() below. Stating the action + # explicitly keeps that boundary from being silently widened if the + # queryset is ever refactored, and keeps this route out of the + # unauthorized-mixin-action class. The response carries the invite token, + # so the predicate is credential-grade and must not be relaxed. + return super().list(request) + @invalidate_cache(path="/api/workspaces/", user=False) @invalidate_cache(path="/api/users/me/workspaces/", multiple=True) def create(self, request): diff --git a/apps/api/plane/tests/unit/views/test_routed_action_authorization.py b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py new file mode 100644 index 00000000000..417c90a2043 --- /dev/null +++ b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py @@ -0,0 +1,422 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +""" +Repo-wide invariant: no routed action may be served by a DRF generic mixin +while the viewset relies on the bare default permission class. + +Authorization in this codebase lives on the concrete method — an +@allow_permission decorator or an inline role check. DRF's ModelViewSet supplies +list/retrieve/create/update/partial_update/destroy for free, so when a URLconf +maps a verb to an action the viewset does not implement, the request is served +by the mixin and runs with nothing but `permission_classes`. Where that is the +bare default, the request is authenticated but not authorized at all: the only +thing standing between the caller and the object is whatever get_queryset() +happens to filter on. + +That class of defect has been found repeatedly, one endpoint at a time. The +runtime guard on BaseViewSet closes it; this test keeps it closed. + +The route list is taken from Django's own resolver rather than by parsing the +URLconf files, and each verdict comes from calling the real guard rather than +reimplementing it. Both choices are deliberate. An earlier version of this scan +parsed `as_view({...})` with a regex matching `\\w+ViewSet` and silently missed +every class spelled `Viewset` — including three routes on +ProjectInvitationsViewset that expose invitation tokens. Asking the resolver and +the guard directly cannot drift from what actually ships. +""" + +import pytest + +# Actions DRF's generic mixins implement for us. +MIXIN_PROVIDED_ACTIONS = frozenset({"list", "retrieve", "create", "update", "partial_update", "destroy"}) + + +def _routed_actions(): + """Every (viewset class, verb, action) triple Django actually routes to a + BaseViewSet subclass.""" + from django.urls import get_resolver + + from plane.app.views.base import BaseViewSet + + found = {} + + def walk(patterns): + for pattern in patterns: + nested = getattr(pattern, "url_patterns", None) + if nested is not None: + walk(nested) + continue + callback = getattr(pattern, "callback", None) + viewset = getattr(callback, "cls", None) + action_map = getattr(callback, "actions", None) + if viewset is None or action_map is None: + continue + if not (isinstance(viewset, type) and issubclass(viewset, BaseViewSet)): + continue + for verb, action in action_map.items(): + found[(viewset.__name__, verb, action)] = viewset + + walk(get_resolver().url_patterns) + return found + + +def _guard_rejected(): + """Routes the runtime guard on BaseViewSet will refuse.""" + rejected = set() + for (name, verb, action), viewset in _routed_actions().items(): + instance = viewset() + instance.action = action + if instance._resolved_action_is_authorized() is False: + rejected.add((name, verb, action)) + return rejected + + +# Routes that map a verb to an action no viewset implements, on a viewset that +# relies on the bare default permission class. The guard refuses these with 405, +# so they are not exploitable — but they are also not reachable, and that has to +# stay a deliberate, reviewed statement rather than an accident. +# +# Every entry was checked against all clients (CE apps/web, space, admin, +# packages; the full EE tree; the frozen plane-one snapshot) and has no caller. +# Notably every PUT here is dead product-wide: the only put() calls that touch +# an app URL are updateModule (no callers anywhere) and updateState (no PUT +# route exists). Every live mutation path uses PATCH. +# +# Adding to this list is a decision, not a formality. Do it only when no client +# calls the route. Otherwise implement the action with the check its sibling +# actions use, give the viewset a real permission class, or drop the verb from +# the route map. +# +# The mobile client was NOT searched when this list was built - no mobile repo +# was available. If mobile calls /api/ app routes rather than /api/v1/, verify +# against it before trusting any entry here. +GUARD_REJECTED_ROUTES = frozenset( + { + ("CycleFavoriteViewSet", "get", "list"), + ("CycleIssueViewSet", "get", "retrieve"), + ("CycleIssueViewSet", "patch", "partial_update"), + ("CycleIssueViewSet", "put", "update"), + ("CycleViewSet", "put", "update"), + ("IntakeViewSet", "get", "retrieve"), + ("IntakeViewSet", "patch", "partial_update"), + ("IssueCommentViewSet", "get", "list"), + ("IssueCommentViewSet", "get", "retrieve"), + ("IssueCommentViewSet", "put", "update"), + ("IssueViewFavoriteViewSet", "get", "list"), + ("IssueViewSet", "put", "update"), + ("IssueViewViewSet", "put", "update"), + ("ModuleIssueViewSet", "get", "retrieve"), + ("ModuleIssueViewSet", "patch", "partial_update"), + ("ModuleIssueViewSet", "put", "update"), + ("ModuleViewSet", "put", "update"), + ("NotificationViewSet", "delete", "destroy"), + ("NotificationViewSet", "get", "retrieve"), + ("ProjectFavoritesViewSet", "get", "list"), + # These three served ProjectMemberInviteSerializer with fields="__all__" + # over a queryset scoped only by slug and project_id, so any + # authenticated caller holding a project id could read every pending + # invitation's email address and accept token, or delete invitations. + # No client calls them in either edition. + ("ProjectInvitationsViewset", "delete", "destroy"), + ("ProjectInvitationsViewset", "get", "list"), + ("ProjectInvitationsViewset", "get", "retrieve"), + ("ProjectViewSet", "put", "update"), + ("UserProjectInvitationsViewset", "get", "list"), + ("WorkspaceStickyViewSet", "get", "retrieve"), + ("WorkspaceViewViewSet", "put", "update"), + } +) + + +@pytest.mark.unit +def test_routes_are_discovered(): + """Guard against this test silently passing because it found nothing.""" + routes = _routed_actions() + assert len(routes) > 150, ( + f"only {len(routes)} routed actions resolved - the resolver walk is broken, so this test is blind" + ) + + +@pytest.mark.unit +def test_guard_rejected_routes_match_the_reviewed_manifest(): + """The set of guard-refused routes must be exactly what we signed off on. + + Asserted both ways on purpose: + + * Something NEW appeared - a verb was routed to an action nobody implements, + or a method was renamed or deleted out from under its route. Left alone it + returns 405 to whatever client calls it, which is a silent outage, and + before the guard existed it was an unauthorized endpoint. + + * Something was FIXED but not removed from the manifest, which lets the list + rot into a description of the past. + """ + rejected = _guard_rejected() + + def render(entries): + return "\n".join(f" {name}.{action} routed from {verb.upper()}" for name, verb, action in sorted(entries)) + + appeared = rejected - GUARD_REJECTED_ROUTES + resolved = GUARD_REJECTED_ROUTES - rejected + + messages = [] + if appeared: + messages.append( + "New routed actions with no authorization. The guard on BaseViewSet " + "refuses these with 405, so any client calling them breaks.\n" + + render(appeared) + + "\n\nImplement the action with the check its sibling actions use, give the " + "viewset a real permission class, or remove the verb from the route map. " + "Only add it to GUARD_REJECTED_ROUTES once you have confirmed no client " + "calls it - including mobile." + ) + if resolved: + messages.append( + "These are listed as guard-refused but are now authorized. Remove them " + "from GUARD_REJECTED_ROUTES so the list keeps describing reality.\n" + render(resolved) + ) + + assert not messages, "\n\n".join(messages) + + +# The same fall-through shape on the other two surfaces. `plane.api` (external +# v1) and `plane.space` (published boards) each define their OWN BaseViewSet, +# duplicated from the app one, so the runtime guard does not reach them. +# +# `plane.api` is currently clean: both of its fall-throughs sit on viewsets with +# real permission classes. Everything below is `plane.space`, all reads, all on +# the bare default permission class. They are listed rather than fixed here +# because the published-board clients have not been checked against them and a +# blanket refusal could break public boards. +# +# This manifest exists so the surface cannot grow silently while the app surface +# is guarded — the exact "fixed app/, left api/" pattern this whole class of bug +# keeps arriving through. +UNGUARDED_OTHER_SURFACE_ROUTES = frozenset( + { + ("plane.space.views.issue", "CommentReactionPublicViewSet", "get", "list"), + ("plane.space.views.issue", "IssueCommentPublicViewSet", "get", "list"), + ("plane.space.views.issue", "IssueCommentPublicViewSet", "get", "retrieve"), + ("plane.space.views.issue", "IssueReactionPublicViewSet", "get", "list"), + ("plane.space.views.issue", "IssueVotePublicViewSet", "get", "list"), + } +) + + +def _other_surface_fall_throughs(): + """Fall-throughs outside plane.app, found structurally. + + Cannot call the runtime guard here: these viewsets descend from a different, + duplicated BaseViewSet that does not have it. + """ + from django.urls import get_resolver + from rest_framework.permissions import IsAuthenticated + + found = set() + + def owner(cls, action): + for klass in cls.__mro__: + if action in vars(klass): + return klass + return None + + def walk(patterns): + for pattern in patterns: + nested = getattr(pattern, "url_patterns", None) + if nested is not None: + walk(nested) + continue + callback = getattr(pattern, "callback", None) + viewset = getattr(callback, "cls", None) + action_map = getattr(callback, "actions", None) + if viewset is None or action_map is None: + continue + if viewset.__module__.startswith("plane.app"): + continue + for verb, action in action_map.items(): + if action not in MIXIN_PROVIDED_ACTIONS: + continue + implemented = owner(viewset, action) + if implemented is not None and not implemented.__module__.startswith("rest_framework"): + continue + if action == "create" and owner(viewset, "perform_create") is not None: + continue + declared = tuple(getattr(viewset, "permission_classes", ()) or ()) + if declared not in ((IsAuthenticated,), ()): + continue + found.add((viewset.__module__, viewset.__name__, verb, action)) + + walk(get_resolver().url_patterns) + return found + + +@pytest.mark.unit +def test_other_surfaces_do_not_grow_new_fall_throughs(): + """plane.api and plane.space must not accumulate more of this class. + + Asserted both ways, same reasoning as the app-surface manifest. + """ + found = _other_surface_fall_throughs() + + def render(entries): + return "\n".join( + f" {mod}.{name}.{action} routed from {verb.upper()}" for mod, name, verb, action in sorted(entries) + ) + + appeared = found - UNGUARDED_OTHER_SURFACE_ROUTES + resolved = UNGUARDED_OTHER_SURFACE_ROUTES - found + + messages = [] + if appeared: + messages.append( + "New routed actions with no authorization outside plane.app. These surfaces " + "have their own BaseViewSet and are NOT covered by the runtime guard, so " + "each of these is served by a DRF mixin under the bare default permission " + "class.\n" + render(appeared) + ) + if resolved: + messages.append( + "These are listed as unguarded but are now authorized. Remove them from " + "UNGUARDED_OTHER_SURFACE_ROUTES.\n" + render(resolved) + ) + + assert not messages, "\n\n".join(messages) + + +@pytest.mark.unit +def test_guard_is_actually_wired_into_request_handling(): + """The helper being correct is worthless if nothing calls it. + + Drives a real request through dispatch() to prove the refusal happens during + request handling, not just that a predicate returns False somewhere. Without + this, deleting BaseViewSet.initial's guard clause leaves every other test in + this module green. + """ + from rest_framework.test import APIRequestFactory, force_authenticate + + from plane.app.views.base import BaseViewSet + + class _User: + is_authenticated = True + is_active = True + user_timezone = "UTC" + pk = 1 + id = 1 + + class Unguarded(BaseViewSet): + """Defines partial_update but not update - the shape that PUT falls + through on.""" + + def partial_update(self, request, *args, **kwargs): # pragma: no cover + ... + + class Guarded(BaseViewSet): + def update(self, request, *args, **kwargs): + from rest_framework.response import Response + + return Response({"ok": True}) + + factory = APIRequestFactory() + + # PUT routed to an action nobody implements must be refused. + request = factory.put("/x/", {}, format="json") + force_authenticate(request, user=_User()) + response = Unguarded.as_view({"put": "update"})(request) + assert response.status_code == 405, f"guard did not refuse the fall-through (got {response.status_code})" + + # The same route, with the action implemented, must still work - proving the + # guard discriminates rather than blocking PUT wholesale. This is the + # positive control. + request = factory.put("/x/", {}, format="json") + force_authenticate(request, user=_User()) + response = Guarded.as_view({"put": "update"})(request) + assert response.status_code == 200, f"guard wrongly refused an implemented action (got {response.status_code})" + + +@pytest.mark.unit +def test_guard_recognises_the_patterns_it_must_not_reject(): + """The guard's exemptions, checked against DRF's real mixins. + + Each of these is a shape that looks like a fall-through but is authorized, + and rejecting any of them would break working endpoints. + """ + from plane.app.views.base import BaseViewSet + + class OwnImplementation(BaseViewSet): + def partial_update(self, request): # pragma: no cover - never called + ... + + class RidesCreateMixin(BaseViewSet): + def perform_create(self, serializer): # pragma: no cover - never called + ... + + class TransitivelyAuthorizesPatch(BaseViewSet): + def update(self, request): # pragma: no cover - never called + ... + + class HasRealPermissionClass(BaseViewSet): + permission_classes = [object] + + def verdict(cls, action): + instance = cls() + instance.action = action + return instance._resolved_action_is_authorized() + + # The defect itself: nobody implements it, bare default permission class. + assert verdict(OwnImplementation, "update") is False + + # Implemented on our own class. + assert verdict(OwnImplementation, "partial_update") is True + + # perform_create override is the documented way to ride CreateModelMixin. + assert verdict(RidesCreateMixin, "create") is True + + # DRF's partial_update delegates to self.update(), so implementing update() + # authorizes PATCH transitively. + assert verdict(TransitivelyAuthorizesPatch, "partial_update") is True + + # A class-level permission class authorizes every action uniformly. + assert verdict(HasRealPermissionClass, "update") is True + + # A declaration WEAKER than the default must not be mistaken for a + # deliberate restrictive one. Testing "is it different from the default" + # instead of "does it actually authorize" would exempt these - on the two + # settings where a fall-through is most dangerous. + from rest_framework.permissions import AllowAny, IsAuthenticated + + class Anonymous(BaseViewSet): + permission_classes = [AllowAny] + + class NoPermissionsAtAll(BaseViewSet): + permission_classes = [] + + class IdentityOnly(BaseViewSet): + permission_classes = [IsAuthenticated] + + assert verdict(Anonymous, "update") is False + assert verdict(NoPermissionsAtAll, "update") is False + assert verdict(IdentityOnly, "update") is False + + # A real permission class alongside the identity one still authorizes. + class MixedPermissions(BaseViewSet): + permission_classes = [IsAuthenticated, object] + + assert verdict(MixedPermissions, "update") is True + + # A custom @action is always explicitly written. + assert verdict(OwnImplementation, "some_custom_action") is True + + # No action resolved (e.g. an OPTIONS probe) - leave it to DRF. + instance = OwnImplementation() + instance.action = None + assert instance._resolved_action_is_authorized() is True + + # Inheriting the implementation from another class in this codebase counts: + # the authorization check travels with the implementation. + class InheritsFromOurs(TransitivelyAuthorizesPatch): + pass + + assert verdict(InheritsFromOurs, "update") is True + assert InheritsFromOurs._action_owner("update") is TransitivelyAuthorizesPatch From e43770738f4f2e65878a28267d425afb8ff856f7 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 20 Aug 2026 18:01:07 +0530 Subject: [PATCH 2/2] fix(security): share the non-authorizing permission set with the route scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught a real divergence. The runtime guard treats `{IsAuthenticated, AllowAny}` as non-authorizing, but the scan covering plane.api and plane.space restated the rule as "not (IsAuthenticated,) and not ()". That silently accepted `[AllowAny]` as a deliberate restrictive declaration, so a viewset there declaring AllowAny and falling through to a DRF mixin would never appear in the manifest — on plane.space, the one surface where AllowAny is routine (10+ classes use it today, all APIViews rather than viewsets, so nothing is currently masked). The two tests also directly contradicted each other: one asserts that shape is unauthorized while the other skipped it. The scan now imports `_NON_AUTHORIZING_PERMISSIONS` from the guard instead of restating it, so the two cannot drift apart again. This is the same mistake the guard itself had before review — "differs from the default" is not the same question as "actually authorizes" — and restating a rule in a second place is what let it survive in one of them. Also: correct the comment on that set, which claimed both classes "establish identity" when AllowAny does not; make initial()'s comment precise about what ordering after super() actually buys (the permission classes' own rejection and status code win, rather than a 405 disclosing that the route exists); and use `pass` rather than a bare ellipsis for the test stub bodies. Co-authored-by: Plane AI --- apps/api/plane/app/views/base.py | 21 ++++++++++++------- .../views/test_routed_action_authorization.py | 19 +++++++++++------ 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/apps/api/plane/app/views/base.py b/apps/api/plane/app/views/base.py index 2a2ed0ccc79..f94943c8586 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -52,12 +52,15 @@ def initial(self, request, *args, **kwargs): # is authenticated but not authorized at all. _MIXIN_PROVIDED_ACTIONS = frozenset({"list", "retrieve", "create", "update", "partial_update", "destroy"}) -# Permission classes that establish identity but authorize nothing: they say -# who is calling, never what they may touch. A viewset carrying only these has +# Permission classes that authorize nothing: neither says anything about what +# the caller may touch. `IsAuthenticated` only establishes that there is a +# caller; `AllowAny` does not even do that. A viewset carrying only these has # delegated all of its authorization to per-method checks, so a mixin-served -# action has none. Tested for membership rather than comparing against the -# default, so that a weaker declaration than the default — `[AllowAny]`, or an -# empty list — is not mistaken for a deliberate, restrictive one. +# action has none. +# +# Membership-tested rather than compared against the default, so a declaration +# that is *weaker* than the default — `[AllowAny]`, or an empty list — is not +# mistaken for a deliberate restrictive one. _NON_AUTHORIZING_PERMISSIONS = frozenset({IsAuthenticated, AllowAny}) @@ -142,9 +145,11 @@ def _resolved_action_is_authorized(self): return owner is not None and not owner.__module__.startswith("rest_framework") def initial(self, request, *args, **kwargs): - # Runs after authentication and permission checks, so an anonymous - # caller still gets 401 rather than having the route's existence - # confirmed or denied first. + # Deliberately after super(), which runs authentication and the + # permission classes. Whatever they would have rejected is still + # rejected first and with their own status — an unauthenticated caller + # gets 401 from IsAuthenticated rather than learning from a 405 that the + # route exists. super().initial(request, *args, **kwargs) if not self._resolved_action_is_authorized(): diff --git a/apps/api/plane/tests/unit/views/test_routed_action_authorization.py b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py index 417c90a2043..c4ce6afc4b8 100644 --- a/apps/api/plane/tests/unit/views/test_routed_action_authorization.py +++ b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py @@ -212,7 +212,13 @@ def _other_surface_fall_throughs(): duplicated BaseViewSet that does not have it. """ from django.urls import get_resolver - from rest_framework.permissions import IsAuthenticated + + # Imported rather than restated. An earlier version of this helper listed + # only `(IsAuthenticated,)` and `()` as non-authorizing, which silently + # treated `[AllowAny]` as a deliberate restrictive declaration — on + # plane.space, the one surface where AllowAny is routine. Sharing the guard's + # own definition keeps the two from drifting apart again. + from plane.app.views.base import _NON_AUTHORIZING_PERMISSIONS found = set() @@ -244,7 +250,7 @@ def walk(patterns): if action == "create" and owner(viewset, "perform_create") is not None: continue declared = tuple(getattr(viewset, "permission_classes", ()) or ()) - if declared not in ((IsAuthenticated,), ()): + if any(permission not in _NON_AUTHORIZING_PERMISSIONS for permission in declared): continue found.add((viewset.__module__, viewset.__name__, verb, action)) @@ -310,7 +316,8 @@ class Unguarded(BaseViewSet): through on.""" def partial_update(self, request, *args, **kwargs): # pragma: no cover - ... + + pass class Guarded(BaseViewSet): def update(self, request, *args, **kwargs): @@ -346,15 +353,15 @@ def test_guard_recognises_the_patterns_it_must_not_reject(): class OwnImplementation(BaseViewSet): def partial_update(self, request): # pragma: no cover - never called - ... + pass class RidesCreateMixin(BaseViewSet): def perform_create(self, serializer): # pragma: no cover - never called - ... + pass class TransitivelyAuthorizesPatch(BaseViewSet): def update(self, request): # pragma: no cover - never called - ... + pass class HasRealPermissionClass(BaseViewSet): permission_classes = [object]