Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
When user.roles cache contexts are used for anonymous or authenticated users, the values for these contexts are used around the wrong way.
Anonymous users will have a user.roles:authenticated=1 cache context while authenticated users while have user.roles:anonymous=1 and user.roles:authenticated=0.
The code snippet below is from Drupal\Core\Cache\Context\UserRolesCacheContext. Note the final return statement where the 1 and 0 on the ternary statement are around the wrong way.
/**
* {@inheritdoc}
*/
public function getContext($role = NULL) {
// User 1 does not actually have any special behavior for roles; this is
// added as additional security and backwards compatibility protection for
// SA-CORE-2015-002.
// @todo Remove in Drupal 9.0.0.
if ($this->user->id() == 1) {
return 'is-super-user';
}
if ($role === NULL) {
return implode(',', $this->user->getRoles());
}
else {
return (in_array($role, $this->user->getRoles()) ? '0' : '1');
}
}
Since values remain unique, this issue is largely semantic in core. However, I've been trying to integrate user.roles with Varnish which relies on the these values by correct.
Proposed resolution
Swap the return numbers, so anonymous users will have a user.roles:authenticated=0 or user.roles:anonymous=1.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3101664
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
matroskeenI noticed this issue as a part of Bug Smash Initiative. My understanding is the same as it states in the description, so I'm curious to see what it breaks.
Comment #4
matroskeenComment #5
wim leersWoah! 😅 😂 The painful thing here is that fixing this will in fact lead to cache pollution and potentially information disclosure … 😱
So can we instead of using
0and1usefalseandtrue? Otherwise we need to be absolutely certain that the cache gets wiped and locked while the code gets deployed.P.S.: why ? While confusing, this is not causing bugs AFAICT, it's purely cosmetic?
Comment #6
matroskeenTotally agree about the Priority, I think "Minor" is fair enough.
I assume empty
hook_post_update_NAME()won't give us this level of certainty to apply this cosmetic change, will it?I'll try to research and find similar issues that were fixed in the past to see what was the strategy :)
Comment #7
matroskeenI reviewed existing cache contexts and I didn't find similar changes in the commit history, so I'm not sure if there is any right way to be "absolutely certain that the cache gets wiped".
I also didn't find cases when the return value has
true/false.Given this is just a cosmetic change that might potentially lead to information disclosure, maybe it's a "won't fix"?
Comment #12
claudiu.cristea@Matroskeen
The information disclosure might come in the case the cache is NOT wiped when this change will deployed on a site. Suddenly, all cached content for authenticated users will be exposed to anonymous and vice versa. Using a something different than
0and1will prevent that and also there's no need for a "clear cache" post-update function.Comment #14
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Tagging for test case to show this issue.
Did not test patch.
Comment #17
bvoynickI've rebased claudiu.cristea's MR (in a separate -11.x branch) and added a test.
If the MR can be changed to point to that branch, I think this would be ready for review.
Comment #19
bvoynickI didn't read docs closely enough, reusing an MR for a different branch is not possible.
Opened new MR #4170 for the -11.x branch.
Comment #20
claudiu.cristeaAdded some remarks in MR. Tests were added, removing the tag
Comment #21
bvoynickThanks, issues with MR 4170 should be addressed and ready for re-review.
Comment #22
smustgrave commentedSeems like a simple change
Verified the tests fail without the fix with this output
Failed asserting that two strings are identical.
Expected :'true'
Actual :'0'
Which is good.
LGTM
Comment #24
catchCommitted 2231e59 and pushed to 11.x. Thanks!
Thought about backporting to 10.1.x but if someone was actually calling this directly somewhere and relying on the result (seems unlikely) it could cause issues, so leaving for 10.2.x for now.
Comment #25
wim leersWow, how did I get this so wrong in #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles')?! 🙈
Nice fix!