From 96c33544e610d634d25285b3f65faa977c845065 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 27 Aug 2026 10:51:06 +0530 Subject: [PATCH] fix(security): resolve the guard's permissions from get_permissions(), mirror partial_update on the other-surface scan The class-level check read self.permission_classes directly, so a viewset that restricts a mixin-served action only through a get_permissions() override - without mutating the class attribute - would be invisible to the guard and refused with a false 405. Nothing in the codebase requires an override to mutate permission_classes as a side effect, so the guard now calls self.get_permissions() and checks the effective, instantiated permissions instead. get_permissions() is already invoked once earlier in the same request via check_permissions(); calling it again here is the same pattern DRF itself relies on and is safe given the one existing override in this codebase has no side effects beyond reassigning permission_classes. The other-surface (plane.api/plane.space) scan special-cased create's perform_create delegation but had no equivalent for partial_update delegating to an overridden update() - DRF's UpdateModelMixin.partial_update calls self.update(), so that shape is actually authorized transitively, same as the runtime guard already recognises. Added the matching exemption so the scan does not misclassify it as an unguarded fall-through, and extracted the per-route classification into _other_surface_route_is_unguarded() so it can be exercised directly with synthetic viewsets instead of only through the URL resolver. The Copilot comment about _NON_AUTHORIZING_PERMISSIONS claiming both permissions "establish identity" was already corrected in a prior commit on this branch (e43770738f4) - verified against current code, no further change needed there. Co-authored-by: Plane AI --- apps/api/plane/app/views/base.py | 23 ++- .../views/test_routed_action_authorization.py | 161 +++++++++++++++--- 2 files changed, 153 insertions(+), 31 deletions(-) diff --git a/apps/api/plane/app/views/base.py b/apps/api/plane/app/views/base.py index 06bdcddd71..07169a0b30 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -119,9 +119,26 @@ class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePagi 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): + # A genuinely restrictive permission class authorizes every action + # uniformly, including mixin-served ones. + # + # Reads the *effective* permissions for this action via + # get_permissions() rather than the static permission_classes + # attribute. A viewset can override get_permissions() to return a + # restrictive permission for a specific action without ever mutating + # self.permission_classes, and nothing enforces that it must - reading + # the class attribute directly would miss that override and refuse an + # actually-authorized action with a false 405. + # + # get_permissions() was already called once this request, by + # super().initial() above (through DRF's own check_permissions()). + # Calling it again here is the same pattern DRF itself relies on + # whenever it needs the current permission set, and is safe as long as + # get_permissions() has no side effects beyond what it already has - + # the one override in this codebase today only reassigns + # self.permission_classes before delegating to super(), which is + # idempotent to run twice. + if any(type(permission) not in _NON_AUTHORIZING_PERMISSIONS for permission in self.get_permissions()): return True # Overriding perform_create is the documented way to ride 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 0f65bec1f9..a9d8a29f46 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 @@ -205,6 +205,56 @@ UNGUARDED_OTHER_SURFACE_ROUTES = frozenset( ) +def _other_surface_owner(cls, action): + for klass in cls.__mro__: + if action in vars(klass): + return klass + return None + + +def _other_surface_route_is_unguarded(viewset, action): + """Structural classification mirroring the runtime guard's rules. + + Pulled out of `_other_surface_fall_throughs()`'s walk so it can be unit + tested directly against synthetic viewsets, without needing a real routed + URL to drive it through the resolver. + """ + # 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 + + if action not in MIXIN_PROVIDED_ACTIONS: + return False + + implemented = _other_surface_owner(viewset, action) + if implemented is not None and not implemented.__module__.startswith("rest_framework"): + return False + + if action == "create": + perform_create_owner = _other_surface_owner(viewset, "perform_create") + if perform_create_owner is not None and not perform_create_owner.__module__.startswith("rest_framework"): + return False + + # Mirrors the runtime guard's partial_update exemption: DRF's + # partial_update delegates to self.update(), so a viewset that implements + # update() authorizes PATCH transitively even without its own + # partial_update. Without this, the scan misclassifies that shape as an + # unguarded fall-through. + if action == "partial_update": + update_owner = _other_surface_owner(viewset, "update") + if update_owner is not None and not update_owner.__module__.startswith("rest_framework"): + return False + + declared = tuple(getattr(viewset, "permission_classes", ()) or ()) + if any(permission not in _NON_AUTHORIZING_PERMISSIONS for permission in declared): + return False + + return True + + def _other_surface_fall_throughs(): """Fall-throughs outside plane.app, found structurally. @@ -213,21 +263,8 @@ def _other_surface_fall_throughs(): """ from django.urls import get_resolver - # 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() - 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) @@ -242,21 +279,8 @@ def _other_surface_fall_throughs(): 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": - perform_create_owner = owner(viewset, "perform_create") - if perform_create_owner is not None and not perform_create_owner.__module__.startswith( - "rest_framework" - ): - continue - declared = tuple(getattr(viewset, "permission_classes", ()) or ()) - if any(permission not in _NON_AUTHORIZING_PERMISSIONS for permission in declared): - continue - found.add((viewset.__module__, viewset.__name__, verb, action)) + if _other_surface_route_is_unguarded(viewset, action): + found.add((viewset.__module__, viewset.__name__, verb, action)) walk(get_resolver().url_patterns) return found @@ -295,6 +319,44 @@ def test_other_surfaces_do_not_grow_new_fall_throughs(): assert not messages, "\n\n".join(messages) +@pytest.mark.unit +def test_other_surface_scan_treats_inherited_partial_update_as_authorized_via_update(): + """The other-surface scan must mirror the runtime guard's partial_update rule. + + A secondary-surface viewset (`plane.api`/`plane.space`) that overrides + update() but inherits DRF's partial_update() authorizes PATCH transitively, + exactly like the app-surface guard already recognises in + `_resolved_action_is_authorized()`. Without the matching exemption here, the + scan would misclassify the PATCH route as an unguarded fall-through - a + false positive in the scan's own classification, not a real vulnerability. + """ + from rest_framework.permissions import IsAuthenticated + from rest_framework.viewsets import ModelViewSet + + class OverridesUpdateOnly(ModelViewSet): + """Shape from the CodeRabbit report: owns update(), inherits + partial_update() from DRF's UpdateModelMixin, bare default permission + class - same as any other secondary-surface viewset.""" + + permission_classes = [IsAuthenticated] + + def update(self, request, *args, **kwargs): # pragma: no cover - never called + pass + + # partial_update is not our own - it is DRF's UpdateModelMixin.partial_update + # - but it delegates straight into the update() override above, so this must + # NOT be reported as unguarded. + assert _other_surface_route_is_unguarded(OverridesUpdateOnly, "partial_update") is False + + # Negative control: neither update() nor partial_update() implemented, bare + # default permission class - this is the real defect shape and must still + # be caught. + class ImplementsNeither(ModelViewSet): + permission_classes = [IsAuthenticated] + + assert _other_surface_route_is_unguarded(ImplementsNeither, "partial_update") is True + + @pytest.mark.unit def test_guard_is_actually_wired_into_request_handling(): """The helper being correct is worthless if nothing calls it. @@ -439,3 +501,46 @@ def test_guard_recognises_the_patterns_it_must_not_reject(): assert verdict(InheritsFromOurs, "update") is True assert InheritsFromOurs._action_owner("update") is TransitivelyAuthorizesPatch + + +@pytest.mark.unit +def test_guard_uses_effective_permissions_not_the_static_class_attribute(): + """CodeRabbit's scenario: get_permissions() overridden, permission_classes untouched. + + A viewset can restrict a mixin-provided action entirely from + get_permissions() without ever mutating self.permission_classes - nothing + requires an override to mutate the class attribute as a side effect (the + one override that exists in this codebase today happens to do so, but that + is not a contract the guard may rely on). Reading self.permission_classes + directly would miss a restrictive get_permissions() override entirely and + refuse the request with a false 405. The guard must consult the effective, + per-action permissions instead. + """ + from rest_framework.permissions import IsAuthenticated + + from plane.app.views.base import BaseViewSet + + class DenyAll: + """A real, restrictive permission that is not in the non-authorizing set.""" + + def has_permission(self, request, view): # pragma: no cover - never called + return False + + class RestrictsOnlyViaGetPermissions(BaseViewSet): + # Stays at the bare, non-authorizing default - the guard must not be + # fooled by this into thinking the action is unguarded. + permission_classes = [IsAuthenticated] + + def get_permissions(self): + # Deliberately does NOT touch self.permission_classes - returns a + # restrictive instance straight from the method, which is the gap + # CodeRabbit flagged. + return [DenyAll()] + + instance = RestrictsOnlyViaGetPermissions() + instance.action = "update" + + assert instance.permission_classes == [IsAuthenticated], "class attribute must stay untouched by this shape" + assert instance._resolved_action_is_authorized() is True, ( + "guard must authorize via the effective get_permissions() result, not the static class attribute" + )