Changes introduced in SquirrelMail 1.4.10-svn src/webmail.php close session before webmail_top hook is executed. It breaks plugins that modify session in order to redirect right frame to specific option page (see askuserinfo plugin).
But it is not an excuse for breaking things with incorrect order of includes. "require_once(SM_PATH . 'include/load_prefs.php');" loads validate.php and validate.php closes session. Session information can't be modified in webmail_top and webmail_bottom hooks.
Instead of using standard 1.4.x order of includes in authenticated state, you've use custom order with some system includes buried deep inside the code.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The askuserinfo plugin accesses $_SESSION directly itself, which is not condoned by the SM plugin specs. This plugin would not work anyway before the last change to webmail.php because prior to that change, prefs were not available in the webmail_top hook. The problem is solved if the plugin is updated to use correct SM library calls to store session information (sqsession_register()), which will start the session if it is not started already (yes, this could be considered the wrong way to look at a solution, but as we are in a code freeze right now...).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I did not test askuserinfo. The change was made when testing another plugin and discovering the pref values were not available without the change. What I tested was a test plugin that uses the correct API calls and it worked as expected. Here is the code, which checks that both prefs and session registration are working:
Did you debug why this happens? sqsession_register() clearly calls sqsession_is_active(). I don't have time to change PHP version and test now. Perhaps the initial proposed change on the squirrelmail-devel list (putting include of validate.php like all other source files) is better. It was not changed in that way so as to have a smaller code impact, but the include hierarchy in load_prefs (yuck) apparently makes this less than ideal. Thijs I think was the one who had some influence or explanation as to why validate.php is not used in webmail.php.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Session ID is reported even when session is closed. sqsession_register() calls sqsession_is_active(). sqsession_is_active() does not restart the session because session id is set.
session.auto_start Off
session.bug_compat_42 On
session.bug_compat_warn On
session.cache_expire 180
session.cache_limiter nocache
session.cookie_domain no value
session.cookie_httponly Off
session.cookie_lifetime 0
session.cookie_path /
session.cookie_secure Off
session.entropy_file no value
session.entropy_length 0
session.gc_divisor 100
session.gc_maxlifetime 1440
session.gc_probability 0
session.hash_bits_per_character 4
session.hash_function 0
session.name PHPSESSID
session.referer_check no value
session.save_handler files
session.save_path /var/lib/php5
session.serialize_handler php
session.use_cookies On
session.use_only_cookies Off
session.use_trans_sid 0
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1) why isn't validate.php included at top of webmail.php, and would doing so break anything (if this is implemented, can/should remove the load_prefs include right above the webmail_top hook)
2) sqsession_is_active is thusly broken in both STABLE and DEVEL; according to php.net, "as of PHP 4.3.3, calling session_start() while the session has already been started will result in an error of level E_NOTICE. Also, the second session start will simply be ignored," so it should be OK AFAICT to rewrite sqsession_is_active without the session ID check and instead just this:
function sqsession_is_active() {
@sqsession_start();
}
But maybe there are other issues, as PHP sessions can be rather fussy.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Weird. The opening and closing of the session occurs fairly close together, during which point, I don't believe any other code is really executed that should alter any of the content that should cause a plugin to fail.
As for the comments about session_id still returning an ID after the session has been destroyed, that certainly sounds like a PHP bug, or an undocumented change of behavior. The sample code dumping session_id shows an empty string after I have destroyed the session (PHP 4.4.4).
Paul, in regards to the E_NOTICE, it should never happen. session_id will return an id if the session is active, or apparently in some PHP versions cases, even inactive, so should never show an error saying the session has already been started.
If validate is moved to the top of webmail.php, the call to load_prefs wouldn't be needed.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Jon, I don't know what you mean in your first paragraph - the reason plugins break is clear - the session is closed when the hook happens. There are two solutions to that: (1) put validate.php at top of the page, where it should have been to start with (2) make sure plugins use correct SM API for storing session values, but this is where we apparently run into the session ID bug/issue.
You also miss the point of why sqsession_is_active should be changed. It's not due to any E_NOTICE issues (that issue comes up in the proposed change and is addressed by adding the @ character to the proposed change), instead the issue is that when you call something like sqsession_register() when the session has been destroyed and expect the session to restart, it will not do so in some versions of PHP as Tomas has found in this tracker, because the session ID still exists -- the sqsession_is_active() code is plain wrong. The proposed change, according to the PHP manual should do no harm, at least if we supress notice generation when calling it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
You can ignore my first statement. The point of it was that the session was opened in webmail.php, validate.php opened and closed it, so there should be no harm. That was before considering the whole point of this bug report, and your original fix to webmail.php, and that was there is no preferences loaded for the webmail_top hook.
As for the second point, I didn't miss any points. I know the simple fix would be to just change the call in _is_active. The point I was making is that it is a change in application behavior on PHPs part, and undocumented change at that. With that in mind, it should probably be considered a PHP bug, or at least queried with them to find out if it was intentional.
The only issue I see with loading validate.php at the top of webmail.php is the additional includes from mime.php, and the i18n.php libraries. This will probably tack on a few ms for load time, but would probably not be substantial. Depends on the system load I guess.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> You can ignore my first statement. The point of it was that the session
> was opened in webmail.php, validate.php opened and closed it, so there
> should be no harm.
But there is no call to validate.php in webmail.php. But anyway, I think we get to the point below:
> That was before considering the whole point of this bug
> report, and your original fix to webmail.php, and that was there is
> no preferences loaded for the webmail_top hook.
>
> As for the second point, I didn't miss any points.
Yes, re-reading your text, I see what you are saying is that we should just address the first of the two issues I have enumerated. However, I think the 2nd one should be addressed as well, since it is flawed code.
> I know the simple fix
> would be to just change the call in _is_active. The point I was making is
> that it is a change in application behavior on PHPs part, and undocumented
> change at that. With that in mind, it should probably be considered a PHP
> bug, or at least queried with them to find out if it was intentional.
OK, that's fine; maybe this should be entered as a new tracker item. I don't feel like I have the time to follow up with php.net at this time; I would hope that someone else would. OTOH, the proposed fix should solve any issues with PHP environments like Tomas tested with and not create problems with any others that do not display such behavior. Do you actually have any specific reason why that code is not as good (better since it helps address this issue) than what is there now?
> The only issue I see with loading validate.php at the top of webmail.php
> is the additional includes from mime.php, and the i18n.php libraries. This
> will probably tack on a few ms for load time, but would probably not be
> substantial. Depends on the system load I guess.
From my perspective, I didn't originally fix it that way because no one could tell me why validate.php wasn't there and what might break if I put it there. I was trying to be as non-intrusive (given that this is STABLE) as possible. At this point, however, seems like this is probably the better fix.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
including validate.php adds only MIME classes (class/mime.class.php, 1 defined call, 8 require_once, 80KB total), include/load_prefs.php and functions/page_header.php. src/webmail.php also loads functions/imap.php. Rest of validate.php functions are already loaded with other 1.4.9a src/webmail.php includes.
main difference between current 1.4.10-svn and include/validate.php on top of webmail.php - order of code execution. validate.php closes session.auto_start=on session before own session is started. current code closes active SquirrelMail session.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Patch ends up looking like this removing unneeded includes. validate.php seems to include all but the imap.php includes, so we strip everything else out as unneeded. As I don't have a vast array of plugins to validate against, can somebody give the below mods a shot? Also stripped out include for display_messages based on other submitted ticket.
Logged In: YES
user_id=476981
Originator: NO
Just wondering, without looking at the plugin code, isn't it possible to modify askuserinfo and not make use of the hooks in webmail.php?
Logged In: YES
user_id=225877
Originator: YES
Plugin can do that. See naguser plug.
But it is not an excuse for breaking things with incorrect order of includes. "require_once(SM_PATH . 'include/load_prefs.php');" loads validate.php and validate.php closes session. Session information can't be modified in webmail_top and webmail_bottom hooks.
Instead of using standard 1.4.x order of includes in authenticated state, you've use custom order with some system includes buried deep inside the code.
Logged In: YES
user_id=508228
Originator: NO
The askuserinfo plugin accesses $_SESSION directly itself, which is not condoned by the SM plugin specs. This plugin would not work anyway before the last change to webmail.php because prior to that change, prefs were not available in the webmail_top hook. The problem is solved if the plugin is updated to use correct SM library calls to store session information (sqsession_register()), which will start the session if it is not started already (yes, this could be considered the wrong way to look at a solution, but as we are in a code freeze right now...).
Logged In: YES
user_id=225877
Originator: YES
Paul,
Have you actually tried using sqsession_register in askuserinfo plugin?
Have you actually tried using plugin when your code modification was disabled?
Logged In: YES
user_id=508228
Originator: NO
I did not test askuserinfo. The change was made when testing another plugin and discovering the pref values were not available without the change. What I tested was a test plugin that uses the correct API calls and it worked as expected. Here is the code, which checks that both prefs and session registration are working:
<?php
function squirrelmail_plugin_init_test() {
global $squirrelmail_plugin_hooks;
$squirrelmail_plugin_hooks['webmail_top']['test']
= 'test_webmail_top';
$squirrelmail_plugin_hooks['right_main_after_header']['test']
= 'test_right_main_after_header';
}
function test_webmail_top($args)
{
global $username, $data_dir;
$x = getPref($data_dir, $username, 'left_size', 0);
if ($x) sqsession_register($x, 'testingx');
}
function test_right_main_after_header()
{
sqGetGlobalVar('testingx', $y, SQ_SESSION);
echo "session test value = $y<HR>";
}
Logged In: YES
user_id=225877
Originator: YES
Your test plugin outputs 'session test value =' in PHP 5.2.0 and 4.3.11 even when left_size is set to custom value in user preferences.
Logged In: YES
user_id=508228
Originator: NO
Did you debug why this happens? sqsession_register() clearly calls sqsession_is_active(). I don't have time to change PHP version and test now. Perhaps the initial proposed change on the squirrelmail-devel list (putting include of validate.php like all other source files) is better. It was not changed in that way so as to have a smaller code impact, but the include hierarchy in load_prefs (yuck) apparently makes this less than ideal. Thijs I think was the one who had some influence or explanation as to why validate.php is not used in webmail.php.
Logged In: YES
user_id=225877
Originator: YES
<?php
session_start();
var_dump(session_id());
session_write_close();
var_dump(session_id());
?>
Session ID is reported even when session is closed. sqsession_register() calls sqsession_is_active(). sqsession_is_active() does not restart the session because session id is set.
session.auto_start Off
session.bug_compat_42 On
session.bug_compat_warn On
session.cache_expire 180
session.cache_limiter nocache
session.cookie_domain no value
session.cookie_httponly Off
session.cookie_lifetime 0
session.cookie_path /
session.cookie_secure Off
session.entropy_file no value
session.entropy_length 0
session.gc_divisor 100
session.gc_maxlifetime 1440
session.gc_probability 0
session.hash_bits_per_character 4
session.hash_function 0
session.name PHPSESSID
session.referer_check no value
session.save_handler files
session.save_path /var/lib/php5
session.serialize_handler php
session.use_cookies On
session.use_only_cookies Off
session.use_trans_sid 0
Logged In: YES
user_id=508228
Originator: NO
So two issues:
1) why isn't validate.php included at top of webmail.php, and would doing so break anything (if this is implemented, can/should remove the load_prefs include right above the webmail_top hook)
2) sqsession_is_active is thusly broken in both STABLE and DEVEL; according to php.net, "as of PHP 4.3.3, calling session_start() while the session has already been started will result in an error of level E_NOTICE. Also, the second session start will simply be ignored," so it should be OK AFAICT to rewrite sqsession_is_active without the session ID check and instead just this:
function sqsession_is_active() {
@sqsession_start();
}
But maybe there are other issues, as PHP sessions can be rather fussy.
Logged In: YES
user_id=508228
Originator: NO
... and askuserinfo plugin should still be correctly coded to use the SM API. Thijs, you should know better. ;-)
Logged In: YES
user_id=620333
Originator: NO
Weird. The opening and closing of the session occurs fairly close together, during which point, I don't believe any other code is really executed that should alter any of the content that should cause a plugin to fail.
As for the comments about session_id still returning an ID after the session has been destroyed, that certainly sounds like a PHP bug, or an undocumented change of behavior. The sample code dumping session_id shows an empty string after I have destroyed the session (PHP 4.4.4).
Paul, in regards to the E_NOTICE, it should never happen. session_id will return an id if the session is active, or apparently in some PHP versions cases, even inactive, so should never show an error saying the session has already been started.
If validate is moved to the top of webmail.php, the call to load_prefs wouldn't be needed.
Logged In: YES
user_id=225877
Originator: YES
session is not destroyed. It is read only.
Logged In: YES
user_id=508228
Originator: NO
Jon, I don't know what you mean in your first paragraph - the reason plugins break is clear - the session is closed when the hook happens. There are two solutions to that: (1) put validate.php at top of the page, where it should have been to start with (2) make sure plugins use correct SM API for storing session values, but this is where we apparently run into the session ID bug/issue.
You also miss the point of why sqsession_is_active should be changed. It's not due to any E_NOTICE issues (that issue comes up in the proposed change and is addressed by adding the @ character to the proposed change), instead the issue is that when you call something like sqsession_register() when the session has been destroyed and expect the session to restart, it will not do so in some versions of PHP as Tomas has found in this tracker, because the session ID still exists -- the sqsession_is_active() code is plain wrong. The proposed change, according to the PHP manual should do no harm, at least if we supress notice generation when calling it.
Logged In: YES
user_id=620333
Originator: NO
You can ignore my first statement. The point of it was that the session was opened in webmail.php, validate.php opened and closed it, so there should be no harm. That was before considering the whole point of this bug report, and your original fix to webmail.php, and that was there is no preferences loaded for the webmail_top hook.
As for the second point, I didn't miss any points. I know the simple fix would be to just change the call in _is_active. The point I was making is that it is a change in application behavior on PHPs part, and undocumented change at that. With that in mind, it should probably be considered a PHP bug, or at least queried with them to find out if it was intentional.
The only issue I see with loading validate.php at the top of webmail.php is the additional includes from mime.php, and the i18n.php libraries. This will probably tack on a few ms for load time, but would probably not be substantial. Depends on the system load I guess.
Logged In: YES
user_id=508228
Originator: NO
> You can ignore my first statement. The point of it was that the session
> was opened in webmail.php, validate.php opened and closed it, so there
> should be no harm.
But there is no call to validate.php in webmail.php. But anyway, I think we get to the point below:
> That was before considering the whole point of this bug
> report, and your original fix to webmail.php, and that was there is
> no preferences loaded for the webmail_top hook.
>
> As for the second point, I didn't miss any points.
Yes, re-reading your text, I see what you are saying is that we should just address the first of the two issues I have enumerated. However, I think the 2nd one should be addressed as well, since it is flawed code.
> I know the simple fix
> would be to just change the call in _is_active. The point I was making is
> that it is a change in application behavior on PHPs part, and undocumented
> change at that. With that in mind, it should probably be considered a PHP
> bug, or at least queried with them to find out if it was intentional.
OK, that's fine; maybe this should be entered as a new tracker item. I don't feel like I have the time to follow up with php.net at this time; I would hope that someone else would. OTOH, the proposed fix should solve any issues with PHP environments like Tomas tested with and not create problems with any others that do not display such behavior. Do you actually have any specific reason why that code is not as good (better since it helps address this issue) than what is there now?
> The only issue I see with loading validate.php at the top of webmail.php
> is the additional includes from mime.php, and the i18n.php libraries. This
> will probably tack on a few ms for load time, but would probably not be
> substantial. Depends on the system load I guess.
From my perspective, I didn't originally fix it that way because no one could tell me why validate.php wasn't there and what might break if I put it there. I was trying to be as non-intrusive (given that this is STABLE) as possible. At this point, however, seems like this is probably the better fix.
Logged In: YES
user_id=225877
Originator: YES
including validate.php adds only MIME classes (class/mime.class.php, 1 defined call, 8 require_once, 80KB total), include/load_prefs.php and functions/page_header.php. src/webmail.php also loads functions/imap.php. Rest of validate.php functions are already loaded with other 1.4.9a src/webmail.php includes.
main difference between current 1.4.10-svn and include/validate.php on top of webmail.php - order of code execution. validate.php closes session.auto_start=on session before own session is started. current code closes active SquirrelMail session.
Logged In: YES
user_id=620333
Originator: NO
Patch ends up looking like this removing unneeded includes. validate.php seems to include all but the imap.php includes, so we strip everything else out as unneeded. As I don't have a vast array of plugins to validate against, can somebody give the below mods a shot? Also stripped out include for display_messages based on other submitted ticket.
Index: src/webmail.php
--- src/webmail.php (revision 12350)
+++ src/webmail.php (working copy)
@@ -20,18 +20,9 @@
define('SM_PATH','../');
/* SquirrelMail required files. */
-require_once(SM_PATH . 'functions/global.php');
-require_once(SM_PATH . 'functions/strings.php');
-require_once(SM_PATH . 'config/config.php');
-require_once(SM_PATH . 'functions/prefs.php');
+require_once(SM_PATH . 'include/validate.php');
require_once(SM_PATH . 'functions/imap.php');
-require_once(SM_PATH . 'functions/plugin.php');
-require_once(SM_PATH . 'functions/i18n.php');
-require_once(SM_PATH . 'functions/auth.php');
-if (!function_exists('sqm_baseuri')){
- require_once(SM_PATH . 'functions/display_messages.php');
-}
$base_uri = sqm_baseuri();
sqsession_is_active();
@@ -60,7 +51,6 @@
is_logged_in();
-require_once(SM_PATH . 'include/load_prefs.php');
do_hook('webmail_top');
/**
Logged In: YES
user_id=508228
Originator: NO
Worked OK for me. Although this will effectively mask the session_start bug, I suggest we fix it too:
--- functions/global.php.orig 2007-03-25 05:35:18.000000000 -0700
+++ functions/global.php 2007-03-25 05:35:38.000000000 -0700
@@ -400,10 +400,7 @@
function sqsession_is_active() {
- $sessid = session_id();
- if ( empty( $sessid ) ) {
- session_start();
- }
+ @session_start();
}
// vim: et ts=4
Logged In: YES
user_id=620333
Originator: NO
Changes committed to CVS.
Thanks for the report.
Logged In: YES
user_id=508228
Originator: NO
Reminder to Thijs: please use the SM API in your plugins from now on. ;-)