mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
fix(security): resolve perform_create against its actual owner, not just its presence
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 <noreply@plane.so>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user