Comment 1 for bug 1570179

Revision history for this message
Aaron Wells (u-aaronw) wrote :

My rough outline of how this should go:

0. Well, option 1 would be to avoid using PHP's built-in session handler system at all. (i.e. http://php.net/manual/en/book.session.php ). Since the PHP session system was *not* built to support non-blocking sessions, we may get the best results by building our own from scratch. There is nothing magic about sessions; it's just a cookie with an ID, and a data store that links serialized data to that ID. However, to avoid the pitfalls of reinventing the wheel, it would be best if we could re-use an existing session library. For instance the "Illuminate" session package, which is based on Laravel and does not use $_SESSION: https://packagist.org/packages/illuminate/session . We can't use that one specifically, though, because it needs PHP 5.5.9

Now, assuming that we *aren't* going to replace $_SESSION entirely, here's what I would recommend:

1. Replace $SESSION's "is_live()" method with two new statuses: "is_initiated()", and "is_open()". The "is_initiated()" method would indicate that we've called session_start() at least once, and hence $_SESSION is populated with data. The "is_open()" would mean that we've called session_start() and have not yet called session_write_close() or session_destroy(), which means that changes to $_SESSION will be recorded in the session store. We'd need to track these statuses ourselves through calls to the $SESSION singleton, because PHP provides no reliable way of detecting them. (PHP 5.4+ does have a session_status() method, but it is only equivalent to my "is_open()" method.)

2. User our knowledge of $SESSION->is_initiated() to make it so $SESSION->get() doesn't call session_start() or session_write_close(), thus reducing the number of duplicate session cookies generated.

3. Rewrite clear_duplicate_cookies() to use "header_remove()" instead of "header_remove('Set-Cookie')". The no-params version removes all headers (which we'd need to record first and add back), while the one-param version only removes the named type of header. It appears that in PHP 5.3 the duplicate session headers are not removed by the one-param version, but they are removed by the no-params version. Both forms remove the duplicate session headers, in later PHP versions.

4. Have $SESSION record the session's ID when the session is first initiated, or regenerated, and use that knowledge to remove any incorrect session cookies in clear_duplicate_cookies(). Currently we remove any non-unique cookies; but it seems this can cause problems when you regenerate the session ID. Both the old and new session ID may then be sent out in the response, and it's uncertain which the browser will use on its next request.

5. Stop using the session to send messages to the user on the next page request. This causes a lot of unpredictability and unreproduceability in session bugs, by changing when the session is initiated. Instead, write error messages to a database table, and fetch them using an "UPDATE... WHERE..." statement to ensure atomicity, and use a second ID cookie alongside the session cookie, to tie IDs to users. This would also reduce our need to create sessions for logged-out users.

6. Move the session's initiation as early as we can in the page load. This too will help reduce unpredictability. However, one difficulty here is that we would ideally not create sessions for logged out users (which may just be automated scripts that don't need a session). To workaround that, I'd suggest that if we see a session cookie included in the request, we launch the session early on in the page lifetime. If not, we avoid initiating until we get to auth_setup() and need to ascertain that the user is logged in, or until there's a call to $SESSION->set().

7. Bug 1567799: Get rid of the code in auth/session.php remove_user_sessions($userid) that highjacks sessions as a hacky way of deleting those sessions. It *is* the only way the PHP session library will let you clear a given session ID across all session back-ends, but as mentioned in that bug report, if it fails it causes the user's session ID to change. We either need to maintain a list of valid sessions in the database and compare the session ID to that on each page load, or we need to move the session highjacking code into a server-side script... but it seems the CLI SAPI can't access sessions, so that's probably not possible.