Further clean-up following #477944. From: damz --- bootstrap.inc | 2 ++ session.inc | 39 ++++++++++++++++++---------------- 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 ++--- 7 files changed, 56 insertions(+), 30 deletions(-) diff --git includes/bootstrap.inc includes/bootstrap.inc index 12ba6f5..2134dc6 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/session.inc includes/session.inc index 2aec07d..386706a 100644 --- includes/session.inc +++ includes/session.inc @@ -124,14 +124,6 @@ function _sess_read($key) { function _sess_write($key, $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)) ->fields(array( @@ -212,12 +204,12 @@ function drupal_session_commit() { global $user; if (empty($user->uid) && empty($_SESSION)) { - if (drupal_session_started()) { + if (drupal_session_started() && drupal_save_session()) { // Destroy empty anonymous sessions. session_destroy(); } } - else { + else if (drupal_save_session()) { if (!drupal_session_started()) { drupal_session_start(); } @@ -243,11 +235,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(); @@ -304,12 +291,28 @@ function drupal_session_count($timestamp = 0, $anonymous = TRUE) { * Session ID. */ function _sess_destroy_sid($sid) { + // 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); + + // If the session ID being destroyed is the one of the current user, + // clean-up his/her session data and cookie. + if ($sid == session_id()) { + global $user; + + // 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()]); + } + } } /** diff --git modules/simpletest/drupal_web_test_case.php modules/simpletest/drupal_web_test_case.php index c6c2910..45e5ae5 100644 --- modules/simpletest/drupal_web_test_case.php +++ modules/simpletest/drupal_web_test_case.php @@ -947,11 +947,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 32cca23..7728548 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 6d2ffac..a8928e0 100644 --- modules/user/user.module +++ modules/user/user.module @@ -2019,10 +2019,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 def9e06..4e4a69c 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(); }