Fixed #27775 -- Support custom expiry in signed cookies. by buugaj · Pull Request #16207 · django/django

buugaj · 2022-10-20T22:12:54Z

https://code.djangoproject.com/ticket/27775

Until now signed cookies didn't validate custom expiry times stored in _session_expiry. The proposed solution introduces expiration_key to TimestampSinger and in it's presence defers the age validation to happen after payload serialization.

There was old proposal on tackling this problem - #7885 - and while it was less lines of code I've decided to go with the described approach to avoid calling signing.loads twice.

github-actions

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

felixxm · 2022-10-24T10:41:44Z

@pelme Does it work for you?

pelme · 2022-10-24T15:31:31Z

From a quick look, this looks really good! It is very nice that you managed to avoid the double loads()!

Will try to find some time to verify that this works in my use case. Have been using the double loads() middleware in my own project for years to workaround this.

pelme · 2022-10-25T11:51:45Z

I have no easy way to test this in my current project (a couple of Django versions behind). But it looks good from what I can see!

I have some test cases in my own code base for my own implementation this -- not sure they are necessary or useful but here they go, feel free to adapt use any or none of them. They use pytest style and time_machine so they need a little bit of adaption to be useful in the Django test suite:

from datetime import timedelta

import pytest
import time_machine

from x.utils.datetime import datetime_utc
from x.utils.signed_cookies_session import SessionStore

start = datetime_utc(2000, 1, 1)


@pytest.fixture
@time_machine.travel(start)
def session_key_default_expiry(settings):
    settings.SESSION_COOKIE_AGE = 2 * 60 * 60
    session = SessionStore()
    session["foo"] = "bar"
    session.save()
    return session.session_key


@pytest.fixture
@time_machine.travel(start)
def session_key_100_hours(settings):
    settings.SESSION_COOKIE_AGE = 1  # 1 second, should not be used
    session = SessionStore()
    session["foo"] = "bar"
    session.set_expiry(100 * 60 * 60)
    session.save()
    return session.session_key


@pytest.fixture
@time_machine.travel(start)
def session_key_browser_expiry(settings):
    settings.SESSION_COOKIE_AGE = 2 * 60 * 60
    session = SessionStore()
    session["foo"] = "bar"
    session.set_expiry(0)  # 0 means browser expiry
    session.save()
    return session.session_key


@time_machine.travel(start + timedelta(hours=1))
def test_default_expiry_not_expired(session_key_default_expiry):
    session = SessionStore(session_key_default_expiry)
    assert session["foo"] == "bar"


@time_machine.travel(start + timedelta(hours=3))
def test_default_expiry_expired(session_key_default_expiry):
    session = SessionStore(session_key_default_expiry)
    assert "foo" not in session


@time_machine.travel(start + timedelta(hours=3))
def test_custom_expiry_not_expired(session_key_100_hours):
    second = SessionStore(session_key_100_hours)
    assert second["foo"] == "bar"


@time_machine.travel(start + timedelta(hours=101))
def test_custom_expiry_expired(session_key_100_hours):
    session = SessionStore(session_key_100_hours)
    assert "foo" not in session


@time_machine.travel(start + timedelta(hours=1))
def test_browser_expiry_not_expired(session_key_browser_expiry):
    session = SessionStore(session_key_browser_expiry)
    assert session["foo"] == "bar"


@time_machine.travel(start + timedelta(hours=3))
def test_browser_expiry_expired(session_key_browser_expiry):
    session = SessionStore(session_key_browser_expiry)
    assert "foo" not in session

felixxm

@buugaj Thanks for this patch 👍 and sorry for the slow feedback. I left initial comments.

tests/sessions_tests/tests.py

felixxm · 2022-11-25T12:24:34Z

django/core/signing.py

+    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))


We could add this hook in a separate commit (1st) and move the main change to the 2nd commit.

How important is this?
Should i redo my commits? so that the refactoring of _verify_max_age is in its own commit?

django/core/signing.py

felixxm · 2022-11-25T12:37:43Z

django/core/signing.py

    serializer=JSONSerializer,
    max_age=None,
    fallback_keys=None,
+    expiration_key=None,


loads() signature should be updated in docs in a versionchanged annotation.

I'd add tests for loads() calls with custom expiration_key.

felixxm · 2022-11-25T12:39:07Z

django/core/signing.py

 class TimestampSigner(Signer):
+    def __init__(self, *args, **kwargs):
+        self.expiration_key = kwargs.pop("expiration_key", None)
+        self.payload_timestamp = None


Changes to the TimestampSigner should be expressed in docs.

felixxm · 2022-11-25T12:44:41Z

django/contrib/sessions/backends/signed_cookies.py

            return signing.loads(
                self.session_key,
                serializer=self.serializer,
-                # This doesn't handle non-default expiry dates, see #19201


Can we remove this comment? Does it actually solves ticket-19201 as well? 🤔 I think we need more tests to actually confirm this.

This doesn't solve #19201, however the comment is no longer valid as signing now handles non-default expiry dates.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>

apollo13

First and foremost this needs more tests especially for the changes in core/signing.py. Secondly I wonder if it were possible instead to simply expose the timestamp via a kwarg to unsign/unsign_object. This way more complex extra validation could be done by the caller.

Something along the lines of:

def unsign(…, return_timestamp=False):
   if return_timestamp:
      return unsigned_data, timestamp
  return unsigned_data

apollo13 · 2022-12-29T10:00:03Z

django/core/signing.py

+            return super().unsign_object(signed_obj, max_age=max_age, **kwargs)
+
+        # If using expiration key use it's value when present
+        value = super().unsign_object(signed_obj, **kwargs, max_age=None)


I do not think that we should skip max_age. I have the feeling that max_age should be verified no matter what and the expiration_key should be an additional validation -- if one wouldn't like to have max_age validation then the caller wouldn't have to pass it in.

apollo13 · 2022-12-29T10:04:04Z

django/core/signing.py


 class TimestampSigner(Signer):
+    def __init__(self, *args, **kwargs):
+        self.expiration_key = kwargs.pop("expiration_key", None)


Thinking about this in terms of max_age, why is this part of the ctor and not solely passed to usign_object?

felixxm · 2023-02-23T07:35:04Z

@buugaj Do you have time to keep working on this?

buugaj · 2023-02-23T07:44:51Z

@felixxm Yup, I should find some time soon.

felixxm · 2023-03-15T12:36:39Z

Closing due to inactivity. Feel-free to send a new patch.

abe-101 · 2025-02-19T03:25:21Z

I am attempting to bring this PR back to life
@buugaj Do I have your permission?

abe-101 · 2025-02-19T05:25:59Z

#19191

Fixed #27775 -- Validate custom expiry in signed cookies.

59bd55e

felixxm added the DjangoCon 🦄 label Oct 20, 2022

github-actions bot reviewed Oct 20, 2022

View reviewed changes

#27775 -- Remove test_actual_expiry test override

df6896d

apollo13 self-requested a review October 25, 2022 20:14

felixxm reviewed Nov 25, 2022

View reviewed changes

Update django/core/signing.py

142c611

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>

apollo13 reviewed Dec 29, 2022

View reviewed changes

felixxm closed this Mar 15, 2023

jacobtylerwalls mentioned this pull request Feb 19, 2025

Fixed #27775 -- Added support for custom expiry in signed cookies. #19191

Open

5 tasks

Uh oh!

Conversation

buugaj commented Oct 20, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

felixxm commented Oct 24, 2022

Uh oh!

pelme commented Oct 24, 2022

Uh oh!

pelme commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apollo13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixxm commented Feb 23, 2023

Uh oh!

buugaj commented Feb 23, 2023

Uh oh!

felixxm commented Mar 15, 2023

Uh oh!

abe-101 commented Feb 19, 2025

Uh oh!

abe-101 commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pelme commented Oct 25, 2022 •

edited

Loading