Problem/Motivation

There is no validation of logout requests, so users can be unknowingly logged out, by clicking on a misleading link or (as in OP) if there is an image on the page with the logout path as the source (<img srg="/user/logout">)

We should add a method to validate that logout requests are legitimate, or show a confirmation page to the user to ensure they know they are logging out.

Steps to reproduce

  1. Log in to a Drupal site
  2. Create a page with <img srg="/user/logout"> in the markup
  3. Visit the page, see you are logged out

Proposed resolution

The current MR adds a token as a query parameter to programatic logout links, e.g. those created by Drupal in the menus. If the link is to /user/logout without a token, the user sees a confirmation page:

Remaining tasks

  1. Another manual test to verify current changes?
  2. Review of updated solution
  3. Change record for the new route option - _csrf_token_or_confirm
  4. Commit!

User interface changes

Adds confirmation page to the logout process if the link does not contain a token, as shown above.

API changes

Adds a new route option _csrf_token_or_confirm with a token for programatic links to logout.

Data model changes

N/A

Release notes snippet

?

Original report by XANO

One of the users on my site posted the following code yesterday: <img src="/logout">. This causes every user to log out when visiting the page that code is on. He suggested making a special page at that address that uses a form to log out.

While we're at that, a system similar to that at Tweakers.net might be made. There you can select the session you want to log out. In that way you can log out the session you started on work bur forgot to end and you can do it from your home computer.

CommentFileSizeAuthor
#176 Screenshot 2024-03-26 at 4.47.30 pm.png71.42 KBpameeela
#170 144538-170.patch10.51 KBkarishmaamin
#164 logout_csrf_144538--164.patch9.88 KBdaften
#160 logout_csrf_144538--160.patch9.87 KBtuutti
#158 logout_csrf_144538-158-D8.patch10.14 KBTwoD
#158 logout_csrf_144538-158.patch9.73 KBTwoD
#155 logout_csrf_144538-154.patch10.14 KBTwoD
#152 144538-152.patch9.6 KBtuutti
#149 144538-149.patch9.06 KBL_VanDamme
#143 144538-143.patch9 KBtuutti
#141 144538-140.patch9.11 KBtuutti
#141 interdiff-144538-138-140.txt779 bytestuutti
#139 144538-138.patch8.19 KBtuutti
#132 144538-132.patch9.07 KBtuutti
#132 interdiff-144538-132.txt718 bytestuutti
#130 144538-130.patch9.16 KBtuutti
#126 Screenshot-2017-02-13-0836-47.png63.98 KBlarowlan
#126 Screenshot-2017-02-13-0835-50.png46.08 KBlarowlan
#117 144538-117.patch9.03 KBtuutti
#117 interdiff-144538-117.txt1.12 KBtuutti
#114 144538-114.patch9.03 KBtuutti
#114 interdiff-144538-114.txt2.31 KBtuutti
#109 144538--109.patch9.02 KBtuutti
#109 interdiff-144538--109.txt2.51 KBtuutti
#106 144538-105.patch8.91 KBtuutti
#106 interdiff-144538-105.txt4.55 KBtuutti
#102 144538_102.patch9.63 KBtuutti
#102 interdiff-144538_102.txt757 bytestuutti
#100 144538_100.patch8.75 KBtuutti
#100 interdiff-144538_100.txt2.32 KBtuutti
#98 144538_98.patch8.02 KBtuutti
#98 interdiff-144538_98.txt8.95 KBtuutti
#92 drupal_144538_92.patch3.72 KBg.oechsler
#82 drupal_144538_82.patch3.64 KBXano
#78 interdiff-144538-78.txt3.35 KBdamiankloip
#78 144538-78.patch4.52 KBdamiankloip
#76 drupal_144538_76.patch2.52 KBXano
#64 interdiff.txt2.89 KBXano
#64 drupal_144538_64.patch3.65 KBXano
#61 144538-61.patch2.73 KBdamiankloip
#39 144538.39-harden-logout-link.patch7.52 KBdeviantintegral
#31 144538-harden-logout-link.patch9.11 KBDamien Tournoud
#29 144538-harden-logout-link.patch9.11 KBDamien Tournoud
#26 144538-harden-logout-link.patch7.17 KBDamien Tournoud
#18 144538-harden-logout-link.patch6.79 KBDamien Tournoud
#14 144538-harden-logout-link.patch5.34 KBDamien Tournoud
#12 144538-harden-logout-link.patch5.34 KBDamien Tournoud

Issue fork drupal-144538

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Just curious: has this already been fixed?

drumm’s picture

Title: security: Image with logout url as source » Image with logout url as source
Version: 5.x-dev » 7.x-dev
Category: feature » task
Issue tags: +Security improvements

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.

drumm’s picture

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.

Xano’s picture

How do you suggest we filter this? HTML is themable and users may create exactly the same links in their posts as the official logout link.

alexanderpas’s picture

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.)

wretched sinner - saved by grace’s picture

Would this be a good use of [#3374646] 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.

alexanderpas’s picture

sun’s picture

Title: Image with logout url as source » Image with /logout URL as source
Issue tags: +FilterSystemRevamp

The filter system should prevent this.

Xano’s picture

How? There's nothing wrong with an element pointing to /logout, as long as the page isn't requested without the user wanting it.

alexanderpas’s picture

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!!! )

sun’s picture

Category: task » bug
Issue tags: -FilterSystemRevamp

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>';
Damien Tournoud’s picture

Status: Active » Needs review
FileSize
5.34 KB

Let's try that. Dead simple.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
5.34 KB

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

- The test bot

sun’s picture

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?

Damien Tournoud’s picture

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

sun’s picture

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.

Damien Tournoud’s picture

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).

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

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.

andypost’s picture

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();
  }
}
Damien Tournoud’s picture

Status: Needs work » Postponed
grendzy’s picture

Issue tags: +CSRF

subscribing

hunmonk’s picture

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

moshe weitzman’s picture

Priority: Critical » Normal
Damien Tournoud’s picture

Status: Postponed » Needs review
FileSize
7.17 KB

Reroll.

sun’s picture

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

Status: Needs review » Needs work

The last submitted patch, 144538-harden-logout-link.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
9.11 KB

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

andypost’s picture

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()

Damien Tournoud’s picture

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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs documentation

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

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

grendzy’s picture

Status: Reviewed & tested by the community » Needs work

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).

meba’s picture

@grendzy good catch

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

kwinters’s picture

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.

grendzy’s picture

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

kwinters’s picture

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).

jhodgdon’s picture

Issue tags: -Needs documentation

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?

deviantintegral’s picture

Version: 7.x-dev » 8.x-dev
FileSize
7.52 KB

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.

deviantintegral’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.

sun’s picture

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.

kwinters’s picture

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.

andypost’s picture

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

greggles’s picture

Title: Image with /logout URL as source » User logout is vulnerable to CSRF

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.

Xano’s picture

I believe this has been fixed in Drupal 7 by adding a random hash path component to the logout URL.

webchick’s picture

Nope. /user/logout still logs you out.

Abhishek Verma’s picture

Status: Needs work » Needs review
Issue tags: -Security improvements, -CSRF

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

Status: Needs review » Needs work
Issue tags: +Security improvements, +CSRF

The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.

chx’s picture

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)) {
chx’s picture

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

Fabianx’s picture

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.

webchick’s picture

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.

larowlan’s picture

Issue tags: +WSCCI

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

greggles’s picture

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

Crell’s picture

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. :-)

b8x’s picture

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?

yannickoo’s picture

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

b8x’s picture

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.

Anonymous’s picture

Beyond the potential for CSRF attacks, we should still consider whether we want to switch to using a POST request for logout instead of GET.

http://stackoverflow.com/questions/3521290/logout-get-or-post

(Not sure if this is the best issue for addressing this, but wanted to make a quick note).

damiankloip’s picture

Issue summary: View changes
FileSize
2.73 KB

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).

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 61: 144538-61.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
2.89 KB

Status: Needs review » Needs work

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

damiankloip’s picture

This interdiff confuses me.. :)

Xano’s picture

64: drupal_144538_64.patch queued for re-testing.

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

dawehner’s picture

+++ 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?

dibyadel’s picture

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

d dt

regilero’s picture

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.

greggles’s picture

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.

Crell’s picture

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.)

Xano queued 64: drupal_144538_64.patch for re-testing.

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 76: drupal_144538_76.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
3.35 KB

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?

Status: Needs review » Needs work

The last submitted patch, 78: 144538-78.patch, failed testing.

Xano queued 78: 144538-78.patch for re-testing.

The last submitted patch, 78: 144538-78.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.64 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 82: drupal_144538_82.patch, failed testing.

Xano’s picture

We've run into the old problem of session synchronization. After the user is logged in, the seeds used for CSRF token generation are different within the environment that runs the tests and the site under test. As such a token generated in a test can never be validated by the site under test.

pwolanin’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

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

Wim Leers’s picture

I don't understand why this is not major.

klausi’s picture

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

Wim Leers’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

g.oechsler’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
3.72 KB

Reroll against 8.3.x

Wim Leers’s picture

Status: Needs work » Needs review
Fabianx’s picture

.

Status: Needs review » Needs work

The last submitted patch, 92: drupal_144538_92.patch, failed testing.

klausi’s picture

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?

Fabianx’s picture

#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 :).

tuutti’s picture

Status: Needs work » Needs review
FileSize
8.95 KB
8.02 KB

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

Status: Needs review » Needs work

The last submitted patch, 98: 144538_98.patch, failed testing.

tuutti’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
8.75 KB

Lets try again.

Status: Needs review » Needs work

The last submitted patch, 100: 144538_100.patch, failed testing.

tuutti’s picture

tuutti’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work

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.

tuutti’s picture

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.

tuutti’s picture

klausi’s picture

Status: Needs work » Needs review
klausi’s picture

+++ 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.

tuutti’s picture

klausi’s picture

Issue tags: +Needs change record

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.

Fabianx’s picture

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.

Crell’s picture

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. :-)

dawehner’s picture

+++ 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

tuutti’s picture

dawehner’s picture

Status: Needs review » Needs work

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

klausi’s picture

_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.

tuutti’s picture

tuutti’s picture

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

tuutti’s picture

Status: Needs review » Needs work
klausi’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

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

dawehner’s picture

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

Fabianx’s picture

Are we sure we want _requirement vs. _option?

klausi’s picture

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

larowlan’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
46.08 KB
63.98 KB

Works well, screenshots of manual testing

Showing token in logout link URI

Showing confirm form with a made up token

pwolanin’s picture

+++ 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?

klausi’s picture

@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.

pwolanin’s picture

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.

tuutti’s picture

Re-rolled the patch against 8.4.x.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

tuutti’s picture

Status: Needs work » Needs review
FileSize
718 bytes
9.07 KB

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

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 132: 144538-132.patch, failed testing.

tuutti’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Reviewed & tested by the community

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

cilefen’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 132: 144538-132.patch, failed testing.

tuutti’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

Re-rolled the patch.

Status: Needs review » Needs work

The last submitted patch, 139: 144538-138.patch, failed testing.

tuutti’s picture

Status: Needs work » Needs review
FileSize
779 bytes
9.11 KB

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tuutti’s picture

Rerolled for 8.5.x

SocialNicheGuru’s picture

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');

tuutti’s picture

Please reroll against 8.4.2 also

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

L_VanDamme’s picture

Rerolled for later versions of 8.6.x

klausi’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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.

tuutti’s picture

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');
mpp’s picture

Glad to see this is nearly ready!

Shouldn't we inject stringtranslation into UserLogoutConfirm?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

TwoD’s picture

Status: Needs work » Needs review
FileSize
10.14 KB

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.

klausi’s picture

Status: Needs review » Needs work

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?

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

TwoD’s picture

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.

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.

tuutti’s picture

Re-rolled for 9.2.x.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

Chi’s picture

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

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.

daften’s picture

Re-rolled the patch for 9.2.6

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.

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

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.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.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mxr576’s picture

Issue tags: +Needs reroll
karishmaamin’s picture

Status: Needs work » Needs review
FileSize
10.51 KB

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

smustgrave’s picture

Status: Needs review » Needs work

Moving to NW for the issue summary update and change record

Did not review.

solideogloria’s picture

The patch should be converted into a merge request.

solideogloria’s picture

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

solideogloria’s picture

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

pameeela’s picture

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.