Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-08-29 10:08:51 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): resolve the guard's permissions from get_permissions(), mirror partial_update on the other-surface scan

Browse Source 
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 (e43770738f) - verified against current code, no further change
needed there.

Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
Manish Gupta
2026-08-27 10:51:06 +05:30
parent c31bb76efa
commit 96c33544e6
2 changed files with 153 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
apps/api/plane/app/views/base.py
View File

@@ -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

159
apps/api/plane/tests/unit/views/test_routed_action_authorization.py
View File

@@ -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,20 +279,7 @@ 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
if _other_surface_route_is_unguarded(viewset, action):
found.add((viewset.__module__, viewset.__name__, verb, action))
walk(get_resolver().url_patterns)
@@ -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"
)