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

[Jacob Walls]

The ​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:

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.

[Natalia Bidart]

Thanks!

[Jacob Walls]

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.

[Amar]

No worries at all, Jacob! You are welcome to take it. A combined PR makes perfect sense here.

[Jacob Walls]

​PR

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

[Jacob Walls]

Hi there. The attached PR is almost mergable, awaiting re-review on the release note. Any comments welcome!

[Jacob Walls <jacobtylerwalls@…>]

In 8acd000:

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.

[Jacob Walls <jacobtylerwalls@…>]

In 02f94d2:

Refs #37101, #37174 -- Added release note for cache misses when varying on arguments.

Thanks Natalia Bidart for the review.

[Jacob Walls <jacobtylerwalls@…>]

In 0383d12:

Refs #37174 -- Doc'd that args to the {% cache %} tag are stringified.

[Jacob Walls <jacobtylerwalls@…>]

In 0317b61:

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

[Jacob Walls <jacobtylerwalls@…>]

In e4cda2e:

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

[Jacob Walls <jacobtylerwalls@…>]

In d5ff018:

[6.1.x] Refs #37174 -- Doc'd that args to the {% cache %} tag are stringified.

Backport of 0383d12ea3451a75be184bd1783f11031903e7ee from main.