bug #32455 [HttpFoundation] Clear invalid session cookie (Toflar)

This PR was submitted for the 4.2 branch but it was squashed and merged into the 4.3 branch instead (closes #32455).

Discussion
----------

[HttpFoundation] Clear invalid session cookie

| Q             | A
| ------------- | ---
| Branch?       | 4.2 (actually maybe should also go to 3.4, see below)
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | TODO
| Fixed tickets |
| License       | MIT
| Doc PR        | not required

Currently, invalid session cookies are not cleaned up.

If the session is empty, the `AbstractSessionHandler::write()` destroys the session. If a new session has been started in the current process (meaning `session_start()` has sent the `Set-Cookie` header) then the `AbstractSessionHandler` will make sure this cookie is not sent to the client. If, however, `session_start()` did not send a cookie (meaning there was already a valid session ID in your request cookie), the `AbstractSessionHandler` will clear the session cookie (send a 0-lifetime cookie).
If, however, the request does contain a session ID cookie but it is not valid, `session_start()` will send a new cookie which is then again cleared by the `AbstractSessionHandler`. But it will not clear the old cookie sent by the request.

Here's a more complex example of what happens in the code flow when a user logs out and we regenerate a new session id for security reasons:

1. You have no `PHPSESSID` cookie yet.
2. You log into the system, you get a new `PHPSESSID` assigned. Let's go for session ID `1`.
3. You log out of the system, for security reasons you get session ID `2` regenerated.
4. The `AbstractSessionListener` pops in and calls `->save()` on your session handler.
5. The `NativeSessionStorage` calls the `StrictSessionHandler` (in fact the abstract parent, `AbstractSessionHandler`) which `write()`s the session data. In case the session data is empty, it will actually `destroy()` the session which means it will invalidate the session cookie. In that case, however, it won't send a 0-lifetime cookie because `$cookie = SessionUtils::popSessionCookie($this->sessionName, $sessionId);` will **not** return `null`. That is because after regeneration we actually do have a `Set-Cookie: PHPSESSID=2` header present.
6. This means, our `PHPSESSID=1` cookie is never deleted.

Why is this a problem?
Well, we have an invalid cookie that remains floating around forever. Loads of reverse proxies consider requests with cookies as being private and thus disable caching.

I'm not sure this is the correct fix here but it felt like the only place we can do this because it has to happen during or after `$session->save()`.

Looking for feedback first before we finish this with tests etc.

Regarding Symfony 3.4: Not sure how this is affected because there's not even a `SessionUtils` class so I'd prefer to leave that fix to somebody who feels more comfortable with that code base 😄

/cc @aschempp

Commits
-------

b22a726 [HttpFoundation] Clear invalid session cookie

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/AbstractSessionHandler.php

@@ -124,7 +124,15 @@ public function destroy($sessionId)
                 throw new \LogicException(sprintf('Session name cannot be empty, did you forget to call "parent::open()" in "%s"?.', \get_class($this)));
             }
             $cookie = SessionUtils::popSessionCookie($this->sessionName, $sessionId);
-            if (null === $cookie) {
+
+            /*
+             * We send an invalidation Set-Cookie header (zero lifetime)
+             * when either the session was started or a cookie with
+             * the session name was sent by the client (in which case
+             * we know it's invalid as a valid session cookie would've
+             * started the session).
+             */
+            if (null === $cookie || isset($_COOKIE[$this->sessionName])) {
                 if (\PHP_VERSION_ID < 70300) {
                     setcookie($this->sessionName, '', 0, ini_get('session.cookie_path'), ini_get('session.cookie_domain'), filter_var(ini_get('session.cookie_secure'), FILTER_VALIDATE_BOOLEAN), filter_var(ini_get('session.cookie_httponly'), FILTER_VALIDATE_BOOLEAN));
                 } else {

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

@@ -241,6 +241,7 @@ public function regenerate($destroy = false, $lifetime = null)
      */
     public function save()
     {
+        // Store a copy so we can restore the bags in case the session was not left empty
         $session = $_SESSION;
 
         foreach ($this->bags as $bag) {
@@ -266,7 +267,11 @@ public function save()
             session_write_close();
         } finally {
             restore_error_handler();
-            $_SESSION = $session;
+
+            // Restore only if not empty
+            if ($_SESSION) {
+                $_SESSION = $session;
+            }
         }
 
         $this->closed = true;

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/storage.expected

@@ -11,10 +11,11 @@ $_SESSION is not empty
 write
 destroy
 close
-$_SESSION is not empty
+$_SESSION is empty
 Array
 (
     [0] => Content-Type: text/plain; charset=utf-8
     [1] => Cache-Control: max-age=0, private, must-revalidate
+    [2] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
 )
 shutdown

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected

@@ -20,5 +20,6 @@ Array
     [0] => Content-Type: text/plain; charset=utf-8
     [1] => Cache-Control: max-age=10800, private, must-revalidate
     [2] => Set-Cookie: abc=def
+    [3] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
 )
 shutdown