Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#11555 closed Bug (fixed)

Undocumented contrib.sessions behaviour

Reported by: Tomasz Zieliński Owned by: nobody
Component: contrib.sessions Version: 1.0
Severity: Normal Keywords:
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When new session starts (sessionid not passed in cookies) and request.session.session_key is accessed before request.session["abc"], then:

1) request.session.session_key is generated,

2) first access to request.session[ "abc" ] calls request.session.load(), which overrides (by calling create()) request.session.session_key because the session_key cannot be found in DB.

This can be manually fixed by calling request.session.load() before using request.session.session_key.

I'm not sure if this is a bug or undocumented feature - I can send relevant patch depending on the answer.

Attachments (1)

11555.diff (4.7 KB) - added by Aymeric Augustin 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by Thejaswi Puthraya

Component: Uncategorizeddjango.contrib.sessions

comment:3 Changed 6 years ago by Aymeric Augustin

Cc: Aymeric Augustin added

When no session exists, create will generate a new session key, regardless of the existing session key; this is an expected behavior, used for key regeneration and checked by the tests. It should not be changed.

So, one way to solve the OP's problem is to create the session when session.session_key is accessed. That makes sense, since the only way to know if a session key is usable is to create a session with that key (that is what the backends do).

Accessing session.session_key either returns an existing session key or creates a new session key with _get_new_session_key and returns it. Furthermore, all create methods in the backends begin by calling _get_new_session_key. So the following patch looks reasonable.

Index: django/contrib/sessions/backends/base.py
===================================================================
--- django/contrib/sessions/backends/base.py	(revision 15655)
+++ django/contrib/sessions/backends/base.py	(working copy)
@@ -172,7 +172,7 @@
         if self._session_key:
             return self._session_key
         else:
-            self._session_key = self._get_new_session_key()
+            self.create()
             return self._session_key
 
     def _set_session_key(self, session_key):

Unfortunately, it breaks a seemingly unrelated test (DatabaseSessionTests.test_session_get_decoded) in a way I do not understand.


Looking at the problem differently, creating a session key is a real task; it should not occur as a side effect of accessing a public attribute (e.g. writing session.session_key).

So another possible fix is to return session._session_key when session.session_key is called; returning None would mean that there is no session key at this time (possibly because the session is not loaded or created yet). From the user's point of view, returning None instead of a wrong session key is probably better, although it might break code that relies on getting a string.

Then, a get_or_create_session_key method should be created and used instead of session_key wherever it is expected that a new session key may be generated.

That is a rather big change, so I'd like to hear from someone else before working on it.

comment:4 Changed 6 years ago by Aymeric Augustin

Session key randomization is also an important security feature: see #13478.

That is one more reason to make the public API for session_key read-only.

Changed 6 years ago by Aymeric Augustin

Attachment: 11555.diff added

comment:5 Changed 6 years ago by Aymeric Augustin

Has patch: set

Attached patch implements the second solution described above.

comment:6 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by Preston Holmes

Easy pickings: unset
Patch needs improvement: set
UI/UX: unset

patch not applying cleanly on 16843 - looks like minor update needed

comment:8 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [17155]:

(The changeset message doesn't reference this ticket)

comment:5 Changed 4 years ago by Aymeric Augustin

In [17911]:

Clarified that Django randomizes session keys. Refs #11555, #13478, #18128.

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