From e43770738f4f2e65878a28267d425afb8ff856f7 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 20 Aug 2026 18:01:07 +0530 Subject: [PATCH] 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 2a2ed0ccc7..f94943c858 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -52,12 +52,15 @@ class TimezoneMixin: # 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 @@ class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePagi 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 417c90a204..c4ce6afc4b 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 _other_surface_fall_throughs(): 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 @@ def test_guard_is_actually_wired_into_request_handling(): 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]