Problem/Motivation

There is an old todo in CsrfRequestHeaderAccessCheck::access():

      // @todo Remove validate call using 'rest' in 8.3.
      //   Kept here for sessions active during update.

Steps to reproduce

Proposed resolution

Remove this check:

&& !$this->csrfToken->validate($csrf_token, 'rest')) {

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 3585891-nr-bot_mqzc2b2i.txt91 bytesneeds-review-queue-bot
#10 3585891-nr-bot_1k0sc03_.txt1.27 KBneeds-review-queue-bot

Issue fork drupal-3585891

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

Working on this.

sourav_paul opened merge request !15510

sourav_paul’s picture

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

smustgrave commented
Status: Needs review » Needs work

Appears to have test failures

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

Tests where failing because UserAuthenticationController was still generating a CSRF token with the old key.
This makes me wonder if it should be deprecated first (contrib could also be generating CSRF tokens with this key).

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

yea deprecation sounds best. Almost wonder if at the time deprecations weren't as enforced since the todo is pointing to 8.4.

macsim made their first commit to this issue’s fork.

macsim’s picture

macsim commented
Status: Needs work » Needs review

Rebased the branch on main and added the deprecation.
Though I am not certain we can deprecate it now and remove it in Drupal 12 since Drupal 12 is almost here - it might need to target Drupal 13 instead.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3585891-nr-bot_1k0sc03_.txt1.27 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

macsim’s picture

macsim commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Would be deprecated in 11.4 and depends on how disruptive it would be if it can be removed in 12 or have to wait till 13 but would need to reflect in a CR

Thanks

macsim’s picture

macsim commented

CR is here: https://www.drupal.org/node/3591939

macsim’s picture

macsim commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Small comments on the MR, leaving in review.

smustgrave’s picture

smustgrave commented
Title: Old todo in CsrfRequestHeaderAccessCheck » Deprecate Validating CSRF tokens with the 'rest' key in CsrfRequestHeaderAccessCheck
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Looks like a good deprecation to me

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3585891-nr-bot_mqzc2b2i.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

macsim’s picture

macsim commented
Status: Needs work » Needs review
smustgrave’s picture

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

Rebase seems fine

godotislate closed merge request !15510

  • godotislate committed b91680f9 on main 
    task: #3585891 Deprecate Validating CSRF tokens with the 'rest' key in...

  • godotislate committed 2cb4b6f0 on 11.x 
    task: #3585891 Deprecate Validating CSRF tokens with the 'rest' key in...

  • godotislate committed 41b8957d on 11.4.x 
    task: #3585891 Deprecate Validating CSRF tokens with the 'rest' key in...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Consulted with @longwave, and it should be good for 11.4 backport.

Committed and pushed b91680f to main, and 2cb4b6f to 11.x, and 41b8957 to 11.4.x. Thanks!

I also reviewed and published the CR.

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.

Status: Fixed » Closed (fixed)

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