Further clean-up following #477944. From: damz --- bootstrap.inc | 2 + cache.inc | 4 +- session.inc | 77 +++++++++++++++++++--------------- simpletest/drupal_web_test_case.php | 9 ++-- simpletest/tests/session.test | 12 +++++ simpletest/tests/session_test.module | 14 ++++++ user/user.module | 4 -- user/user.pages.inc | 6 +-- 8 files changed, 79 insertions(+), 49 deletions(-) diff --git includes/bootstrap.inc includes/bootstrap.inc index 47d28c9..e5c95b7 100644 --- includes/bootstrap.inc +++ includes/bootstrap.inc @@ -436,6 +436,8 @@ function drupal_initialize_variables() { ini_set('session.use_trans_sid', '0'); // Don't send HTTP headers using PHP's session handler. ini_set('session.cache_limiter', 'none'); + // Use httponly session cookies. + ini_set('session.cookie_httponly', '1'); } /** diff --git includes/cache.inc includes/cache.inc index 2bbbcd1..049b08b 100644 --- includes/cache.inc +++ includes/cache.inc @@ -351,7 +351,7 @@ class DrupalDatabaseCache implements DrupalCacheInterface { // If enforcing a minimum cache lifetime, validate that the data is // currently valid for this user before we return it by making sure the cache // entry was created before the timestamp in the current session's cache - // timer. The cache variable is loaded into the $user object by _sess_read() + // timer. The cache variable is loaded into the $user object by _drupal_session_read() // in session.inc. If the data is permanent or we're not enforcing a minimum // cache lifetime always return the cached data. if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) { @@ -394,7 +394,7 @@ class DrupalDatabaseCache implements DrupalCacheInterface { if (empty($cid)) { if (variable_get('cache_lifetime', 0)) { // We store the time in the current user's $user->cache variable which - // will be saved into the sessions bin by _sess_write(). We then + // will be saved into the sessions bin by _drupal_session_write(). We then // simulate that the cache was flushed for this user by not returning // cached data that was cached before the timestamp. $user->cache = REQUEST_TIME; diff --git includes/session.inc includes/session.inc index 2aec07d..f7d9f31 100644 --- includes/session.inc +++ includes/session.inc @@ -6,12 +6,12 @@ * User session handling functions. * * The user-level session storage handlers: - * - _sess_open() - * - _sess_close() - * - _sess_read() - * - _sess_write() - * - _sess_destroy_sid() - * - _sess_gc() + * - _drupal_session_open() + * - _drupal_session_close() + * - _drupal_session_read() + * - _drupal_session_write() + * - _drupal_session_destroy() + * - _drupal_session_garbage_collection() * are assigned by session_set_save_handler() in bootstrap.inc and are called * automatically by PHP. These functions should not be called directly. Session * data should instead be accessed via the $_SESSION superglobal. @@ -29,7 +29,7 @@ * @return * This function will always return TRUE. */ -function _sess_open() { +function _drupal_session_open() { return TRUE; } @@ -45,7 +45,7 @@ function _sess_open() { * @return * This function will always return TRUE. */ -function _sess_close() { +function _drupal_session_close() { return TRUE; } @@ -59,13 +59,13 @@ function _sess_close() { * This function should not be called directly. Session data should * instead be accessed via the $_SESSION superglobal. * - * @param $key + * @param $sid * Session ID. * @return * Either an array of the session data, or an empty string, if no data * was found or the user is anonymous. */ -function _sess_read($key) { +function _drupal_session_read($sid) { global $user; // Write and Close handlers are called after destructing objects @@ -83,7 +83,7 @@ function _sess_read($key) { // Otherwise, if the session is still active, we have a record of the // client's session in the database. - $user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid", array(':sid' => $key))->fetchObject(); + $user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid", array(':sid' => $sid))->fetchObject(); // We found the client's session record and they are an authenticated user. if ($user && $user->uid > 0) { @@ -114,26 +114,18 @@ function _sess_read($key) { * This function should not be called directly. Session data should * instead be accessed via the $_SESSION superglobal. * - * @param $key + * @param $sid * Session ID. * @param $value * Serialized array of the session data. * @return * This function will always return TRUE. */ -function _sess_write($key, $value) { +function _drupal_session_write($sid, $value) { global $user; - // If saving of session data is disabled, or if a new empty anonymous session - // has been started, do nothing. This keeps anonymous users, including - // crawlers, out of the session table, unless they actually have something - // stored in $_SESSION. - if (!drupal_save_session() || empty($user) || (empty($user->uid) && empty($_COOKIE[session_name()]) && empty($value))) { - return TRUE; - } - db_merge('sessions') - ->key(array('sid' => $key)) + ->key(array('sid' => $sid)) ->fields(array( 'uid' => $user->uid, 'cache' => isset($user->cache) ? $user->cache : 0, @@ -163,7 +155,7 @@ function _sess_write($key, $value) { function drupal_session_initialize() { global $user; - session_set_save_handler('_sess_open', '_sess_close', '_sess_read', '_sess_write', '_sess_destroy_sid', '_sess_gc'); + session_set_save_handler('_drupal_session_open', '_drupal_session_close', '_drupal_session_read', '_drupal_session_write', '_drupal_session_destroy', '_drupal_session_garbage_collection'); if (isset($_COOKIE[session_name()])) { // If a session cookie exists, initialize the session. Otherwise the @@ -211,13 +203,21 @@ function drupal_session_start() { function drupal_session_commit() { global $user; + if (!drupal_save_session()) { + // We don't have anything to do if we are not allowed to save the session. + return; + } + if (empty($user->uid) && empty($_SESSION)) { + // There is no session data to store, destroy the session if it was + // previously started. if (drupal_session_started()) { - // Destroy empty anonymous sessions. session_destroy(); } } else { + // There is session data to store. Start the session if it is not already + // started. if (!drupal_session_started()) { drupal_session_start(); } @@ -243,11 +243,6 @@ function drupal_session_started($set = NULL) { function drupal_session_regenerate() { global $user; - // Set the session cookie "httponly" flag to reduce the risk of session - // stealing via XSS. - extract(session_get_cookie_params()); - session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE); - if (drupal_session_started()) { $old_session_id = session_id(); session_regenerate_id(); @@ -255,7 +250,7 @@ function drupal_session_regenerate() { else { // Start the session when it doesn't exist yet. // Preserve the logged in user, as it will be reset to anonymous - // by _sess_read. + // by _drupal_session_read. $account = $user; drupal_session_start(); $user = $account; @@ -303,13 +298,25 @@ function drupal_session_count($timestamp = 0, $anonymous = TRUE) { * @param string $sid * Session ID. */ -function _sess_destroy_sid($sid) { +function _drupal_session_destroy($sid) { + global $user; + + // Delete session data. db_delete('sessions') ->condition('sid', $sid) ->execute(); - // Unset cookie. - extract(session_get_cookie_params()); - setcookie(session_name(), '', time() - 3600, $path, $domain, $secure, $httponly); + + // Reset $_SESSION and $user to prevent a new session from being started + // in drupal_session_commit(). + $_SESSION = array(); + $user = drupal_anonymous_user(); + + // Unset the session cookie. + if (isset($_COOKIE[session_name()])) { + $params = session_get_cookie_params(); + setcookie(session_name(), '', REQUEST_TIME - 3600, $params['path'], $params['domain'], $params['secure'], $params['httponly']); + unset($_COOKIE[session_name()]); + } } /** @@ -333,7 +340,7 @@ function drupal_session_destroy_uid($uid) { * The value of session.gc_maxlifetime, passed by PHP. * Sessions not updated for more than $lifetime seconds will be removed. */ -function _sess_gc($lifetime) { +function _drupal_session_garbage_collection($lifetime) { // Be sure to adjust 'php_value session.gc_maxlifetime' to a large enough // value. For example, if you want user sessions to stay in your database // for three weeks before deleting them, you need to set gc_maxlifetime diff --git modules/simpletest/drupal_web_test_case.php modules/simpletest/drupal_web_test_case.php index e19ab9d..3d53d8a 100644 --- modules/simpletest/drupal_web_test_case.php +++ modules/simpletest/drupal_web_test_case.php @@ -948,11 +948,10 @@ class DrupalWebTestCase extends DrupalTestCase { * Logs a user out of the internal browser, then check the login page to confirm logout. */ protected function drupalLogout() { - // Make a request to the logout page. - $this->drupalGet('user/logout'); - - // Load the user page, the idea being if you were properly logged out you should be seeing a login screen. - $this->drupalGet('user'); + // 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' => 'destination=user')); $pass = $this->assertField('name', t('Username field found.'), t('Logout')); $pass = $pass && $this->assertField('pass', t('Password field found.'), t('Logout')); diff --git modules/simpletest/tests/session.test modules/simpletest/tests/session.test index 6d85e30..2ef63f3 100644 --- modules/simpletest/tests/session.test +++ modules/simpletest/tests/session.test @@ -195,6 +195,17 @@ class SessionTestCase extends DrupalWebTestCase { $this->assertNoText(t('This is a dummy message.'), t('Message was not cached.')); $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', t('Page was cached.')); $this->assertFalse($this->drupalGetHeader('Set-Cookie'), t('New session was not started.')); + + // Verify that no session is created if drupal_save_session(FALSE) is called. + $this->drupalGet('session-test/set-message-but-dont-save'); + $this->assertSessionCookie(FALSE); + $this->assertSessionEmpty(TRUE); + + // Verify that no message is displayed. + $this->drupalGet(''); + $this->assertSessionCookie(FALSE); + $this->assertSessionEmpty(TRUE); + $this->assertNoText(t('This is a dummy message.'), t('The message was not saved.')); } /** @@ -205,6 +216,7 @@ class SessionTestCase extends DrupalWebTestCase { function sessionReset($uid = 0) { // Close the internal browser. $this->curlClose(); + $this->loggedInUser = FALSE; // Change cookie file for user. $this->cookieFile = file_directory_temp() . '/cookie.' . $uid . '.txt'; diff --git modules/simpletest/tests/session_test.module modules/simpletest/tests/session_test.module index 9855f79..ab8b52d 100644 --- modules/simpletest/tests/session_test.module +++ modules/simpletest/tests/session_test.module @@ -37,6 +37,12 @@ function session_test_menu() { 'access arguments' => array('access content'), 'type' => MENU_CALLBACK, ); + $items['session-test/set-message-but-dont-save'] = array( + 'title' => t('Session value'), + 'page callback' => '_session_test_set_message_but_dont_save', + 'access arguments' => array('access content'), + 'type' => MENU_CALLBACK, + ); $items['session-test/set-not-started'] = array( 'title' => t('Session value'), 'page callback' => '_session_test_set_not_started', @@ -109,6 +115,14 @@ function _session_test_set_message() { } /** + * Menu callback, sets a message but call drupal_save_session(FALSE). + */ +function _session_test_set_message_but_dont_save() { + drupal_save_session(FALSE); + _session_test_set_message(); +} + +/** * Menu callback, stores a value in $_SESSION['session_test_value'] without * having started the session in advance. */ diff --git modules/user/user.module modules/user/user.module index 0584752..7441e5a 100644 --- modules/user/user.module +++ modules/user/user.module @@ -1970,10 +1970,8 @@ function _user_cancel($edit, $account, $method) { // After cancelling account, ensure that user is logged out. if ($account->uid == $user->uid) { - // Destroy the current session. + // Destroy the current session, and reset $user to the anonymous user. session_destroy(); - // Load the anonymous user. - $user = drupal_anonymous_user(); } else { drupal_session_destroy_uid($account->uid); diff --git modules/user/user.pages.inc modules/user/user.pages.inc index dbad8ab..3e39adc 100644 --- modules/user/user.pages.inc +++ modules/user/user.pages.inc @@ -136,12 +136,10 @@ function user_logout() { watchdog('user', 'Session closed for %name.', array('%name' => $user->name)); - // Destroy the current session: - session_destroy(); module_invoke_all('user_logout', NULL, $user); - // Load the anonymous user - $user = drupal_anonymous_user(); + // Destroy the current session, and reset $user to the anonymous user. + session_destroy(); drupal_goto(); }