User logout is vulnerable to CSRF

Re-rolled patch against 11.x and Tried to fix custom command failure at #164

Moving to NW for the issue summary update and change record

Did not review.

The patch should be converted into a merge request.

Though still NW for the change record and issue summary, please review the changes.

Updated issue summary.

In my manual testing, the token links didn't seem to work. Clicking on them just refreshed the page, so I'm not sure whether that needs another look? I didn't debug. The confirmation page works well.

Just noticed there are already two draft change records:

User logout route is now CSRF protected →
New route option for adding CSRF tokens to URLs without enforced validation. →

This was suggested in #121 to have one for the new feature and one for the change to logout links.

I have reviewed them both and made minor tweaks to the wording but I think they are all good. So we just need to fix the tests.

Working on the fix for those tests

In my manual testing, the token links didn't seem to work. Clicking on them just refreshed the page, so I'm not sure whether that needs another look

Confirmed - logout links currently do not work, luckily the UserLogoutTest proves this as it fails. Now working on a fix for that.

Also hiding all patch files.

Finishing up for the day so will post a summary:

1. Fixed the user/logout callback when there was a valid token
2. Fixed majority of test failures due to an invalid parameter passed to submitForm in drupalLogout
3. Started fixing all the other test failures. There were some strange ones like JsonApiFunctionalTest which randomly tests some auto logout stuff with maintenance mode. Really unsure why this is there but it needed a drupalResetSession. A couple of other spots need this same call to ensure the session is properly cleaned up when things in the background log the user out.
4. Marked the new confirm form as workspace safe - this fixed a few workspace related failures
5. Attempted to fix Nightwatch and Performance tests - neither of which I can successfully run locally (yet) so they may not pass

Not sure what's going on with that unit test though...

Performance tests have a merge conflict so might need a rebase, we added state caching so some of the state queries aren't executed at all any more, which could mean no changes here after all. If you're able to run functional javascript tests, you should be able to run performance tests, but please ping me in slack if they're specifically failing for you.

Rebase is done, let's see how it goes...

I pushed a change to add some typehint returns. If we aren't adding a description I removed that function.

Actually found an issue that return '' doesn't match the docs of the inherited docs.

But still needs fixes for performance in the javascript tests. Will see if anyone else can pick to not lose review ability.

If you're able to run functional javascript tests, you should be able to run performance tests

Last time I tried running these locally I was getting different counts than in CI, looks like that is no longer the case! :)

Feedback appears to be addressed and all tests passing

Only concern was the deprecation link goes to the link, but based on other conversations that may be okay for new parameters. And a 3rd CR seems a lot.

Last time I tried running these locally I was getting different counts than in CI, looks like that is no longer the case!

Ahh yeah that is fixed for a few weeks now after we separated cache / cache tags etc. out from queries. It was a problem with the chained fast backend sometimes executing a database query and sometimes not (by design for the chained fast backend, not ideal for trying to count queries).

The thing that gives me pause about this change is relying on the controller to do the csrf check. It feels like the wrong architecture.

I would if we could change it to work something like this:

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _user_is_logged_in: 'TRUE'
  options:
    _csrf_token: 'TRUE'
    _csrf_confirm_form: '\Drupal\user\Form\UserLogoutConfirm'

And then have event that catches the access denied and changes it to a form. This way the CSRF api works as expected and don't rely on the controller to do the security correctly.

Also added a comment to the MR - not sure why the performance test has a logout removed.

The thing that gives me pause about this change is relying on the controller to do the csrf check. It feels like the wrong architecture.

Yeah it felt a bit wrong to me as well, will look at how involved it would be to refactor with what you've suggested.

I think the best approach would be:

1. Change user.logout to have a _csrf_token requirement
2. Add a new _csrf_confirm_form_route option, which is detected in a new (or existing) EventSubscriber (that probably lives in a generic place like core/lib/Drupal along side the CsrfAccessCheck)
3. Add a user.logout.confirm route in user.routing.yml which is confirm form route. That will have a requirement on _user_is_logged_in only
4. Detect AccessDeniedHttpException and _csrf_confirm_form_route in that and return a RedirectResponse
5. Add a case for user.logout.confirm in user module's AccessDeniedSubscriber so logged out users hitting the confirm form are also redirected to the homepage.

The user.logout route then becomes

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _user_is_logged_in: 'TRUE'
    _csrf_token: 'TRUE'
  options:  
    _csrf_confirm_form_route: 'user.logout.confirm'

We can then revert all the handling for the _csrf_token_or_confirm option.

Pushed the new approach in #193

Feedback appears to be addressed

We need to update rhe CR and I left a minro comment on the MR.

@acbramley also fantastic work implementing #191

CRs updated and nightwatch fix applied.

Appears to be two open threads on the MR.

Will leave in review but CR updates seem fine.

Appears that all threads have been resolved

Saw a javascript error and I can't re-run the tests so I rebased the MR. All green

Updated for 📌 Add an API for allowing modules to mark their forms as workspace-safe Needs work

I think #162/ @Chi's suggestion is pretty interesting. As the standard is still in draft I don't think we can use it yet but I think we should open a follow-up to track it and if it does become a standard that is supported by all the browsers we support then we should use it.

Added issue credits to commenters and MR contributors.

https://caniuse.com/?search=sec-fetch-dest is pretty good now but not 100%, tagging for follow-up.

The three new cache IDs being requested on login are css:stark:enNXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM1, js:stark:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11 and js:stark:en:4-hTlRsZH1cF9ra9AHElDWka9bDSVy0q3vkHIjrTKIU11 which seems interesting because standard should not be using stark... but StandardPerformanceTest has protected $defaultTheme = 'stark'; which is unnecessary and wrong as the theme can be derived from the profile.

Anyhow I think we get these extra cache gets because the logout link is placeholdered so bigpipe gets involved.

but StandardPerformanceTest has protected $defaultTheme = 'stark'; which is unnecessary and wrong as the theme can be derived from the profile.

It's on purpose so that we're testing the modules in the standard profile rather than Olivero, but we should probably have separate stark and Olivero tests or something.

I checked that we weren't somehow generating extra CSS/JS aggregates because of the placeholdering, and we're not, we're running into 📌 Return early in AssetResolver::getJsAssets() and AssetResolver::getCssAssets() when there are no libraries to load Needs review , specifically bigpipe responses with no assets to add, do an extra cache get each time. We should fix that over there, and that will then bring the numbers back down.

Opened 📌 Use sec-fetch-dest header for CSRF protection Active .

Had a look at commit credit, this is a very long issue so apologies if anyone got overlooked etc.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

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

Corrected a typo in the IS.

Per @neclimdul, this is potentially disruptive and merits a release note. Thanks!

Adding the tag as well.

Instructions for writing release notes: https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

Thanks Adam! This looks good to me.

Thanks!

One improvement that we could probably add is explaining who is affected, and how. For example, one audience that is affected is site users, who will encounter a behavior change. I added a bit about that to the release note.

@neclimdul also described it as "a pretty ugly unthemed confirmation form", so I added a bit about that to the release note too. I wonder if there is something we could do to mitigate the theming issue? Separate possible followup, I guess.

Could someone check my modified release note above and ensure it's accurate? Meanwhile, I'm adding it to the 10.3.0 draft. Thanks!

"a pretty ugly unthemed confirmation form"

It looked OK to me in testing but that was using Olivero (see screenshot in IS). I'm not sure why it would not appear in the site's theme? I think a follow up would be good with some additional details and what the proposed fixes would be to address the concern. But it would definitely be good to clarify for the release note as I think the current text is not very clear.

Re #218 where are these comments coming from? The form is definitely themed as a frontend page (i.e the site's themed). Reworded the release note to remove references to this and provide better guidance. I was trying to keep the note brief as more information is available in the CR (which I linked to).

@acbramley, again, I linked the issue where the comment was posted above.

I asked @neclimdul to comment here so that he can speak for himself.

Meanwhile, fixed the link text accessibility issue.

Link to the comment 📌 10.3.0 release notes Active

Not much information to go on.

I think the issue is that not all front end themes, particularly custom themes, will render the confirmation form nicely. Previously a link to /user/logout would log the user out immediately, whereas now it shows a confirmation form. So custom themes should follow the steps in the change record → to avoid this.

again, I linked the issue where the comment was posted above.

Sorry, I saw you link the issue but didn't (and still don't) see that was where the comments were coming from. Should've checked it I guess.

Presumably it will have whatever styles apply to other forms though, and if you have users logging in I'd expect your login form to have some styles. I see the snippet is already updated to remove the part about the site's theme. I do think the rest can go in a follow up but just wondering about why a theme would have no styles or why this would look particularly ugly when it's just a heading and two buttons.

I have a few points here:

1. Can we update the procedural user_logout() in
   

   core/modules/user/src/Form/UserLogoutConfirm.php 

   with the service from https://www.drupal.org/project/drupal/issues/2012976 📌 Deprecate user_logout() and user_login_finalize() and replace with a service Needs review ?

2. CancelURL is always <front>, Should not this be conditional based on the destination parameter?

@Prashant.c That issue hasn't been finished yet, so the service doesn't exist.

I see how my comment was misunderstood. It was technically in the sites themes but we have several form designs so a new form has to be assigned to a specific look or it just ends up "unthemed". On our site, unthemed was quite ugly.

More my point was more that any form isn't desirable and a fair few years of building Drupal sites has made me lazy about dropping a menu link in a template.

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

This is probably worth opening a followup issue. Discussion on this issue should only be about what needs to go in the release note so that it can be marked back to fixed. @neclimdul, does the current release note snippet in the IS give sufficient info for a site owner to address what you encountered? Thanks!

@acbramley, sorry, I just realized this was actually my fault. I thought I had specifically stated where the comment had come from but apparently it never made it from my brain to the issue comment. :) My bad!

@xjm with the exception of my concern the link generated from the code in the changelog might store csrf links in caches, yes that addresses it.

Okay great, thanks! Let's mark this issue back to fixed, then.

It already has the "Needs followup" tag, which looks like it was left on by accident when @catch filed a different followup in #206. However, it looks like we have two additional potential followups to file, for #230 and the last paragraph in #23. I'll levae it to the respective contributors to file those followups since they will be best able to articulate the issues. Thanks again!

Belatedly saving additional credits for folks who helped sort the release note.

@neclimdul re caching see RouteProcessorCsrf::processOutbound() which handles lazy building - so as long as the URL is built using the routing system, that should be fine.

Obviously it won't work if a logout link is sent in an email or something, but that shouldn't happen for logout links really (and if it did, you'd get the form fallback).

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

liuyuanchao → changed the visibility of the branch 144538-user-logout-csrf to hidden.