SquirrelMail

Reminder to Thijs: please use the SM API in your plugins from now on. ;-)

Changes committed to CVS.

Thanks for the report.

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

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');

/**

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.

> 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.

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.

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.

session is not destroyed. It is read only.

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.

... and askuserinfo plugin should still be correctly coded to use the SM
API. Thijs, you should know better. ;-)

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.

<?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

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.

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.

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>";

}

Paul,

Have you actually tried using sqsession_register in askuserinfo plugin?

Have you actually tried using plugin when your code modification was
disabled?

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...).

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.

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?