[Django] #20936: When logging out/ending a session, don't create a new, empty session
Django
--------------------------------------+-----------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.sessions | Version: master
Severity: Normal | Keywords: session, logout, auth
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-----------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20936>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mattrobenolt | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.sessions | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: session, logout, | Needs documentation: 0
auth | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by ptone):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:1>
Django
Reporter: mattrobenolt | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.sessions | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: session, logout, | Needs documentation: 0
auth | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mattrobenolt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:2>
Django
Reporter: mattrobenolt | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.sessions | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: session, logout, | Needs documentation: 0
auth | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by ptone):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:3>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by mattrobenolt):
* status: new => assigned
* needs_better_patch: 0 => 1
* owner: nobody => mattrobenolt
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:4>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:5>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mattrobenolt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:6>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by ptone):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:7>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mattrobenolt):
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:8>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Component: contrib.sessions | Version: master
Severity: Normal | Resolution: fixed
Keywords: session, logout, | Triage Stage: Accepted
auth | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"393c0e24223c701edeb8ce7dc9d0f852f0c081ad"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:9>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: session, logout, | Triage Stage: Accepted
auth |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2f5485346ee6f84b4e52068c04e043092daf55f7" 2f54853]:
{{{
#!CommitTicketReference repository=""
revision="2f5485346ee6f84b4e52068c04e043092daf55f7"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:10>
Django
Reporter: mattrobenolt | Owner:
Type: | mattrobenolt
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: session, logout, | Triage Stage: Accepted
auth |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"575f59f9bc7c59a5e41a081d1f5f55fc859c5012" 575f59f]:
{{{
#!CommitTicketReference repository=""
revision="575f59f9bc7c59a5e41a081d1f5f55fc859c5012"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20936#comment:11>