Problem/Motivation
Discovered on Drupal core 11.3.7. After #3485174: Menu APIs provide invalid CSRF tokens, RouteProcessorCsrf::processOutbound() returns an actual CSRF token (instead of a render placeholder) for non-HTML requests. The bubbleable metadata is not updated with the session cache context, so when the URL is rendered into a cacheable response (e.g. via REST or JSON:API), the response is not keyed by session.
Same-role users sharing other cache contexts (e.g. user.permissions) get cached responses carrying each other's session-specific CSRF tokens.
The HTML branch's lazy-builder render callback already declares '#cache' => ['contexts' => ['session']] (RouteProcessorCsrf lines
79-81). The non-HTML branch should have the same semantics.
Steps to reproduce
Steps to reproduce
Reproducible against the existing core unit test:
- Check out a clean Drupal 11.x and run
core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php— all 5 tests pass. - In
testProcessOutboundJsonFormat(), after the secondprocessOutbound()call (the one that passes a freshBubbleableMetadata), add:
$this->assertContains('session', $bubbleable_metadata->getCacheContexts()); - Run again — the new assertion fails, demonstrating that the non-HTML branch does not bubble the
sessioncache context.
As manifested in practice: any code that generates a URL to a _csrf_token-protected route from a non-HTML context (e.g. a REST/JSON:API resource serving
a logout link) and embeds the URL in a cacheable response, will not see session in the response's bubbled cache contexts. If the response is otherwise
keyed only by role (user.permissions), users sharing the same role receive each other's cached tokens.
Proposed resolution
In RouteProcessorCsrf::processOutbound(), in the non-HTML / no-bubbleable-metadata branch, add the session cache context to the provided
metadata:
That would mirror the HTML-branch render callback (lines 79-81) which already declares the session context.
Remaining tasks
User interface changes
Introduced terminology
API changes
None. The method signature is unchanged. Bubbleable metadata gains an additional cache context; additive and BC-safe for callers reading metadata.
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3587298-3-bubble-session-cache-context-11x.patch | 9.46 KB | petar_basic |
| #5 | 3587298-2-bubble-session-cache-context.patch | 1.99 KB | petar_basic |
Issue fork drupal-3587298
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:
Comments
Comment #2
quietone commentedHi, Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.
Comment #3
petar_basic commentedComment #5
petar_basic commentedMR + patch attached. One-liner that bubbles session on the non-HTML branch — mirrors what the HTML branch's render callback already declares. Existing unit test extended.
Validated end-to-end with a two-user reproduction on a vanilla 11.3.7 install. BTW, a companion contrib issue: https://www.drupal.org/project/rest_menu_items/issues/3587299
Comment #6
petar_basic commentedComment #7
smustgrave commentedDidn’t have a chance to heavily dig into it but appears to break the functional tests? Again didn’t heavily dig into it but need a passing pipeline please
Comment #8
petar_basic commentedComment #9
petar_basic commentedCI is green now
Fixes for tests:
- Block/SearchPage/View REST fixtures and LinksetControllerTest::testUserAccountMenu now include 'session' in expected cache contexts — these all expose CSRF-protected admin URLs that legitimately bubble it.
- For basic_auth, HEAD vs GET Link headers get a ?token= placeholder before comparison. Stateless auth has no session continuity between requests, so the CSRF seed regenerates each time and the tokens differ. The old assertion only passed because the dynamic page cache was serving one session's cached response — CSRF tokens included — to a different session's request, which is exactly the bug this MR fixes.
- small unit-test change to give BubbleableMetadata::addCacheContexts() a cache_contexts_manager stub.
Comment #10
fagoI reviewed the patch. The fix addresses the problem and is right, token must vary by session. I tested the API response also and correctly got the cache-context header and varying token by session. MR comes with test-coverage also, seems ready!
Comment #11
alexpottNice find and fix.
Committed fc5ba39 and pushed to main. Thanks!
The MR does not cherry-pick cleanly to 11.x so we need a new branch against 11.x. Thanks!
Comment #13
petar_basic commentedI created an 11.x port - branch 11.x-port on the issue fork.
The cherry-pick required one trivial conflict resolution in RouteProcessorCsrfTest.php setUp where 11.x still uses the legacy getMockBuilder() for $this->csrfToken while main switched to createStub(). Otherwise identical to the merged commit.
Patch attached.
Comment #15
smustgrave commentedSeems like a good rebase. https://git.drupalcode.org/project/drupal/-/merge_requests/15743
Comment #16
alexpottCommitted c280911 and pushed to 11.x. Thanks!
Couldn't backport it to 11.3.x either so atleast fixing in 11.x so the bug will be resolved in 11.4.0.