From c31bb76efa41498d97c6e8e72bb4b6e820c32af2 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 27 Aug 2026 10:22:58 +0530 Subject: [PATCH] fix(security): resolve perform_create against its actual owner, not just its presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The create branch of _resolved_action_is_authorized() checked only whether perform_create was defined anywhere in the MRO. DRF's CreateModelMixin always defines perform_create, so the check was unconditionally true and never refused an unauthorized create fall-through — the exact vulnerability class this guard exists to close, just live on the one action the guard's own review missed. The same broken check was independently duplicated in the routed-action test's other-surface scan, so the regression suite couldn't catch it either. Both call sites now require the owner to actually be ours, mirroring the existing partial_update branch. Added the missing negative test: a viewset that overrides neither create nor perform_create must still be refused. Re-ran the full route manifest scan (app and other-surface) after the fix - no new routes appeared, confirming no live create endpoint was relying on the bug. Co-authored-by: Plane AI --- apps/api/plane/app/views/base.py | 14 ++++++++++++-- .../views/test_routed_action_authorization.py | 16 ++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/apps/api/plane/app/views/base.py b/apps/api/plane/app/views/base.py index f94943c858..06bdcddd71 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -129,8 +129,18 @@ class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePagi # 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 + # + # perform_create must be resolved the same way every other action is: + # CreateModelMixin itself always defines perform_create, so checking + # only "is it defined" is always true and never rejects anything. The + # owner has to actually be ours (not rest_framework's) for this to mean + # the viewset overrode it. + if action == "create": + perform_create_owner = self._action_owner("perform_create") + if perform_create_owner is not None and not perform_create_owner.__module__.startswith( + "rest_framework" + ): + return True # DRF's partial_update delegates to self.update(), so a viewset that # implements update() authorizes PATCH transitively even without its own 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 c4ce6afc4b..0f65bec1f9 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 @@ -247,8 +247,12 @@ def _other_surface_fall_throughs(): 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 + 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 @@ -374,6 +378,14 @@ def test_guard_recognises_the_patterns_it_must_not_reject(): # The defect itself: nobody implements it, bare default permission class. assert verdict(OwnImplementation, "update") is False + # The same defect on "create" specifically: CreateModelMixin always + # defines perform_create, so a check that only asks "is perform_create + # defined" (rather than "did *we* define it") is always true and never + # rejects anything. A viewset that overrides neither create nor + # perform_create, with the bare default permission class, must still be + # refused. + assert verdict(OwnImplementation, "create") is False + # Implemented on our own class. assert verdict(OwnImplementation, "partial_update") is True