Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#180 session is closed before webmail_top hook

closed-fixed
None
5
2007-03-29
2007-03-21
Tomas Kuliavas
No

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

Discussion

<< < 1 2 3 > >> (Page 2 of 3)
  • 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.

     
  • Tomas Kuliavas
    Tomas Kuliavas
    2007-03-24

    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.

     
  • Tomas Kuliavas
    Tomas Kuliavas
    2007-03-26

    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

     
<< < 1 2 3 > >> (Page 2 of 3)