[Django] #27604: Use set_signed_cookie for contrib.messages Cookie storage
Django
------------------------------------------------+------------------------
Reporter: Anthony King | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.messages | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27604>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Anthony King | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.messages | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:1>
Django
Reporter: Anthony King | Owner: reficul31
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by reficul31):
* status: new => assigned
* owner: nobody => reficul31
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:2>
Django
Reporter: Anthony King | Owner: reficul31
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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)```?
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:3>
Django
Reporter: Anthony King | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: contrib.messages | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by reficul31):
* owner: reficul31 => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:4>
Django
Reporter: Anthony King | Owner: craiga
Type: Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by craiga):
* status: new => assigned
* owner: (none) => craiga
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:5>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:6>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11220 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:7>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Florian Apolloner):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:8>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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"
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:9>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:10>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* needs_better_patch: 1 => 0
Comment:
I created a [https://github.com/django/django/pull/12397 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:11>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:12>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Component: contrib.messages | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8ae84156d62bfc24d71e65cfe4d5cb84b9b1bd91" 8ae8415]:
{{{
#!CommitTicketReference repository=""
revision="8ae84156d62bfc24d71e65cfe4d5cb84b9b1bd91"
Fixed #27604 -- Used the cookie signer to sign message cookies.
Co-authored-by: Craig Anderson <cra...@craiga.id.au>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:14>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"bcc9fa25285f506666fa5074fc43c7114d61bb79" bcc9fa25]:
{{{
#!CommitTicketReference repository=""
revision="bcc9fa25285f506666fa5074fc43c7114d61bb79"
Refs #27604 -- Added CookieStorage.key_salt to allow customization.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:13>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Component: contrib.messages | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"75daea2fc24da1c987d4fd979adb31a2c5a29d22" 75daea2]:
{{{
#!CommitTicketReference repository=""
revision="75daea2fc24da1c987d4fd979adb31a2c5a29d22"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:15>
Django
Reporter: Anthony King | Owner: Craig
Type: | Anderson
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"831a05b1859f960dba0aff3ac46daa40ca70704e" 831a05b]:
{{{
#!CommitTicketReference repository=""
revision="831a05b1859f960dba0aff3ac46daa40ca70704e"
Refs #27604 -- Removed support for the pre-Django 3.1 encoding format in
CookieStorage.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27604#comment:16>