#144538 by Damien Tournoud: harden the logout link. From: Damien Tournoud --- menu.inc | 17 +++++++++++++++++ path.inc | 6 +++--- menu/menu.test | 2 +- simpletest/drupal_web_test_case.php | 13 +++++++++---- simpletest/tests/session.test | 23 ++++++++++++++++++++++- system/system.install | 4 ++-- toolbar/toolbar.module | 2 +- user/user.module | 2 +- user/user.test | 33 +++++++++++++++++++++++++++++++++ 9 files changed, 89 insertions(+), 13 deletions(-) diff --git includes/menu.inc includes/menu.inc index 3c355ce..4799938 100644 --- includes/menu.inc +++ includes/menu.inc @@ -757,11 +757,28 @@ function _menu_link_map_translate(&$map, $to_arg_functions) { } } +/** + * Menu to arg function: convert %menu_tail by the tail used in the current page. + */ function menu_tail_to_arg($arg, $map, $index) { return implode('/', array_slice($map, $index)); } /** + * Loader function: check that a token is valid. + */ +function drupal_token_load($token) { + return drupal_valid_token($token) ? $token : FALSE; +} + +/** + * Menu to arg function: convert %token into a user specific token. + */ +function drupal_token_to_arg() { + return drupal_get_token(); +} + +/** * This function is similar to _menu_translate() but does link-specific * preparation such as always calling to_arg functions * diff --git includes/path.inc includes/path.inc index 68a92a7..e2310c9 100644 --- includes/path.inc +++ includes/path.inc @@ -589,9 +589,9 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) { elseif ($dynamic_allowed && preg_match('/\/\%/', $path)) { // Path is dynamic (ie 'user/%'), so check directly against menu_router table. if ($item = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $path))->fetchAssoc()) { - $item['link_path'] = $form_item['link_path']; - $item['link_title'] = $form_item['link_title']; - $item['external'] = FALSE; + $item['link_path'] = $path; + $item['link_title'] = ''; + $item['external'] = FALSE; $item['options'] = ''; _menu_link_translate($item); } diff --git modules/menu/menu.test modules/menu/menu.test index 3c99e26..44e6fab 100644 --- modules/menu/menu.test +++ modules/menu/menu.test @@ -478,7 +478,7 @@ class MenuTestCase extends DrupalWebTestCase { */ private function getStandardMenuLink() { // Retrieve menu link id of the Log out menu link, which will always be on the front page. - $mlid = db_query("SELECT mlid FROM {menu_links} WHERE module = 'system' AND router_path = 'user/logout'")->fetchField(); + $mlid = db_query("SELECT mlid FROM {menu_links} WHERE module = 'system' AND router_path = 'user/logout/%'")->fetchField(); $this->assertTrue($mlid > 0, 'Standard menu link id was found'); // Load menu link. // Use api function so that link is translated for rendering. diff --git modules/simpletest/drupal_web_test_case.php modules/simpletest/drupal_web_test_case.php index e70b84b..68c946c 100644 --- modules/simpletest/drupal_web_test_case.php +++ modules/simpletest/drupal_web_test_case.php @@ -1070,7 +1070,7 @@ class DrupalWebTestCase extends DrupalTestCase { */ protected function drupalGetToken($value = '') { $private_key = drupal_get_private_key(); - return md5($this->session_id . $value . $private_key); + return md5(drupal_get_hash_salt() . md5($this->session_id . $value . $private_key)); } /* @@ -1080,7 +1080,7 @@ class DrupalWebTestCase extends DrupalTestCase { // 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', array('query' => array('destination' => 'user'))); + $this->drupalGet('user/logout/' . $this->drupalGetToken(), array('query' => array('destination' => 'user'))); $pass = $this->assertField('name', t('Username field found.'), t('Logout')); $pass = $pass && $this->assertField('pass', t('Password field found.'), t('Logout')); @@ -1388,8 +1388,7 @@ class DrupalWebTestCase extends DrupalTestCase { curl_setopt_array($this->curlHandle, $this->additionalCurlOptions + $curl_options); if (!$redirect) { - // Reset headers, the session ID and the redirect counter. - $this->session_id = NULL; + // Reset headers and the redirect counter. $this->headers = array(); $this->redirect_count = 0; } @@ -1472,6 +1471,12 @@ class DrupalWebTestCase extends DrupalTestCase { if (isset($this->curlHandle)) { curl_close($this->curlHandle); unset($this->curlHandle); + + // Reset the state of the internal browser. + $this->loggedInUser = FALSE; + $this->cookies = array(); + $this->session_id = NULL; + $this->cookieFile = NULL; } } diff --git modules/simpletest/tests/session.test modules/simpletest/tests/session.test index 74ffab9..d2cfc4c 100644 --- modules/simpletest/tests/session.test +++ modules/simpletest/tests/session.test @@ -186,14 +186,35 @@ class SessionTestCase extends DrupalWebTestCase { * @param $uid User id to set as the active session. */ function sessionReset($uid = 0) { + // Save the current state of the browser. + if (isset($this->cookieFile)) { + $state = array( + 'loggedInUser' => $this->loggedInUser, + 'cookies' => $this->cookies, + 'session_id' => $this->session_id, + ); + file_put_contents($this->cookieFile . '.state', serialize($state)); + } + // Close the internal browser. $this->curlClose(); - $this->loggedInUser = FALSE; // Change cookie file for user. $this->cookieFile = file_directory_path('temporary') . '/cookie.' . $uid . '.txt'; $this->additionalCurlOptions[CURLOPT_COOKIEFILE] = $this->cookieFile; $this->additionalCurlOptions[CURLOPT_COOKIESESSION] = TRUE; + + // Reopen the internal browser. + $this->curlInitialize(); + + // Load the old state, if it exists. + if (file_exists($this->cookieFile . '.state')) { + $state = unserialize(file_get_contents($this->cookieFile . '.state')); + foreach ($state as $k => $v) { + $this->$k = $v; + } + } + $this->drupalGet('session-test/get'); $this->assertResponse(200, t('Session test module is correctly enabled.'), t('Session')); } diff --git modules/system/system.install modules/system/system.install index d26064c..f0fb2dc 100644 --- modules/system/system.install +++ modules/system/system.install @@ -1929,12 +1929,12 @@ function system_update_7014() { */ function system_update_7015() { db_update('menu_links') - ->fields(array('link_path' => 'user/logout')) + ->fields(array('link_path' => 'user/logout/%')) ->condition('link_path', 'logout') ->execute(); db_update('menu_links') - ->fields(array('router_path' => 'user/logout')) + ->fields(array('router_path' => 'user/logout/%')) ->condition('router_path', 'logout') ->execute(); } diff --git modules/toolbar/toolbar.module modules/toolbar/toolbar.module index 90c06f1..3267d03 100644 --- modules/toolbar/toolbar.module +++ modules/toolbar/toolbar.module @@ -208,7 +208,7 @@ function toolbar_view() { ), 'logout' => array( 'title' => t('Log out'), - 'href' => 'user/logout', + 'href' => 'user/logout/' . drupal_get_token(), ), ); } diff --git modules/user/user.module modules/user/user.module index 74db63c..fb7986a 100644 --- modules/user/user.module +++ modules/user/user.module @@ -1471,7 +1471,7 @@ function user_menu() { 'file' => 'user.pages.inc', ); - $items['user/logout'] = array( + $items['user/logout/%drupal_token'] = array( 'title' => 'Log out', 'access callback' => 'user_is_logged_in', 'page callback' => 'user_logout', diff --git modules/user/user.test modules/user/user.test index 64af9aa..9a8fd54 100644 --- modules/user/user.test +++ modules/user/user.test @@ -929,6 +929,39 @@ class UserPictureTestCase extends DrupalWebTestCase { } } +/** + * Test basic user login and logout. Verify that the logout link is protected. + */ +class UserLogoutTestCase extends DrupalWebTestCase { + public static function getInfo() { + return array( + 'name' => t('User login / Logout'), + 'description' => t('Test user login and logout.'), + 'group' => t('User') + ); + } + + function setUp() { + parent::setUp(); + + $this->user = $this->drupalCreateUser(); + } + + /** + * Change user permissions and check user_access(). + */ + function testUserLoginLogout() { + // No assertion needed here: assertions are already verified in drupalLogin(). + $this->drupalLogin($this->user); + + $this->drupalGet('user/logout/invalidtoken'); + $this->assertResponse(404, t('Visiting user/logout/[invalid token] results in 404 page.')); + + // No assertion needed here: assertions are already verified in drupalLogin(). + $this->drupalLogout(); + } + +} class UserPermissionsTestCase extends DrupalWebTestCase { protected $admin_user;