#18194 closed Bug (fixed)
File-based session never expire
Reported by: | Edwin | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | contrib.sessions | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | crodjer@…, tomas.ehrlich@… | 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
Unlike other session backends, session expiry is never checked for file-based session when loading a session. The consequence of this is that, although browser will not send an expired cookie with Django session ID, user can workaround this by modifying his system time so the browser still sends the expired cookie. Since Django doesn't check for the session expiry on the server, this expired session ID is seen as valid to Django.
I am running 1.3.1, but looking at 1.4 source, the bug is still there. Looking at the source code, this loading method should check for session expiry:
def load(self): session_data = {} try: session_file = open(self._key_to_file(), "rb") try: file_data = session_file.read() # Don't fail if there is no data in the session file. # We may have opened the empty placeholder file. if file_data: try: session_data = self.decode(file_data) except (EOFError, SuspiciousOperation): self.create() finally: session_file.close() except IOError: self.create() return session_data
The bug was first brought up in the mailing list:
https://groups.google.com/forum/#!msg/django-developers/tB_F9rRx7Lg/y70AEdejud8J
Attachments (4)
Change History (27)
comment:1 by , 13 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 13 years ago
Attachment: | files-sessions-server-expiry-1.patch added |
---|
comment:2 by , 13 years ago
Similar to expire_date
field in loading from database, the patch - 1 checks the last modify time of the file for age. As far as analogy to updating expire_date
is concerned, that will be automatically done with os.write
.
Where are the existing tests for contrib.sessions
? I was unable to find them for any layer of the app.
comment:3 by , 13 years ago
For contrib apps, the tests are stored alongside the apps themselves. In the case of sessions, that's here:
https://github.com/django/django/blob/master/django/contrib/sessions/tests.py
comment:4 by , 13 years ago
Cc: | added |
---|
by , 12 years ago
Attachment: | files-sessions-server-expiry-2.patch added |
---|
Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.
by , 12 years ago
Attachment: | files-sessions-server-expiry-3.patch added |
---|
Patch 3 - Use the signing frework to handle session expiry and integrity
by , 12 years ago
Attachment: | sessions-cleanup-files-1.patch added |
---|
Cleanup management command support for file based sessions.
comment:5 by , 12 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I made some attempts with this in file-session-server-expiry-{2,3}.patch
. The 2nd uses files modification time to decide on expiry. The 3rd uses the signing framework's timed signer for handling the expiration. Also added a test which is supposed to emulate the ticket behaviour.
Moreover, sessions-cleanup-files-1.patch
adds support for cleaning of file sessions with management cleanup command. It moves the session backend specific cleanup logic to their own modules and the management command invokes cleanup command according to the current backend. The base backend implements a cleanup class method which is supposed to be overridden by the backends.
comment:6 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
follow-up: 8 comment:7 by , 12 years ago
Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.
Using modification time seems to me interesting (although simple signing would be useful here), but both solutions (TimestampSigner and modification time) have slight caveat: While in database backend we can specify exact expiration date and check that it's < timezone.now(), here we have modification date (or date of signing) and we check that it's < timezone.now() - SESSION_COOKIE_AGE. It could be solved with setting modification date (or date of signing) in future, but i'm not sure if it's allowed for every file system. TimestampSigner unfortunatelly doesn't support change of time.
follow-up: 11 comment:8 by , 12 years ago
Linking this issue to my corresponding pull request over github: https://github.com/django/django/pull/453
Replying to Elvard:
Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.
This makes sense, considering each backend all of the others have alternate ways of verifying expiry. Putting timed signer only in file session seems more appropriate. It'll also save us from compatibility issues for other backends.
Though in database backend the exist
seems faulty. It doesn't check for expiry there. But its out of scope of this ticket.
Using modification time seems to me interesting (although simple signing would be useful here), but both solutions (TimestampSigner and modification time) have slight caveat: While in database backend we can specify exact expiration date and check that it's < timezone.now(), here we have modification date (or date of signing) and we check that it's < timezone.now() - SESSION_COOKIE_AGE. It could be solved with setting modification date (or date of signing) in future, but i'm not sure if it's allowed for every file system.
I also think using file system might be unreliable.
TimestampSigner unfortunatelly doesn't support change of time.
I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of signer.unsign
:
signer.unsign(data, age)
We can have settings.SESSION_COOKIE_AGE
in place of age
parameter.
comment:10 by , 12 years ago
Owner: | changed from | to
---|
comment:11 by , 12 years ago
Cc: | added |
---|---|
Version: | 1.4 → master |
Replying to crodjer:
TimestampSigner unfortunatelly doesn't support change of time.
I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of
signer.unsign
:
signer.unsign(data, age)We can have
settings.SESSION_COOKIE_AGE
in place ofage
parameter.
It's probably just a detail. DB backend stores datetime of expiration, while file-based backend stores datetime of signing.
In database backend I can do:
session = SessionBase() session[...] = ... # set data session.set_expiry(arbitraty_time_in_future) session.save()
Now we have database record with expiration_date set to 'arbitrary_time_in_future' (which doesn't depend on SESSION_COOKIE_AGE).
While in file-based backend, using TimestampSigner, I can't set arbitrary expiration_date independent on SESSION_COOKIE_AGE because I can't store expiration_date along with data. Whole file containing data is signed and current timestamp is stored.
Probably it's not a big issue. I've came across it when I was trying to write unit tests.
comment:12 by , 12 years ago
I've worked on this ticket today. Here's an analysis of how session expiration is handled by django.contrib.sessions, and specifically by the built-in backends.
Expiration is handled both client-side (by setting cookie expiry) and server-side (because we don't want to keep stale data forever).
This ticket is about synchronizing server-side expiration with client-side expiration, especially for the file backend. Historically Django hasn't paid much attention to server-side expiration.
The default session expiration policy is:
- client-side: expire after
settings.SESSION_COOKIE_AGE
seconds ifsettings.SESSION_EXPIRE_AT_BROWSER_CLOSE = False
(default), at browser close otherwise - server-side: expire after
settings.SESSION_COOKIE_AGE
(no matter the value ofsettings.SESSION_EXPIRE_AT_BROWSER_CLOSE
)
When a non-default expiration is set with session.set_expiry(...)
, it is saved in the session under the _session_expiry
key. The semantic of this value is:
- if it's missing or
None
: use the default expiration policy described above - if it's
0
: expire at browser close (client side), aftersettings.SESSION_COOKIE_AGE
(server side) - if it's an
int
: expire in that many seconds (both sides) - if it's a
datetime
: expire at that time (both sides)
This analysis is based on the implementation of SessionMiddleware
, session.get_expire_at_browser_close()
, session.get_expiry_age()
(which controls client-side expiry) and session.get_expiry_date()
(which controls server-side expiry).
tl;dr The expiry date of a session is always defined by session.get_expiry_date()
/ session.get_expiry_age()
at the time the session is saved — because it relies on timezone.now()
in some cases.
How do the various backends deal with that?
- cache: the expiry age is computed and passed to the cache engine. The cache engine is responsible for not returning stale data. Since correct expiry is a major feature for a cache, I think we can rely on cache engine to enforce expiry properly. There's no need to clear expired sessions, the cache does it by itself.
- cached_db: there's a bug here — expiry age is hardcoded to
settings.SESSION_COOKIE_AGE
instead ofsession.get_expiry_age()
. Otherwise it should work like cache and db. EDIT: fixed in 04b00b668d0d56c37460cbed19671f4b1b5916c3.
- db: the session expiry date is computed and stored alongside the session data when the session is saved. Only sessions whose expiry dates are in the future can be re-loaded. Sessions whose expiry dates are in the past can be cleared.
- file: the session expiry date isn't stored. It can be rebuilt with the algorithm of
session.get_expiry_age()
, by substituting the file's last modification time totimezone.now()
. The patches above attempt to do that in order to clear expired sessions.
- signed_cookies: server-side expiration is provided by timestamping and signing the cookies. However non-default expiry dates aren't handled; the maximum expiration time is hardcoded at
settings.SESSION_COOKIE_AGE
. There's no need to clear expired sessions because they're stored client-side. Edit: could be fixed in the context of #19201
That was a bit long but necessary to frame this ticket :)
Now there are two actions:
1) Clear expired sessions with the file backend. The patches above are on the right track. This depends on #18978.
2) In the longer term, always store the expiration date in the session and refuse to load an expired session in SessionBase.load
. This is already how signed cookies work. It overlaps significantly with _session_expiry
as described above. It's a major refactor of django.contrib.sessions
that will require backwards-compatibility.
Since 1.5 is now feature-frozen, let's restrict this ticket to 1) and create a new ticket for 2).
comment:13 by , 12 years ago
comment:14 by , 12 years ago
This branch fixes this ticket as described above: https://github.com/aaugustin/django/compare/session-fixes
Review appreciated :)
comment:15 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:18 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Patch 1 - Use OS file info to expire sessions