destroy session first on regenerate session when session is active

[samsonasik]

Q	A
Bugfix	yes
BC Break	no
New Feature	no
RFC	no

Description

re-create PR for #1 . There is a use case when session already started before, and already set some value, eg: on csrf session data on a login form. When authenticate, it call the session regenerate, which session id changed, but left the old session not persisted (unset not applied as session id changed), so old session with the value remain in the disk.

To avoid it, I think we can apply session_destroy() before session_write_close() whenever session is active.

[michalbundyra]

@samsonasik I've made some improvements to your test and all seems to be working fine - on travis and on local. Please have a look.

Tests in that file run in separate process so there is no need for clear the session at the end of the test.

[samsonasik]

@michalbundyra thank you, that's ok 👍

[samsonasik]

@michalbundyra any chance for merging ? thank you.

[Xerkus]

I am concerned with the change of initial intended behavior to exact opposite.

Why was it done in a way explicitly preserving old session with data? session_regenerate_id(true) could have been used otherwise to achieve what this PR wants to introduce.

Could it possibly be that race condition actually is an issue that cause new session id to be overwritten by one of the parallel requests? As mentioned in docs, say, strict mode regenerating id on empty/missing old session and sending cookie superseding actual session?

[samsonasik]

@Xerkus I've removed session_write_close() function call

[Xerkus]

This is not a bugfix, but rather improvement that changes behavior. Should target next minor.

[Xerkus]

So far these are the only effect of this change that I can think of:

1. Refreshed session id is lost on bad connection (like mobile network). Session data is effectively lost as previous session is deleted.
   Example:

   • User comes as a guest
   • Multiple items are placed into the cart
   • On checkout user logs in, session id is regenerated
   • Request fails to load, new session id is lost
   • Cart reference in session is lost. No recovery available.

   Generally we see this as an acceptable very low probability security tradeoff.

2. With session.use_strict_mode enabled, deleting session might have client side parallel request race condition leading to scenario 1. Strict mode is off by default.
   Example:

   • Browser launches request that will regenerate session id.
   • Browsers launches additional parallel requests with original session id. Those requests are waiting on blocking session_start()
   • Browser receives response with regenerated session id
   • Waiting parallel request proceed once session lock is released by first request. Since session was deleted strict mode kicks in and regenerates session id.
   • With SAPI emitter session cookie is automatically sent (This should probably be investigated in laminas/laminas-httphandlerrunner - reset php queued headers before emitting PSR response)
   • Browser receives parallel response and overwrites proper regenerated session id
   • We got scenario 1.

   I did not try to recreate this scenario.
   If it is an actual issue when strict mode is enabled we can replace session data instead of deleting with say ['__deleted__' => true] . Needs further investigation.

[weierophinney]

Why was it done in a way explicitly preserving old session with data? session_regenerate_id(true) could have been used otherwise to achieve what this PR wants to introduce.

We chose not to use session_regenerate_id() because we needed to be able to call session_start() with custom options (use_cookies, use_only_cookies, and cache_limiter). Unfortunately, when you call session_regenerate_id, PHP sets the set-cookie header itself, preventing us from setting the options; this meant having custom logic when regenerating the cookie.

As such, the approach here by @samsonasik is the correct one.

[weierophinney]

🚢

[samsonasik]

@michalbundyra merge-able ?

[michalbundyra]

@samsonasik Yes, I'll merge it soon, it's on my list, but first I am working on laminas-form.

[michalbundyra]

Thanks, @samsonasik!