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:

  1. Check out a clean Drupal 11.x and run core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php — all 5 tests pass.
  2. In testProcessOutboundJsonFormat(), after the second processOutbound() call (the one that passes a fresh BubbleableMetadata), add:
    $this->assertContains('session', $bubbleable_metadata->getCacheContexts());
  3. Run again — the new assertion fails, demonstrating that the non-HTML branch does not bubble the session cache 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

Issue fork drupal-3587298

Command icon 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

petar_basic created an issue. See original summary.

quietone’s picture

Version: 11.x-dev » main
Issue summary: View changes
Related issues: +#3485174: Menu APIs provide invalid CSRF tokens

Hi, 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.

petar_basic’s picture

Issue summary: View changes

petar_basic’s picture

MR + 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

petar_basic’s picture

Assigned: petar_basic » Unassigned
Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Didn’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

petar_basic’s picture

Assigned: Unassigned » petar_basic
petar_basic’s picture

Assigned: petar_basic » Unassigned
Status: Needs work » Needs review

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

fago’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

alexpott’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Nice 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!

  • alexpott committed fc5ba393 on main
    fix: #3587298 RouteProcessorCsrf::processOutbound() should bubble '...
petar_basic’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new9.46 KB

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed c2809119 on 11.x
    fix: #3587298 RouteProcessorCsrf::processOutbound() should bubble '...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.