User logout is vulnerable to CSRF

The security team has decided to re-open this as a public issue. It is well-known and a good solution requires community input.

Please do report any other security issues to security@drupal.org and not the public issue queue.

We can't add a token, many themes that expect the path as-is. Detecting if the request was a forgery is impossible. Specifically filtering for that tag with that attribute might be doable.

I guess this explains how img tags can be used maliciously. Users posting images can be useful, I would like to see a solution implemented if something is possible.

if we implement tweakers.net solution, we certainly need usability testing for it!

however, a simpler might be a form on /logout, just like we handle delete actions.

also we can catch the logout link on other pages with JS to add a (session-specific) token so it can be handled automatically (can be combined with previous option.)

Would this be a good use of #3374646: eca 1.1.4 for D7, if we put the confirmation page in place - it would then degrade to the current delete style confirmation page for non-JS browsers and D6 (and D5 I guess?)

It will need to have a graceful degredation, otherwise the issue will still be there for non-JS browsers.

indeed a good place to implement #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations

The filter system should prevent this.

filter is not a solution, as this does not protect against malicious external websites

for example <img src="http://drupal.org/logout" /> on http://buytaert.net/ ( fictional!!! )

Good point. I agree that the filter system can't handle this.

So, the usual thing to do in cases like this is to ask: How do others do it?

Looking at some other system, I found an approach that really is bad-ass simple:

Just append a query string to the logout link, a md5 hash of $user->login (timestamp of last login). So the menu callback of user/logout simply validates that query string before proceeding.

If we really want or have to aid in theming of custom logout links, we can simply use and add a helper function user_logout_query_string(), which only builds the query string bit that can be used both in user_translated_menu_link_alter() as well as in custom themes:

// page.tpl.php

echo '<div class="logout-link">';
echo l('Logout', 'user/logout', array('query' => user_logout_query_string()));
echo '</div>';

Let's try that. Dead simple.

Dear DamZ, at least run the code you wrote before throwing it at my face next time.

- The test bot

Awesome! Even simpler than I envisioned :)

+  $ret[] = update_sql("UPDATE {menu_links} SET link_path = 'user/logout/%' WHERE link_path = 'logout'");
+  $ret[] = update_sql("UPDATE {menu_links} SET router_path = 'user/logout/%' WHERE router_path = 'logout'");
...
+  $items['user/logout/%token'] = array(

I guess that router_path should use %token as well?

Nope, paths are stored in their simple forms (% instead of %token) in the database. The loader functions themselves are in menu_router.load_functions.

I think this is ready to go...

Only remaining idea: Would a test to ensure that user/logout returns a 403 make sense? We would be testing the menu system actually... Not sure.

Well, it wasn't working, because the load function was missing from the previous patch. Added a very simple login / logout test (this is very well tested already by DrupalWebTestCase).

Erm. This looks like a bug in the menu module:

function menu_reset_item($item) {
  $new_item = menu_link_load(menu_get_item($item['router_path']));
  foreach (array('mlid', 'has_children') as $key) {
    $new_item[$key] = $item[$key];
  }
  menu_link_save($new_item);
  return $new_item;
}

menu_reset_item() is calling menu_get_item() with a $path like 'node/%', where menu_get_item() is expected a non-replaced path, like 'node/123'.

I'm not sure how to fix that.

I use following in my own module:

function andy_menu_alter(&$items) {
  $items['logout']['options']['alter'] = TRUE;
  $items['logout']['page callback'] = 'andy_logout';
}

function andy_translated_menu_link_alter(&$item, $map) {
  if (user_is_logged_in()) {
    $item['href'] = 'logout/'.drupal_get_token();
  }
}

function andy_logout($token = NULL) {
  if (drupal_valid_token($token)) {
  	user_logout();
  }
  else {
  	print '?';
  	exit();
  }
}

This requires #204077: Allow menu links pointing to dynamic paths.

subscribing

posting this here for reference -- perhaps somebody could turn Damien's patch into a small module as a workaround for now?

#620280-1: Protect the logout link against CSRF attacks

Reroll.

Interesting. This patch contains at least 50% of what would be required for #755584: Built-in support for csrf tokens in links and menu router

We now need to store the state of the browser (session ID, cookies, isLoggedIn) when switching back from one session to the other. Fixed.

Great fix!

+++ includes/path.inc
@@ -589,9 +589,9 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
-      $item['link_path']  = $form_item['link_path'];
-      $item['link_title'] = $form_item['link_title'];
-      $item['external']   = FALSE;
+      $item['link_path']  = $path;
+      $item['link_title'] = '';

Interesting, how does it works before without notices ($form_item is undefined)

+++ modules/simpletest/tests/session.test
@@ -186,14 +186,35 @@ class SessionTestCase extends DrupalWebTestCase {
+      $state = array(
+        'loggedInUser' => $this->loggedInUser,
+        'cookies' => $this->cookies,
+        'session_id' => $this->session_id,
+      );
+      file_put_contents($this->cookieFile . '.state', serialize($state));

Another file that makes simpletest getting slower

+++ modules/user/user.test
@@ -929,6 +929,39 @@ class UserPictureTestCase extends DrupalWebTestCase {
+    // No assertion needed here: assertions are already verified in drupalLogin().
+    $this->drupalLogout();

Suppose drupalLogin() should be drupalLogout()

Interesting, how does it works before without notices ($form_item is undefined)

I was neither working nor tested before.

Another file that makes simpletest getting slower

Not measurable. And this only concerns the session tests.

Suppose drupalLogin() should be drupalLogout()

D'oh. Rerolled for that.

A patch is good to go, also tests are fixed.

this change should be documented at http://drupal.org/node/719612

shouldn't the calls to drupal_get_token() pass a value (a fixed string like 'user/logout' would be fine), to prevent a sort of replay attack? (e.g. if a hypothetical contrib module used drupal_get_token() with no value to authenticate a different action, a user might be able to forge the token by copying the argument from the logout link).

@grendzy good catch

Somebody suggested a logout function to help users creating the link. Shouldn't we create it?

Is this going to have an impact on menu block caching? It seems like blocks that once cached per role (anon vs not) now must cache per uid AND the cache has to be cleared every time their last login time changes.

According to http://api.drupal.org/api/function/menu_block_info/7 , menu blocks are not cached.

Well, it also says that "menu.inc manages its own caching." So, there is still the question of whether the menu caching system will handle it appropriately.

Custom blocks that don't use the menu module directly, but still want to link to to the logout function still seem like they're going to be affected. Or for that matter, it will no longer be possible to embed a logout link into a node body (like a page explaining to users how to manage their accounts or security or something along those lines).

If the logout function worked automatically with the token and displayed a delete-form-like confirm if the token was missing or incorrect, that would be acceptable. It may have some caching effects but they'd be greatly mitigated (the cost of an incorrectly cached link is very low then).

Removing the Needs Documentation tag, as I think this patch didn't go in and it's clogging the Needs Doc issue queue. Does it need to be moved to D8 at this point?

Here's a rerolled patch against 8.x. The patch seems to work fine with manual testing, but I'm running into an issue with the test. For some reason, the "Log out" link doesn't render in the simpletest environment. The token load function doesn't even get called in in the test. Any ideas?

This patch also adds 'user/logout' as a token value as suggested in #33.

Setting to needs review so others can see the test failure.

This issue is kind of a duplicate of #1344078: Local image input filter in core now, which attacks the original cause instead (malicious user posting HTML containing arbitrary references). It's the same code that runs on drupal.org today.

I'm not sure whether adding and requiring a token for user/logout, as proposed here, is a sensible answer and solution, and whether this issue shouldn't be marked as duplicate.

No such luck, Sun. This issue is for a CSRF exploit, and the other issue can't prevent posting of said image tag on random forums, etc.

Maybe better to postpone this as pointed in #22 for #204077: Allow menu links pointing to dynamic paths

The title described one potential way to exploit the issue rather than the fundamental nature (which probably caused the confusion in 42/43).

Better title.

Nope. /user/logout still logs you out.

#39: 144538.39-harden-logout-link.patch queued for re-testing.

Actually, while the CSRF is valid, the local image filter does protect because it checks whether the image source is a valid image :

      $local_image_path = $local_dir . drupal_substr($src, $base_path_length);
      if (@getimagesize($local_image_path)) {

Of course there can be many other ways to exploit this but -- this is more a nuisance than anyhing else.

Needs work:

To fallback it should be:

user/logout => Page which says: Do you really want to logout? Click here to logout, such the image is harmless
user/logout/hash => Logout directly

That way it is backwards compatible and links to /user/logout still work and then themes can update.

The extra step does not hurt.

That could be a security hole in the other direction, though... someone clicks "log out" before leaving a public computer, doesn't notice the confirm page, since you typically don't get one elsewhere on the web. H4X0R3D.

Tagging, I recall seeing auto CSRF protection discussions in/around router patch.

I think the discussion with CSRF and WSCCI was "this will need a re-roll post the router patch" so wait for that.

WSCCI is looking at a CSRF token integrated into routes: #1798296: Integrate CSRF link token directly into routing system

Otherwise this issue is entirely new to me. :-)

so what?
problem still exist, you can put in code of any site :
<img src="https://drupal.org/logout" border="0" alt="" />
and it will log out you from drupal.org. all i want to know is how i can disable or modify reactions on "user/logout" path? i can simply use token but i don't know where to put this code to preprocess logout. or i can create my own logout page, but how to disable core "user/logout" path?

You could download the Menu token protect (CSRF protection) module :/

Now I comment out the code in user.module:

  $items['user/logout'] = array(
    'title' => 'Log out',
    'access callback' => 'user_is_logged_in',
    'page callback' => 'user_logout',
    'weight' => 10,
    'menu_name' => 'user-menu',
    'file' => 'user.pages.inc',
  );

and created my own log out page. my project already has 100 contrib. modules, and increasing its number is last i want.

So, now we have csrf integrated into the routing system. We can do something just like this. I guess alot of tests will break with this. Should work in a normal site env though, with a token being added to the logout link (in the toolbar user section in this patch, as a demo).

This interdiff confuses me.. :)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -719,7 +719,7 @@ protected function drupalLogout() {
-    $this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
+    $this->drupalGet(\Drupal::url('user.logout'), array('query' => array('destination' => 'user')));
     $this->assertResponse(200, 'User was logged out.');

So I guess this will not longer work?

I corrected all files as above and it worked well for my D 7.22 site. Thanks a lot.

d dt

A good way to fix that is to require a POST usage. On the client side you can capture the click on logout buttons (maybe with a central "a.drupal-logout" javascript behavior) and transform it in POST requests via jQuery.

For user not having js activated, so still using a GET, or modules not implementing this class on the logout link, a confirmation form should be presented, with a POST confirmation.

This would prevent nasty images and will be transparent for most users.

I don't think POST alone completely solves the problem unless the post includes a csrf-token. 3rd party sites could still have javascript that executes the POST and logs someone out.

Yes, the POST would need a token. The same logic applies to any link-action we have in Drupal: We should set a CSRF token in a meta or header or JS setting or something; then the link itself goes to a confirmation form. Then JS can, if enabled, mutate that link to just send a POST request itself, using the provided token, sans confirmation form.

That is probably worth a standard operation in Drupal that we can then leverage for logout and various other places (eg, flag module's various flag links, etc.)

So I think we may need to add a drupalGetRoute() or something that we can use to generate the user logout link in this case. If we use the path we will not get outbound url processing.

A couple of other fixes too, like the user edit route name.

So I think now we might just have a problem with the token seeds or something?

Not sure that test addition makes sense - or you want to avoid breaking drupalLogout()?

#2403307: RPC endpoints for user authentication: log in, check login status, log out introduced another instance of a CSRF vulnerable logout route.

I don't understand why this is not major.

Because the attack does not have severe consequences. The user gets logged out, which is just an annoyance.

But I could perform that very attack on this very issue… which would literally mean nobody would be able to read & comment on this issue (unless they manually construct that URL).

/me is now tempted :P

Reroll against 8.3.x

.

Ha, so this fails because we are running into our own CSRF protection, which is good :)

The point is that a logout link can only be used if it has been rendered on a page with the CSRF token. But we don't have a dedicated page in Drupal where we can guarantee that a logout link will even be shown. Logout links are in menus, and the menu might not even be shown.

I propose that we show a confirm form on /user/logout when the CSRF token is not present. This would mean that we cannot use the automatic _csrf_token of the routing system but we would have a better experience for existing sites which render /user/logout links wrongly without token. That way users don't see a 403 access denied page when they click such an old wrong link that is printed somewhere.

And this solution helps us with automated testing: WebTestBase::drupalLogout() can always work by just going to this confirm form and clicking the button. We do not rely on any logout links in menus to be present, yay!

Then we just need one explicit test which has the correct CSRF logout link in some menu. There we can test that logging out directly from a menu link also works.

What do you think about that approach?

#96+1 to that, I think that was even proposed before in some issue somewhere.

So, yes +10 to showing a form in case no CSRF token is present.

That is a good solution :).

Not sure if there is an easier way to add CSRF token to the route, but here's an initial patch.

Lets try again.

Overall makes sense to me.

1. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
   @@ -32,7 +32,7 @@ function __construct(CsrfTokenGenerator $csrf_token) {
      public function processOutbound($route_name, Route $route, array &$parameters, BubbleableMetadata $bubbleable_metadata = NULL) {
   -    if ($route->hasRequirement('_csrf_token')) {
   +    if ($route->hasRequirement('_csrf_token') || $route->hasRequirement('_csrf_token_optional')) {
   

   Hm, _csrf_token_optional is a route requirement? That does not seem right. The user logout route does not strictly require a CSRF token, because it falls back to a confirm form. Can it be a route option instead?

   What would be a good name for a route option? Stay with _csrf_token_optional? Don't have a better idea yet.

   Why do we even need _csrf_token_optional? We could also fix our cache contexts in user_toolbar() and LoginLogoutMenuLink.php, right? We might have to duplicate the renderPlaceholderCsrfToken logic from RouteProcessorCsrf, so not sure if that approach would be better. At least we would avoid introducing a new _csrf_token_optional routing concept with this patch ...

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -274,10 +286,25 @@ public function userTitle(UserInterface $user = NULL) {
   +      return $this->formBuilder()
   +        ->getForm('\Drupal\user\Form\UserLogoutConfirm');
   

   use ::class syntax instead of putting the class name into a string.

3. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -274,10 +286,25 @@ public function userTitle(UserInterface $user = NULL) {
   +    if (!$this->csrfToken->validate($token, 'user/logout')) {
   +      drupal_set_message($this->t('Invalid csrf token.'), 'error');
   +
   +      return $this->redirect('<front>');
   +    }
   

   That message is not useful to end users. Maybe we don't need a message at all?

   I think we should also return the confirm form in this case.

4. +++ b/core/modules/user/src/Form/UserLogoutConfirm.php
   @@ -0,0 +1,51 @@
   +  public function getDescription() {
   +    return NULL;
   +  }
   

   getDescription() should always return a string, so the empty string in this case.

5. +++ b/core/modules/user/tests/src/Functional/UserLogoutTest.php
   @@ -0,0 +1,52 @@
   +    // Enable the theme.
   +    \Drupal::service('theme_installer')->install(['bartik']);
   +    $theme_config = \Drupal::configFactory()->getEditable('system.theme');
   +    $theme_config->set('default', 'bartik');
   +    $theme_config->save();
   

   the comment should say why we have to enable Bartik. Because of the menu with logout link or something else? Shouldn't that also work with the default stark theme?

   Instead of \Drupal we should use $this-get() instead.

6. +++ b/core/modules/user/tests/src/Functional/UserLogoutTest.php
   @@ -0,0 +1,52 @@
   +    $this->drupalGet('user');
   +    $this->getSession()->getPage()->clickLink(t('Log out'));
   

   Never use t() in tests, we are not testing the translation system here.

7. +++ b/core/modules/user/tests/src/Functional/UserLogoutTest.php
   @@ -0,0 +1,52 @@
   +    // Make sure user gets logged out.
   

   "Make sure the user gets logged out."

8. +++ b/core/modules/user/user.module
   @@ -1328,6 +1328,7 @@ function user_toolbar() {
          'logout' => array(
            'title' => t('Log out'),
            'url' => Url::fromRoute('user.logout'),
   +        'route_name' => 'user.logout',
   

   why do we need to specify the route name AND the url here? That confuses me. Shouldn't the route name be enough? Why do we need both?

9. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -797,7 +797,7 @@ protected function drupalLogout() {
   -    $this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
   +    $this->drupalPostForm('user/logout', [], t('Confirm'), ['query' => ['destination' => 'user']]);
   

   never use t() in tests.

I left _csrf_token_optional unchanged (for now) and fixed other things you pointed out.

Why do we even need _csrf_token_optional?

It was an easiest way I could think of how to add a CSRF token parameter to the user.logout route.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -407,7 +407,7 @@
-    $this->drupalPostForm('user/logout', [], t('Confirm'), ['query' => ['destination' => 'user/login']]);
+    $this->drupalPostForm('user/logout', [], 'Confirm', ['query' => ['destination' => 'user/login']]);

Hm, I think the button label should be "Log out" not "Confirm" to make the action clear.

Thanks a lot! The patch looks ready to me now.

I looked a bit into avoiding _csrf_token_optional in the routing processor, but it just would make this more complicated and we could not leverage the code that is already there.

We should probably add test coverage to RouteProcessorCsrfTest for _csrf_token_optional.

And we also need a change record where we document the changes to user logout behavior and what people need to change in their code (for example if they hard coded /user/logout in a template what they need to do instead).

It looks like we have no special places in core where we document _csrf_token, so I guess it is fine if we don't document _csrf_token_optional in the source code? We can later add it to https://www.drupal.org/docs/8/api/routing-system/access-checking-on-routes for example.

I'm +1 to this. Before we put more work into developing this further somebody other than me should probably as well confirm that this is the way we want to go.

Looks indeed really good. I also think that csrf_token_optional is not really yet describing what it does. I am giving a +1, but leaving for someone else to RTBC it.

csrf_token_passthrough? (Ie, it passes through to a confirmation form.)

In hindsight, we should have made ALL CSRF-ish-links do that; go to a confirm form, and then JS-enhance with a token to turn into a direct link. Notes to self for Drupal 9. :-)

+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -32,7 +32,7 @@ function __construct(CsrfTokenGenerator $csrf_token) {
-    if ($route->hasRequirement('_csrf_token')) {
+    if ($route->hasRequirement('_csrf_token') || $route->hasRequirement('_csrf_token_optional')) {

What about making it explicit: _csrf_token_or_confirm

Thank you @tuutti !
We certainly still needs the change record though

_csrf_token_or_confirm is a bit better than _csrf_token_optional, yay!

+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -32,7 +32,7 @@ function __construct(CsrfTokenGenerator $csrf_token) {
-    if ($route->hasRequirement('_csrf_token')) {
+    if ($route->hasRequirement('_csrf_token') || $route->hasRequirement('_csrf_token_or_confirm')) {

As I said this should not be a route requirement. It should be a route option instead because the routing system does never block access with this.

Setting back to 'Needs work' due to #115.

Added a change record draft. I didn't mention _csrf_token_or_confirm, not sure if we want to say something about that?

IMHO we should educate people about the new feature. Maybe having a second change record could be useful?

Are we sure we want _requirement vs. _option?

I would say it should be a route option, because you really have to do the token validation yourself in the route callback. A route requirement usually implies that some checking will we performed on the incoming request, which is not the case with our _csrf_token_or_confirm.

The _csrf_token_or_confirm route option only makes sure that a CSRF token is added when the URL is generated outbound and the appropriate cache contexts are set.

This issue is still ready to be reviewed, please check the latest patch and change notice proposal :)

Works well, screenshots of manual testing

Showing token in logout link URI

Showing confirm form with a made up token

+++ b/core/modules/user/tests/src/Functional/UserLogoutTest.php
@@ -0,0 +1,46 @@
+    // Make sure the user gets logged out.
+    $this->drupalGet('user/login');
+    $this->assertSession()->fieldExists('name');

Don't we have a better way of verifying the user is logged out - like the session cookie gets deleted?

@pwolanin: that is a good question! From the philosophy of functional/system level tests we are doing here ideally you should only test the behavior that is observable by the user. A session cookie is a technical implementation detail the user does not care about. The user expects to see a login form element again after they have logged out and that is exactly what we are testing here. So I would argue that assertion is perfectly fine and in line with testing best practices.

Ok, I see the point of testing it from the standpoint of user-observable behaviors.

Hopefully we will add to the doc on d.o to warn people away from this route option unless they know what they are doing.

Re-rolled the patch against 8.4.x.

+++ b/core/modules/user/src/Controller/UserController.php
@@ -274,10 +287,20 @@ public function userTitle(UserInterface $user = NULL) {
+    $token = $request->query->get('token');
+
+    // Show confirm form when no$this->drupalPostForm('user/logout', [], 'Log out', ['query' => ['destination' => 'user']]); valid csrf token is present.
+    if (!$token || !$this->csrfToken->validate($token, 'user/logout')) {

Something went wrong with this comment line?

Good catch! I was wondering why the new patch was bigger than the previous one, but couldn't see any difference.

Thanks, looks good!

Looks like a testbot glitch.

https://dispatcher.drupalci.org/job/drupal_patches/9567/console

Yep, that was not a test fail but some Jenkins problem.

I updated credit because a lot of people have contributed to the direction this issue has taken.

Re-rolled the patch.

Rerolled for 8.5.x

Edit
I applied 140 since that it passed Drupal 8.4 tests.
I will try the one that passed 8.5 tests too.
---
Please reroll against 8.4.2 also

git apply 144538-140.patch
error: patch failed: core/tests/Drupal/Tests/BrowserTestBase.php:739
error: core/tests/Drupal/Tests/BrowserTestBase.php: patch does not apply

patch -p1 < 144538-140.patch
patching file core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
patching file core/modules/simpletest/src/WebTestBase.php
Hunk #1 succeeded at 326 (offset -51 lines).
patching file core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
Hunk #1 succeeded at 63 (offset -1 lines).
patching file core/modules/user/src/Controller/UserController.php
patching file core/modules/user/src/Form/UserLogoutConfirm.php
patching file core/modules/user/tests/src/Functional/UserLogoutTest.php
patching file core/modules/user/user.routing.yml
patching file core/tests/Drupal/Tests/BrowserTestBase.php
Hunk #1 FAILED at 739.
1 out of 1 hunk FAILED -- saving rejects to file core/tests/Drupal/Tests/BrowserTestBase.php.rej

more core/tests/Drupal/Tests/BrowserTestBase.php.rej

--- core/tests/Drupal/Tests/BrowserTestBase.php
+++ core/tests/Drupal/Tests/BrowserTestBase.php
@@ -739,7 +739,7 @@
// idea being if you were properly logged out you should be seeing a login
// screen.
$assert_session = $this->assertSession();
- $this->drupalGet('user/logout', ['query' => ['destination' => 'user']]);
+ $this->drupalPostForm('user/logout', [], 'Log out', ['query' => ['destination' => 'user']]);
$assert_session->statusCodeEquals(200);
$assert_session->fieldExists('name');
$assert_session->fieldExists('pass');

Please reroll against 8.4.2 also

#143 seems to apply against 8.4.x and 8.4.2 as well.

Rerolled for later versions of 8.6.x

Thanks, still looking good!

This issue does look very nearly ready.

+++ b/core/modules/user/tests/src/Functional/UserLogoutTest.php
@@ -0,0 +1,46 @@
+    // Test invalid csrf token.
+    $this->drupalGet('user/logout', ['query' => ['token' => '123']]);
+    $this->assertSession()->buttonExists('Log out');

There's no test coverage that clicking on the button on the confirm form actually logs the user out. That looks like pretty important functionality to test.

We also should have a separate change record for the new route option - _csrf_token_or_confirm that describes how it works and should be used.

Also the docs in RouteProcessorCsrf feel a bit light.

Re-rolled for 8.8.x.

There's no test coverage that clicking on the button on the confirm form actually logs the user out. That looks like pretty important functionality to test.

This should already be covered by drupalLogout(), but I added a test for it to be sure.

diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php
index d614bc2b1..c233c1212 100644
--- a/core/modules/simpletest/src/WebTestBase.php
+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -327,7 +327,7 @@ protected function drupalLogout() {
     // Make a request to the logout page, and redirect to the user page, the
     // idea being if you were properly logged out you should be seeing a login
     // screen.
-    $this->drupalGet('user/logout', ['query' => ['destination' => 'user/login']]);
+    $this->drupalPostForm('user/logout', [], 'Log out', ['query' => ['destination' => 'user/login']]);
     $this->assertResponse(200, 'User was logged out.');
     $pass = $this->assertField('name', 'Username field found.', 'Logout');
     $pass = $pass && $this->assertField('pass', 'Password field found.', 'Logout');

Glad to see this is nearly ready!

Shouldn't we inject stringtranslation into UserLogoutConfirm?

Re-rolled against 8.9.x and used the same pattern for the new injected dependency as was done for $flood.

Not sure what documentation to expand in RouteProcessorCsrf.

Test coverage looks good to me. Some minor things:

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -81,6 +92,11 @@ public function __construct(DateFormatterInterface $date_formatter, UserStorageI
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $token_generator parameter is deprecated in drupal:8.8.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/1681832', E_USER_DEPRECATED);
   

   needs to be updated to version 8.9.0

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -313,10 +330,20 @@ public function userTitle(UserInterface $user = NULL) {
       *   A redirection to home page.
   

   we need to update this comment. "A redirect response if a CSRF token was provided, otherwise a logout confirmation form render array."

Then please write the second change record for the new _csrf_token_or_confirm route option as Alex said.

For the RouteProcessorCsrf docs: We could add to the class "This class will add a token URL query parameter to all routes that have a _csrf_token or _csrf_token_or_confirm requirement set."

We could add to the method before the first if(): "Add a CSRF token URL query parameter to all routes that have a _csrf_token or _csrf_token_or_confirm requirement set."

That is a bit redundant, but does not hurt :)

@alexpott: any more specific documentation you would like to see?

Re-rolled for 9.1.x and 8.9.x.

I guess we're stuck with the dependency deprecation message until D10 now?

Created a change record draft and added comments similar to those mentioned in #156.

Re-rolled for 9.2.x.

This also needs an issue summary update. If nothing else, it would help if it included the proposed resolution and the remaining tasks.

There is a simple way to mitigate this issue on HTTPS sites.
https://w3c.github.io/webappsec-fetch-metadata

public function logout(Request  $request) {
  $fetch_destination = $request->headers->get('sec-fetch-dest');
  if ($fetch_destination && $fetch_destination != 'document') {
    throw new AccessDeniedHttpException('Wrong fetch destination.');
  }

  if ($this->currentUser()->isAuthenticated()) {
    user_logout();
  }
  return $this->redirect('<front>');
}

Chrome based browsers already support this.

Eventually we could implement a global access checker to allow any modules to specify fetch metadata requirements for their routes. Something like this.

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _fetch_destination: document
    _fetch_mode: navigate

Re-rolled the patch for 9.2.6

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.

The new UserLogoutConfirm form calls user_logout(), which would need to be changed if/when this issue is committed.

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 #3208390: Add an API for allowing modules to mark their forms as workspace-safe

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 #3414398: AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets, 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.

edit: updated that MR #3414398: AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets. Once this gets in, the test changes on there will need updating (to show a bigger relative improvement).

Opened #3442917: Use sec-fetch-dest header for CSRF protection.

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!

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.

The release note does at least need to include information for the reader to decide whether or not they are the affected audience so that they know whether to read the CR (because otherwise, they won't). Aside from the hypothetical theming issue, I think the current note still does a fair job of addressing that, once we fix a small accessibility issue with it.