Opened 3 years ago

Closed 3 years 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.

Change History (5)

comment:1 Changed 3 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: wontfix
Status: newclosed

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 3 years ago by victor@…

Needs documentation: set
Needs tests: set
Resolution: wontfix
Status: closednew

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 3 years ago by victor@…

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

['foo']

comment:4 Changed 3 years ago by Tim Graham

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 3 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

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

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