Problem/Motivation

CsrfRequestHeaderAccessCheck::applies() checks the _method requirement to see which methods are supported by the route.
But this requirement is deprecated, the modern version to allow methods is with the methods property: https://www.drupal.org/docs/drupal-apis/routing-system/structure-of-rout...

Steps to reproduce

Create a route like this:

test:
  path: '/test_csrf_foo'
  methods: [ 'GET' ]
  defaults:
    _controller: '\Drupal\test\Controller\TestController::test'
    _title: 'Hello World'
  requirements:
    _permission: 'access content'
    _csrf_request_header_token: 'TRUE'

applies() returns TRUE even though the route only supports GET requests.

Proposed resolution

Check for the methods property instead.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3585886

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:

About issue forks

Comments

prudloff created an issue. See original summary.

sourav_paul’s picture

sourav_paul
Location Kolkata
commented

Checking...

sourav_paul opened merge request !15515

sourav_paul’s picture

sourav_paul
Location Kolkata
commented

Verified this locally by following the reproduction steps, and I can confirm the issue.

I also checked the route docs here:
https://www.drupal.org/docs/drupal-apis/routing-system/structure-of-rout...

methods is the modern way to define allowed HTTP methods on a route, while _method is the older legacy/deprecated style.

Local result:
when I used a route with _csrf_request_header_token: 'TRUE' and methods: ['GET'], CsrfRequestHeaderAccessCheck::applies() still returned TRUE. Since this is a read-only route, it should not apply the CSRF request header check.

So the fix should be to use the route methods first, and keep _method only as a fallback for older routes if needed.

[Used AI to re-verify the fix]

sourav_paul’s picture

sourav_paul
Location Kolkata
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Do we have test coverage for this?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3613245: Validate route requirements

Rediscovered this issue when looking at #3613245: Validate route requirements

I think that we can convert this access check to a static checker, it doesn't need to be dynamic at all.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

If we add applies_to: _csrf_request_header_token to the service definition then we can drop the applies() method entirely.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Nice! Everything looks good to me. Not sure how else to review.

  • catch committed 831ce148 on 11.x 
    task: #3585886 Outdated _method requirement in...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

This looks great, nice to be able to replace all that code with a one-liner! Committed/pushed to main and 11.x, thanks!

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.

  • catch committed fbfe9c0c on main 
    task: #3585886 Outdated _method requirement in...

Status: Fixed » Closed (fixed)

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