Opened 10 years ago

Closed 8 years ago

Last modified 6 years ago

#21608 closed Bug (fixed)

Logged out sessions are resurrected by concurrent requests

Reported by: Jonas Borgström Owned by: Tore Lundqvist
Component: contrib.sessions Version: 1.9
Severity: Normal Keywords:
Cc: m17.admin@…, tlt@… 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 (last modified by jnnt)

  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.

Attachments (2)

session_fix.patch (1.1 KB ) - added by Jonas Borgström 10 years ago.
Proposed fix (against django 1.4)
21608.diff (2.7 KB ) - added by Tore Lundqvist 8 years ago.

Download all attachments as: .zip

Change History (29)

by Jonas Borgström, 10 years ago

Attachment: session_fix.patch added

Proposed fix (against django 1.4)

comment:1 by Jonas Borgström, 10 years ago

bump

comment:2 by Russell Keith-Magee, 10 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Seems like a reasonable request, and the patch looks like a decent start -- but it needs tests.

comment:3 by nikl@…, 10 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:4 by nikl@…, 10 years ago

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

comment:5 by nikl@…, 10 years ago

Has patch: set
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:6 by Tim Graham, 10 years ago

Triage Stage: Ready for checkinAccepted

Please don't mark your own patch as RFC. Someone who reviews the patch should do that.

comment:7 by Sasha Romijn, 10 years ago

I'm not entirely getting this. When a user logs out, the session is flushed. Flushing the session clears it and deletes it. The database session store performs this deletion by actually deleting the record from the DB. The cached_db backend 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, 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.

comment:8 by Tim Graham, 10 years ago

Patch needs improvement: set

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.

comment:9 by Jonas Borgström, 9 years ago

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.

comment:10 by Collin Anderson, 9 years ago

would session.save(force_update=True) fix this issue?

comment:11 by Sergey Kolosov, 9 years ago

Cc: m17.admin@… added

comment:12 by jnnt, 8 years ago

Description: modified (diff)

comment:13 by Tore Lundqvist, 8 years ago

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 in 1.8 and up.

Last edited 8 years ago by Tore Lundqvist (previous) (diff)

comment:14 by Tore Lundqvist, 8 years ago

Version: 1.41.8

comment:15 by Tore Lundqvist, 8 years ago

Cc: tlt@… added
Owner: changed from anonymous to Tore Lundqvist
Patch needs improvement: unset
Version: 1.81.9

by Tore Lundqvist, 8 years ago

Attachment: 21608.diff added

comment:16 by Tore Lundqvist, 8 years ago

Resolution: fixed
Status: assignedclosed

comment:17 by Tore Lundqvist, 8 years ago

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.

comment:18 by Tore Lundqvist, 8 years ago

Resolution: fixed
Status: closednew

comment:19 by Tim Graham, 8 years ago

Are you able to submit the patch as a pull request?

comment:20 by Tore Lundqvist, 8 years ago

Absolutely, which branch should I create a PR against?

comment:21 by Tim Graham, 8 years ago

Master please.

comment:22 by Tore Lundqvist, 8 years ago

Hi again, pull request is up.

https://github.com/django/django/pull/5950

comment:23 by Esbjörn Ekberg, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:24 by Esbjörn Ekberg, 8 years ago

I've gone through the patch review checklist and everything is looking good.

comment:25 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 3389c5e:

Fixed #21608 -- Prevented logged out sessions being resurrected by concurrent requests.

Thanks Simon Charette for the review.

comment:26 by Tim Graham <timograham@…>, 8 years ago

In 5faf745:

Refs #21608 -- Fixed incorrect cache key in cache session backend's save().

The bug was introduced commit 3389c5ea229884a1943873fe7e7ffc2800cefc22.

comment:27 by Dan Tao, 6 years ago

I commented 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?

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