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
-------
b22a7263b9
[HttpFoundation] Clear invalid session cookie
This commit is contained in:
commit
51eb41b4c6
@ -124,7 +124,15 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
|
||||
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 {
|
||||
|
@ -241,6 +241,7 @@ class NativeSessionStorage implements SessionStorageInterface
|
||||
*/
|
||||
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,8 +267,12 @@ class NativeSessionStorage implements SessionStorageInterface
|
||||
session_write_close();
|
||||
} finally {
|
||||
restore_error_handler();
|
||||
|
||||
// Restore only if not empty
|
||||
if ($_SESSION) {
|
||||
$_SESSION = $session;
|
||||
}
|
||||
}
|
||||
|
||||
$this->closed = true;
|
||||
$this->started = false;
|
||||
|
@ -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
|
||||
|
@ -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
|
||||
|
Reference in New Issue
Block a user