Django: An admin extension to prevent state leaking between requests

2024-04-29 Let’s talk about sealing classes from further attributes.

Here’s a small protection I added to a project a few years ago. I was considering it again since I saw a similar potential bug in a Django middleware.

Long live the `ModelAdmin` instances

Django’s admin site is configured by the ModelAdmin class. You register this per model:

from django.contrib import admin

from example.models import Book


@admin.register(Book)
class BookAdmin(admin.ModelAdmin):
    fields = [
        "title",
        "special",
    ]

The @admin.register() decorator calls the AdminSite.register() method, which creates an instance of the ModelAdmin class (which can also be auto-generated):

class AdminSite:
    ...

    def register(self, model_or_iterable, admin_class=None, **options):
        ...
        for model in model_or_iterable:
            ...
            # Ignore the registration if the model has been
            # swapped out.
            if not model._meta.swapped:
                ...
                # Instantiate the admin class to save in the registry
                self._registry[model] = admin_class(model, self)

So, ModelAdmin instances are created once at import time and reused between all requests. That means it’s not safe to use ModelAdmin instance variables to store state because they can affect later requests. (And if you run your project with a threaded WSGI server or ASGI server, ModelAdmin instance variables may be read by concurrent requests!)

This may be a surprising revelation if you have come from PHP, which has a “fresh process per request” model.

A leaky construction

Update (2024-05-03): Clarified the first example by using only instance-level attributes.

In one project, I encountered a bug due to a developer misunderstanding this behaviour. Below is a simplified version of what the code did. Can you spot the bug?

from django.contrib import admin

from example.models import Book


@admin.register(Book)
class BookAdmin(admin.ModelAdmin):
    fields = [
        "title",
        "special",
    ]

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.is_special_popup = False

    def changelist_view(self, request, extra_context=None):
        request.GET._mutable = True
        if request.GET.pop("special-popup", None):
            self.is_special_popup = True
        request.GET._mutable = False
        return super().changelist_view(request, extra_context=extra_context)

    def get_queryset(self, request):
        qs = super().get_queryset(request)
        if self.is_special_popup:
            qs = qs.filter(special=True)
        return qs

This admin class’s changelist could be opened in a popup for a particular process, which I’ve relabelled the “special popup”. In that case, the QuerySet needed to be limited since only particular objects were relevant for selection.

changelist_view() pulls the query parameter from request.GET and stores it in is_special_popup. This “sneaky stashing” is needed because the default ModelAdmin.changelist_view() drops unknown query parameters before calling get_queryset().

The issue was that the is_special_popup variable would persist as True, once set, leaking into future requests. Later requests to a non-special-popup list would still be filtered.

This behaviour occurred because is_special_popup was set to False as a class-level and set to True as an instance-level variable. The accessing code, self.is_special_popup, reads either version, preferring the instance-level variable. Once a request set the instance-level variable, it was never cleared, leaving it around for future requests.

This issue went undiscovered because:

In development, no one checked the main listing after opening the popup listing, at least without a runserver restart in between.
In production, users either did not notice the limited results or the server processes always happened to restart between problematic requests.
The unit tests used per-test admin class instances as a minor optimization over using Django’s test client.

I discovered the issue when changing the tests to use setUpTestData() (a huge optimization I always like applying).

Plugging the leak

The fix I came up with was to store state on the request object:

@@ -8,21 +8,16 @@
     fields = [
         "title",
         "special",
     ]

-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.is_special_popup = False
-
     def changelist_view(self, request, extra_context=None):
         request.GET._mutable = True
-        if request.GET.pop("special-popup", None):
-            self.is_special_popup = True
+        request.is_special_popup = bool(request.GET.pop("special-popup", None))
         request.GET._mutable = False
         return super().changelist_view(request, extra_context=extra_context)

     def get_queryset(self, request):
         qs = super().get_queryset(request)
-        if self.is_special_popup:
+        if getattr(request, "is_special_popup", False):
             qs = qs.filter(special=True)
         return qs

Giving:

from django.contrib import admin

from example.models import Book


@admin.register(Book)
class BookAdmin(admin.ModelAdmin):
    fields = [
        "title",
        "special",
    ]

    def changelist_view(self, request, extra_context=None):
        request.GET._mutable = True
        request.is_special_popup = bool(request.GET.pop("special-popup", None))
        request.GET._mutable = False
        return super().changelist_view(request, extra_context=extra_context)

    def get_queryset(self, request):
        qs = super().get_queryset(request)
        if getattr(request, "is_special_popup", False):
            qs = qs.filter(special=True)
        return qs

This version stores the “special popup” state on the request object, preventing inter-request leaking. Attaching extra attributes to request is the general pattern in Django, such as for request.session or request.site.

Permanent protection

After the initial fix, I wanted to ensure similar issues did not exist elsewhere, nor could they recur. I did this by moving the project to use custom AdminSite and ModelAdmin classes that block new attributes post-registration:

from functools import partial

from django.contrib import admin
from django.db.models.base import ModelBase


class ModelAdmin(admin.ModelAdmin):
    def __setattr__(self, name: str, value: bool | None) -> None:
        """
        Prevent setting attributes post-registration. See:
        https://adamj.eu/tech/2024/04/29/django-admin-prevent-leaking-requests/
        """
        if getattr(self, "_prevent_attr_setting", False):
            clsname = self.__class__.__qualname__
            raise AttributeError(
                f"Cannot set attribute {name!r} on {clsname} after "
                + "registration. If you are trying to store per-request "
                + " attributes, they will leak between requests."
            )
        return super().__setattr__(name, value)


class AdminSite(admin.AdminSite):
    def register(self, model_or_iterable, admin_class=None, **options):
        if admin_class is None:
            raise TypeError("Must provide a ModelAdmin class")

        if not issubclass(admin_class, ModelAdmin):
            raise TypeError(f"Only subclasses of {__name__}.ModelAdmin may be used.")

        super().register(model_or_iterable, admin_class, **options)

        # Prevent further attributes from being set on the ModelAdmin class.
        # This cannot be done in ModelAdmin.__init__ because that prevents
        # subclasses from adding attributes.
        if isinstance(model_or_iterable, ModelBase):
            model_or_iterable = [model_or_iterable]
        for model in model_or_iterable:
            if model in self._registry:
                self._registry[model]._prevent_attr_setting = True


admin_site = AdminSite()

register = partial(admin.register, site=admin_site)

The custom ModelAdmin class wraps __setattr__() to conditionally block new attributes. This “lock“ is enabled in the custom AdminSite.register() method. See Django’s documentation for more on using a custom site.

This block worked well enough for me, and I believe it found another issue. But it isn‘t perfect—state may persist in other places, like module-level variables.

Should Django do something here?

This issue isn’t specific to ModelAdmin. State can also persist between requests in other long-lived objects, at least in middleware classes. But class-based views are instantiated per request, at least (source).

I wonder if Django could add an “attribute block” to ModelAdmin, MiddlewareMixin, and maybe other places. It would require a deprecation path, but could flush out many latent and future bugs.

I have opened a forum discussion linked to this blog post. Please share your experiences and opinions there: “Add protection to some classes to prevent state leaking between requests?”.

Fin

May your request bucket never leak,

—Adam

Newly updated: my book Boost Your Django DX now covers Django 5.0 and Python 3.12.

One summary email a week, no spam, I pinky promise.

Related posts:

Tags: django