Problem/Motivation

Follow-up to #3111506: Properly deprecate _access_rest_csrf route requirement
As the requirement were not properly deprecated before 9.0 it was moved to d10 removal

Proposed resolution

Remove route requirement
Remove test module and test case

Remaining tasks

wait 10.0.x branch and remove

User interface changes

no

API changes

no

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#24 3115308-24.patch9.98 KBandypost
#24 interdiff.txt1.23 KBandypost
#21 3115308-21.patch9.92 KBpaulocs
#16 interdiff_3115308_12-16.txt4.3 KBankithashetty
#16 3115308-16.patch9.04 KBankithashetty
#12 interdiff_3115308_9-12.txt4.38 KBankithashetty
#12 3115308-12.patch5.41 KBankithashetty
#9 3115308-9.patch9.22 KBandypost
#9 interdiff.txt1.05 KBandypost
#7 3115308-7.patch8.75 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Postponed » Active

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 9.4.x-dev » 10.0.x-dev
Issue tags: -@deprecated +Deprecation Removal, +Drupal 10, +ContributionWeekend2022
Parent issue: » #3213895: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 10 branch
Related issues: +#3261252: Remove deprecated system.module functions
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
3115308-7.patch8.75 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
@@ -53,12 +53,7 @@ public function applies(Route $route) {
     $applicable_requirements = [
       '_csrf_request_header_token',

This array only has one item now, then we do array_intersect() on it - I feel this could be refactored to be simpler?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Issue tags: +PHPStan-0
FileSize
interdiff.txt1.05 KB
3115308-9.patch9.22 KB

Thank you, good point to simplify

Meantime I see that function may return NULL so tagging

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thank you, that is much cleaner.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work

Re-reviewed this and I think we removed too much - we no longer have a test for _csrf_request_header_token.

  1. +++ /dev/null
@@ -1,27 +0,0 @@
-csrf_test.protected:
-  path: csrf/protected
-  defaults:
-    _controller: '\Drupal\csrf_test\Controller\TestController::testMethod'
-  requirements:
-    _csrf_request_header_token: 'TRUE'
-    _method: 'POST'

    This should not be removed. 

  2. +++ /dev/null
@@ -1,85 +0,0 @@
-      // Check both test routes.
-      $route_names = ['csrf_test.protected', 'csrf_test.deprecated.protected'];

    This test needs to stay, but only test the non-deprecated route.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3115308-12.patch5.41 KB
interdiff_3115308_9-12.txt4.38 KB

Updated the patch with changes mentioned in #11, thanks!

Status: Needs review » Needs work

The last submitted patch, 12: 3115308-12.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/modules/system/tests/src/Functional/CsrfRequestHeaderTest.php
@@ -0,0 +1,85 @@
+    $this->expectDeprecation('Route requirement _access_rest_csrf is deprecated in drupal:8.2.0 and is removed in drupal:10.0.0. Use _csrf_request_header_token instead. See https://www.drupal.org/node/2772399');

there should not be deprecation for token route

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/modules/system/tests/src/Functional/CsrfRequestHeaderTest.php
@@ -42,7 +42,7 @@ public function testRouteAccess() {
     foreach ($csrf_token_paths as $csrf_token_path) {
...
       foreach ($route_names as $route_name) {

Both these foreach() can be refactored away, there is only one route to test now.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3115308-16.patch9.04 KB
interdiff_3115308_12-16.txt4.3 KB

Tried to address both #14 and #15. Please review.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 16: 3115308-16.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review

Looks ready, re-queued as bot was wrong

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@ankithashetty thank you, it looks rtbc for me

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Needs work

This will need a re-roll and possibly some more adjustment after #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Needs review
FileSize
3115308-21.patch9.92 KB

Attaching a new patch.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Just one nitpick to check

+++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
@@ -69,7 +69,6 @@ protected function setUp(): void {
    * @group legacy
    */
   public function testCacheIntegration() {
-    $this->expectDeprecation('Route requirement _access_rest_csrf is deprecated in drupal:9.2.0 and is removed in drupal:10.0.0. Use _csrf_request_header_token instead. See https://www.drupal.org/node/2772399');

group should not be @legacy if there's no deprecations

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

+1 for #22

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.23 KB
3115308-24.patch9.98 KB

Fixed the same way second affected test - there's no deprecation now

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thank you.

  • catch committed aad867e on 10.0.x 
    Issue #3115308 by andypost, ankithashetty, paulocs, longwave: Remove...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed aad867e and pushed to 10.0.x. Thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Good riddance! :D

Status: Fixed » Closed (fixed)

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