Hi @abe-101, thanks for picking this up!

There are a few issues on the previous PR that aren't yet addressed, for instance this one. Would you like to start with those? When you're caught up on that set of comments, please unset the "Needs ..." flags on the Trac ticket. Thanks.

Hi @abe-101, thanks for picking this up!

There are a few issues on the previous PR that aren't yet addressed, for instance this one. Would you like to start with those? When you're caught up on that set of comments, please unset the "Needs ..." flags on the Trac ticket. Thanks.

thank you @jacobtylerwalls for the review
This is my first Django PR so i appreciate your input.
I've added the versionchanged to the docs.
I've reviewed the comments and to the best of my knowledge I've cover it all. if I've missed anything else please point it out

@sarahboyce can we add the Djangonaut label to this PR?

Thank you for the PR @abe-101 ⭐

Did you investigate this comment from Florian? I think it might be interesting to reduce the signature changes, for example:

I argue that the checking for expiration (max_age or exp_key) belongs in the unsign methods of TimestampSigner.
The unsign method handles validation for max_age so I argue unsign_object should also handle validation for exp_key.

Perhaps we should introduce a new method to TimestampSigner that would not do any time validation and instead return the unsigned value with the timestamp and leave it up to the caller to do their custom validation.

--- a/django/contrib/sessions/backends/signed_cookies.py
+++ b/django/contrib/sessions/backends/signed_cookies.py
@@ -15,7 +15,6 @@ class SessionStore(SessionBase):
                 serializer=self.serializer,
                 max_age=self.get_session_cookie_age(),
                 salt="django.contrib.sessions.backends.signed_cookies",
-                expiration_key="_session_expiry",
             )
         except Exception:
             # BadSignature, ValueError, or unpickling exceptions. If any of
diff --git a/django/core/signing.py b/django/core/signing.py
index 770ea1c8b5..9221167734 100644
--- a/django/core/signing.py
+++ b/django/core/signing.py
@@ -159,20 +159,21 @@ def loads(
     serializer=JSONSerializer,
     max_age=None,
     fallback_keys=None,
-    expiration_key=None,
 ):
     """
     Reverse of dumps(), raise BadSignature if signature fails.
 
     The serializer is expected to accept a bytestring.
     """
-    return TimestampSigner(
-        key=key,
-        salt=salt,
-        fallback_keys=fallback_keys,
-    ).unsign_object(
-        s, serializer=serializer, max_age=max_age, expiration_key=expiration_key
+    signer = TimestampSigner(key=key, salt=salt, fallback_keys=fallback_keys)
+    unsigned_data = signer.unsign_object(
+        s,
+        serializer=serializer,
+        max_age=max_age,
     )
+    if expiry := unsigned_data.get("_session_expiry"):
+        signer.verify_max_age(expiry)
+    return unsigned_data

@sarahboyce I'm confused by your suggestion to remove the line (from the signing.loads method):

-    expiration_key=None,

and instead only support a predefined key (_session_expiry)

+    if expiry := unsigned_data.get("_session_expiry"):

my understanding is that the signing.loads method is a way to unsign an object. it has nothing to do with session

in this PR we add a way to specify a key to be used to lookup an expiration value (from within the signed obj) - to do that there needs to be a change to the signing module

@sarahboyce I appreciate you review and i understand you hesitation to prevent signature changes - as it can be hard to know the consequences...

Let me assure you that:
the new parameter expiration_key=None introduced in signing.loads and in TimestampSigner.unsign_object both default to None.

I think can avoid changing the TimestampSigner.unsign_object signature with this approach (simular to yours)

--- signing_original.py	2025-02-25 22:21:39.185793764 -0500
+++ signing_suggested.py	2025-02-25 22:46:25.955527054 -0500
@@ -159,19 +159,23 @@
     serializer=JSONSerializer,
     max_age=None,
     fallback_keys=None,
+    expiration_key=None,
 ):
     """
     Reverse of dumps(), raise BadSignature if signature fails.
 
     The serializer is expected to accept a bytestring.
     """
-    return TimestampSigner(
-        key=key, salt=salt, fallback_keys=fallback_keys
-    ).unsign_object(
+    signer = TimestampSigner(key=key, salt=salt, fallback_keys=fallback_keys)
+    unsigned_data = signer.unsign_object(
         s,
         serializer=serializer,
         max_age=max_age,
     )
+    if expiration_key:
+        expiry = unsigned_data.get(expiration_key)
+        signer.verify_max_age(expiry)
+    return unsigned_data
 
 
 class Signer:
@@ -252,6 +256,10 @@
 
 
 class TimestampSigner(Signer):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.payload_timestamp = None
+
     def timestamp(self):
         return b62_encode(int(time.time()))
 
@@ -266,12 +274,18 @@
         """
         result = super().unsign(value)
         value, timestamp = result.rsplit(self.sep, 1)
-        timestamp = b62_decode(timestamp)
-        if max_age is not None:
-            if isinstance(max_age, datetime.timedelta):
-                max_age = max_age.total_seconds()
-            # Check timestamp is not older than max_age
-            age = time.time() - timestamp
-            if age > max_age:
-                raise SignatureExpired("Signature age %s > %s seconds" % (age, max_age))
+        self.payload_timestamp = b62_decode(timestamp)
+
+        self.verify_max_age(max_age)
         return value
+
+    def verify_max_age(self, max_age=None):
+        if max_age is None or self.payload_timestamp is None:
+            return
+        if isinstance(max_age, datetime.timedelta):
+            max_age = max_age.total_seconds()
+
+        # Check timestamp is not older than max_age
+        age = time.time() - self.payload_timestamp
+        if age > max_age:
+            raise SignatureExpired("Signature age %s > %s seconds" % (age, max_age))

I don't see a way to avoid a change in the signing.loads

Perhaps we should introduce a new method to TimestampSigner that would not do any time validation and instead return the unsigned value with the timestamp and leave it up to the caller to do their custom validation.

I think that's a nice idea 👍

Perhaps we should introduce a new method to TimestampSigner that would not do any time validation and instead return the unsigned value with the timestamp and leave it up to the caller to do their custom validation.

I think that's a nice idea 👍

ok - Exploring this option, where should the time validation happen?

1. In the loads method (see above [patch](Fixed #27775 -- Added support for custom expiry in signed cookies. #19191 (comment) for example) - still requires a API change in signing
2. The signing module doesn't do any validation just exposes a new way to retrieve the timestamp - Then we'd modify signed cookies session backend to no longer use signing.loads and instead use the TimestampSigner.new_method and retrieve the timestamp and handle the validations.
3. add a flag return_timestamp=False to signing.loads and TimestampSigner.unsign
   and leave it to the caller to make the validations

anyone have any other suggestions?

@abe-101 these look like reasonable options, and I can’t think of any others. Of the options you’ve listed, which one are you leaning toward?

Is it a better option to change how signed_cookies uses the signing module:

class SessionStore(SessionBase):
    def load(self):
        """
        Load the data from the key itself instead of fetching from some
        external data store. Opposite of _get_session_key(), raise BadSignature
        if signature fails.
        """
        try:
            signer = signing.TimestampSigner(
                salt="django.contrib.sessions.backends.signed_cookies"
            )
            session_data = signer.unsign_object(
                self.session_key,
                serializer=self.serializer,
                max_age=self.get_session_cookie_age(),
            )
            expiry = session_data.get("_session_expiry")
            signer.verify_max_age(expiry)
            return session_data

this approach only makes a small change to the signing module (refactoring verify_max_age to be a public function)
but required a (big) change to how signed_cookies uses the signing module

I wonder if ther will be a side affect if not using signing.loads()

My problem with this solution is that it creates a forked serialization path:
loads is typically used to serialize a python object, so e.g. calling obj.loads() returns the obj as a json object. If obj contains one or more properties that are complex types, it relies on the nested objects loads implementation, so serialization is quasi-recursive.

The problem here is that if someone asks for a jsonified TimestampSigner directly, it will go through signer.loads() but if you serialize it by calling SessionStore.load(), it skips over signer.loads() and goes straight to the next level of serialization (unsign_object), so now there's the possibility for inconsistent serialization.

That said, I also believe this is the same implementation as here, but moved up one level, introducing the fork.

Apologies if I got any of this wrong, it's been a long day but I couldn't stop thinking about this until I got this down :)

Current Approach

The current approach provides a public way to tell the signing module to check for an expiry key. One or more signatures must change for that to happen (to specify what the expiry keys is).
Pros:

• Users of the signing module can specify an expiry_key to check for expiration. (new feature)
• Solves ticket 27775

Cons:

• Required a signature change

Sarahs Approach

Another approach (@sarahboyce suggestion) is to only make a check for the session backend (_session_expiry). This is possible without any signature changes.

Pros:

• Solves ticket 27775
• No signature change

Cons:

• Users of the signing module cannot specify an expiry_key to check for expiration.

Trying to make a new proposal to be closer to what Florian suggested:

--- a/django/contrib/sessions/backends/signed_cookies.py
+++ b/django/contrib/sessions/backends/signed_cookies.py
@@ -1,3 +1,6 @@
+import datetime
+import time
+
 from django.contrib.sessions.backends.base import SessionBase
 from django.core import signing
 
@@ -10,13 +13,15 @@ class SessionStore(SessionBase):
         if signature fails.
         """
         try:
-            return signing.loads(
+            payload_value, timestamp = signing.loads(
                 self.session_key,
                 serializer=self.serializer,
                 max_age=self.get_session_cookie_age(),
                 salt="django.contrib.sessions.backends.signed_cookies",
-                expiration_key="_session_expiry",
+                return_timestamp=True,
             )
+            signing.check_signature_expiry(timestamp, max_age=payload_value.get("_session_expiry"))
+            return payload_value
         except Exception:
             # BadSignature, ValueError, or unpickling exceptions. If any of
             # these happen, reset the session.
diff --git a/django/core/signing.py b/django/core/signing.py
index 770ea1c8b5..d58a3fd890 100644
--- a/django/core/signing.py
+++ b/django/core/signing.py
@@ -115,6 +115,17 @@ def get_cookie_signer(salt="django.core.signing.get_cookie_signer"):
     )
 
 
+def check_signature_expiry(timestamp, max_age=None):
+    if max_age is None:
+        return
+    if isinstance(max_age, datetime.timedelta):
+        max_age = max_age.total_seconds()
+    # Check timestamp is not older than max_age.
+    age = time.time() - timestamp
+    if age > max_age:
+        raise SignatureExpired("Signature age %s > %s seconds" % (age, max_age))
+
+
 class JSONSerializer:
     """
     Simple wrapper around json to be used in signing.dumps and
@@ -159,7 +170,7 @@ def loads(
     serializer=JSONSerializer,
     max_age=None,
     fallback_keys=None,
-    expiration_key=None,
+    return_timestamp=False,
 ):
     """
     Reverse of dumps(), raise BadSignature if signature fails.
@@ -171,7 +182,7 @@ def loads(
         salt=salt,
         fallback_keys=fallback_keys,
     ).unsign_object(
-        s, serializer=serializer, max_age=max_age, expiration_key=expiration_key
+        s, serializer=serializer, max_age=max_age, return_timestamp=return_timestamp
     )
 
 
@@ -268,27 +279,12 @@ class TimestampSigner(Signer):
         result = super().unsign(value)
         value, timestamp = result.rsplit(self.sep, 1)
         self.payload_timestamp = b62_decode(timestamp)
-        self._verify_max_age(max_age)
+        check_signature_expiry(self.payload_timestamp, max_age=max_age)
         return value
 
-    def unsign_object(self, signed_obj, max_age=None, expiration_key=None, **kwargs):
+    def unsign_object(self, signed_obj, max_age=None, return_timestamp=False, **kwargs):
         # First verify the outer max_age boundary
         value = super().unsign_object(signed_obj, max_age=max_age, **kwargs)
-
-        # Then if expiration_key is provided, check it as an additional constraint
-        if expiration_key is not None:
-            expiry = value.get(expiration_key)
-            if expiry is not None:
-                self._verify_max_age(expiry)
-
+        if return_timestamp:
+            return value, self.payload_timestamp
         return value
-
-    def _verify_max_age(self, max_age=None):
-        if max_age is None:
-            return
-        if isinstance(max_age, datetime.timedelta):
-            max_age = max_age.total_seconds()
-        # Check timestamp is not older than max_age
-        age = time.time() - self.payload_timestamp
-        if age > max_age:
-            raise SignatureExpired("Signature age %s > %s seconds" % (age, max_age))

This tries to use return_timestamp

Trying to make a new proposal to be closer to what Florian suggested:
....
This tries to use return_timestamp

@sarahboyce with the above approach signing.loads and TimestampSigner.unsign_object have 2 return paths:

1. the unsigned value, or
2. a tuple containing the unsigned data and timestamp

is it acceptable in Django to have 2 return paths?
also, as @haileyajohnson alluded to changing the return type of the signing.loads function may interfere with the serialization...

I think we should either have a test project, or (even better) write a test to show the issues you're afraid of occurring

@abe-101 my concern was slightly different, it was different code paths rather than different return types, e.g, the case where someone down the line changes signing.loads and that change never gets picked up by SessionStore because it was skipping that function call.

Whether loads returning either an object or a tuple(object, timestamp) could cause problems is a different question, and in my opinion it's fine, since the signer will only return the tuple if the client explicitly requests it by setting returnTimestamp=True, and I think it's reasonable to expect them to realize they're requesting a tuple. Ultimately you're not changing what's retuned by SessionStore.load, so I think if it doesn't break existing tests call that, it should be good 👍

@apollo13 would you mind taking a look at this PR? 🙏
This has been updated based off some feedback you gave on a previous PR for this ticket 👍

What is the point of a method solely calling it's parent? Sounds like you can just drop test_actual_expiry[_async] from this class?

You're right about this. I checked and removing the test_actual_expiry and test_actual_expiry_async methods doesn't change the number of tests being run.
(I think its because the parent class tests would still be executed through inheritance)

Been triaging ticket-36506 and it makes me wonder if we should implement django.contrib.sessions.backends.signed_cookies.SessionStore.clear_expired() in this ticket