Deprecate passing positional arguments to Signer.

[Daniel Finch]

We discovered a vulnerability in one of our applications recently which was caused by an inaccurate instantiation of django.core.signing.Signer. The developer intended to use the user's email address as the salt for the Signing instance but instead caused it to be used as the key. Here's an example code block that demonstrates the problem:

signer = Signer(self.context['request'].user.email)
signed_data = signer.sign_object(dict(
    license_number='...',
    product_id='...',
    device_count='...'
))

In our case, this signed data was then being used to verify a later request and generate an active license. This meant that an attacker could feasibly generate their own licenses if they realised that their email address was the key. The fix for this was to add salt= in front of the email variable. It occurred to us that this is a relatively easy mistake to make and could be avoided if the signature of Signer.__init__ was changed thusly:

- def __init__(self, key=None, sep=':', salt=None, algorithm=None):
+ def __init__(self, *, key=None, sep=':', salt=None, algorithm=None):

That is, adding a * after self to force the developer to name the parameters.

[Mariusz Felisiak]

Agreed. This is a backward incompatible change so release notes and a versionchanged annotation is necessary, we also need to audit all ​documented uses.

[Mariusz Felisiak]

Daniel, would you like to prepare a patch?

[Daniel Finch]

I've opened a PR to address this ticket: ​https://github.com/django/django/pull/14995

[Nikita Marchant]

Hi!

It looks like Daniel ​does not have time to handle this ticket so i would like to have a try.

I spent a little time thinking about the best way to do this and I see a few options:

1. Write a decorator that makes the function accept positional arguments, sends a warning and rewrites the call to using kwargs only (​Proof of concept)
2. Add *args to the signature and use a trick with locals() to inject those into the local variables (​Proof of concept, thanks to ​t00n for the idea and code)
3. Change the signature to __init__(self, *args, **kwargs)

1 and 2 have the advantage of not changing the signature and body of the function too much so it will be easy to remove when the depreciation period ends but are using maybe too much of dark magic. 3 is the opposite.

What do you think would be best in Django's case ? When i know what approach to take, i'd be happy to update ​Daniel's PR.

[Jacob Walls]

I think prepending *args before the existing arguments is good, but if you can avoid tinkering with locals(), the better, I think.

What about something like:

diff --git a/django/core/signing.py b/django/core/signing.py
index 5ee19a9336..724a7df507 100644
--- a/django/core/signing.py
+++ b/django/core/signing.py
@@ -37,10 +37,12 @@ import base64
 import datetime
 import json
 import time
+import warnings
 import zlib
 
 from django.conf import settings
 from django.utils.crypto import constant_time_compare, salted_hmac
+from django.utils.deprecation import RemovedInDjango50Warning
 from django.utils.encoding import force_bytes
 from django.utils.module_loading import import_string
 from django.utils.regex_helper import _lazy_re_compile
@@ -145,16 +147,25 @@ def loads(s, key=None, salt='django.core.signing', serializer=JSONSerializer, ma
 
 
 class Signer:
-    def __init__(self, key=None, sep=':', salt=None, algorithm=None):
+    def __init__(self, *args, key=None, sep=':', salt=None, algorithm=None):
         self.key = key or settings.SECRET_KEY
         self.sep = sep
+        self.salt = salt or '%s.%s' % (self.__class__.__module__, self.__class__.__name__)
+        self.algorithm = algorithm or 'sha256'
+        if args:
+            warnings.warn(
+                'Providing positional arguments to Signer is deprecated.',
+                RemovedInDjango50Warning,
+                stacklevel=2,
+            )
+            for arg, attr in zip(args, ['key', 'sep', 'salt', 'algorithm']):
+                if arg or attr == 'sep':
+                    setattr(self, attr, arg)
         if _SEP_UNSAFE.match(self.sep):
             raise ValueError(
                 'Unsafe Signer separator: %r (cannot be empty or consist of '
                 'only A-z0-9-_=)' % sep,
             )
-        self.salt = salt or '%s.%s' % (self.__class__.__module__, self.__class__.__name__)
-        self.algorithm = algorithm or 'sha256'
 

[Abhinav Yadav]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In b8738aea:

Fixed #33199 -- Deprecated passing positional arguments to Signer/TimestampSigner.

Thanks Jacob Walls for the implementation idea.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3a3e737:

Refs #33199 -- Removed support for passing positional arguments to Signer/TimestampSigner.

Per deprecation timeline.