Fix for GHSA-w2j3-x3j3-mm43

dojo/admin.py

@@ -14,6 +14,20 @@
     TextQuestion,
 )
 
+
+# Django's default admin gate is `is_active and is_staff`. Under the legacy
+# OS auth model is_staff is already a near-superuser bypass for queryset
+# filters and most permission checks, so the standard UserAdmin change form
+# would let any is_staff user with auth.change_user tick is_superuser on
+# themselves. Rather than narrowing each ModelAdmin individually, restrict
+# the entire admin site to superusers — there is no DefectDojo OS role that
+# legitimately needs /admin/ access without being a superuser.
+def _admin_site_has_permission(request):
+    return request.user.is_active and request.user.is_superuser
+
+
+admin.site.has_permission = _admin_site_has_permission
+
 # Conditionally unregister LogEntry from auditlog if it's registered
 try:
     from auditlog.models import LogEntry

dojo/api_v2/serializers.py

@@ -613,6 +613,12 @@ def validate(self, data):
             msg = "Only superusers are allowed to add or edit superusers."
             raise ValidationError(msg)
 
+        instance_is_staff = self.instance.is_staff if self.instance is not None else False
+        data_is_staff = data.get("is_staff", instance_is_staff)
+        if not self.context["request"].user.is_superuser and data_is_staff != instance_is_staff:
+            msg = "Only superusers are allowed to add or edit staff users."
+            raise ValidationError(msg)
+
         if self.context["request"].method in {"PATCH", "PUT"} and "password" in data:
             msg = "Update of password though API is not allowed"
             raise ValidationError(msg)

dojo/settings/settings.dist.py

@@ -51,7 +51,7 @@
     DD_DJANGO_METRICS_ENABLED=(bool, False),
     DD_LOGIN_REDIRECT_URL=(str, "/"),
     DD_LOGIN_URL=(str, "/login"),
-    DD_DJANGO_ADMIN_ENABLED=(bool, True),
+    DD_DJANGO_ADMIN_ENABLED=(bool, False),
     DD_SESSION_COOKIE_HTTPONLY=(bool, True),
     DD_CSRF_COOKIE_HTTPONLY=(bool, True),
     DD_SECURE_SSL_REDIRECT=(bool, False),

dojo/settings/template-env

@@ -1,8 +1,9 @@
 # Django Debug, don't enable on production!
 DD_DEBUG=False
 
-# Enables Django Admin
-DD_DJANGO_ADMIN_ENABLED=True
+# Enables the Django admin interface at /admin/. Off by default; uncomment
+# this line to expose it. Access is restricted to superusers regardless.
+# DD_DJANGO_ADMIN_ENABLED=True
 
 # A secret key for a particular Django installation.
 DD_SECRET_KEY=#DD_SECRET_KEY#

unittests/test_adminsite.py

@@ -1,5 +1,8 @@
 import django.apps
 from django.contrib import admin
+from django.contrib.auth import get_user_model
+from django.contrib.auth.models import AnonymousUser
+from django.http import HttpRequest
 
 from .dojo_test_case import DojoTestCase
 
@@ -20,3 +23,56 @@ def test_is_model_defined(self):
                 else:
                     with self.subTest(type="tag", subclass=subclass):
                         self.assertIn(subclass, admin.site._registry.keys(), f"{subclass} is not registered in 'tagulous.admin' in models.py")
+
+
+class AdminAccessGate(DojoTestCase):
+
+    """
+    is_staff is a near-superuser bypass under the legacy OS auth model, so
+    /admin/ must require is_superuser. Django's default UserAdmin change
+    form would otherwise let any is_staff user with auth.change_user tick
+    is_superuser on themselves. Tested at the gate-function level so the
+    assertions hold regardless of whether DD_DJANGO_ADMIN_ENABLED mounts
+    the admin URLConf in the current environment.
+    """
+
+    @staticmethod
+    def _request_for(user):
+        req = HttpRequest()
+        req.user = user
+        return req
+
+    def test_staff_non_superuser_denied(self):
+        User = get_user_model()
+        password = "testTEST1234!@#$"
+        staff = User.objects.create_user(
+            username="staff-no-root", password=password, is_staff=True,
+        )
+        self.assertFalse(admin.site.has_permission(self._request_for(staff)))
+
+    def test_non_staff_non_superuser_denied(self):
+        User = get_user_model()
+        password = "testTEST1234!@#$"
+        plain = User.objects.create_user(username="plain-user", password=password)
+        self.assertFalse(admin.site.has_permission(self._request_for(plain)))
+
+    def test_anonymous_denied(self):
+        self.assertFalse(admin.site.has_permission(self._request_for(AnonymousUser())))
+
+    def test_inactive_superuser_denied(self):
+        User = get_user_model()
+        password = "testTEST1234!@#$"
+        root = User.objects.create_superuser(
+            username="inactive-root", email="i@example.com", password=password,
+        )
+        root.is_active = False
+        root.save(update_fields=["is_active"])
+        self.assertFalse(admin.site.has_permission(self._request_for(root)))
+
+    def test_active_superuser_allowed(self):
+        User = get_user_model()
+        password = "testTEST1234!@#$"
+        root = User.objects.create_superuser(
+            username="root-test", email="r@example.com", password=password,
+        )
+        self.assertTrue(admin.site.has_permission(self._request_for(root)))

unittests/test_apiv2_user.py

@@ -1,3 +1,4 @@
+from django.contrib.auth.models import Permission
 from django.urls import reverse
 from django.utils import timezone
 from rest_framework.authtoken.models import Token
@@ -201,6 +202,118 @@ def test_user_reset_api_token_denies_non_privileged(self):
         r = nonpriv_client.post(url)
         self.assertEqual(r.status_code, 403, r.content[:1000])
 
+    def test_non_superuser_cannot_set_is_staff_via_api(self):
+        """
+        A delegated user-manager (auth.change_user) must not be able to
+        flip is_staff on themselves or anyone else — is_staff is a
+        superuser-only flag under the legacy OS auth model, and granting
+        it via API would let a non-superuser pivot into Django admin /
+        full RBAC bypass.
+        """
+        password = "testTEST1234!@#$"
+        r = self.client.post(reverse("user-list"), {
+            "username": "api-user-mgr",
+            "email": "admin@dojo.com",
+            "password": password,
+        }, format="json")
+        self.assertEqual(r.status_code, 201, r.content[:1000])
+        mgr = User.objects.get(username="api-user-mgr")
+        mgr.user_permissions.add(
+            Permission.objects.get(codename="change_user"),
+            Permission.objects.get(codename="add_user"),
+        )
+
+        token_resp = self.client.post(reverse("api-token-auth"), {
+            "username": "api-user-mgr",
+            "password": password,
+        }, format="json")
+        self.assertEqual(token_resp.status_code, 200, token_resp.content[:1000])
+        mgr_client = APIClient()
+        mgr_client.credentials(HTTP_AUTHORIZATION="Token " + token_resp.json()["token"])
+
+        # Self-escalation: setting is_staff on own account must be rejected.
+        r = mgr_client.patch("{}{}/".format(reverse("user-list"), mgr.id), {
+            "is_staff": True,
+        }, format="json")
+        self.assertEqual(r.status_code, 400, r.content[:1000])
+        self.assertIn(
+            "Only superusers are allowed to add or edit staff users.",
+            r.content.decode("utf-8"),
+        )
+        mgr.refresh_from_db()
+        self.assertFalse(mgr.is_staff)
+
+        # Target-escalation: setting is_staff on another user must be rejected.
+        r = self.client.post(reverse("user-list"), {
+            "username": "api-user-target",
+            "email": "admin@dojo.com",
+            "password": password,
+        }, format="json")
+        self.assertEqual(r.status_code, 201, r.content[:1000])
+        target_id = r.json()["id"]
+
+        r = mgr_client.patch("{}{}/".format(reverse("user-list"), target_id), {
+            "is_staff": True,
+        }, format="json")
+        self.assertEqual(r.status_code, 400, r.content[:1000])
+        target = User.objects.get(id=target_id)
+        self.assertFalse(target.is_staff)
+
+        # Create-time escalation must also be rejected.
+        r = mgr_client.post(reverse("user-list"), {
+            "username": "api-user-staff-on-create",
+            "email": "admin@dojo.com",
+            "password": password,
+            "is_staff": True,
+        }, format="json")
+        self.assertEqual(r.status_code, 400, r.content[:1000])
+        self.assertFalse(User.objects.filter(username="api-user-staff-on-create").exists())
+
+    def test_non_superuser_can_patch_self_without_touching_is_staff(self):
+        """
+        Negative control for the is_staff guard: a delegated user-manager
+        can still PATCH non-privileged fields on their own account; the
+        new check only fires when is_staff actually changes.
+        """
+        password = "testTEST1234!@#$"
+        r = self.client.post(reverse("user-list"), {
+            "username": "api-user-mgr2",
+            "email": "admin@dojo.com",
+            "password": password,
+        }, format="json")
+        self.assertEqual(r.status_code, 201, r.content[:1000])
+        mgr = User.objects.get(username="api-user-mgr2")
+        mgr.user_permissions.add(Permission.objects.get(codename="change_user"))
+
+        token_resp = self.client.post(reverse("api-token-auth"), {
+            "username": "api-user-mgr2",
+            "password": password,
+        }, format="json")
+        self.assertEqual(token_resp.status_code, 200, token_resp.content[:1000])
+        mgr_client = APIClient()
+        mgr_client.credentials(HTTP_AUTHORIZATION="Token " + token_resp.json()["token"])
+
+        r = mgr_client.patch("{}{}/".format(reverse("user-list"), mgr.id), {
+            "first_name": "Renamed",
+        }, format="json")
+        self.assertEqual(r.status_code, 200, r.content[:1000])
+
+    def test_superuser_can_set_is_staff_via_api(self):
+        """Positive control: a superuser is still allowed to toggle is_staff."""
+        r = self.client.post(reverse("user-list"), {
+            "username": "api-user-promotable",
+            "email": "admin@dojo.com",
+            "password": "testTEST1234!@#$",
+        }, format="json")
+        self.assertEqual(r.status_code, 201, r.content[:1000])
+        user_id = r.json()["id"]
+
+        r = self.client.patch("{}{}/".format(reverse("user-list"), user_id), {
+            "is_staff": True,
+        }, format="json")
+        self.assertEqual(r.status_code, 200, r.content[:1000])
+        self.assertTrue(User.objects.get(id=user_id).is_staff)
+
     def test_user_reset_api_token_denies_global_owner_legacy(self):
         """
         Legacy: Global_Role(role=Owner) is inert. Resetting another