Code

Opened 10 months ago

Closed 8 months ago

#21124 closed Bug (wontfix)

Default session data serializer doesn't support extended data types

Reported by: sorcio@… Owned by: nobody
Component: contrib.sessions Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#20922 introduced the option to choose a custom session data serializer. The default option is to use the new JSONSerializer starting from 1.6, since using pickle would lead to a remote code execution vulnerability when session data is stored in cookies.

While this can be considered a sensible security choice, it becomes inconvenient as the JSON encoder used by JSONSerializer is not the same used elsewhere in Django, as it only support basic data types: string, int/floats, booleans, nested dicts and lists, None.

The inconvenience is breaking compatibility with all third party apps that rely on storing extended data types (such as those supported by DjangoJSONEncoder) with the default settings. Properly serializing datetime (possibly tz-aware) can be hard, and changing the default puts the burden on third party apps coders.

They would have the option to either add two complexity layers (properly serializing/deserializing datetime objects, and not breaking compatibility with the previous versions of the same app), or to break compatibility with Django default settings.

As an example of commonly used data types that can't be stored anymore with default settings:

  • datetime, timedelta objects (supported by DjangoJSONEncoder)
  • decimal objects (supported by DjangoJSONEncoder)
  • arbitrary binary strings
  • Geometry objects

I think the option of reverting the default to pickle should be also considered.

Attachments (0)

Change History (5)

comment:1 Changed 10 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

Thanks for your thoughts. I think most of the points you've raised were discussed during the implementation of this, either on the ticket (#20922) or on the linked pull request (or the documentation itself). Could you please take a look at the discussion there if you haven't? If after reading that you still have disagreements, please raise the issue on django-developers rather than this ticket tracker. Thanks!

Suggestions for documentations edits or additions would also be welcome.

p.s. To address one of your points, one of the decisions was indeed to put the burden on third party app coders to serialize session data as simple data types like strings which would be compatible with JSON. We made this change to contrib.messages for example.

comment:2 Changed 8 months ago by victor@…

  • Needs documentation set
  • Needs tests set
  • Resolution wontfix deleted
  • Status changed from closed to new

Hey, I also ran into this issue that broke quite huge apps such as django-allauth and its entire oauth2 feature. Took me hours and hours to find out that the session cache was not serializing well - there was no clue whatsoever.

In a nutshell, deep down django-allauth puts objects such as foo? (a list of one string) in the session cache; when it's read again, it's been transformed to 'foo' (no more list). No exception, or warning of any sort - I guess that's the real issue, right ?

I'm not sure about how to cope with this, but there are several ways: maybe communicate more on this change, add some tests (I place X in session data, I retrieve X, not Y) and probably exceptions if trying to store something unsupported.

comment:3 Changed 8 months ago by victor@…

Sorry for the formatting above, the object put in the session cache reads

['foo']

comment:4 Changed 8 months ago by timo

I'm unable to reproduce the list to non-list transformation you are describing. For example, I modified django.contrib.sessions.tests.DatabaseSessionTests.test_session_get_decoded to store a list (['1']) and get_decoded() return a list as expected. Could you include a sample shell session or a test for Django's test suite that demonstrates the behavior?

comment:5 Changed 8 months ago by timo

  • Resolution set to wontfix
  • Status changed from new to closed

Please open a new ticket if there is indeed a bug here.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.