[Django] #25125: Add a check in the checks framework for colliding LANGUAGE_COOKIE_NAME & SESSION_COOKIE_NAME
Django
SESSION_COOKIE_NAME
--------------------------------------+--------------------
Reporter: kezabelle | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Noticed while about to do a separate ticket.
If the settings page needs an implied admonition under both settings to
explain the names should differ, I think its probably worthy of a check,
however infrequently it might come up in practice.
{{{
The name of the cookie to use for sessions. This can be whatever you want
(but should be different from LANGUAGE_COOKIE_NAME).
}}}
{{{
The name of the cookie to use for the language cookie. This can be
whatever you want (but should be different from SESSION_COOKIE_NAME)}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25125>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Changes (by timgraham):
* needs_better_patch: => 0
* component: Core (System checks) => contrib.sessions
* needs_tests: => 0
* easy: 0 => 1
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
I guess it should be added to as part of the sessions app (which currently
doesn't have any checks), but whoever works on this can check the
`check_user_model` check in contrib.auth. Tests can go
in`tests/sessions_tests/test_checks.py` and docs in `docs/ref/checks.txt`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:1>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
* owner: nobody => kswiat
* cc: konrad.swiat@… (added)
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:2>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by kswiat):
I wonder which tag should be registered (in case of checks for sessions
app). Is built in 'security' tag appropriate? Or a new one should be
defined?
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:3>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by timgraham):
I think "sessions" could have its own tag.
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:4>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:5>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by timgraham):
As I commented on the pull request, there's also `CSRF_COOKIE_NAME` which
I guess we could add to the mix. At that point though, it would be nice to
have a generic way for third-party apps that use cookies to add their
cookies to the mix. But this is starting to get complicated for what I
suspect probably isn't a very common problem. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:6>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by kezabelle):
I don't think worrying about third party apps is worth additional
complexity. They already have a generic way to ensure their cookie name
doesn't collide - write a check of their own and add it to their default
AppConfig. I appreciate this means that there's semi-duplicate code at the
3rd party end, but it means:
- they know it will be checked.
- they know which cookies '''could''' collide, given Django + their app
- their check is subsequently unlikely to change much, because Django only
ships N (3?) cookie names.
If collisions do happen downstream, users can open tickets there to try
and get them separately namespaced, or come back here and demand better.
Given the overall lack of reports about the possible problem, prior to my
noticing the settings notes, I'm not convinced the problem ''exists'' -
we're just introducing a check in Django to ensure it would always be
noticed if incorrect given the known variables. Better to have a check
which works now, and add some sort of hook down the road for third party
app authors, if the desire for such is presented.
To my mind, the check could go one of 2 ways:
- Stay as a session check (with the implication that it is about sessions
specifically) and make the warning say something like "session name cannot
be the same as language code name or csrf token name" and make the hint
say which one(s) it collided with.
- Stop being a session-oriented check, and emit an error for whichever
names collide (ie: if language code and csrf token names match, that
shouldn't be a "session check", because it's unrelated)
Addendum:
`CSRF_COOKIE_NAME` settings docs should probably have the same
parenthesised note the other cookies do, as I didn't pick that one up :)
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:7>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
If I am not missing something, since there are no conventions/restrictions
about cookie naming and we do not track a list of cookie names, such a
generic check must wait till runtime (I mean, we don't know which setting
is a cookie name till then)?
Maybe we should replace colliding names enumeration in docs with something
like "cookie name must be unique"?
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:8>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by timgraham):
I'd update the documentation to something like "The name can be whatever
you want as long as it's different from the other cookie names in your
application."
Now that `CSRF_COOKIE_NAME` is in the mix, it makes sense to be a core
check, but having a contrib app's setting in a core check isn't ideal
(maybe practicality beats purity). While third-party apps could write a
check to verify their cookies don't collide with Django's, they couldn't
write a check to verify their cookies don't collide with other apps'.
All in all, I don't think it's common problem that would be mitigated much
by a check so I tend to favor just updating the docs and moving on (other
opinions welcome).
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:9>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by kswiat):
I updated the PR with generic approach. Please let me now if this is
redundant since we have only 3 cookie names in Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:10>
Django
SESSION_COOKIE_NAME
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by timgraham):
I'm not sure what you mean by "redundant". I don't think it's very
elegant/useful if that's what you meant.
Regarding a potential implementation, I meant that we should put the check
in "core" because an error that says "The `LANGUAGE_COOKIE_NAME` and
`CSRF_COOKIE_NAME` settings must be different." coming from "sessions"
makes no sense. But if we put the check in core then we have to reference
a contrib setting `SESSION_COOKIE_NAME` in core which is bad because we
shouldn't give special treatment to contrib apps in core code. I suppose
we could have a sessions check and a core check if you are really
enthusiastic about it, but there are more useful tickets that could be
worked on, IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:11>
Django
-------------------------------+------------------------------------
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by kswiat):
* needs_better_patch: 1 => 0
* component: contrib.sessions => Documentation
Comment:
I closed the previous PR and created new one with only docs update on
cookie naming conventions
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:12>
Django
-------------------------------+------------------------------------
Reporter: kezabelle | Owner: kswiat
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2f6bdab1597ee42b36752bf9f624d3386e951379" 2f6bdab1]:
{{{
#!CommitTicketReference repository=""
revision="2f6bdab1597ee42b36752bf9f624d3386e951379"
Fixed #25125 -- Updated docs on cookie naming conventions.
Thanks Tim Graham for the review and kezabelle for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:13>
Django
-------------------------------+------------------------------------
Reporter: kezabelle | Owner: kswiat
Type: New feature | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"7355437e49833d68f8a02c91f8e0ed2059266e5f" 7355437e]:
{{{
#!CommitTicketReference repository=""
revision="7355437e49833d68f8a02c91f8e0ed2059266e5f"
[1.8.x] Fixed #25125 -- Updated docs on cookie naming conventions.
Thanks Tim Graham for the review and kezabelle for the report.
Backport of 2f6bdab1597ee42b36752bf9f624d3386e951379 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25125#comment:14>