Opened 2 years ago

Last modified 3 weeks ago

#21608 assigned Bug

Logged out sessions are resurrected by concurrent requests

Reported by: jonasborgstrom Owned by: anonymous
Component: contrib.sessions Version: 1.4
Severity: Normal Keywords:
Cc: m17.admin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (1)

session_fix.patch (1.1 KB) - added by jonasborgstrom 2 years ago.
Proposed fix (against django 1.4)

Download all attachments as: .zip

Change History (13)

Changed 2 years ago by jonasborgstrom

Proposed fix (against django 1.4)

comment:1 Changed 22 months ago by jonasborgstrom

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset


comment:2 Changed 22 months ago by russellm

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 19 months ago by nikl@…

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:4 Changed 19 months ago by nikl@…

Finalized on the train and airport:

Thanks to everybody at DjangoIsland who helped me tackle this - looking forward to your feedback!

  • Nikl

comment:5 Changed 19 months ago by nikl@…

  • Has patch set
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 19 months ago by timo

  • Triage Stage changed from Ready for checkin to Accepted

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

comment:7 Changed 16 months ago by erikr

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 Changed 16 months ago by timgraham

  • 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 Changed 13 months ago 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:

See README.txt for details.

comment:10 Changed 11 months ago by collinanderson

would fix this issue?

comment:11 Changed 6 months ago by sergeykolosov

  • Cc m17.admin@… added

comment:12 Changed 3 weeks ago by jnnt

  • Description modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top