Skip to content
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

Use itsdangerous library to sign cookie #11

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sblondon
Copy link
Contributor

@sblondon sblondon commented Apr 3, 2020

This PR is about issue #6.

The previous serialization/deserialization is still in the code, until it will be removed in a another future release.

What do you think about it? Do you see improvements to add?
Perhaps the itsdangerous library version should be more restricted?


@classmethod
def _mac_unserialize(cls, string, secret_key):
warnings.warn("Obsolete serialization method used", DeprecationWarning)
Copy link
Member

@davidism davidism Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message needs to be more exact. Something like "Unserializing using the old scheme. This is deprecated and the fallback will be removed in version 2.0. Ensure cookies are re-serialized using the new ItsDangerous scheme."

Should also use stacklevel=3 or whatever level makes the error show where in user code caused it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this part. The new string is longer than required by black so I added a # noqa comment to ignore it. If you prefer, I can extract it in a variable to reformat it.
I don't think it's necessary because the _mac_unserialize() method will disappear in a future release. What is your opinion about it?

setup.py Outdated Show resolved Hide resolved
@davidism
Copy link
Member

davidism commented Apr 3, 2020

Is this code similar to Flask's use of ItsDangerous for the session cookie? Haven't had a chance to compare yet. If not, we should identify how it's different and why here.

@sblondon
Copy link
Contributor Author

sblondon commented Apr 4, 2020

The current added code for serialize() and unserialize() methods are based on the previous implementations (which are located in this patch in _mac_serialize() and _mac_unserialize() methods).

SecureCookieSessionInterface() class use classes from itsdangerous library (BadSignature and URLSafeTimedSerializer) but don't have serialize() and unserialize() methods.

So I don't understand what needs to be compared. Could you give me some hints?

@northernSage northernSage added enhancement New feature or request security Pull requests that address a security vulnerability labels Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request security Pull requests that address a security vulnerability
Development

Successfully merging this pull request may close these issues.

3 participants