Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#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)

files-sessions-server-expiry-1.patch (1.1 KB ) - added by Rohan Jain 13 years ago.
Patch 1 - Use OS file info to expire sessions
files-sessions-server-expiry-2.patch (3.0 KB ) - added by Rohan Jain 12 years ago.
Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.
files-sessions-server-expiry-3.patch (3.5 KB ) - added by Rohan Jain 12 years ago.
Patch 3 - Use the signing frework to handle session expiry and integrity
sessions-cleanup-files-1.patch (3.1 KB ) - added by Rohan Jain 12 years ago.
Cleanup management command support for file based sessions.

Download all attachments as: .zip

Change History (27)

comment:1 by Paul McMillan, 13 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

by Rohan Jain, 13 years ago

Patch 1 - Use OS file info to expire sessions

comment:2 by Rohan Jain, 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 Paul McMillan, 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 Rohan Jain, 13 years ago

Cc: crodjer@… added

by Rohan Jain, 12 years ago

Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.

by Rohan Jain, 12 years ago

Patch 3 - Use the signing frework to handle session expiry and integrity

by Rohan Jain, 12 years ago

Cleanup management command support for file based sessions.

comment:5 by Rohan Jain, 12 years ago

Has patch: set
Owner: changed from nobody to Rohan Jain
Status: newassigned

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 Rohan Jain, 12 years ago

Owner: changed from Rohan Jain to Paul McMillan
Status: assignednew

comment:7 by Tomáš Ehrlich, 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.

Last edited 12 years ago by Tomáš Ehrlich (previous) (diff)

in reply to:  7 ; comment:8 by Rohan Jain, 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:9 by Aymeric Augustin, 12 years ago

Related: #18978

comment:10 by Aymeric Augustin, 12 years ago

Owner: changed from Paul McMillan to Aymeric Augustin

in reply to:  8 comment:11 by Tomáš Ehrlich, 12 years ago

Cc: tomas.ehrlich@… added
Version: 1.4master

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 of age 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 Aymeric Augustin, 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 if settings.SESSION_EXPIRE_AT_BROWSER_CLOSE = False (default), at browser close otherwise
  • server-side: expire after settings.SESSION_COOKIE_AGE (no matter the value of settings.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), after settings.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 of session.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 to timezone.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).

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

comment:13 by Aymeric Augustin, 12 years ago

I've created #19200 to track the bug in cached_db and signed_cookies and #19201 for proposal 2).

I'm now going to attempt to fix 1) which is still marked as a release blocker for 1.5.

comment:14 by Aymeric Augustin, 12 years ago

This branch fixes this ticket as described above: https://github.com/aaugustin/django/compare/session-fixes

Review appreciated :)

comment:15 by Aymeric Augustin, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In cd17a24083f3ef17cf4c40a41c9d03c250d817c6:

Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

comment:17 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 882c47cd405cfd29194f2e968678a5aa1d6ec75f:

Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

comment:18 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: newclosed

In 5fec97b9df6ea075483276de159e522a29437773:

Fixed #18194 -- Expiration of file-based sessions

  • Prevented stale session files from being loaded
  • Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

comment:19 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 845d8408e7aa09a63c98f160b2c6bc9fa6dbb20f:

[1.5.x] Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

Backport of cd17a24 from master.

comment:20 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In e6b0ee768c46368a41a5b278180d1d1ecbd3d5c6:

[1.5.x] Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

Backport of 882c47c from master.

comment:21 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 39082494e62d70be62cde1eb10620548fd62dfb6:

[1.5.x] Fixed #18194 -- Expiration of file-based sessions

  • Prevented stale session files from being loaded
  • Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

Backport of 5fec97b from master.

comment:22 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 93e0ec553dae73a2a86cb23b058414d8f9ae3bd6:

[1.5.x] Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.

Backport of 11fd00c from master.

comment:23 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 11fd00c46e2826ce5852e096487762a96909b717:

Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.

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