[Django] #37025: Deprecate the prefixing of HTTP_ to header names in RemoteUserMiddleware.aprocess_request()

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
-----------------------------+----------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: contrib.auth
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+----------------------------------------
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:

{{{#!diff
diff --git a/django/contrib/auth/middleware.py
b/django/contrib/auth/middleware.py
index a28e1705a3..7136a64c44 100644
--- a/django/contrib/auth/middleware.py
+++ b/django/contrib/auth/middleware.py
@@ -184,7 +184,7 @@ class RemoteUserMiddleware:
" 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()

------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

------------------------------+---------------------------------------
Changes (by Jacob Walls):

* owner: (none) => Jacob Walls
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:1>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Changes (by MANAS MADESHIYA):

* cc: MANAS MADESHIYA (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:2>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:3>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Changes (by Sarah Boyce):

* cc: Carlton Gibson (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:4>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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. 🤷
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:5>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:6>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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.

{{{#!diff
diff --git a/django/contrib/auth/middleware.py
b/django/contrib/auth/middleware.py
index a28e1705a3..7136a64c44 100644
--- a/django/contrib/auth/middleware.py
+++ b/django/contrib/auth/middleware.py
@@ -184,7 +184,7 @@ class RemoteUserMiddleware:
" 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

diff --git a/tests/auth_tests/test_remote_user.py
b/tests/auth_tests/test_remote_user.py
index 4a97fc2120..6745009589 100644
--- a/tests/auth_tests/test_remote_user.py
+++ b/tests/auth_tests/test_remote_user.py
@@ -3,8 +3,9 @@ from datetime import UTC, datetime
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,15 +18,24 @@ from django.test import (

@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,7 +75,7 @@ class RemoteUserTest(TestCase):
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,7 +152,7 @@ class RemoteUserTest(TestCase):
"""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,7 +160,7 @@ class RemoteUserTest(TestCase):

# 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,14 +186,14 @@ class RemoteUserTest(TestCase):
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,7 +231,7 @@ class RemoteUserTest(TestCase):
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,7 +239,7 @@ class RemoteUserTest(TestCase):
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,7 +269,7 @@ class RemoteUserTest(TestCase):
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,12 +305,12 @@ class RemoteUserTest(TestCase):
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,7 +325,7 @@ class RemoteUserTest(TestCase):
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,7 +353,7 @@ class RemoteUserNoCreateTest(RemoteUserTest):
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,7 +372,7 @@ class
AllowAllUsersRemoteUserBackendTest(RemoteUserTest):
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,6 +446,10 @@ class RemoteUserCustomTest(RemoteUserTest):
self.assertEqual(newuser.email, "us...@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,13 +468,17 @@ class CustomHeaderRemoteUserTest(RemoteUserTest):
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,7 +500,7 @@ class PersistentRemoteUserTest(RemoteUserTest):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:7>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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. 🫠
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:8>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------------------------

Comment (by 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:

{{{#!py

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

header = "HTTP_AUTHUSER"
}}}


When those test cases run, the 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 `self.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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:9>

[Django]

#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()

-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage:

RemoteUserMiddleware | Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* cc: Jake Howard (added)
* has_patch: 0 => 1
* keywords: => RemoteUserMiddleware

Comment:

[https://github.com/django/django/pull/21079 PR] to adjust the async path
to look up against `request.headers` without breaking anything.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:10>