Use set_signed_cookie for contrib.messages Cookie storage

[Anthony King]

This relates to django.contrib.messages.storage.cookie.

In its current state, the Cookie store implements it's own signing method (called _hash).
This uses a it's own approach to verifying the data inside the cookie.

using set_signed_cookie removes duplicate code, as well as allows the message cookie to use custom signing backends.

There is, perhaps, another change that can be made, which is to use signing.dumps to take advantage of the zlib compression. However this has potential to be a breaking change for people that read the JSON in the cookie, and may not yield better results in size.

[Tim Graham]

I haven't investigated but since contrib.messages was added in 2009 and set_signed_cookie() in 2011, there probably wasn't an intentional change not to use it. I guess a deprecation period that offers backwards-compatibility for the old format will be needed.

[reficul31]

Instead of using set_signed_cookie method we could probably replace the _hash method by signing.get_cookie_signer(salt=key + salt).sign(messages)?

[Craig Anderson]

What should happen to cookies signed with the existing hashing method?

I haven't tested this (yet), but it looks like messages stored with the old hashing method will be silently ignored.

We could:

1. accept that messages from Django < 2.3 (?) are ignored;
2. add the existing _hash implementation into _decode for these legacy messages; or
3. just close this ticket.

I'll go ahead and implement option 2 as it's the safest.

However, as I've never left messages in storage for more than one or two request-response cycles, option 1 strikes me as reasonable and much cleaner.

[Mariusz Felisiak]

​PR

[Florian Apolloner]

Replying to Craig Anderson:

However, as I've never left messages in storage for more than one or two request-response cycles, option 1 strikes me as reasonable and much cleaner.

this is true, but even "just for one request" can be an issue for large sites. The safest bet is option two where we keep the dual decoding for a whole LTS period and then drop it. Given that 2.2 is already out and we cannot really safely introduce any new changes there I think we should merge option 2 into master and remove the second codepath in 4.2. This would mean that people upgrading from 2.2 (LTS) -> 3.2 (LTS) -> 4.2 (LTS) would not have (many) issues unless they directly jump from 2.2 -> 4.2

We'd need a proper mention in the relevant release notes.

[Florian Apolloner]

Btw looking through the comments on this ticket, is there any reason not to use set_signed_cookie()? Sure the changes will be bigger, but if we do a change like this we can just as well make it "nice"

[Claude Paroz]

I created a ​new PR where the storage is using the Signer sign and unsign methods.

About using set_signed_cookie, I don't think it makes sense, because CookieStorage has to calculate the length of the signed encoded messages before setting the cookie value. So when we are setting the real cookie value, we already have the signed value at hand. Using set_signed_cookie instead of set_cookie would simply re-compute needlessly the messages, and would also need refetching a new signer when we have already one at hand (as we need it for the length stuff).

[Mariusz Felisiak <felisiak.mariusz@…>]

In bcc9fa25:

Refs #27604 -- Added CookieStorage.key_salt to allow customization.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8ae8415:

Fixed #27604 -- Used the cookie signer to sign message cookies.

Co-authored-by: Craig Anderson <craiga@…>

[Mariusz Felisiak <felisiak.mariusz@…>]

In 75daea2:

Refs #27604 -- Fixed loading of legacy cookie hashes when CookieStorage.key_salt is changed.

This partially reverts bcc9fa25285f506666fa5074fc43c7114d61bb79 to
not break legacy hashes when key_salt is actually changed.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 831a05b:

Refs #27604 -- Removed support for the pre-Django 3.1 encoding format in CookieStorage.

Per deprecation timeline.