Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#27604 closed Cleanup/optimization (fixed)

Use set_signed_cookie for contrib.messages Cookie storage

Reported by: Anthony King Owned by: Craig Anderson
Component: contrib.messages Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (14)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

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.

comment:3 by reficul31, 7 years ago

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

comment:5 by Craig Anderson, 5 years ago

Owner: set to Craig Anderson
Status: newassigned

comment:6 by Craig Anderson, 5 years ago

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.

comment:7 by Mariusz Felisiak, 5 years ago

Has patch: set

comment:8 by Florian Apolloner, 5 years ago

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.

Last edited 5 years ago by Florian Apolloner (previous) (diff)

comment:9 by Florian Apolloner, 5 years ago

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"

comment:10 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:11 by Claude Paroz, 4 years ago

Patch needs improvement: unset

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

comment:12 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In bcc9fa25:

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

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 8ae8415:

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

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

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 831a05b:

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

Per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top