Finally fix up session restore functionality. Move session handling from login.php into init.php and fix the mess in redirect.php. There are some important notes that need to be reviewed in redirect.php, which I am including here to get your attention: FIXME! IMPORTANT! SOMEONE PLEASE EXPLAIN THE SECURITY CONCERN HERE; THIS session_destroy() BORKS ANY SESSION INFORMATION ADDED ON THE LOGIN PAGE (SPECIFICALLY THE SESSION RESTORE DATA, BUT ALSO ANYTHING ADDED BY PLUGINS, ETC)... I HAVE DISABLED THIS (AND NOTE THAT THE LOGIN PAGE ALREADY EXECUTES A session_destroy() (see includes/init.php)), SO PLEASE, WHOEVER ADDED THIS, PLEASE ANALYSE THIS SITUATION AND COMMENT ON IF IT IS OK LIKE THISsvn diff include/init.php src/login.php src/redirect.php src/compose.php WHAT HIJACKING ISSUES ARE WE SUPPOSED TO BE PREVENTING HERE?

include/init.php

@@ -18,6 +18,13 @@
 error_reporting(E_ALL);
 
 
+/**
+ * Make sure we have a page name
+ *
+ */
+if ( !defined('PAGE_NAME') ) define('PAGE_NAME', NULL);
+
+
 /**
  * If register_globals are on, unregister globals.
  * Second test covers boolean set as string (php_value register_globals off).
@@ -199,18 +206,32 @@ if (!isset($session_name) || !$session_name) {
 }
 
 /**
- * if session.auto_start is On then close the session
+ * When on login page or if session.auto_start is On 
+ * we want to destroy/close the session (save off 
+ * possible session restoration values first)
  */
+if (!sqGetGlobalVar('session_expired_post', $sep, SQ_SESSION))
+    $sep = '';
+if (!sqGetGlobalVar('session_expired_location', $sel, SQ_SESSION))
+    $sel = '';
 $sSessionAutostartName = session_name();
 $sCookiePath = null;
-if ((isset($sSessionAutostartName) || $sSessionAutostartName == '') &&
-     $sSessionAutostartName !== $session_name) {
+if (PAGE_NAME == 'login' 
+ || (isset($sSessionAutostartName) && $sSessionAutostartName !== $session_name)) {
     $sCookiePath = ini_get('session.cookie_path');
     $sCookieDomain = ini_get('session.cookie_domain');
     // reset the cookie
     setcookie($sSessionAutostartName,'',time() - 604800,$sCookiePath,$sCookieDomain);
     @session_destroy();
     session_write_close();
+
+    /**
+     * in some rare instances, the session seems to stick
+     * around even after destroying it (!!), so if it does,
+     * we'll manually flatten the $_SESSION data
+     */
+    if (!empty($_SESSION))
+        $_SESSION = array();
 }
 
 /**
@@ -311,7 +332,6 @@ if (! sqgetGlobalVar('squirrelmail_language',$squirrelmail_language,SQ_COOKIE))
  * Do something special for some pages. This is based on the PAGE_NAME constand
  * set at the top of every page.
  */
-if ( !defined('PAGE_NAME') ) define('PAGE_NAME', NULL);
 switch (PAGE_NAME) {
     case 'style':
 
@@ -354,6 +374,13 @@ switch (PAGE_NAME) {
         require(SM_PATH . 'functions/page_header.php');
         require(SM_PATH . 'functions/html.php');
 
+        // put session restore data back into session if necessary
+        if (!empty($sel)) {
+            sqsession_register($sel, 'session_expired_location');
+            if (!empty($sep))
+                sqsession_register($sep, 'session_expired_post');
+        }
+
         // reset template file cache
         //
         $sTemplateID = Template::get_default_template_set();

src/compose.php

@@ -329,11 +329,8 @@ if (sqsession_is_registered('session_expired_post')) {
      * extra check for username so we don't display previous post data from
      * another user during this session.
      */
-    if ($session_expired_post['username'] != $username) {
-        unset($session_expired_post);
-        sqsession_unregister('session_expired_post');
-        session_write_close();
-    } else {
+    if (!empty($session_expired_post['username']) 
+     && $session_expired_post['username'] == $username) {
         // these are the vars that we can set from the expired composed session
         $compo_var_list = array ('send_to', 'send_to_cc', 'body',
             'startMessage', 'passed_body', 'use_signature', 'signature',

src/login.php

@@ -30,34 +30,6 @@ require_once(SM_PATH . 'functions/forms.php');
  */
 set_up_language($squirrelmail_language, TRUE, TRUE);
 
-/**
- * In case the last session was not terminated properly, make sure
- * we get a new one, but make sure we preserve session_expired_*
- */
-$sep = '';
-$sel = '';
-sqGetGlobalVar('session_expired_post', $sep, SQ_SESSION);
-sqGetGlobalVar('session_expired_location', $sel, SQ_SESSION);
-
-/* blow away session */
-sqsession_destroy();
-
-/**
- * in some rare instances, the session seems to stick
- * around even after destroying it (!!), so if it does,
- * we'll manually flatten the $_SESSION data
- */
-if (!empty($_SESSION)) {
-    $_SESSION = array();
-}
-
-/* start session and put session_expired_* variables back in session */
-@sqsession_is_active();
-if (!empty($sep) && !empty($sel)) {
-    sqsession_register($sep, 'session_expired_post');
-    sqsession_register($sel, 'session_expired_location');
-}
-
 /**
  * This detects if the IMAP server has logins disabled, and if so,
  * squelches the display of the login form and puts up a message

src/redirect.php

@@ -71,9 +71,10 @@ $imapConnection = sqimap_login($login_username, $key, $imapServerAddress, $imapP
 /* From now on we are logged it. If the login failed then sqimap_login handles it */
 
 /* regenerate the session id to avoid session hyijacking */
-sqsession_destroy();
-@sqsession_is_active();
-session_regenerate_id();
+//FIXME!  IMPORTANT!  SOMEONE PLEASE EXPLAIN THE SECURITY CONCERN HERE; THIS session_destroy() BORKS ANY SESSION INFORMATION ADDED ON THE LOGIN PAGE (SPECIFICALLY THE SESSION RESTORE DATA, BUT ALSO ANYTHING ADDED BY PLUGINS, ETC)... I HAVE DISABLED THIS (AND NOTE THAT THE LOGIN PAGE ALREADY EXECUTES A session_destroy() (see includes/init.php)), SO PLEASE, WHOEVER ADDED THIS, PLEASE ANALYSE THIS SITUATION AND COMMENT ON IF IT IS OK LIKE THIS!!  WHAT HIJACKING ISSUES ARE WE SUPPOSED TO BE PREVENTING HERE?
+//sqsession_destroy();
+//@sqsession_is_active();
+//session_regenerate_id();
 /**
 * The cookie part. session_start and session_regenerate_session normally set
 * their own cookie. SquirrelMail sets another cookie which overwites the
@@ -148,10 +149,12 @@ if ( sqgetGlobalVar('session_expired_location', $session_expired_location, SQ_SE
         if ($compose_new_win) {
             // do not prefix $location here because $session_expired_location is set to the PAGE_NAME
             // of the last page
-            $redirect_url = $session_expired_location.'.php';
+            $redirect_url = $location . $session_expired_location . '.php';
         } else {
-            $redirect_url = $location.'/webmail.php?right_frame='.urlencode($session_expired_location).'php';
+            $redirect_url = $location . '/webmail.php?right_frame=compose.php';
         }
+    } else {
+        $redirect_url = $location . '/webmail.php?right_frame=' . urlencode($session_expired_location) . '.php';
     }
     unset($session_expired_location);
 }