Important! Discussion has moved to #1858196: Turn session.inc into a service leveraging symfony session components where work has been divided up into a smaller patch and sub-issues.
Important note to committers: make sure before commit this is cleared by the security team (see process concerns in #185).
I recommend jumping straight to comment #153. A lot of earlier comment relates to code that predates latest Symfony 2 improvements.
We want to get rid of our custom bastard Session handling, and we need to make it pluggable in an easier way than rewritting the full session.inc file. For that, we want to try to use the Symfony 2 HttpFoundation component's session handling.
What will it bring to Drupal
- Code consistency, and unification (using Symfony 2) and a lot less of legacy code to maintain
- Session storage pluggability (session storage in Symfony vocabulary is the in-memory $_SESSION array)
- Session handler pluggability (session handler in both PHP and Symfony vocabulary is the physical session data storage mecanism, for us, it's the database, but later, it will be anything compatible with Symfony)
- A full OOP API to handle the session, session component interface and injection throught the DIC, thus removing yet again an global state out there (not totally true because PHP will allow only one session handler to be set, but conceptually and in design)
Where do we need to be extra careful
We have two major features that Symfony today does not handle natively we absolutely want to keep:
- Lazy cookie sending (not lazy session initialization): this allows anonymous user to avoid having useless session, and allow core page cache mecanism to work seamlessly
- HTTP/HTTPS dual cookie handling: this is a real security measure that disallow HTTP session hijack and re-use the HTTP session identifier with HTTPS session.
Proposed plan (where I need review, please!)
Here is the proposed plan (taken, simplified and updated from #153:
- Replace the full session.inc using custom Symfony component's code, at this stade we actually loose the features described upper [DONE in patch #166]
- Restore the custom cookie handling, needed it for the next point, by implement a proxy pattern arround Symfony's
SessionProxy. At this stade, we still don't handle dual HTTP/HTTPS session. [DONE in patch #166]
- Put back lazy cookie sending in action, implement a proxy pattern arround Symfony's
Sessionobject (for exposing the public API) and arround
NativeSessionStorage(because a method is missing for fully relying on the interface). I hope I will have the time and courage to post a Symfony PR for that issue (I will explain later). [DONE in patch #166]
- Put back the dual session cookie handling in action, using our previous implementation of
SessionProxy. This until Symfony implements iself this mecanism or any alternative that brings the same security level.
- Find an upgrade path! And this one is not easy.
Once we have a clean solution for the problem enounced below, and working code for dual session cookie handling, we are good to go for final review process and core commit if the code pleases all eyes.
The upgrade problem
Symfony's session handling does not care about UID, but Drupal does: the actual core code uses a
sessions database table composed of multiple mandatory fields, including the UID. Because per design Symfony's does not care, we cannot use the actual core table for storing sessions. This has a major impact for us: upgrade path is not possible, because in the
DatabaseSessionHandler implementation, we have two possibilities:
- Either we set a false UID always, but since the SQL query will be hardcoded, we cannot fix the database table because depend on the old schema.
- Either we do not set a UID at all in the SQL insert/merge queries, but we will raise PDOExceptions because the field is mandatory, this until we fixed the table.
In both case, we are blocked, because in the first case, we cannot upgrade properly, in the second, we cannot even access the update once the code has been updated.
There is some solutions thought:
- If we ask Drupal 7 maintainers to set a default value to the
uidfield to 0, our Drupal 8 handler will work flawlessly on the old schema, we can then update. But this is invasive in Drupal 7 (and the most easy solution thought)
- Create a new table with a new name (dropping the trailing 's' in
sessions) and use it directly: problem almost solved because we can drop the old one and just not use it, but we have to consider that doing that, we will have PDOExceptions trying to use an non existing database table.
- Using another session handler for update.php (using PHP native one) was another solution, but not exploitable because we can't login the user in update.php.
Note that doing runtime check on the table schema is out of question, it's really a bad idea that will have a negative impact on performances on such early and ominpresent core system.
This issue needs brains for telling if that plan is bad, and if so why.
It also needs some more brains mostly for the update problem.
And finally it needs reviewers to see if the the code doesn't sound bad (please no whitespace reviews in such early development phase).
- As part of the WSCCI initiative, it has been decided that we should use the Symphony2 HTTP Foundation Library (#40). That library comes with a session handler that we could build upon instead of rolling our own.
- The session library comes with an equivalent function to drupal_set_message() ("flash"). We need to decide if we can use that, get it refactored upstream, or not use the session handler at all.
- We could also just use their SessionStorage interface, and write our own Session class. This looks to be the way to go as of #50.
- Since using the Session classes requires HttpFoundation, this issue has been marked as postponed pending the resolution of #1178246: Add Symfony2 HttpFoundation library to core
- Do not repurpose the $_SESSION global unless it would work *perfectly* to expectations and behavior of general language usage and not impact performance. Otherwise layer a session object over it for use by Drupal. (#31, #32, #47)