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

Unexpected default salt in 2.0.0 where I used to pass None as a salt in 1.1.0 #237

Closed
clj opened this issue May 12, 2021 · 1 comment
Closed
Milestone

Comments

@clj
Copy link

clj commented May 12, 2021

Hi,

Not sure if this is anything you want to fix, but I wanted to at least highlight the issue:

Calling Serializer in 1.1.0 with a salt set to None used to work fine:

>>> from itsdangerous.serializer import Serializer
... s = Serializer("secret-key", salt=None)
... print(s.dumps([1, 2, 3, 4]))
[1, 2, 3, 4].d6HwOZMIWe0UH9BUzXfN7AiMWXk

but in version 2.0.0 this throws an exception:

>>> from itsdangerous.serializer import Serializer
... s = Serializer("secret-key", salt=None)
... print(s.dumps([1, 2, 3, 4]))
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File ".../python3.8/site-packages/itsdangerous/serializer.py", line 203, in dumps
    rv = self.make_signer(salt).sign(payload)
  File ".../python3.8/site-packages/itsdangerous/signer.py", line 211, in sign
    return value + self.sep + self.get_signature(value)
  File ".../python3.8/site-packages/itsdangerous/signer.py", line 204, in get_signature
    key = self.derive_key()
  File ".../python3.8/site-packages/itsdangerous/signer.py", line 190, in derive_key
    bytes, self.digest_method(self.salt + b"signer" + secret_key).digest()
TypeError: unsupported operand type(s) for +: 'NoneType' and 'bytes'

In my code, this happens because the Serializer is called from another function that has a salt keyword argument that is set to None, something along the lines of:

from itsdangerous.serializer import Serializer
def init_serializer_old(secret, salt=None):
    # do stuff
    return Serializer(secret, salt)

Naively I now changed the code to only pass the salt keyword argument on if it is not None, something along the lines of:

from itsdangerous.serializer import Serializer
def init_serializer_new(secret, salt=None):
    kwargs = {}
    if salt:
        kwargs['salt'] = salt
    # do stuff
    return Serializer(secret, **kwargs)

Using that though, generates a different signature, than the first code snippet above:

>>> s = init_serializer_new("secret-key")
... print(s.dumps([1, 2, 3, 4]))
[1, 2, 3, 4].r7R9RhGgDPvvWl3iNzLuIIfELmo

It turns out that this is because both Serializer and Signer provide a default value for the salt argument, b"itsdangerous" and b"itsdangerous.Signer" respectively. However in 1.1.0 Signer accepted None as a salt and if so would use the value b"itsdangerous.Signer". This is no longer the case due to this change:
aed5b26#diff-1d0e89827237bbe1bf2e65ff4801829cb4737a4679b4f14edaddd4b0414030cdL137

If I no longer pass None in version 2.0.0 as the salt, the default is taken from Serializer and is therefore different from when I explicitly passed None in 1.1.0 and it was taken from Signer. This can be fixed in my own code fairly easily:

from itsdangerous.serializer import Serializer
def init_serializer_new_v2(secret, salt=b"itsdangerous.Signer"):
    # do stuff
    return Serializer(secret, salt)

We caught this in our tests, but others may not be so lucky, so I at least wanted to document this new behaviour here.

Environment:

  • Python version: python3.8
  • ItsDangerous version: 2.0.0
@davidism
Copy link
Member

davidism commented May 13, 2021

I didn't think using None just to replace it with a string made sense, typically the "default None" pattern is used for mutable or complex defaults. I also didn't expect users to be passing None, either they would not pass it and get the serializer's default b"itsdangerous", or they would pass it explicitly and get that value for namespace purposes.

Since it can affect the validity of tokens, I'll make a bugfix release. However, I'd recommend you set an explicit default regardless, as it makes more sense to set a default namespace that is related to your own project.

@davidism davidism added this to the 2.0.1 milestone May 13, 2021
@davidism davidism mentioned this issue May 18, 2021
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants