[Django] #37174: Template fragment cache key collision for vary_on values containing ":"

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
------------------------------+-------------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Template system
Version: 6.0 | Severity: Normal
Keywords: not-security | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+-------------------------------------------
The [https://docs.djangoproject.com/en/6.0/topics/cache/#template-
fragment-caching documented example] for template fragment caching
demonstrates using `request.user.username` as a `vary_on` argument. If
that username contained `:`, and another `vary_on` argument was present,
then two cache keys might collide, and the wrong content could be served.

See this PoC provided to the Security Team:

{{{#!py
from django.core.cache.utils import make_template_fragment_key
a = make_template_fragment_key("frag", ["alice", "b:c"])
b = make_template_fragment_key("frag", ["alice:b", "c"])
assert a == b # same key
}}}


We decided against accepting this as a security issue given the
unlikelihood of colons in the data most important to vary on from a
security perspective, e.g. usernames in a ''username'' + ''language code''
vary_on pair, but there is a correctness issue to fix here.

One fix strategy would involve incorporating the lengths of the arguments
into the cache key.

Since this will cause cache busting, we should probably document in the
release note something similar to the note from
5cb3ed187b283059589cb442c56a66a795800cac.
--
Ticket URL: <https://code.djangoproject.com/ticket/37174>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"

---------------------------------+------------------------------------
Reporter: Jacob Walls | Owner: (none)

Type: Bug | Status: new
Component: Template system | Version: 6.0

Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

---------------------------------+------------------------------------
Changes (by Natalia Bidart):

* stage: Unreviewed => Accepted

Comment:

Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:1>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
---------------------------------+------------------------------------

Reporter: Jacob Walls | Owner: Amar
Type: Bug | Status: assigned

Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------

Changes (by Amar):

* owner: (none) => Amar
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:2>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"

---------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls

Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

---------------------------------+---------------------------------------
Changes (by Jacob Walls):

* owner: Amar => Jacob Walls

Comment:

Amar, this one is almost identical to #37101, so if you don't mind, I'm
going to fix them both in the same PR to aid review.
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:3>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
---------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------

Comment (by Amar):

No worries at all, Jacob! You are welcome to take it. A combined PR makes
perfect sense here.
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:4>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
---------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/21516 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:5>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
---------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------

Comment (by Juan Pedro Roldán):

Hi,

I reviewed this ticket while looking for a small code-related
contribution. I noticed it is already assigned, so I won’t work on it
directly.
I reproduced the issue locally using the PoC from the description with
`make_template_fragment_key("frag", ["alice", "b:c"])` and
`make_template_fragment_key("frag", ["alice:b", "c"])`. Both calls
currently produce the same cache key:
`template.cache.frag.486f15e693cb74d155ff00eb53e7e306`
I also added a small local regression test in `tests/cache/tests.py` under
`TestMakeTemplateFragmentKey`:
`test_vary_on_values_with_colons_do_not_collide`
As expected, the test fails on the current branch, confirming the reported
behavior.

I hope this helps with triage/testing. I’d be happy to test a proposed
patch if needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:6>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
---------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:
Keywords: not-security | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------

Comment (by Jacob Walls):

Hi there. The attached PR is almost mergable, awaiting re-review on the
release note. Any comments welcome!
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:7>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"

-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Template system | Version: 6.0
Severity: Normal | Resolution:

Keywords: not-security | 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 Natalia Bidart):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:8>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls

Type: Bug | Status: closed

Component: Template system | Version: 6.0

Severity: Normal | Resolution: fixed

Keywords: not-security | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"02f94d2a899b6573f90dee5417afb8a763ee7f25" 02f94d2]:
{{{#!CommitTicketReference repository=""
revision="02f94d2a899b6573f90dee5417afb8a763ee7f25"
Refs #37101, #37174 -- Added release note for cache misses when varying on
arguments.

Thanks Natalia Bidart for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:10>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Template system | Version: 6.0
Severity: Normal | Resolution: fixed
Keywords: not-security | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"0383d12ea3451a75be184bd1783f11031903e7ee" 0383d12]:
{{{#!CommitTicketReference repository=""
revision="0383d12ea3451a75be184bd1783f11031903e7ee"
Refs #37174 -- Doc'd that args to the {% cache %} tag are stringified.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:11>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Template system | Version: 6.0
Severity: Normal | Resolution: fixed
Keywords: not-security | 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 Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"8acd000725838f73c5e6781114f11866f6674c46" 8acd000]:
{{{#!CommitTicketReference repository=""
revision="8acd000725838f73c5e6781114f11866f6674c46"
Fixed #37174 -- Used netstring delimiter between vary on arguments for
cached template fragments.

This algorithm used a delimiter, but because it didn't use a length,
argument values containing the delimiter could still cause collisions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:9>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Template system | Version: 6.0
Severity: Normal | Resolution: fixed
Keywords: not-security | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"0317b612e67868a796f0ccc90a498f2172e1b59f" 0317b61]:
{{{#!CommitTicketReference repository=""
revision="0317b612e67868a796f0ccc90a498f2172e1b59f"
[6.1.x] Fixed #37174 -- Used netstring delimiter between vary on arguments

for cached template fragments.

This algorithm used a delimiter, but because it didn't use a length,
argument values containing the delimiter could still cause collisions.

Backport of 8acd000725838f73c5e6781114f11866f6674c46 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:12>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Template system | Version: 6.0
Severity: Normal | Resolution: fixed
Keywords: not-security | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"d5ff018475c3613372d45ce8a58c25ec7eb857af" d5ff018]:
{{{#!CommitTicketReference repository=""
revision="d5ff018475c3613372d45ce8a58c25ec7eb857af"
[6.1.x] Refs #37174 -- Doc'd that args to the {% cache %} tag are
stringified.

Backport of 0383d12ea3451a75be184bd1783f11031903e7ee from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:14>

[Django]

#37174: Template fragment cache key collision for vary_on values containing ":"
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Template system | Version: 6.0
Severity: Normal | Resolution: fixed
Keywords: not-security | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"e4cda2e5dd314bb5f9a15b6b4d74a14a87ae1dec" e4cda2e]:
{{{#!CommitTicketReference repository=""
revision="e4cda2e5dd314bb5f9a15b6b4d74a14a87ae1dec"
[6.1.x] Refs #37101, #37174 -- Added release note for cache misses when

varying on arguments.

Thanks Natalia Bidart for the review.

Backport of 02f94d2a899b6573f90dee5417afb8a763ee7f25 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37174#comment:13>