When logging out/ending a session, don't create a new, empty session

[Matt Robenolt]

Previously, when logging out, the existing session is overwritten by a new sessionid instead of deleting the session all together.

This behavior adds overhead by creating a new session record in whichever backend being used, db, cache, etc.

This extra session is unnecessary at the time since no session data is meant to be preserved when explicitly logging out.

See: ​https://github.com/django/django/pull/1487

[Preston Holmes]

this makes sense, no reason to build a new empty session on logout - PR looks good on read through, but can't review/run the tests now this late

[Matt Robenolt]

ptone, do you have an opinion about modifying SessionStore.flush() to take over the behavior of what I introduced as SessionStore.end? That was my original intent, and inside Django, logout is the only place this use exists.

Not sure if this would break for others in the wild since this is technically a documented API and may have some expected behavior. In my opinion, doing this end/flush thing is wrong, and flush should just do the job.

[Preston Holmes]

mattrobenolt looking at this, and if I'm reading things right - won't the session middleware just try to save a new session on process_response?

session.clear() sets modified=True (inside end or flush)

and process_response will request.session.save() - also in your patch you set the _session_key to '' for the db backend, while most of the session module tests explicitly for None

this actually makes it less purposeful for auth to be creating it via flush, as if you want to use sessions in a custom way, you could use your own middleware that does something different.

not sure on changing flush vs adding end. No way to deprecate a behavior, only the whole method, always surprising how people are using expected behavior.

[Matt Robenolt]

You are right, mostly. It's desirable to set the _session_key to '' because that tells the browser to effectively remove the data. Then the step that I've missed, is sending it back an expiry time in the past. So basically time.time() - 1. I hope that makes sense. The combination of those two will tell the browser to actually delete the cookie.

With these combined, the process_response will behave correctly. I'm going to spend a little more time on this to get it to set an expiry to a date in the past. While I'm at it, I'm going to check out what Django does when you explicitly delete cookies anyways. I have a hunch that this is all behaving slightly wonky, but I don't know for sure off thet op of my head.

[Aymeric Augustin]

FYI I remember (but can't locate right now) a ticket about preserving the language, which is stored in the session, after logout. It's related to this discussion.

[Matt Robenolt]

I've updated my patch to update some of the logic. To me, it looks good.

@aaugustin, this doesn't affect any existing logic around that. The middleware is what's keeping track of deleting the session or not. If the language was preserved, the session would not be empty, meaning it wouldn't get deleted.

[Preston Holmes]

The language question is a specific instance of a general problem of an "outer session", that is a session that transcends user login/logout.

We already have this notion established by preserving session data across login - anything added to the session while an anon user is logged in is "upgraded" into the authed session.

On logout, the session is flushed. This currently seems watertight as far as any data-leakage, so removing the cookie on the client doesn't add any security value.

Agreed that anything that would preserve "outer session" state on logout will have no issue with this patch, as the session is lazily recreated if accessed (an explicit create() is not required).

There is some small optimization in not creating a session in the DB that you are likely not to use. @mattrobenolt - is there a benefit of removing the cookie beyond just good housekeeping?

Because auth.logout is the one to call and end to a session, there could be a test added to verify that this happens. The current test just checks for the auth session key - and there was never a test to check the current flush behavior - but it would be a nice extra touch to the patch.

I can't see any possible security issue here at all, but wouldn't mind a quick glance from Donald or Paul - I'll see if I can't recruit them to have a look.

[Matt Robenolt]

@ptone, the performance issue isn't a huge concern since we don't use the db for sessions, so it's not something that affects us in that regard. Was just an unexpected behavior.

The main concern here is dealing with upstream caches. Speaking from the context of Varnish, if a Cookie header is present, it wants to skip caches, which is a good thing in general since you don't want to cache too much. But when someone logs out, we have no real way of determining the difference between an empty session, and a user that actually has session data.

Again, it's just a very unexpected behavior. In my opinion, if the session is empty, and the framework is explicitly deleting the session, we should just destroy the cookie as well until someone sets new session data.

[Ramiro Morales <cramm0@…>]

In 393c0e24223c701edeb8ce7dc9d0f852f0c081ad:

Fixed #20936 -- When logging out/ending a session, don't create a new, empty session.

Previously, when logging out, the existing session was overwritten by a
new sessionid instead of deleting the session altogether.

This behavior added overhead by creating a new session record in
whichever backend was in use: db, cache, etc.

This extra session is unnecessary at the time since no session data is
meant to be preserved when explicitly logging out.

[Tim Graham <timograham@…>]

In 2f54853:

[1.7.x] Fixed DoS possiblity in contrib.auth.views.logout()

Refs #20936 -- When logging out/ending a session, don't create a new, empty session.

Previously, when logging out, the existing session was overwritten by a
new sessionid instead of deleting the session altogether.

This behavior added overhead by creating a new session record in
whichever backend was in use: db, cache, etc.

This extra session is unnecessary at the time since no session data is
meant to be preserved when explicitly logging out.

Backport of 393c0e24223c701edeb8ce7dc9d0f852f0c081ad,
088579638b160f3716dc81d194be70c72743593f, and
2dee853ed4def42b7ef1b3b472b395055543cc00 from master

Thanks Florian Apolloner and Carl Meyer for review.

This is a security fix.

[Tim Graham <timograham@…>]

In 575f59f:

[1.4.x] Fixed DoS possiblity in contrib.auth.views.logout()

Refs #20936 -- When logging out/ending a session, don't create a new, empty session.

Previously, when logging out, the existing session was overwritten by a
new sessionid instead of deleting the session altogether.

This behavior added overhead by creating a new session record in
whichever backend was in use: db, cache, etc.

This extra session is unnecessary at the time since no session data is
meant to be preserved when explicitly logging out.

Backport of 393c0e24223c701edeb8ce7dc9d0f852f0c081ad,
088579638b160f3716dc81d194be70c72743593f, and
2dee853ed4def42b7ef1b3b472b395055543cc00 from master

Thanks Florian Apolloner and Carl Meyer for review.

This is a security fix.