Add nonce to the logout link (#3264)

* Add nonce to the logout link
* Add tests for cookies being set or reset
* More tests: check nonces are different for different actions & users

Fixes #3170

.gitignore

@@ -31,6 +31,7 @@ includes/**/tests/
 build/
 coverage/
 phpunit.xml
+.phpunit.result.cache
 tests/yourls-tests-config.php
 tests/vendor/
 tests/data/auth/config-test-auth-hashed.php

admin/admin-ajax.php

@@ -40,11 +40,6 @@
 		echo json_encode(array('success'=>$query));
 		break;
 
-	case 'logout':
-		// unused for the moment
-		yourls_logout();
-		break;
-
 	default:
 		yourls_do_action( 'yourls_ajax_'.$action );
 

includes/functions-auth.php

@@ -20,6 +20,7 @@ function yourls_maybe_require_auth() {
 /**
  * Check for valid user via login form or stored cookie. Returns true or an error message
  *
+ * @return bool|string|mixed true if valid user, error message otherwise. Can also call yourls_die() or redirect to login page. Oh my.
  */
 function yourls_is_valid_user() {
 	// Allow plugins to short-circuit the whole function
@@ -32,7 +33,9 @@ function yourls_is_valid_user() {
 	$unfiltered_valid = false;
 
 	// Logout request
-	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' ) {
+	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' && isset( $_REQUEST['nonce'] ) ) {
+        // The logout nonce is associated to fake user 'logout' since at this point we don't know the real user
+        yourls_verify_nonce('admin_logout', $_REQUEST['nonce'], 'logout');
 		yourls_do_action( 'logout' );
 		yourls_store_cookie( null );
 		return yourls__( 'Logged out successfully' );
@@ -444,6 +447,8 @@ function yourls_store_cookie( $user = null ) {
 	if ( $domain == 'localhost' )
 		$domain = '';
 
+	yourls_do_action( 'pre_setcookie', $user, $time, $path, $domain, $secure, $httponly );
+
     if ( !headers_sent( $filename, $linenum ) ) {
         yourls_setcookie( yourls_cookie_name(), yourls_cookie_value( $user ), $time, $path, $domain, $secure, $httponly );
 	} else {
@@ -576,7 +581,7 @@ function yourls_salt( $string ) {
  *
  */
 function yourls_create_nonce( $action, $user = false ) {
-	if( false == $user )
+	if( false === $user )
 		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
 	$tick = yourls_tick();
 	$nonce = substr( yourls_salt($tick . $action . $user), 0, 10 );
@@ -612,24 +617,25 @@ function yourls_nonce_url( $action, $url = false, $name = 'nonce', $user = false
  *
  */
 function yourls_verify_nonce( $action, $nonce = false, $user = false, $return = '' ) {
-	// get user
-	if( false == $user )
-		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
+	// Get user
+	if( false === $user ) {
+        $user = defined('YOURLS_USER') ? YOURLS_USER : '-1';
+    }
 
-	// get current nonce value
-	if( false == $nonce && isset( $_REQUEST['nonce'] ) )
-		$nonce = $_REQUEST['nonce'];
+	// Get nonce value from $_REQUEST if not specified
+	if( false === $nonce && isset( $_REQUEST['nonce'] ) ) {
+        $nonce = $_REQUEST['nonce'];
+    }
 
 	// Allow plugins to short-circuit the rest of the function
-	$valid = yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return );
-	if ($valid) {
+	if (yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return ) === true) {
 		return true;
 	}
 
-	// what nonce should be
+	// What nonce should be
 	$valid = yourls_create_nonce( $action, $user );
 
-	if( $nonce == $valid ) {
+	if( $nonce === $valid ) {
 		return true;
 	} else {
 		if( $return )

includes/functions-html.php

@@ -750,7 +750,11 @@ function yourls_html_menu() {
 
 	// Build menu links
 	if( defined( 'YOURLS_USER' ) ) {
-		$logout_link = yourls_apply_filter( 'logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . yourls_admin_url( 'index.php' ) . '?action=logout" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
+	    // Create a logout link with a nonce associated to fake user 'logout' : the user is not yet defined
+	    // when the logout check is done -- see yourls_is_valid_user()
+	    $logout_url = yourls_nonce_url( 'admin_logout',
+		yourls_add_query_arg(['action' => 'logout'], yourls_admin_url('index.php')), 'nonce', 'logout');
+		$logout_link = yourls_apply_filter('logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . $logout_url . '" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
 	} else {
 		$logout_link = yourls_apply_filter( 'logout_link', '' );
 	}

tests/tests/auth/login_cookie.php

@@ -11,15 +11,20 @@ class Auth_Login_Cookie_Tests extends PHPUnit\Framework\TestCase {
 
     protected $cookie;
     protected $request;
+    protected $backup_yourls_actions;
 
     protected function setUp(): void {
         $this->cookie = $_COOKIE;
         $this->request = $_REQUEST;
+        global $yourls_actions;
+        $this->backup_yourls_actions = $yourls_actions;
     }
 
     protected function tearDown(): void {
         $_COOKIE = $this->cookie;
         $_REQUEST = $this->request;
+        global $yourls_actions;
+        $yourls_actions = $this->backup_yourls_actions;
     }
 
     public static function setUpBeforeClass(): void {
@@ -52,27 +57,31 @@ public function test_cookie_life() {
     }
 
 	/**
-	 * Test login with valid cookie
+	 * Test login with valid cookie - also check that cookie is set
 	 */
 	public function test_login_valid_cookie() {
         global $yourls_user_passwords;
         $random_user = array_rand($yourls_user_passwords);
         $_COOKIE[yourls_cookie_name()] = yourls_cookie_value( $random_user );
         unset($_REQUEST);
 
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
         $this->assertTrue(yourls_check_auth_cookie());
         $this->assertTrue(yourls_is_valid_user());
+        $this->assertSame( 1, yourls_did_action('pre_setcookie') );
     }
 
 	/**
-	 * Test login with invalid cookie
+	 * Test login with invalid cookie - also check that no cookie is set
 	 */
 	public function test_login_invalid_cookie() {
         $_COOKIE[yourls_cookie_name()] = yourls_cookie_value( rand_str() );
         unset($_REQUEST);
 
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
         $this->assertFalse(yourls_check_auth_cookie());
         $this->assertNotTrue(yourls_is_valid_user());
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
     }
 
 }

tests/tests/auth/logout.php

@@ -9,44 +9,60 @@ class Logout_Func_Tests extends PHPUnit\Framework\TestCase {
 
     protected $backup_get;
     protected $backup_request;
+    private static $user;
 
     protected function setUp(): void {
         $this->backup_get     = $_GET;
         $this->backup_request = $_REQUEST;
+        self::$user = false;
     }
 
     protected function tearDown(): void {
         $_GET = $this->backup_get;
         $_REQUEST = $this->backup_request;
     }
 
+    public static function setUpBeforeClass(): void {
+        yourls_add_action( 'pre_setcookie', function ($in) {
+            self::$user = $in[0]; // $in[0] is the user ID passed to yourls_setcookie()
+        } );
+    }
+
+    public static function tearDownAfterClass(): void {
+        yourls_remove_all_actions('pre_setcookie');
+    }
+
     /**
-     * Check logout procedure - phase 1
+     * Check logout procedure - phase 1 - we're logging in
      */
     public function test_logout_user_is_logged_in() {
         $_REQUEST['nonce'] = yourls_create_nonce('admin_login');
         $valid = yourls_is_valid_user();
         $this->assertTrue($valid);
+        $this->assertSame(self::$user, 'yourls');
     }
 
     /**
-     * Check logout procedure - phase 2
+     * Check logout procedure - phase 2 - we're logging out and checking that cookie was reset
      * @depends test_logout_user_is_logged_in
      */
     public function test_logout_user_logs_out() {
         $_GET['action'] = 'logout';
+        $_REQUEST['nonce'] = yourls_create_nonce('admin_logout', 'logout');
         $invalid = yourls_is_valid_user();
         $this->assertNotTrue( $invalid );
+        $this->assertSame(self::$user, null);
     }
 
     /**
-     * Check logout procedure - phase 3
+     * Check logout procedure - phase 3 - check we can log in again
      * @depends test_logout_user_logs_out
      */
     public function test_logout_user_is_logged_in_back() {
         $_REQUEST['nonce'] = yourls_create_nonce('admin_login');
         $valid = yourls_is_valid_user();
         $this->assertTrue( $valid );
+        $this->assertSame(self::$user, 'yourls');
     }
 
 }

tests/tests/auth/misc.php

@@ -11,4 +11,6 @@ public function test_yourls_skip_password_hashing_is_bool() {
         $this->assertIsBool(yourls_skip_password_hashing());
     }
 
+
+
 }

tests/tests/auth/nonces.php

@@ -47,7 +47,6 @@ public function test_create_nonce_field() {
 	public function test_create_nonce_url() {
         $url = yourls_nonce_url( rand_str(), rand_str(), rand_str(), rand_str() );
         $this->assertTrue( is_string($url) );
-        // $this->assertIsString($url);
     }
 
 	/**
@@ -77,4 +76,23 @@ public function test_invalid_nonce() {
         $this->assertTrue(yourls_verify_nonce(rand_str(), rand_str(), rand_str()));
     }
 
+    /**
+     * Check nonces are different for different actions & users
+     */
+    public function test_nonce_different_for_different_actions_and_users() {
+        $action1 = rand_str();
+        $action2 = rand_str();
+        $user1 = rand_str();
+        $user2 = rand_str();
+
+        $nonce_a1 = yourls_create_nonce($action1);
+        $nonce_a2 = yourls_create_nonce($action2);
+        $nonce_a1_u1 = yourls_create_nonce($action1, $user1);
+        $nonce_a1_u2 = yourls_create_nonce($action1, $user2);
+
+        $this->assertNotEquals($nonce_a1, $nonce_a2);
+        $this->assertNotEquals($nonce_a1_u1, $nonce_a1_u2);
+        $this->assertNotEquals($nonce_a1, $nonce_a1_u1);
+    }
+
 }