[Session] cleanup behavior #12325

Tobion · 2014-10-26T22:01:59Z

So are dozens of Session bug reports across the Symfony and Silex and other related libraries. Most of them are because of wrong usage or misunderstanding. This ticket sums up how I see sessions should behave after researching many, many tickets.

Session State

    /**
     * Whether the session can be resumed from the previous request.
     *
     * It means that there is a cookie with the given session name.
     *
     * @return bool
     */
    public function isResumable();

    /**
     * Returns whether the session was started.
     *
     * When the session is saved, i.e. not active anymore, it will still return true. It only means that start() was called at least once (directly or indirectly).
     *
     * @return bool
     */
    public function isStarted();

   /**
     * Returns whether the session is currently open, i.e. started but not saved yet.
     *
     * Equivalent to session_status() === PHP_SESSION_ACTIVE. But needs special treatment for PHP 5.3.
     *
     * @return bool
     */
    public function isActive();

Session Usage

Session methods should throw exceptions when misused and when this misuse would cause wrong expectations. If the "misuse" does not cause side-effects or unexpected behavior, it should not throw exceptions.

SessionStorage::regenerate should throw exception if no session is active because otherwise it has no effect. This fixes Session invalidate does not work (session not started automatically) silexphp/Silex#746 and https://bugs.php.net/bug.php?id=49462
SessionStorage::start should NOT throw an exception because starting a session twice makes no sense but has no downsides.
If the session is already started by symfony, just ignore the start (this is already how it is).
If the session was started by third party (e.g. facebook sdk) or legacy apps and is still ACTIVE, save and close the third party session and start the symfony session instead so it uses it's configuration (save handler especially). Optionally, one can also merge the current $_SESSION data into the symfony session data, in case you need access to the third party session data from within the symfony session.
SessionStorage::save should throw an exception if session is not active because there is something wrong in program logic. You cannot save something that does not exist.
SessionStorage::setId should throw an exception if session is active (partly as-is). But the question is why there is a setter at all? There should not be a case where you need to explicitly set an id. It's exactly the point that the id is random and not guessable.

Session Lifecycle

Always save session before sending response / doing a redirect. This will fix many common problems:

When doing a redirect after a Session::regenerate login, the session data is actually available on the next request. Fixes NativeSessionStorage::regenerate bug #7380 and Symfony 2.2.1 - session lost after success login rediraction #7885 and https://bugs.php.net/bug.php?id=61470. Locking won't help in this case because the ID is different, which is the whole point. [HttpFoundation] fixed issue with session_regenerate_id (closes #7380) #8270 should be reverted.
Locking solves all other cases with concurrency problems. See [HttpFoundation] Strange behaviour with empty session, Swift and fastcgi_finish_request function #6417 But making sure that the session is saved before sending response will improve performance when people forgot to call save themselves. It prevents that the session is locked longer than needed due to long running things after sending the response.
And if the dev uses a save handler without locking (not available or on purpose, see [HttpFoundation] Session handlers should use a lock #4976), then it also ensures the data is available on the next request how it is expected.

Auto-start behavior when reading/writing

See also #6036

Reading from the session when there is no session cookie, i.e. there is no previous session: It SHOULD NOT start the session. There is no reason to create a session with a cookie when only reading data. It just means the data is not available as we already know since there is no session. This is also what @TerjeBr suggested. @Drak argument that people need to check themselves whether the session is active is also possible. But the example he gave {% if app.session.isstarted() %}{% for flashMessage in app.session.flashbag.get('notice') %} is wrong. If the session is closed/saved this won't even work and not show the messages. So actually one would need to check Request::hasPreviousSession each time one reads session data at the moment. But the session part does not even have a method for that. So when injecting the session alone (without request) you don't have access to this important method. So in reality, forcing users to check the session state everywhere where they read data is not viable.
Reading from the session when there is IS a session cookie . It should auto-start the session which reads the information stored in the session and makes them available. Otherwise reading data would not find the data unless it has been started manually which is also not developer friendly.
Reading from the session when the session has already been started in the current request but is currently closed/saved: In this case we don't necessarily need to re-start the session because the data is already available. This is a performance enhancement because it prevents to read the data again that we already have. The only disavantage is that it does not synchronize the data written in another request between your save() and the get(). But this is highly unlikely and rather strange case anway. And devs could just call start() themselves if they want that sync.
Writing to the session should always start the session so that the data is actually persisted. Otherwise there would be no point in writing to the session when it's not available in the next request. Zend Session raises an immutable exception when writing to a session that is already closed/saved. But I don't think that's a good idea because then you cannot add session data later in the lifecycle anymore.

The text was updated successfully, but these errors were encountered:

znerol · 2014-11-08T10:11:29Z

Let me add the following item (currently implemented in Drupal): If the session is empty when attempting to write it at the end of the request, destroy it and remove the session cookie. This optimization is important for sites behind reverse proxies.

Seldaek · 2014-11-15T23:35:39Z

Related issue I just experienced: If a user is logged in and you close the session early (in the controller in my case) the Symfony\Component\Security\Http\Firewall\ContextListener comes in kernel.response and tries to write the security context to the session, therefore resulting in an exception / 500 instead of the response being sent. Maybe that should be taken into account by this refactoring, it seems way too hard to even do the right thing at the moment :/

Tobion · 2014-11-16T01:40:08Z

@Seldaek What exception is thrown? And how is this related to this ticket?

Seldaek · 2014-11-16T12:18:14Z

@Tobion I got: request.CRITICAL: Uncaught PHP Exception RuntimeException: "Failed to start the session: already started by PHP ($_SESSION is set)." and I checked the backtrace it came from the ContextListener's onKernelResponse. And well, it's not directly related to the issue perhaps but it's an issue related to session opening/closing. If you can't close the session before kernel.response it kind of sucks.

mpdude · 2014-11-17T12:57:39Z

Great research and suggestion, @Tobion!

kbond · 2014-11-23T16:39:35Z

I started getting the exact same error as @Seldaek after upgrading to 2.5.7 - should I open another issue, seems like BC break to me..

Seldaek · 2014-11-24T11:45:01Z

If it's a regrression then yes please open an issue. I just tried doing it
with the latest 2.3 and it failed so I figured it never worked.
On 23 Nov 2014 16:40, "Kevin Bond" notifications@github.com wrote:

I started getting the exact same error as @Seldaek
https://github.com/Seldaek after upgrading to 2.5.7 - should I open
another issue, seems like BC break to me..

—
Reply to this email directly or view it on GitHub
#12325 (comment).

Tobion · 2014-12-19T16:00:06Z

See also https://wiki.php.net/rfc/session-lock-ini

…s-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpFoundation] Make sessions secure and lazy | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | not yet | Fixed tickets | #6388, #6036, #12375, #12325 | License | MIT | Doc PR | - The `SessionUpdateTimestampHandlerInterface` (new to PHP 7.0) is mostly undocumented, and just not implemented anywhere. Yet, it's required to implement session fixation preventions and lazy write in userland session handlers (there is https://wiki.php.net/rfc/session-read_only-lazy_write which describes the behavior.) By implementing it, we would make Symfony session handling much better and stronger. Meanwhile, doing some cookie headers management, this also gives the opportunity to fix the "don't start if session is only read issue". So, here we are for the general idea. Now needs more (and green) tests, and review of course. Commits ------- 347939c [HttpFoundation] Make sessions secure and lazy

Tobion mentioned this issue Oct 27, 2014

[Session] remove invalid hack in session regenerate #12326

Merged

stof added HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Nov 17, 2014

kbond mentioned this issue Nov 24, 2014

[BC Break?] "Failed to start the session" error #12562

Closed

dbu mentioned this issue Mar 10, 2015

[Session] SessionInterface should have a destroy() method. Currently there is no way to simply destroy a session #12375

Closed

ro0NL mentioned this issue Mar 31, 2016

RFC process? #18383

Closed

TerjeBr mentioned this issue Oct 8, 2017

[Session] Do not start the session unless it is (or has been) written to #6388

Closed

nicolas-grekas mentioned this issue Oct 11, 2017

[HttpFoundation] Make sessions secure and lazy #24523

Merged

fabpot closed this as completed Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Session] cleanup behavior #12325

[Session] cleanup behavior #12325

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!