-
Notifications
You must be signed in to change notification settings - Fork 41
Vendor the PickleSerializer #2866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cdedd70
to
3d0b008
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2866 +/- ##
=======================================
Coverage 56.69% 56.70%
=======================================
Files 602 603 +1
Lines 43971 43983 +12
=======================================
+ Hits 24931 24942 +11
- Misses 19040 19041 +1 ☔ View full report in Codecov by Sentry. |
3d0b008
to
11234fb
Compare
|
11234fb
to
679239a
Compare
679239a
to
4986ee8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, but the vendored-in module needs a docstring that explain why it's there: I.e. to make NAVs ancient methods keep working on Django 5 and newer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
Hopefully we can get rid of the entire thing at some point...
fc8c564
to
2234aa7
Compare
If we find a good way to erase existing cookies stored with $old_serializer and forcing a new login then we can get rid of this file, yes. |
Replaces #2852 and does not depend on #2828
If using
signed_cookies
as the session engine, serializing with pickle is a security problem. We do not use signed_cookies, so we can continue to use PickleSerializer.This vendored version makes the serializer incompatible with the signed_cookies engine just in case.
See also #2865