Patch for ini-setting session cookie lifetime != 0

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Patch for ini-setting session cookie lifetime != 0

Sebastian Petters / 4wd media
Dear RoundCube developers,

first of all thank you for the development of RoundCube! Even in this
pre-1.0 state, this project looks very promising. I'm looking forward
for the further development.

For a customer we are using an ini-setting session.cookie_lifetime which
is non-zero, so the session is still open if the browser gets closed.
With this setting, a login to RoundCube was not possible. I tracked the
problem down to the function sess_regenerate_id() in
program/include/session.inc. The parameters passed to the call to
setcookie are not right. A possible solution may be the following:

    session_id($random);
    $cookie = session_get_cookie_params();
-  setcookie(session_name(), $random, $cookie['lifetime'], $cookie['path']);
+  $expire = ($cookie['lifetime'] == 0) ? 0 : time() + $cookie['lifetime'];
+  setcookie(session_name(), $random, $expire, $cookie['path']);

    return true;

setcookie does not expect the lifetime but an expiry unix timestamp.
If the cookie lifetime is 0 (session cookie) nothing changes.
Otherwise, if the lifetime is non-zero we need to add the current time.

I'm sorry for not using your bugtracker, but i didn't find a "report
bug" form. Anyhow, i hope the patch will get applied.

Thanks in advance and best regards,
Sebastian

--
Sebastian Petters
www.4wdmedia.de

_______________________________________________
List info: http://lists.roundcube.net/dev/
tfk
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Patch for ini-setting session cookie lifetime != 0

tfk
On 8/10/07, Sebastian Petters / 4wd media <[hidden email]> wrote:

> Dear RoundCube developers,
>
> first of all thank you for the development of RoundCube! Even in this
> pre-1.0 state, this project looks very promising. I'm looking forward
> for the further development.
>
> For a customer we are using an ini-setting session.cookie_lifetime which
> is non-zero, so the session is still open if the browser gets closed.
> With this setting, a login to RoundCube was not possible. I tracked the
> problem down to the function sess_regenerate_id() in
> program/include/session.inc. The parameters passed to the call to
> setcookie are not right. A possible solution may be the following:
>
>     session_id($random);
>     $cookie = session_get_cookie_params();
> -  setcookie(session_name(), $random, $cookie['lifetime'], $cookie['path']);
> +  $expire = ($cookie['lifetime'] == 0) ? 0 : time() + $cookie['lifetime'];
> +  setcookie(session_name(), $random, $expire, $cookie['path']);
>
>     return true;
>
> setcookie does not expect the lifetime but an expiry unix timestamp.
> If the cookie lifetime is 0 (session cookie) nothing changes.
> Otherwise, if the lifetime is non-zero we need to add the current time.

Standard behavior on webmails is, that when you close the browser, the
session is gone. I wouldn't want it any other way. Of course this is
not as "important" as online banking but on the other hand important
enough.

I think what you are asking for would ask for a config-option, so if
you implement it that way I would have no objections. ;-)

Feedback?

> I'm sorry for not using your bugtracker, but i didn't find a "report
> bug" form. Anyhow, i hope the patch will get applied.

We run everything on http://trac.roundcube.net

Till
_______________________________________________
List info: http://lists.roundcube.net/dev/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Patch for ini-setting session cookie lifetime != 0

Sebastian Petters / 4wd media
Hi Till!

> Standard behavior on webmails is, that when you close the browser, the
> session is gone. I wouldn't want it any other way. Of course this is
> not as "important" as online banking but on the other hand important
> enough.
>
> I think what you are asking for would ask for a config-option, so if
> you implement it that way I would have no objections. ;-)
>
> Feedback?

I totally agree, finishing the session when the browser is closed should
be the normal behavior. This is the default PHP configuration and is
sufficient for most applications. But PHP also allows to explicitly set
the lifetime of the session cookie via the session.cookie_lifetime
option to a value which is non-zero.

Currently RoundCube does not support this feature. More severe, it is
not possible to log-in to RoundCube if session.cookie_lifetime is
non-zero, because the session cookie expiry time is not set properly in
this case.

In my opinion, there are two possibilities:

1) Restrict RoundCube to not support the session.cookie_lifetime PHP
configuration option. This way, the session will always be terminated if
the browser gets closed.
In this case, the third parameter of the setcookie call should always be 0.

2) Support the session.cookie_lifetime option by properly set the cookie
expiration time. In nearly all cases this will make no difference, since
session.cookie_lifetime is normally set to 0.
In this case, the third parameter for setcookie should be time() +
$cookie['lifetime'], because this is the expiry time (unix timestamp)
and not the lifetime (in seconds) (see also
http://de.php.net/manual/en/function.setcookie.php).

Using $cookie['lifetime'] as third parameter makes no sense if non-zero,
because it is a date in the past if interpreted as an unix timestamp and
the session cookie is immediately expired.

 From my point of view, there is no reason to restrict RoundCube to case
1). session.cookie_lifetime is a normal PHP configuration, so why forbid
the user to enable it?
The change i suggested fixes the cookie expiration time as suggested in
the second case. There are no other parts of the code affected and no
RoundCube specific configuration option has to be introduced. If
session.cookie_lifetime is 0 as in most hosting environments, nothing
changes.

Sebastian

--
Sebastian Petters
www.4wdmedia.de
_______________________________________________
List info: http://lists.roundcube.net/dev/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Patch for ini-setting session cookie lifetime != 0

Michael Baierl-2
Sebastian Petters / 4wd media wrote:
>> Feedback?

For such a critical app like Webmail a cookie lifetime does not make any
sense. If the user closes the browser the session should be gone. I'm
really sure most users don't log out but just close the browser - this
would be a huge security hole!

The longer cookie lifetime makes sense for i.e. Google to remember
preferences without logging in, but not for Webmail.

A checkbox ("remember me for XXX days", defaults to 0) on the login page
might make most sense, so the users can decide what they want to have.

My 2 cents,

Mike
--
Michael Baierl
mbaierl.com   http://mbaierl.com/
- - - - - - - - - - - - - - - - -
Windows Vista has been rumoured to be a complete port of the original
PERL Windows code into C. This would explain, well, everything.

_______________________________________________
List info: http://lists.roundcube.net/dev/
Loading...