Per #2239009: Remove public direct usage of the '_system_path' request attribute, _system_path is an internal concept that nothing other than collaborators of the router itself should deal with.

CsrfAccessCheck::access() needs to validate a token generated by RouteProcessorCsrf::processOutbound(). The extra string passed to the token generator/validator doesn't need to be a _system_path; it could be $route_name and $route_parameters serialized in any other desired way.

CommentFileSizeAuthor
#12 2293501-remove-system-path-from-csrf-access-check-12.patch5.35 KBWim Leers
#9 2293501-remove-system-path-from-csrf-access-check-9.diff5.2 KBznerol
#4 interdiff.txt23.48 KBdawehner
#4 access-2293501-4.patch23.48 KBdawehner
#2 drupal-2293501-2.patch7.73 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Major
Issue tags: +beta target
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
Issue tags: +PHPUnit
FileSize
drupal-2293501-2.patch7.73 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-2293501-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
access-2293501-4.patch23.48 KB
interdiff.txt23.48 KB

The access manager seems to be not aware of route matches.

Status: Needs review » Needs work

The last submitted patch, 4: access-2293501-4.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Closed (duplicate)

This was resolved by #2331079: Use RouteMatch in access-checks and remove RequestHelper::duplicate()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Closed (duplicate) » Needs work

Meh ... I was looking at the wrong CSRFAccessCheck class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Closed (duplicate)

Meh alex, I gave you the other issue in IRC. #2302065: Be able to pull of the current path from the request/route match

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Closed (duplicate) » Needs review
FileSize
2293501-remove-system-path-from-csrf-access-check-9.diff5.2 KB

Reopening this, because this is not resolved with #2331079: Use RouteMatch in access-checks and remove RequestHelper::duplicate().

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll due to AccessResult in #2340507: Make the new AccessResult API and implementation even better

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Reviewed & tested by the community
FileSize
2293501-remove-system-path-from-csrf-access-check-12.patch5.35 KB

Straight reroll.

  • catch committed 4691e43 on 8.0.x 
    Issue #2293501 by dawehner, Wim Leers, znerol: Use route name and params...
catch’s picture

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

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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