PHP session is always started

[ichaber]

The session init in HybridSessionMiddleware for SS4 always leads to a PHP session start once a PHP session has been initialised on any other. This is problematic, if you try to optimise the page load performance by using a CDN or a WAF (e.g. Cloudflare or Incapsula). The session cookie will essentially prevent that page from being cached.

One example I found was a home page (without the need for a session cookie) had a session cookie set after visiting a site with a form (which needs a session cookie).

The issue is caused by an interplay of session init in HybridSessionMiddleware (https://github.com/silverstripe/silverstripe-hybridsessions/blob/master/src/Control/HybridSessionMiddleware.php#L15) and the framework Session which checks if the request contains a sessionId and if so starts the (PHP) session (https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Session.php#L222).

[chillu]

Note that this module is only enabled for Active DR mode in CWP (via an environment constant), so it'll be easily missed.

One example I found was a home page (without the need for a session cookie) had a session cookie set after visiting a site with a form (which needs a session cookie).

Wait, that's expected behaviour, right? If you start a PHP session, it sets a cookie. If you then send that cookie with subsequent requests, that session is continued.

I thought the issue was that subsequent requests (session previously started) would respond with another Set-Cookie call with is superfluous and mucks with Incapsula sticky sessions?

[caffeineinc]

Yeah, Ideally, it shouldn't start a session until its saving data as there's no reason to have a sticky session unless it's storing something, or something needs to be saved.

The other problem i'm seeing is it's setting multiple session headers on requests without a session, and resets the cookie on every request. (which means sticky sessions can never be used, as the WAF see's each request as "new")

< Set-Cookie: PHPSESSID=vnlvdsg85h0kq7gjq634242325; expires=Mon, 29-Oct-2018 21:38:08 GMT; Max-Age=3600; path=/; HttpOnly
< Set-Cookie: PHPSESSID=vnlvdsg85h0kq7gjq634242325; expires=Mon, 29-Oct-2018 21:38:08 GMT; Max-Age=3600; path=/; HttpOnly

The other issue is, as this is starting a session on every request - once the session contents get too large, the session has to be tied to a DB call to bootstrap.

Set-Cookie: SECSESSID_2=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; HttpOnly
Set-Cookie: SECSESSID_2=<encryptedcontent>; expires=Mon, 29-Oct-2018 21:57:46 GMT; Max-Age=3600; path=/; secure; HttpOnly
Set-Cookie: SECSESSID=<sessionID>; expires=Mon, 29-Oct-2018 21:57:43 GMT; Max-Age=3600; path=/; secure; HttpOnly

[chillu]

OK I've marked this as impact/high since it affects the performance of cached websites. This module is mostly used in CWP Active DR contexts.

The other issue is, as this is starting a session on every request - once the session contents get too large, the session has to be tied to a DB call to bootstrap.

That's by design in the module. Are you saying that the repeat start of the session makes the session itself too large and triggers that behaviour?

[caffeineinc]

No the problem is more, it should only bootstrap the session, if it's either read or written too.

Now, t's lazy started, but if you send a header cookie, it should only bootstrap and init the query it if it's requested.

Hybrid sessions is only starting sessions on every request if there's something accessing or writing to session which should be the correct behaviour.

[robbieaverill]

Hybrid sessions is only starting sessions on every request if there's something accessing or writing to session which should be the correct behaviour.

Sounds correct to me as well.

Is there a bug here?

[ScopeyNZ]

I know that @caffeineinc is working on an issue with sending too many session cookies on framework. Currently this module uses the same session start logic as core Session::init() which only starts the session if required. I'll close this for now unless we can be sure there's some broken functionality in this module 🙂.

Thanks all!