[Django] #21608: Logged out sessions are resurrected by concurrent requests
Django
----------------------------------+--------------------
Reporter: jonasborgstrom | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
1. User logs in
2. User loads a slow page in separate tab or as an ajax request
3. User logs out before request in step 2 completes. This will delete the
session from the db
== Expected behavior ==
User/session stays logged out since the user explicitly logged out and the
session row was delete in step 3.
== Actual behavior ==
The previously deleted session is re-inserted into the database when the
request from step 2 completes. So the previously logged out user is now
logged in again.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jonasborgstrom | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
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 jonasborgstrom):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
bump
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:1>
Django
Reporter: jonasborgstrom | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by russellm):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Seems like a reasonable request, and the patch looks like a decent start
-- but it needs tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:2>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by nikl@…):
* owner: nobody => anonymous
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:3>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by nikl@…):
Finalized on the train and airport:
https://github.com/django/django/pull/2678
Thanks to everybody at DjangoIsland who helped me tackle this - looking
forward to your feedback!
- Nikl
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:4>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by nikl@…):
* needs_better_patch: 1 => 0
* has_patch: 0 => 1
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:5>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Ready for checkin => Accepted
Comment:
Please don't mark your own patch as RFC. Someone who reviews the patch
should do that.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:6>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by erikr):
I'm not entirely getting this. When a user logs out,
[[https://github.com/django/django/blob/master/django/contrib/auth/__init__.py#L120|the
session is flushed]]. Flushing the session
[[https://github.com/django/django/blob/master/django/contrib/sessions/backends/base.py#L271|clears
it and deletes it]]. The database session store performs this deletion by
actually
[[https://github.com/django/django/blob/master/django/contrib/sessions/backends/db.py#L70|deleting
the record from the DB]]. The cached_db backend
[[https://github.com/django/django/blob/master/django/contrib/sessions/backends/cached_db.py#L67|deletes
it from the DB and the cache]]. So basically, all records of this session
should be deleted. If you would post a new request with the now deleted
session ID,
[[https://github.com/django/django/blob/master/django/contrib/sessions/backends/db.py#L17|Django
will reject it, and assign you a new session with a new session ID]].
The reporter says that django re-inserts the session when a request
arrives with the old session ID, and will re-insert it with the old
session data. But I don't see that anywhere in the code. As far as I can
see, Django would reject the session ID, as loading would fail as the
session object has been deleted, and the user would be assigned a new
session. Even if there were a flaw in that logic: once the session data
has been deleted, how would any code know how to recreate the session? The
request doesn't contain any hint on what user should be logged in.
The only explanation I can come up with is that we're talking about cookie
backed sessions, for which this is a documented limitation: you can't
guarantee deletion of a cookie backed session or it's data, no matter what
we do in Django: its the nature of cookies.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:7>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
I couldn't reproduce this using steps 1-3 in the description (SQLite).
After logging out in a separate tag, the slow page loaded, but subsequent
requests redirected to the admin login page. There also seem to be some
concerns from Nick's review on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:8>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jonasborgstrom):
I think one key detail missing from the initial reproduction steps is that
the "slow page" needs to modify the session to make it dirty. Otherwise
the session will not be resurrected.
Anyway, I've now create a complete reproduction test case here:
https://github.com/jborg/django-21608
See README.txt for details.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:9>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by collinanderson):
would `session.save(force_update=True)` fix this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:10>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by sergeykolosov):
* cc: m17.admin@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:11>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Description changed by jnnt:
Old description:
> 1. User logs in
> 2. User loads a slow page in separate tab or as an ajax request
> 3. User logs out before request in step 2 completes. This will delete the
> session from the db
>
> == Expected behavior ==
>
> User/session stays logged out since the user explicitly logged out and
> the session row was delete in step 3.
>
> == Actual behavior ==
>
> The previously deleted session is re-inserted into the database when the
> request from step 2 completes. So the previously logged out user is now
> logged in again.
New description:
1. User logs in
2. User loads a slow page in separate tab or as an ajax request, which
''modifies'' the session
3. User logs out before request in step 2 completes. This will delete the
session from the db
== Expected behavior ==
User/session stays logged out since the user explicitly logged out and the
session row was delete in step 3.
== Actual behavior ==
The previously deleted session is re-inserted into the database when the
request from step 2 completes. So the previously logged out user is now
logged in again.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:12>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by tltx):
I have reproduced this bug with the test site
(https://github.com/jborg/django-21608) on both Django 1.8.7 and 1.9
(using SQLite).
These were the steps I used:
* Open the admin page in a tab and log in. http://localhost:8000/admin/
* Switch to a new tab and open the slow page. http://localhost:8000/slow/
Act fast after this step to complete the next three steps before the slow
page has finished loaded, <10 sec
* Switch back to tab with the admin page.
* Click the "Logout" link on the top right corner of the page. -> Now you
are on the logout page
* Reload page -> Now you are on the login page.
* Wait for slow page to finish loading.
* Reload the tab with the login page.
* Logged in again without entering credentials!
This is a security issue, not critical though, as someone might think that
they have logged out but is actually still logged in.
If you logout and leave a public computer while a page is still loading in
another tab there is a risk that the next person using that computer can
get access to your account.
It would be nice to have this fixed 1.8 and up.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:13>
Django
Reporter: jonasborgstrom | Owner: anonymous
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by tltx):
* version: 1.4 => 1.8
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:14>
Django
Reporter: jonasborgstrom | Owner: tltx
Type: Bug | Status: assigned
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tltx):
* cc: tlt@… (added)
* owner: anonymous => tltx
* version: 1.8 => 1.9
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:15>
Django
Reporter: jonasborgstrom | Owner: tltx
Type: Bug | Status: assigned
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tltx):
* Attachment "21608.diff" added.
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tltx):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:16>
Django
Reporter: jonasborgstrom | Owner: tltx
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tltx):
Run the unit test without the fix to verify the bug. This fix should also
be back ported to 1.8 as it is a security fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:17>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tltx):
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:18>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Are you able to submit the patch as a pull request?
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:19>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tltx):
Absolutely, which branch should I create a PR against?
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:20>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Master please.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:21>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tltx):
Hi again, pull request is up.
https://github.com/django/django/pull/5950
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:22>
Django
Reporter: jonasborgstrom | Owner: tltx
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by OakNinja):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:23>
Django
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by OakNinja):
I've gone through the patch review checklist and everything is looking
good.
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: jonasborgstrom | Owner: tltx
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"3389c5ea229884a1943873fe7e7ffc2800cefc22" 3389c5e]:
{{{
#!CommitTicketReference repository=""
revision="3389c5ea229884a1943873fe7e7ffc2800cefc22"
Fixed #21608 -- Prevented logged out sessions being resurrected by
concurrent requests.
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: jonasborgstrom | Owner: tltx
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"5faf745999caa3d2588979ae1262cc28652c21a5" 5faf745]:
{{{
#!CommitTicketReference repository=""
revision="5faf745999caa3d2588979ae1262cc28652c21a5"
Refs #21608 -- Fixed incorrect cache key in cache session backend's
save().
The bug was introduced commit 3389c5ea229884a1943873fe7e7ffc2800cefc22.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:26>
Django
-------------------------------------+-------------------------------------
| Lundqvist
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dan Tao):
I commented
[https://github.com/django/django/commit/3389c5ea229884a1943873fe7e7ffc2800cefc22#commitcomment-24605369
here], but just to raise visibility: I'm concerned that the change to fix
this bug resulted in a logical error (or at least unintuitive behavior) in
`SessionStore.save()`. Namely: now `must_create=False` implies
`must_update=True`, which I would argue is wrong. `must_create=False`
''should'' probably mean that either creating or updating is acceptable.
Has this already been discussed elsewhere, and perhaps there's something
I'm missing?
--
Ticket URL: <https://code.djangoproject.com/ticket/21608#comment:27>