#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 , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 8 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 , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 6 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:
- accept that messages from Django < 2.3 (?) are ignored;
- add the existing
_hash
implementation into_decode
for these legacy messages; or - 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:8 by , 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.
comment:9 by , 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 , 5 years ago
Patch needs improvement: | set |
---|
comment:11 by , 5 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 , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I haven't investigated but since
contrib.messages
was added in 2009 andset_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.