RemoteUserMiddleware.header does not correspond to request.META in the async path

[Jacob Walls]

When Django 5.2 added an async path to RemoteUserMiddleware (#35303), it prefixed HTTP_ to the provided (or default) header name before looking it up in request.META.

I figure this was so that the default REMOTE_USER header would work out of the box. (For WSGI compatibility, ASGIRequest maps all user provided headers to HTTP_... variants.)

This has two problematic effects:

• It becomes tortured to document the security considerations, see recent change in #36862:

Under WSGI, this warning doesn’t apply to RemoteUserMiddleware in its default configuration with header = "REMOTE_USER", since a key that doesn’t start with HTTP_ in request.META can only be set by your WSGI server, not directly from an HTTP request header. This warning does apply by default on ASGI, because in the async path, the middleware prepends HTTP_ to the defined header name before looking it up in request.META.

Notice how an implementation detail is leaking into an admonition, making it harder to reason about what's potentially user-controlled (wait, under ASGI, this other stuff is user-controlled, too!)

• Custom headers can't be supplied in a way that works with both WSGI & ASGI:

If you try to do this the "right" way, and define HTTP_MY_CUSTOM_HEADER as the header attribute, then aprocess_request() will lookup HTTP_HTTP_MY_CUSTOM_HEADER. Forcing the 'wrong' version (CUSTOM_HEADER) works on ASGI but breaks on WSGI, because no WSGI server will set that.

---

I'm suggesting we should deprecate the current support for custom headers without "HTTP_" prefixes in the async path (aprocess_request) and remove it in Django 7. The final code in Django 7 would look like:

django/contrib/auth/middleware.py

@@ -184 +184 @@
                 " before the RemoteUserMiddleware class."
             )
         try:
-            username = request.META["HTTP_" + self.header]
+            username = request.META[self.header]
         except KeyError:
             # If specified header doesn't exist then remove any existing
             # authenticated remote-user, or return (leaving request.user set to

Regrettably, we might need a transitional setting (or class attribute) to be able to opt into (or out of) the future, since I'm reluctant to add any automatic checking of other forms of headers, which will just open an avenue for security reports describing header spoofing. Open to ideas.

[Sarah Boyce]

Thank you for the ticket

From reviewing the PR that implemented this, I would say our approach was to get test parity by async-ifying the sync tests. This was then required for the tests to pass and, therefore, seemed required. I am now under the impression that our logic as to how we wrote the async tests must be off. Have you looked into what changes may be required there? I think that will help identify why we got this wrong

This is a partial duplicate of #36443 and #36300

[Carlton Gibson]

Grrr. I'm not sure of the value of this change. It looks like a breaking change without benefit to my eye. Indeed, if I'm using this with ASGI I would be forced to subclass in order to provide the full header value, HTTP_REMOTE_USER, to use the middleware at all.

The WSGI implementation is based on the idea that in the normal case the request.META entry is not in fact an HTTP header, but an environment variable. In contrast in the ASGI case, the value must come from a header value, as there's no other way. Thus the difference in implementation.

I'm pretty sceptical that people are needing both WSGI and ASGI versions of the middleware with the same custom header value. Possibly would could add an resolve_meta_key helper, called on the aprocess_request path that formatted the header, maybe via a class attribute, which could then be overridden to unify the approach if a fully qualified header was provided. 🤷

[Sarah Boyce]

I think perhaps the docs example may need an update to show that the async case of setting header does not need the HTTP_ prefix

If your authentication mechanism uses a custom HTTP header and not
``REMOTE_USER``, you can subclass ``RemoteUserMiddleware`` and set the
``header`` attribute to the desired ``request.META`` key. For example:

.. code-block:: python
   :caption: ``mysite/middleware.py``

    from django.contrib.auth.middleware import RemoteUserMiddleware


    class CustomHeaderRemoteUserMiddleware(RemoteUserMiddleware):
        header = "HTTP_AUTHUSER"

I haven't confirmed this but I wonder if this might be the breaking change in our implementation, perhaps custom headers set like this used to work when RemoteUserMiddleware was using the MiddlewareMixin on ASGI but now do not, yet the default implementation probably didn't work to begin with. So _possibly_ a bug in the #31224 implementation, partially fixed by #35303 and broke something else.

[Jacob Walls]

Thanks both. So the original proposal definitely involves breaking RemoteUserMiddleware + ASGI + default configuration, on the theory that if almost no one is using this, it's OK to make this "broken by default" , because "broken by default" = "secure by default". But the header_prefix idea is interesting!

That said, the recent doc edits are sufficient, so my motivation here was less about security and more "easier to reason about", given that if we lean toward Sarah's proposed doc edit in comment:6, then I wonder if we're doubling down on documenting a bug.

---

To Sarah's question in comment:3, to get the tests working, we would need to adjust how we parameterize the header names in the tests. I recently learned that the sync request factory expects prefixed header names, e.g. "HTTP_MYHEADER", but the async request factory expects unprefixed header names, e.g. "MYHEADER", since they go through ASGIRequest, which applies prefixing.

django/contrib/auth/middleware.py

@@ -184 +184 @@
                 " before the RemoteUserMiddleware class."
             )
         try:
-            username = request.META["HTTP_" + self.header]
+            username = request.META[self.header]
         except KeyError:
             # If specified header doesn't exist then remove any existing
             # authenticated remote-user, or return (leaving request.user set to

tests/auth_tests/test_remote_user.py

@@ -3 +3 @@
 from django.conf import settings
 from django.contrib.auth import aauthenticate, authenticate
 from django.contrib.auth.backends import RemoteUserBackend
-from django.contrib.auth.middleware import RemoteUserMiddleware
+from django.contrib.auth.middleware import PersistentRemoteUserMiddleware, RemoteUserMiddleware
 from django.contrib.auth.models import User
+from django.http.request import HttpHeaders
 from django.middleware.csrf import _get_new_csrf_string, _mask_cipher_secret
 from django.test import (
     AsyncClient,
@@ -17 +18 @@
 
 @override_settings(ROOT_URLCONF="auth_tests.urls")
 class RemoteUserTest(TestCase):
-    middleware = "django.contrib.auth.middleware.RemoteUserMiddleware"
+    # Use a custom middleware if we want this test case to run on both WSGI & ASGI.
+    middleware = "auth_tests.test_remote_user.CustomHeaderMiddleware"
     backend = "django.contrib.auth.backends.RemoteUserBackend"
-    header = "REMOTE_USER"
+    header = "HTTP_AUTHUSER"
     email_header = "REMOTE_EMAIL"
 
     # Usernames to be passed in REMOTE_USER for the test_known_user test case.
     known_user = "knownuser"
     known_user2 = "knownuser2"
 
+    @property
+    def async_header(self):
+        if self.header.startswith(HttpHeaders.HTTP_PREFIX):
+            # The tests like to parameterize headers, but if the defined header
+            # is already prefixed, strip it before using it.
+            return self.header.lstrip(HttpHeaders.HTTP_PREFIX)
+        return self.header
+
     @classmethod
     def setUpClass(cls):
         cls.enterClassContext(
@@ -65 +75 @@
         self.assertTrue(response.context["user"].is_anonymous)
         self.assertEqual(await User.objects.acount(), num_users)
 
-        response = await self.async_client.get("/remote_user/", **{self.header: ""})
+        response = await self.async_client.get("/remote_user/", **{self.async_header: ""})
         self.assertTrue(response.context["user"].is_anonymous)
         self.assertEqual(await User.objects.acount(), num_users)
 
@@ -142 +152 @@
         """See test_unknown_user."""
         num_users = await User.objects.acount()
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: "newuser"}
+            "/remote_user/", **{self.async_header: "newuser"}
         )
         self.assertEqual(response.context["user"].username, "newuser")
         self.assertEqual(await User.objects.acount(), num_users + 1)
@@ -150 +160 @@
 
         # Another request with same user should not create any new users.
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: "newuser"}
+            "/remote_user/", **{self.async_header: "newuser"}
         )
         self.assertEqual(await User.objects.acount(), num_users + 1)
 
@@ -176 +186 @@
         await User.objects.acreate(username="knownuser2")
         num_users = await User.objects.acount()
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(response.context["user"].username, "knownuser")
         self.assertEqual(await User.objects.acount(), num_users)
         # A different user passed in the headers causes the new user
         # to be logged in.
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user2}
+            "/remote_user/", **{self.async_header: self.known_user2}
         )
         self.assertEqual(response.context["user"].username, "knownuser2")
         self.assertEqual(await User.objects.acount(), num_users)
@@ -221 +231 @@
         await user.asave()
 
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertNotEqual(default_login, response.context["user"].last_login)
 
@@ -229 +239 @@
         user.last_login = default_login
         await user.asave()
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(default_login, response.context["user"].last_login)
 
@@ -259 +269 @@
         await User.objects.acreate(username="knownuser")
         # Known user authenticates
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(response.context["user"].username, "knownuser")
         # During the session, the REMOTE_USER header disappears. Should trigger
@@ -295 +305 @@
         await User.objects.acreate(username="knownuser")
         # Known user authenticates
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(response.context["user"].username, "knownuser")
         # During the session, the REMOTE_USER changes to a different user.
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: "newnewuser"}
+            "/remote_user/", **{self.async_header: "newnewuser"}
         )
         # The current user is not the prior remote_user.
         # In backends that create a new user, username is "newnewuser"
@@ -315 +325 @@
     async def test_inactive_user_async(self):
         await User.objects.acreate(username="knownuser", is_active=False)
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: "knownuser"}
+            "/remote_user/", **{self.async_header: "knownuser"}
         )
         self.assertTrue(response.context["user"].is_anonymous)
 
@@ -343 +353 @@
     async def test_unknown_user_async(self):
         num_users = await User.objects.acount()
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: "newuser"}
+            "/remote_user/", **{self.async_header: "newuser"}
         )
         self.assertTrue(response.context["user"].is_anonymous)
         self.assertEqual(await User.objects.acount(), num_users)
@@ -362 +372 @@
     async def test_inactive_user_async(self):
         user = await User.objects.acreate(username="knownuser", is_active=False)
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(response.context["user"].username, user.username)
 
@@ -436 +446 @@
         self.assertEqual(newuser.email, "user@example.com")
 
 
+"""
+These tests are now duplicative in the current demonstration. We wouldn't merge
+it in this state.
+"""
 class CustomHeaderMiddleware(RemoteUserMiddleware):
     """
     Middleware that overrides custom HTTP auth user header.
@@ -454 +468 @@
     header = "HTTP_AUTHUSER"
 
 
+class CustomPersistentRemoteUserMiddleware(PersistentRemoteUserMiddleware):
+    header = "HTTP_AUTHUSER"
+
+
 class PersistentRemoteUserTest(RemoteUserTest):
     """
     PersistentRemoteUserMiddleware keeps the user logged in even if the
     subsequent calls do not contain the header value.
     """
 
-    middleware = "django.contrib.auth.middleware.PersistentRemoteUserMiddleware"
+    middleware = "auth_tests.test_remote_user.CustomPersistentRemoteUserMiddleware"
     require_header = False
 
     def test_header_disappears(self):
@@ -482 +500 @@
         await User.objects.acreate(username="knownuser")
         # Known user authenticates
         response = await self.async_client.get(
-            "/remote_user/", **{self.header: self.known_user}
+            "/remote_user/", **{self.async_header: self.known_user}
         )
         self.assertEqual(response.context["user"].username, "knownuser")
         # Should stay logged in if the REMOTE_USER header disappears.

I had to be aware of that in caf90a971f09323775ed0cacf94eadaf39d040e0, where I added a little change in the AsyncRequestFactory to smooth out this difference with respect to underscores. It's possible we could deprecate and undo that going forward, if we want to standardize how headers are provided in tests? But that would be more disruptive than what I'm proposing here.

---

I'll definitely support wontfix'ing this if there is no value, but something seems off. I wonder if the quirks of AsyncRequestFactory put a little design pressure on the decision to have RemoteUserMiddleware.aprocess_request apply prefixing?

[Carlton Gibson]

I didn’t check but if the ASGI flow is broken by default, we should fix that. The idea definitely wasn’t to have code that doesn’t work. 🫠

[Jacob Walls]

The flows work, but if we leave the status quo alone, this documented blurb is not correct for ASGI:

you can subclass RemoteUserMiddleware and set the header attribute to the desired request.META key.

Under ASGI, you are forced to use a header attribute that is unprefixed -- if it is prefixed, it will be double-prefixed in request.META. Our existing test case accepts the double-prefix route:

class CustomHeaderMiddleware(RemoteUserMiddleware):
    """
    Middleware that overrides custom HTTP auth user header.
    """

    header = "HTTP_AUTHUSER"

When those test cases run, the async client sends HTTP_AUTHUSER = ..., so request.META gets HTTP_HTTP_AUTHUSER.

That doesn't look right, and I doubt anybody is doing this, so I was interested to discuss a code change. However, a better idea than forcing ASGI projects to subclass the middleware would be to perform the lookup against request.headers, which already supports lookups by unprefixed header names. This is a little more defensible to document, I feel. The only behavior change would be that an ASGI-only project could now set header = "WITH-HYPHENS" and benefit from a more flexible lookup, which is backward compatible.

On the security list, Jake suggested roughly, "If we're doing a deprecation, let's change both WSGI/ASGI paths to use self.headers to have consistency". I think we can punt on whether to do a deprecation here, and just start with the docs fix paired with the more flexible lookup. I'll put up a PR to aid triage.

[Jacob Walls]

​PR to adjust the async path to look up against request.headers without breaking anything.

[Carlton Gibson]

Thanks for the draft Jacob. I think that looks good. (And covers the ground that we need to by default IMO — Folks needing more can customise as necessary.)

[Sarah Boyce]

Thank you Jacob, I like where this is going

[Mykhailo Havelia]

Replying to Sarah Boyce:

Thank you for the ticket

From reviewing the PR that implemented this, I would say our approach was to get test parity by async-ifying the sync tests. This was then required for the tests to pass and, therefore, seemed required. I am now under the impression that our logic as to how we wrote the async tests must be off. Have you looked into what changes may be required there? I think that will help identify why we got this wrong

This is a partial duplicate of #36443 and #36300

I don't think we can improve our test tooling to prevent something like this in the future. But we do have experience supporting sync/async middlewares with separate tests (like in the PR), and I can say with certainty that it's a bad approach for long-term maintenance 😌. At first it works fine, you just copy the sync tests, swap in the async equivalents, and you're done. But over time it becomes hard to maintain, and having separate tests doesn't actually guarantee that the sync and async paths behave the same way.

RemoteUserMiddleware is the first and only pure async middleware in Django, so we can experiment with testing tooling and approaches here, and then apply what we learn to future pure async middlewares.

Just a thought, if it makes sense 😌