Opened 3 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#37017 closed Bug (fixed)

alogout() doesn't clear request.user

Reported by: Jacob Walls Owned by: Jacob Walls
Component: contrib.auth Version: 6.0
Severity: Release blocker Keywords: not-security
Cc: Sarah Boyce, Michael Manfre Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As of Django 6.0, alogout() no longer clears request.user, only request.auser. If code accesses request.user before RemoteUserMiddleware (or similar) runs alogout(), then it is possible for request.user to be stale, and for a resource behind authentication to be visible to a logged-out user.

user and auser aren't really two concepts: just two getters for the same underlying concept. (Thanks, function color problem!)

(I'm suggesting this was an oversight in 31a43c571f4d036827d4fd7a5f615591637dc1be. This was discussed during development, but it may not have been clear how this would arise in practice.)

The security team considered a report about this suggesting the following order of middlewares:

MIDDLEWARE = [
    "django.middleware.security.SecurityMiddleware",
    "django.contrib.sessions.middleware.SessionMiddleware",
    "django.contrib.auth.middleware.AuthenticationMiddleware",
    "app.middleware.MaterializeUserMiddleware",  # e.g. a logging middleware like Sentry
    "django.contrib.auth.middleware.RemoteUserMiddleware",
    "django.contrib.auth.middleware.LoginRequiredMiddleware",
]

The problem does not reproduce if RemoteUserMiddleware is moved one position earlier. The security team closed the report on this basis (that is, anything responsible for logout should happen before other code that might be interested in that logout). Our docs say RemoteUserMiddleware should be placed "after" AuthenticationMiddleware, but does not clarify whether this entails directly after.

Still seems like something to fix to make auth easier to reason about.

Thanks Peng Zhou for the report.

Change History (6)

comment:1 by Jacob Walls, 3 weeks ago

Cc: Michael Manfre added

It's likely we should have alogin() also set request.user for parallelism.

comment:2 by Sarah Boyce, 3 weeks ago

Triage Stage: UnreviewedAccepted

Thank you for the report! Apologies that my review was problematic
I suppose request.user will still be used in async contexts by anything that has been made async by a simple sync_to_async call and therefore needs to stay valid.

comment:3 by Jacob Walls, 3 weeks ago

Has patch: set

No worries, here's a patch: PR

comment:4 by Sarah Boyce, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:5 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In a32c707:

Fixed #37017 -- Fixed setting or clearing of request.user after alogin/alogout().

Regression in 31a43c571f4d036827d4fd7a5f615591637dc1be.

comment:6 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

In a5c28dc:

[6.0.x] Fixed #37017 -- Fixed setting or clearing of request.user after alogin/alogout().

Regression in 31a43c571f4d036827d4fd7a5f615591637dc1be.

Backport of a32c7075cf634aee1f4f3deecd27f194097ec0c2 from main.

Note: See TracTickets for help on using tickets.
Back to Top