#477944 by Damien Tournoud: fix and streamline page cache and session handling From: damz --- batch.inc | 8 ++ bootstrap.inc | 107 +++++++++++--------------- common.inc | 44 +++++------ form.inc | 5 + session.inc | 141 +++++++++++++++++++++------------- install.php | 2 book/book.install | 4 - dblog/dblog.admin.inc | 8 -- node/node.admin.inc | 8 -- openid/openid.inc | 8 ++ openid/openid.module | 6 - simpletest/tests/session.test | 113 ++++++++------------------- simpletest/tests/session_test.module | 22 ++--- system/system.admin.inc | 4 - system/system.install | 8 +- user/user.admin.inc | 8 -- user/user.module | 2 update.php | 8 +- 18 files changed, 230 insertions(+), 276 deletions(-) diff --git includes/batch.inc includes/batch.inc index dfd7d82..4767153 100644 --- includes/batch.inc +++ includes/batch.inc @@ -416,6 +416,12 @@ function _batch_finished() { $_batch = $batch; $batch = NULL; + // Clean-up the session. + unset($_SESSION['batches'][$batch['id']]); + if (empty($_SESSION['batches'])) { + unset($_SESSION['batches']); + } + // Redirect if needed. if ($_batch['progressive']) { // Revert the 'destination' that was saved in batch_process(). @@ -443,7 +449,7 @@ function _batch_finished() { // We get here if $form['#redirect'] was FALSE, or if the form is a // multi-step form. We save the final $form_state value to be retrieved // by drupal_get_form(), and redirect to the originating page. - drupal_set_session('batch_form_state', $_batch['form_state']); + $_SESSION['batch_form_state'] = $_batch['form_state']; drupal_goto($_batch['source_page']); } } diff --git includes/bootstrap.inc includes/bootstrap.inc index 6f47c6e..9840e93 100644 --- includes/bootstrap.inc +++ includes/bootstrap.inc @@ -671,7 +671,6 @@ function variable_del($name) { unset($conf[$name]); } - /** * Retrieve the current page from the cache. * @@ -680,36 +679,33 @@ function variable_del($name) { * from a form submission, the contents of a shopping cart, or other user- * specific content that should not be cached and displayed to other users. * - * @param $retrieve - * If TRUE, look up and return the current page in the cache, or start output - * buffering if the conditions for caching are satisfied. If FALSE, only - * return a boolean value indicating whether the current request may be - * cached. * @return - * The cache object, if the page was found in the cache; TRUE if the page was - * not found, but output buffering was started in order to possibly cache the - * current request; FALSE if the page was not found, and the current request - * may not be cached (e.g. because it belongs to an authenticated user). If - * $retrieve is TRUE, only return either TRUE or FALSE. + * The cache object, if the page was found in the cache, NULL otherwise. */ -function page_get_cache($retrieve) { - global $user, $base_root; - static $ob_started = FALSE; +function drupal_page_get_cache() { + global $base_root; - if ($user->uid || ($_SERVER['REQUEST_METHOD'] != 'GET' && $_SERVER['REQUEST_METHOD'] != 'HEAD') || count(drupal_get_messages(NULL, FALSE)) || $_SERVER['SERVER_SOFTWARE'] === 'PHP CLI') { - return FALSE; + if (drupal_page_is_cacheable()) { + return cache_get($base_root . request_uri(), 'cache_page'); } - if ($retrieve) { - $cache = cache_get($base_root . request_uri(), 'cache_page'); - if ($cache) { - return $cache; - } - else { - ob_start(); - $ob_started = TRUE; - } +} + +/** + * Determine the cacheability of the current page. + * + * @param $allow_caching + * Set to FALSE if you want to prevent this page to get cached. + * @return + * TRUE if the current page can be cached, FALSE otherwise. + */ +function drupal_page_is_cacheable($allow_caching = NULL) { + $allow_caching_static = &drupal_static(__FUNCTION__, TRUE); + if (isset($allow_caching)) { + $allow_caching_static = $allow_caching; } - return $ob_started; + + return $allow_caching_static && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD') + && $_SERVER['SERVER_SOFTWARE'] !== 'PHP CLI'; } /** @@ -885,7 +881,7 @@ function drupal_send_headers($default_headers = array(), $only_default = FALSE) * the client think that the anonymous and authenticated pageviews are * identical. * - * @see page_set_cache() + * @see drupal_page_set_cache() */ function drupal_page_header() { $headers_sent = &drupal_static(__FUNCTION__, FALSE); @@ -914,7 +910,7 @@ function drupal_page_header() { * and the conditions match those currently in the cache, a 304 Not Modified * response is sent. */ -function drupal_page_cache_header(stdClass $cache) { +function drupal_serve_page_from_cache(stdClass $cache) { // Negotiate whether to use compression. $page_compression = variable_get('page_compression', TRUE) && extension_loaded('zlib'); $return_compressed = $page_compression && isset($_SERVER['HTTP_ACCEPT_ENCODING']) && strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') !== FALSE; @@ -1169,10 +1165,6 @@ function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NO */ function drupal_set_message($message = NULL, $type = 'status', $repeat = TRUE) { if ($message) { - if (!isset($_SESSION['messages'])) { - drupal_set_session('messages', array()); - } - if (!isset($_SESSION['messages'][$type])) { $_SESSION['messages'][$type] = array(); } @@ -1180,6 +1172,9 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = TRUE) { if ($repeat || !in_array($message, $_SESSION['messages'][$type])) { $_SESSION['messages'][$type][] = $message; } + + // Mark this page has being not cacheable. + drupal_page_is_cacheable(FALSE); } // Messages not set when DB connection fails. @@ -1364,17 +1359,7 @@ function _drupal_bootstrap($phase) { case DRUPAL_BOOTSTRAP_SESSION: require_once DRUPAL_ROOT . '/' . variable_get('session_inc', 'includes/session.inc'); - session_set_save_handler('_sess_open', '_sess_close', '_sess_read', '_sess_write', '_sess_destroy_sid', '_sess_gc'); - // If a session cookie exists, initialize the session. Otherwise the - // session is only started on demand in drupal_session_start(), making - // anonymous users not use a session cookie unless something is stored in - // $_SESSION. This allows HTTP proxies to cache anonymous pageviews. - if (isset($_COOKIE[session_name()])) { - drupal_session_start(); - } - else { - $user = drupal_anonymous_user(); - } + drupal_session_initialize(); break; case DRUPAL_BOOTSTRAP_VARIABLES: @@ -1384,22 +1369,26 @@ function _drupal_bootstrap($phase) { case DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE: $cache_mode = variable_get('cache', CACHE_DISABLED); + // Get the page from the cache. - $cache = $cache_mode == CACHE_DISABLED ? FALSE : page_get_cache(TRUE); + if ($cache_mode != CACHE_DISABLED) { + $cache = drupal_page_get_cache(); + } + else { + $cache = FALSE; + } + // If the skipping of the bootstrap hooks is not enforced, call hook_boot. - if (!is_object($cache) || $cache_mode != CACHE_AGGRESSIVE) { + if (!$cache || $cache_mode != CACHE_AGGRESSIVE) { // Load module handling. require_once DRUPAL_ROOT . '/includes/module.inc'; module_invoke_all('boot'); } + // If there is a cached page, display it. - if (is_object($cache)) { - // Destroy empty anonymous sessions. - if (drupal_session_is_started() && empty($_SESSION)) { - session_destroy(); - } + if ($cache) { header('X-Drupal-Cache: HIT'); - drupal_page_cache_header($cache); + drupal_serve_page_from_cache($cache); // If the skipping of the bootstrap hooks is not enforced, call hook_exit. if ($cache_mode != CACHE_AGGRESSIVE) { module_invoke_all('exit'); @@ -1408,19 +1397,13 @@ function _drupal_bootstrap($phase) { exit; } - // Prepare for non-cached page workflow. - - // If the session has not already been started and output buffering is - // not enabled, the HTTP headers must be sent now, including the session - // cookie. If output buffering is enabled, the session may be started - // at any time using drupal_session_start(). - if ($cache === FALSE) { - drupal_page_header(); - drupal_session_start(); - } - else { + if (!$cache && drupal_page_is_cacheable()) { header('X-Drupal-Cache: MISS'); } + + // Prepare for non-cached page workflow. + ob_start(); + drupal_page_header(); break; case DRUPAL_BOOTSTRAP_LANGUAGE: diff --git includes/common.inc includes/common.inc index de43161..5400939 100644 --- includes/common.inc +++ includes/common.inc @@ -325,11 +325,9 @@ function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response module_invoke_all('exit', $url); } - if (drupal_session_is_started()) { - // Even though session_write_close() is registered as a shutdown function, - // we need all session data written to the database before redirecting. - session_write_close(); - } + // Commit the session, if necessary. We need all session data written to the + // database before redirecting. + drupal_session_commit(); header('Location: ' . $url, TRUE, $http_response_code); @@ -2156,19 +2154,17 @@ function l($text, $path, array $options = array()) { function drupal_page_footer() { global $user; - // Destroy empty anonymous sessions if possible. - if (!headers_sent() && drupal_session_is_started() && empty($_SESSION) && !$user->uid) { - session_destroy(); - } - elseif (!empty($_SESSION) && !drupal_session_is_started()) { - watchdog('session', '$_SESSION is non-empty yet no code has called drupal_session_start().', array(), WATCHDOG_NOTICE); - } + module_invoke_all('exit'); - if (variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED) { - page_set_cache(); - } + // Commit the user session, if needed. + drupal_session_commit(); - module_invoke_all('exit'); + if (variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED && ($cache = drupal_page_set_cache())) { + drupal_serve_page_from_cache($cache); + } + else { + ob_flush(); + } module_implements(MODULE_IMPLEMENTS_WRITE_CACHE); _registry_check_code(REGISTRY_WRITE_LOOKUP_CACHE); @@ -3282,11 +3278,12 @@ function _drupal_bootstrap_full() { * * @see drupal_page_header */ -function page_set_cache() { - global $user, $base_root; +function drupal_page_set_cache() { + global $base_root; - if (page_get_cache(FALSE)) { + if (drupal_page_is_cacheable()) { $cache_page = TRUE; + $cache = (object) array( 'cid' => $base_root . request_uri(), 'data' => ob_get_clean(), @@ -3294,12 +3291,14 @@ function page_set_cache() { 'created' => REQUEST_TIME, 'headers' => array(), ); + // Restore preferred header names based on the lower-case names returned // by drupal_get_header(). $header_names = _drupal_set_preferred_header_name(); foreach (drupal_get_header() as $name_lower => $value) { $cache->headers[$header_names[$name_lower]] = $value; } + if (variable_get('page_compression', TRUE) && function_exists('gzencode')) { // We do not store the data in case the zlib mode is deflate. This should // be rarely happening. @@ -3315,12 +3314,7 @@ function page_set_cache() { if ($cache_page && $cache->data) { cache_set($cache->cid, $cache->data, 'cache_page', $cache->expire, $cache->headers); } - drupal_page_cache_header($cache); - } - else { - // If output buffering was enabled during bootstrap, and the headers were - // not sent in the DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE phase, send them now. - drupal_page_header(); + return $cache; } } diff --git includes/form.inc includes/form.inc index b57888e..5cf3a8a 100644 --- includes/form.inc +++ includes/form.inc @@ -2794,7 +2794,7 @@ function form_clean_id($id = NULL, $flush = FALSE) { * foreach ($results as $result) { * $items[] = t('Loaded node %title.', array('%title' => $result)); * } - * drupal_set_session('my_batch_results', $items); + * $_SESSION['my_batch_results'] = $items; * } * @endcode */ @@ -2952,6 +2952,9 @@ function batch_process($redirect = NULL, $url = NULL) { )) ->execute(); + // Set the batch number in the session to guarantee that it will stay alive. + $_SESSION['batches'][$batch['id']] = TRUE; + drupal_goto($batch['url'], 'op=start&id=' . $batch['id']); } else { diff --git includes/session.inc includes/session.inc index 3958522..58e24ab 100644 --- includes/session.inc +++ includes/session.inc @@ -128,7 +128,7 @@ function _sess_write($key, $value) { // 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() || ($user->uid == 0 && empty($_COOKIE[session_name()]) && empty($value))) { + if (!drupal_save_session() || empty($user) || (empty($user->uid) && empty($_COOKIE[session_name()]) && empty($value))) { return TRUE; } @@ -158,86 +158,117 @@ function _sess_write($key, $value) { } /** - * Propagate $_SESSION and set session cookie if not already set. This function - * should be called before writing to $_SESSION, usually via - * drupal_set_session(). - * - * @param $start - * If FALSE, the session is not actually started. This is only used by - * drupal_session_is_started(). - * @return - * TRUE if session has already been started, or FALSE if it has not. + * Initialize the session handler, starting a session if needed. */ -function drupal_session_start($start = TRUE) { - $started = &drupal_static(__FUNCTION__, FALSE); - if ($start && !$started) { - $started = TRUE; - session_start(); +function drupal_session_initialize() { + global $user; + + session_set_save_handler('_sess_open', '_sess_close', '_sess_read', '_sess_write', '_sess_destroy_sid', '_sess_gc'); + + if (isset($_COOKIE[session_name()])) { + // If a session cookie exists, initialize the session. Otherwise the + // session is only started on demand in drupal_session_commit(), making + // anonymous users not use a session cookie unless something is stored in + // $_SESSION. This allows HTTP proxies to cache anonymous pageviews. + drupal_session_start(); + if (!empty($user->uid) || !empty($_SESSION)) { + drupal_page_is_cacheable(FALSE); + } + } + else { + // Set a session identifier for this request. This is necessary because + // we lazyly start sessions at the end of this request, and some + // processes (like drupal_get_token()) needs to know the future + // session ID in advance. + $user = drupal_anonymous_user(); + session_id(md5(uniqid('', TRUE))); } - return $started; } /** - * Return whether a session has been started and the $_SESSION variable is - * available. + * Forcefully start a session, preserving already set session data. */ -function drupal_session_is_started() { - return drupal_session_start(FALSE); +function drupal_session_start() { + if (!drupal_session_started()) { + // Save current session data before starting it, as PHP will destroy it. + $session_data = isset($_SESSION) ? $_SESSION : NULL; + + session_start(); + drupal_session_started(TRUE); + + // Restore session data. + if (!empty($session_data)) { + $_SESSION += $session_data; + } + } } /** - * Get a session variable. + * Commit the current session, if necessary. * - * @param $name - * The name of the variable to get. If not supplied, all variables are returned. - * @return - * The value of the variable, or FALSE if the variable is not set. + * If an anonymous user already have an empty session, destroy it. */ -function drupal_get_session($name = NULL) { - if (is_null($name)) { - return $_SESSION; - } - elseif (isset($_SESSION[$name])) { - return $_SESSION[$name]; +function drupal_session_commit() { + global $user; + + if (empty($user->uid) && empty($_SESSION)) { + if (drupal_session_started()) { + // Destroy empty anonymous sessions. + session_destroy(); + } } else { - return FALSE; + if (!drupal_session_started()) { + drupal_session_start(); + } + // Write the session data. + session_write_close(); } } /** - * Set a session variable. The variable becomes accessible via $_SESSION[$name] - * in the current and later requests. If there is no active PHP session prior - * to the call, one is started automatically. - * - * Anonymous users generate less server load if their $_SESSION variable is - * empty, so unused entries should be unset using unset($_SESSION['foo']). - * - * @param $name - * The name of the variable to set. - * @param $value - * The value to set. + * Return whether a session has been started. */ -function drupal_set_session($name, $value) { - drupal_session_start(); - $_SESSION[$name] = $value; +function drupal_session_started($set = NULL) { + static $session_started = FALSE; + if (isset($set)) { + $session_started = $set; + } + return $session_started && session_id(); } /** * Called when an anonymous user becomes authenticated or vice-versa. */ function drupal_session_regenerate() { - $old_session_id = session_id(); + global $user; + + // Set the session cookie "httponly" flag to reduce the risk of session + // stealing via XSS. extract(session_get_cookie_params()); - // Set "httponly" to TRUE to reduce the risk of session stealing via XSS. session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE); - session_regenerate_id(); - db_update('sessions') - ->fields(array( - 'sid' => session_id() - )) - ->condition('sid', $old_session_id) - ->execute(); + + if (drupal_session_started()) { + $old_session_id = session_id(); + session_regenerate_id(); + } + 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. + $account = $user; + drupal_session_start(); + $user = $account; + } + + if (isset($old_session_id)) { + db_update('sessions') + ->fields(array( + 'sid' => session_id() + )) + ->condition('sid', $old_session_id) + ->execute(); + } } /** diff --git install.php install.php index 60902f6..5ee2d6d 100644 --- install.php +++ install.php @@ -625,7 +625,7 @@ function install_tasks($profile, $task) { drupal_install_init_database(); drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); - drupal_set_session('messages', $messages); + $_SESSION['messages'] = $messages; // URL used to direct page requests. $url = $base_url . '/install.php?locale=' . $install_locale . '&profile=' . $profile; diff --git modules/book/book.install modules/book/book.install index 4523dd9..913609e 100644 --- modules/book/book.install +++ modules/book/book.install @@ -134,8 +134,8 @@ function book_update_6000() { db_create_table($ret, 'book', $schema['book']); - drupal_set_session('book_update_6000_orphans', array('from' => 0)); - drupal_set_session('book_update_6000', array()); + $_SESSION['book_update_6000_orphans'] = array('from' => 0); + $_SESSION['book_update_6000'] = array(); $result = db_query("SELECT * from {book_temp} WHERE parent = 0"); // Collect all books - top-level nodes. diff --git modules/dblog/dblog.admin.inc modules/dblog/dblog.admin.inc index 3867f3e..8a186f7 100644 --- modules/dblog/dblog.admin.inc +++ modules/dblog/dblog.admin.inc @@ -284,9 +284,6 @@ function _dblog_format_message($dblog) { * @see dblog_filter_form_validate() */ function dblog_filter_form() { - if (!isset($_SESSION['dblog_overview_filter'])) { - drupal_set_session('dblog_overview_filter', array()); - } $session = &$_SESSION['dblog_overview_filter']; $filters = dblog_filters(); @@ -343,15 +340,12 @@ function dblog_filter_form_submit($form, &$form_state) { case t('Filter'): foreach ($filters as $name => $filter) { if (isset($form_state['values'][$name])) { - if (!isset($_SESSION['dblog_overview_filter'])) { - drupal_set_session('dblog_overview_filter', array()); - } $_SESSION['dblog_overview_filter'][$name] = $form_state['values'][$name]; } } break; case t('Reset'): - drupal_set_session('dblog_overview_filter', array()); + $_SESSION['dblog_overview_filter'] = array(); break; } return 'admin/reports/dblog'; diff --git modules/node/node.admin.inc modules/node/node.admin.inc index 55531be..931c7f5 100644 --- modules/node/node.admin.inc +++ modules/node/node.admin.inc @@ -202,9 +202,6 @@ function node_build_filter_query() { * Return form for node administration filters. */ function node_filter_form() { - if (!isset($_SESSION['node_overview_filter'])) { - drupal_set_session('node_overview_filter', array()); - } $session = &$_SESSION['node_overview_filter']; $filters = node_filters(); @@ -320,9 +317,6 @@ function node_filter_form_submit($form, &$form_state) { $flat_options = form_options_flatten($filters[$filter]['options']); if (isset($flat_options[$form_state['values'][$filter]])) { - if (!isset($_SESSION['node_overview_filter'])) { - drupal_set_session('node_overview_filter', array()); - } $_SESSION['node_overview_filter'][] = array($filter, $form_state['values'][$filter]); } } @@ -331,7 +325,7 @@ function node_filter_form_submit($form, &$form_state) { array_pop($_SESSION['node_overview_filter']); break; case t('Reset'): - drupal_set_session('node_overview_filter', array()); + $_SESSION['node_overview_filter'] = array(); break; } } diff --git modules/openid/openid.inc modules/openid/openid.inc index e02c1d5..f248f51 100644 --- modules/openid/openid.inc +++ modules/openid/openid.inc @@ -61,6 +61,10 @@ function openid_redirect_http($url, $message) { $sep = (strpos($url, '?') === FALSE) ? '?' : '&'; header('Location: ' . $url . $sep . implode('&', $query), TRUE, 302); + + // Commit session data before redirecting. + drupal_session_commit(); + exit; } @@ -73,6 +77,10 @@ function openid_redirect($url, $message) { $output .= ''; $output .= "\n"; print $output; + + // Commit session data before redirecting. + drupal_session_commit(); + exit; } diff --git modules/openid/openid.module modules/openid/openid.module index 30f16bc..7fb4b94 100644 --- modules/openid/openid.module +++ modules/openid/openid.module @@ -174,9 +174,6 @@ function openid_begin($claimed_id, $return_to = '', $form_values = array()) { } // Store discovered information in the users' session so we don't have to rediscover. - if (!isset($_SESSION['openid'])) { - drupal_set_session('openid', array()); - } $_SESSION['openid']['service'] = $services[0]; // Store the claimed id $_SESSION['openid']['claimed_id'] = $claimed_id; @@ -434,9 +431,6 @@ function openid_authentication($response) { // We were unable to register a valid new user, redirect to standard // user/register and prefill with the values we received. drupal_set_message(t('OpenID registration failed for the reasons listed. You may register now, or if you already have an account you can log in now and add your OpenID under "My Account"', array('@login' => url('user/login'))), 'error'); - if (!isset($_SESSION['openid'])) { - drupal_set_session('openid', array()); - } $_SESSION['openid']['values'] = $form_state['values']; // We'll want to redirect back to the same place. $destination = drupal_get_destination(); diff --git modules/simpletest/tests/session.test modules/simpletest/tests/session.test index 2b0076f..690ef19 100644 --- modules/simpletest/tests/session.test +++ modules/simpletest/tests/session.test @@ -31,12 +31,15 @@ class SessionTestCase extends DrupalWebTestCase { // Test session hardening code from SA-2008-044. $user = $this->drupalCreateUser(array('access content')); + // Enable sessions. $this->sessionReset($user->uid); + // Make sure the session cookie is set as HttpOnly. $this->drupalLogin($user); $this->assertTrue(preg_match('/HttpOnly/i', $this->drupalGetHeader('Set-Cookie', TRUE)), t('Session cookie is set as HttpOnly.')); $this->drupalLogout(); + // Verify that the session is regenerated if a module calls exit // in hook_user_login(). user_save($user, array('name' => 'session_test_user')); @@ -46,6 +49,7 @@ class SessionTestCase extends DrupalWebTestCase { preg_match('/\s*session_id:(.*)\n/', $this->drupalGetContent(), $matches); $this->assertTrue(!empty($matches[1]) , t('Found session ID before logging in.')); $original_session = $matches[1]; + // We cannot use $this->drupalLogin($user); because we exit in // session_test_user_login() which breaks a normal assertion. $edit = array( @@ -69,12 +73,16 @@ class SessionTestCase extends DrupalWebTestCase { * drupal_session_count() since session data is already generated here. */ function testDataPersistence() { + // At the very start, we have no session. + $expected_anonymous = 0; + $expected_authenticated = 0; + $user = $this->drupalCreateUser(array('access content')); // Enable sessions. $this->sessionReset($user->uid); $this->drupalLogin($user); - $this->session_count_authenticated = $this->session_count++; + $expected_authenticated++; $value_1 = $this->randomName(); $this->drupalGet('session-test/set/' . $value_1); @@ -97,51 +105,56 @@ class SessionTestCase extends DrupalWebTestCase { // Logout the user and make sure the stored value no longer persists. $this->drupalLogout(); + $expected_authenticated--; + $this->sessionReset(); $this->drupalGet('session-test/get'); - // Session count should go up since we're accessing anonymously now. - $this->session_count_anonymous = $this->session_count++; $this->assertNoText($value_1, t("After logout, previous user's session data is not available."), t('Session')); + // Now try to store some data as an anonymous user. $value_3 = $this->randomName(); $this->drupalGet('session-test/set/' . $value_3); $this->assertText($value_3, t('Session data stored for anonymous user.'), t('Session')); $this->drupalGet('session-test/get'); $this->assertText($value_3, t('Session correctly returned the stored data for an anonymous user.'), t('Session')); + // Session count should go up since we have started an anonymous session now. + $expected_anonymous++; + // Try to store data when drupal_save_session(FALSE). $value_4 = $this->randomName(); $this->drupalGet('session-test/no-set/' . $value_4); $this->assertText($value_4, t('The session value was correctly passed to session-test/no-set.'), t('Session')); $this->drupalGet('session-test/get'); $this->assertText($value_3, t('Session data is not saved for drupal_save_session(FALSE).'), t('Session')); - // Logout and get first user back in. Sessions shouldn't persist through - // logout, so the data won't be on the page. + // Login, the data should persist. $this->drupalLogin($user); + $expected_anonymous--; + $expected_authenticated++; $this->sessionReset($user->uid); $this->drupalGet('session-test/get'); $this->assertNoText($value_1, t('Session has persisted for an authenticated user after logging out and then back in.'), t('Session')); - // Logout and create another user. + // Change session and create another user. $user2 = $this->drupalCreateUser(array('access content')); $this->sessionReset($user2->uid); $this->drupalLogin($user2); - $this->session_count_authenticated = $this->session_count++; + $expected_authenticated++; // Perform drupal_session_count tests here in order to use the session data already generated. // Test absolute count. $anonymous = drupal_session_count(0, TRUE); $authenticated = drupal_session_count(0, FALSE); - $this->assertEqual($anonymous + $authenticated, $this->session_count, t('Correctly counted @count total sessions.', array('@count' => $this->session_count)), t('Session')); + $this->assertEqual($anonymous + $authenticated, $expected_anonymous + $expected_authenticated, t('@count total sessions (expected @expected).', array('@count' => $anonymous + $authenticated, '@expected' => $expected_anonymous + $expected_authenticated)), t('Session')); // Test anonymous count. - $this->assertEqual($anonymous, $this->session_count_anonymous, t('Correctly counted @count anonymous sessions.', array('@count' => $anonymous)), t('Session')); + $this->assertEqual($anonymous, $expected_anonymous, t('@count anonymous sessions (expected @expected).', array('@count' => $anonymous, '@expected' => $expected_anonymous)), t('Session')); // Test authenticated count. - $this->assertEqual($authenticated, $this->session_count_authenticated, t('Correctly counted @count authenticated sessions.', array('@count' => $authenticated)), t('Session')); + $this->assertEqual($authenticated, $expected_authenticated, t('@count authenticated sessions (expected @expected).', array('@count' => $authenticated, '@expected' => $expected_authenticated)), t('Session')); // Should return 0 sessions from 1 second from now. - $this->assertEqual(drupal_session_count(time() + 1), 0, t('Correctly returned 0 sessions newer than the current time.'), t('Session')); + $this->assertEqual(drupal_session_count(time() + 1), 0, t('0 sessions newer than the current time.'), t('Session')); } @@ -149,83 +162,39 @@ class SessionTestCase extends DrupalWebTestCase { * Test that empty anonymous sessions are destroyed. */ function testEmptyAnonymousSession() { - // With caching disabled, a session is always started. + // Verify that no session is automatically created for anonymous user. $this->drupalGet(''); $this->assertSessionCookie(FALSE); - $this->assertSessionStarted(TRUE); $this->assertSessionEmpty(TRUE); + // The same behavior is expected when caching is enabled. variable_set('cache', CACHE_NORMAL); - - // During this request the session is destroyed in drupal_page_footer(), - // and the session cookie is unset. $this->drupalGet(''); - $this->assertSessionCookie(TRUE); - $this->assertSessionStarted(TRUE); + $this->assertSessionCookie(FALSE); $this->assertSessionEmpty(TRUE); $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', t('Page was not cached.')); - // When PHP deletes a cookie, it sends "Set-Cookie: cookiename=deleted; - // expires=..." - $this->assertTrue(preg_match('/SESS\w+=deleted/', $this->drupalGetHeader('Set-Cookie')), t('Session cookie was deleted.')); - - // Verify that the session cookie was actually deleted. - $this->drupalGet(''); - $this->assertSessionCookie(FALSE); - $this->assertSessionStarted(FALSE); - $this->assertFalse($this->drupalGetHeader('Set-Cookie'), t('New session was not started.')); // Start a new session by setting a message. $this->drupalGet('session-test/set-message'); - $this->assertSessionCookie(FALSE); - $this->assertSessionStarted(FALSE); + $this->assertSessionCookie(TRUE); $this->assertTrue($this->drupalGetHeader('Set-Cookie'), t('New session was started.')); - // Display the message. + // Display the message, during the same request the session is destroyed + // and the session cookie is unset. $this->drupalGet(''); - $this->assertSessionCookie(TRUE); - $this->assertSessionStarted(TRUE); + $this->assertSessionCookie(FALSE); $this->assertSessionEmpty(FALSE); $this->assertFalse($this->drupalGetHeader('X-Drupal-Cache'), t('Caching was bypassed.')); $this->assertText(t('This is a dummy message.'), t('Message was displayed.')); - - // During this request the session is destroyed in _drupal_bootstrap(), - // and the session cookie is unset. - $this->drupalGet(''); - $this->assertSessionCookie(TRUE); - $this->assertSessionStarted(TRUE); - $this->assertSessionEmpty(TRUE); - $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', t('Page was cached.')); - $this->assertNoText(t('This is a dummy message.'), t('Message was not cached.')); $this->assertTrue(preg_match('/SESS\w+=deleted/', $this->drupalGetHeader('Set-Cookie')), t('Session cookie was deleted.')); // Verify that session was destroyed. $this->drupalGet(''); $this->assertSessionCookie(FALSE); - $this->assertSessionStarted(FALSE); + $this->assertSessionEmpty(TRUE); + $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 modifying $_SESSION without having started a session - // generates a watchdog message, and that no messages have been generated - // so far. - $this->assertEqual($this->getWarningCount(), 0, t('No watchdog messages have been generated')); - $this->drupalGet('/session-test/set-not-started'); - $this->assertSessionCookie(FALSE); - $this->assertSessionStarted(FALSE); - $this->assertEqual($this->getWarningCount(), 1, t('1 watchdog messages has been generated')); - } - - /** - * Count watchdog messages about modifying $_SESSION without having started a - * session. - */ - function getWarningCount() { - return db_select('watchdog') - ->condition('type', 'session') - ->condition('message', '$_SESSION is non-empty yet no code has called drupal_session_start().') - ->countQuery() - ->execute() - ->fetchField(); } /** @@ -250,22 +219,10 @@ class SessionTestCase extends DrupalWebTestCase { */ function assertSessionCookie($sent) { if ($sent) { - $this->assertIdentical($this->drupalGetHeader('X-Session-Cookie'), '1', t('Session cookie was sent.')); - } - else { - $this->assertIdentical($this->drupalGetHeader('X-Session-Cookie'), '0', t('Session cookie was not sent.')); - } - } - - /** - * Assert whether session was started during the bootstrap process. - */ - function assertSessionStarted($started) { - if ($started) { - $this->assertIdentical($this->drupalGetHeader('X-Session-Started'), '1', t('Session was started.')); + $this->assertNotNull($this->session_id, t('Session cookie was sent.')); } else { - $this->assertIdentical($this->drupalGetHeader('X-Session-Started'), '0', t('Session was not started.')); + $this->assertNull($this->session_id, t('Session cookie was not sent.')); } } diff --git modules/simpletest/tests/session_test.module modules/simpletest/tests/session_test.module index c24b721..93edd6d 100644 --- modules/simpletest/tests/session_test.module +++ modules/simpletest/tests/session_test.module @@ -51,22 +51,10 @@ function session_test_menu() { * Implement hook_boot(). */ function session_test_boot() { - header('X-Session-Cookie: ' . intval(isset($_COOKIE[session_name()]))); - header('X-Session-Started: ' . intval(drupal_session_is_started())); header('X-Session-Empty: ' . intval(empty($_SESSION))); } /** - * Implement hook_init(). - */ -function session_test_init() { - // hook_init() is called later in the bootstrap process, but not in cached - // requests. Here the header set in hook_boot() is overwritten, so the - // session state is reported as late in the bootstrap process as possible. - header('X-Session-Started: ' . intval(drupal_session_is_started())); -} - -/** * Page callback, prints the stored session value to the screen. */ function _session_test_get() { @@ -82,7 +70,7 @@ function _session_test_get() { * Page callback, stores a value in $_SESSION['session_test_value']. */ function _session_test_set($value) { - drupal_set_session('session_test_value', $value); + $_SESSION['session_test_value'] = $value; return t('The current value of the stored session variable has been set to %val', array('%val' => $value)); } @@ -100,6 +88,12 @@ function _session_test_no_set($value) { * Menu callback: print the current session ID. */ function _session_test_id() { + // Set a value in $_SESSION, so that drupal_session_commit() will start + // a session. + $_SESSION['test'] = 'test'; + + drupal_session_commit(); + return 'session_id:' . session_id() . "\n"; } @@ -119,7 +113,7 @@ function _session_test_set_message() { * having started the session in advance. */ function _session_test_set_not_started() { - if (!drupal_session_is_started()) { + if (!drupal_session_will_start()) { $_SESSION['session_test_value'] = t('Session was not started'); } } diff --git modules/system/system.admin.inc modules/system/system.admin.inc index abdcfb3..6a3bc4f 100644 --- modules/system/system.admin.inc +++ modules/system/system.admin.inc @@ -1734,7 +1734,7 @@ function system_clean_url_settings() { // When accessing this form using a non-clean URL, allow a re-check to make // sure clean URLs can be disabled at all times. $available = FALSE; - if (strpos(request_uri(), '?q=') === FALSE || drupal_get_session('clean_url')) { + if (strpos(request_uri(), '?q=') === FALSE || !empty($_SESSION['clean_url'])) { $available = TRUE; } else { @@ -1745,7 +1745,7 @@ function system_clean_url_settings() { } if ($available) { - drupal_set_session('clean_url', TRUE); + $_SESSION['clean_url'] = TRUE; $form['clean_url'] = array( '#type' => 'checkbox', diff --git modules/system/system.install modules/system/system.install index deca647..4eed6ac 100644 --- modules/system/system.install +++ modules/system/system.install @@ -1977,10 +1977,10 @@ function system_update_6021() { // Multi-part update if (!isset($_SESSION['system_update_6021'])) { db_add_field($ret, 'menu', 'converted', array('type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0, 'size' => 'tiny')); - drupal_set_session('system_update_6021_max', db_result(db_query('SELECT COUNT(*) FROM {menu}'))); - drupal_set_session('menu_menu_map', array(1 => 'navigation')); + $_SESSION['system_update_6021_max'] = db_result(db_query('SELECT COUNT(*) FROM {menu}')); + $_SESSION['menu_menu_map'] = array(1 => 'navigation'); // 0 => FALSE is for new menus, 1 => FALSE is for the navigation. - drupal_set_session('menu_item_map', array(0 => FALSE, 1 => FALSE)); + $_SESSION['menu_item_map'] = array(0 => FALSE, 1 => FALSE); $table = array( 'fields' => array( 'menu_name' => array('type' => 'varchar', 'length' => 32, 'not null' => TRUE, 'default' => ''), @@ -1991,7 +1991,7 @@ function system_update_6021() { ); db_create_table($ret, 'menu_custom', $table); db_query("INSERT INTO {menu_custom} (menu_name, title, description) VALUES ('%s', '%s', '%s')", $menus['navigation']); - drupal_set_session('system_update_6021', 0); + $_SESSION['system_update_6021'] = 0; } $limit = 50; diff --git modules/user/user.admin.inc modules/user/user.admin.inc index 38c3bfb..095468c 100644 --- modules/user/user.admin.inc +++ modules/user/user.admin.inc @@ -33,9 +33,6 @@ function user_admin($callback_arg = '') { * @see user_filter_form_submit() */ function user_filter_form() { - if (!isset($_SESSION['user_overview_filter'])) { - drupal_set_session('user_overview_filter', array()); - } $session = &$_SESSION['user_overview_filter']; $filters = user_filters(); @@ -103,9 +100,6 @@ function user_filter_form_submit($form, &$form_state) { // Merge an array of arrays into one if necessary. $options = $filter == 'permission' ? call_user_func_array('array_merge', $filters[$filter]['options']) : $filters[$filter]['options']; if (isset($options[$form_state['values'][$filter]])) { - if (!isset($_SESSION['user_overview_filter'])) { - drupal_set_session('user_overview_filter', array()); - } $_SESSION['user_overview_filter'][] = array($filter, $form_state['values'][$filter]); } } @@ -114,7 +108,7 @@ function user_filter_form_submit($form, &$form_state) { array_pop($_SESSION['user_overview_filter']); break; case t('Reset'): - drupal_set_session('user_overview_filter', array()); + $_SESSION['user_overview_filter'] = array(); break; case t('Update'): return; diff --git modules/user/user.module modules/user/user.module index 47321b6..c240457 100644 --- modules/user/user.module +++ modules/user/user.module @@ -1715,10 +1715,12 @@ function user_authenticate_finalize(&$edit) { ->fields(array('login' => $user->login)) ->condition('uid', $user->uid) ->execute(); + // Regenerate the session ID to prevent against session fixation attacks. // This is called before hook_user in case one of those functions fails // or incorrectly does a redirect which would leave the old session in place. drupal_session_regenerate(); + user_module_invoke('login', $edit, $user); } diff --git update.php update.php index 99a8cc2..e579789 100644 --- update.php +++ update.php @@ -292,7 +292,7 @@ function update_batch() { // During the update, bring the site offline so that schema changes do not // affect visiting users. - drupal_set_session('site_offline', variable_get('site_offline', FALSE)); + $_SESSION['site_offline'] = variable_get('site_offline', FALSE); if ($_SESSION['site_offline'] == FALSE) { variable_set('site_offline', TRUE); } @@ -326,9 +326,9 @@ function update_finished($success, $results, $operations) { // clear the caches in case the data has been updated. drupal_flush_all_caches(); - drupal_set_session('update_results', $results); - drupal_set_session('update_success', $success); - drupal_set_session('updates_remaining', $operations); + $_SESSION['update_results'] = $results; + $_SESSION['update_success'] = $success; + $_SESSION['updates_remaining'] = $operations; // Now that the update is done, we can put the site back online if it was // previously turned off.