From d3f96f32271e1c845ff12ccae0fb42992ef09287 Mon Sep 17 00:00:00 2001 From: Jonathan Weth <git@jonathanweth.de> Date: Thu, 13 Feb 2025 21:07:27 +0100 Subject: [PATCH 1/2] Fix protection for batch mutations --- aleksis/core/schema/base.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/aleksis/core/schema/base.py b/aleksis/core/schema/base.py index cd9c29f1c..896585444 100644 --- a/aleksis/core/schema/base.py +++ b/aleksis/core/schema/base.py @@ -111,7 +111,8 @@ class PermissionBatchCreateMixin: @classmethod def after_create_obj(cls, root, info, data, obj, input): # noqa - if isinstance(cls._meta.permissions, Iterable) and not info.context.user.has_perms( + super().after_create_obj(root, info, data, obj, input) + if not isinstance(cls._meta.permissions, Iterable) or not info.context.user.has_perms( cls._meta.permissions, obj ): raise PermissionDenied() @@ -129,7 +130,8 @@ class PermissionBatchPatchMixin: @classmethod def after_update_obj(cls, root, info, input, obj, full_input): # noqa - if isinstance(cls._meta.permissions, Iterable) and not info.context.user.has_perms( + super().after_update_obj(root, info, input, obj, full_input) + if not isinstance(cls._meta.permissions, Iterable) or not info.context.user.has_perms( cls._meta.permissions, obj ): raise PermissionDenied() @@ -147,10 +149,12 @@ class PermissionBatchDeleteMixin: @classmethod def before_save(cls, root, info, ids, qs_to_delete): # noqa - if isinstance(cls._meta.permissions, Iterable): - for obj in qs_to_delete: - if not info.context.user.has_perms(cls._meta.permissions, obj): - raise PermissionDenied() + super().before_save(root, info, ids, qs_to_delete) + if not isinstance(cls._meta.permissions, Iterable): + raise PermissionDenied() + for obj in qs_to_delete: + if not info.context.user.has_perms(cls._meta.permissions, obj): + raise PermissionDenied() class PermissionPatchMixin: @@ -264,10 +268,12 @@ class ModelValidationMixin: @classmethod def after_update_obj(cls, root, info, data, obj, full_input): + super().after_update_obj(root, info, data, obj, full_input) obj.full_clean() @classmethod def before_create_obj(cls, info, data, obj): + super().before_create_obj(info, data, obj) obj.full_clean() -- GitLab From 65c332a70a37198aea928e148bf5ebf061e7e5af Mon Sep 17 00:00:00 2001 From: Jonathan Weth <git@jonathanweth.de> Date: Thu, 13 Feb 2025 21:12:36 +0100 Subject: [PATCH 2/2] Add two checks to prevent security issues with GraphQL mutations/queries --- aleksis/core/checks.py | 29 ++++++++++++++++++++++++++++- aleksis/core/schema/__init__.py | 12 ++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/aleksis/core/checks.py b/aleksis/core/checks.py index 68ccb06ba..d206c808d 100644 --- a/aleksis/core/checks.py +++ b/aleksis/core/checks.py @@ -1,9 +1,11 @@ +from collections.abc import Iterable from typing import Optional import django.apps -from django.core.checks import Tags, Warning, register # noqa +from django.core.checks import Error, Tags, Warning, register # noqa from .mixins import ExtensibleModel, GlobalPermissionModel, PureDjangoModel +from .schema.base import BaseBatchCreateMutation, BaseBatchDeleteMutation, BaseBatchPatchMutation from .util.apps import AppConfig @@ -71,3 +73,28 @@ def check_app_models_base_class( ) return results + + +@register(Tags.security) +def check_all_mutations_with_permissions( + app_configs: Optional[django.apps.registry.Apps] = None, **kwargs +) -> list: + results = [] + for base_class in [BaseBatchCreateMutation, BaseBatchPatchMutation, BaseBatchDeleteMutation]: + for subclass in base_class.__subclasses__(): + if ( + not isinstance(subclass._meta.permissions, Iterable) + or not subclass._meta.permissions + ): + results.append( + Error( + f"Mutation {subclass.__name__} doesn't set required permission", + hint=( + "Ensure that the mutation is protected by setting the " + "permissions attribute in the mutation's Meta class." + ), + obj=subclass, + id="aleksis.core.E001", + ) + ) + return results diff --git a/aleksis/core/schema/__init__.py b/aleksis/core/schema/__init__.py index 696e2867e..97de056ca 100644 --- a/aleksis/core/schema/__init__.py +++ b/aleksis/core/schema/__init__.py @@ -4,6 +4,7 @@ from django.db.models import Q import graphene import graphene_django_optimizer +from graphene.types.resolver import dict_or_attr_resolver, set_default_resolver from guardian.shortcuts import get_objects_for_user from haystack.inputs import AutoQuery from haystack.query import SearchQuerySet @@ -81,6 +82,17 @@ from .two_factor import TwoFactorType from .user import UserType +def custom_default_resolver(attname, default_value, root, info, **args): + """Custom default resolver to ensure resolvers are set for all queries.""" + if info.parent_type.name == "GlobalQuery": + raise NotImplementedError(f"No own resolver defined for {attname}") + + return dict_or_attr_resolver(attname, default_value, root, info, **args) + + +set_default_resolver(custom_default_resolver) + + class Query(graphene.ObjectType): ping = graphene.String(payload=graphene.String()) -- GitLab