- Patch #477944 by DamZ: more streamlining and clean-up of session handling code.

8 files changed, 82 insertions, 47 deletions

diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc
index 86d80dbdd..e4b17ea74 100644
--- a/includes/bootstrap.inc
+++ b/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 a/includes/cache.inc b/includes/cache.inc
index b8fb7dd90..1af01d832 100644
--- a/includes/cache.inc
+++ b/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 a/includes/session.inc b/includes/session.inc
index 90b0b6ebc..272ddc88d 100644
--- a/includes/session.inc
+++ b/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,23 @@ 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;
+  if (!drupal_save_session()) {
+    // We don't have anything to do if we are not allowed to save the session.
+    return;
   }
 
   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 +160,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 +208,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 +248,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 +255,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 +303,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 +345,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 a/modules/simpletest/drupal_web_test_case.php b/modules/simpletest/drupal_web_test_case.php
index 860a18c9f..40b3fbd27 100644
--- a/modules/simpletest/drupal_web_test_case.php
+++ b/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 a/modules/simpletest/tests/session.test b/modules/simpletest/tests/session.test
index d53c14c20..fb82f179d 100644
--- a/modules/simpletest/tests/session.test
+++ b/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 a/modules/simpletest/tests/session_test.module b/modules/simpletest/tests/session_test.module
index 55613c97d..48a655001 100644
--- a/modules/simpletest/tests/session_test.module
+++ b/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 a/modules/user/user.module b/modules/user/user.module
index 6d884b5b9..29fb8c492 100644
--- a/modules/user/user.module
+++ b/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 a/modules/user/user.pages.inc b/modules/user/user.pages.inc
index e9778e32e..bfeea5778 100644
--- a/modules/user/user.pages.inc
+++ b/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();
 }