Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Clear invalid session cookie#32455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rpkamp commentedJul 9, 2019
@Toflar The The code was moved fromSymfony\Component\HttpFoundation\Session\Storage\Handler\AbstractSessionHandler to the new This was just a code re-location though, functionally it did not change. |
Toflar commentedJul 9, 2019
I see, so should be easy to backport. Not doing that right now, though :) Thanks for the feedback! |
nicolas-grekas commentedJul 10, 2019
Any way to move this behind |
Toflar commentedJul 10, 2019
I was thinking the same. We could, indeed for example by changing However, I was reluctant doing that because |
aschempp commentedJul 10, 2019
In that case we don't even need to check if a cookie was unset at all 🙃 |
Toflar commentedJul 16, 2019
Anything I can do here so we can get his fixed@nicolas-grekas? |
nicolas-grekas commentedJul 17, 2019
let's do it in |
| }finally { | ||
| restore_error_handler(); | ||
| $_SESSION =$session; | ||
| $_SESSION =$session;// TODO: why is $_SESSION restored with the meta data and bag keys here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas So what's happening here is that we copy$_SESSION to$session. Then we unset the meta data bag and other bag keys before we call the save handler. At this moment the session will be empty and thus the session will be invalidated. What's the point of setting the bags to$_SESSION again afterwards?
That looks really strange to me, also when looking into the functional tests. Let's say we checkstorage.php andstorage.expected. Here's whatstorage.php looks like atm:
<?php$storage =newNativeSessionStorage();$storage->setSaveHandler(newTestSessionHandler());$flash =newFlashBag();$storage->registerBag($flash);$storage->start();$flash->add('foo','bar');print_r($flash->get('foo'));echoempty($_SESSION) ?'$_SESSION is empty' :'$_SESSION is not empty';echo"\n";$storage->save();echoempty($_SESSION) ?'$_SESSION is empty' :'$_SESSION is not empty';ob_start(function ($buffer) {returnstr_replace(session_id(),'random_session_id',$buffer); });
And this is whatstorage.expected looks like:
openvalidateIdreaddoRead: readArray( [0] => bar)$_SESSION is not emptywritedestroyclose$_SESSION is not emptyArray( [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)shutdownAs you can see, I have adjustedstorage.expected because it indeed did not invalidate the cookie even though the session was empty. It is empty because the flash bag unsets on the firstget(), so that's correct. But what's strange is that afterdestroy andclose we expect$_SESSION is not empty which is of course true at the moment. But that's what makes no sense to me. Why do we restore that? Shouldn't we expect this is really completely empty?
If you remember why this is, I think I'll replace myTODO comment with a comment explaining why we restore the bag keys 😄 But personally I think we should only restore it if$_SESSION is not empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I know why this is. It's to restore the bags in case there were other keys. That's strange though.
Let me add to this PR on what I think would be correct. Maybe that's easier for you to review then when you can actually see the changes.
Toflar commentedJul 18, 2019
Updated the PR. In fact there were already tests that were wrong imho, so I adjusted those 😄 |
Toflar commentedJul 18, 2019
There we go, that's how I think it should be. Ready for a review 😄 |
src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/storage.expectedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Toflar commentedJul 18, 2019
Failing tests unrelated. |
This PR was merged into the 4.3 branch.Discussion----------[Messenger] fix test| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |fixes a test that was added in the `4.3` branch now thatsymfony#32700 is merged upCommits-------16fc81f fix test
* 4.2: make tests forward compatible with DI 4.4 Fix multiSelect ChoiceQuestion when answers have spaces
* 4.2: relax some assertions to make tests forward compatible fix typo
This PR was squashed before being merged into the 3.4 branch (closessymfony#32800).Discussion----------Improve some URLs| Q | A| ------------- | ---| Branch? | 3.4 <!-- see below -->| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/A <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->Commits-------fab17a4 Improve some URLs
* 3.4: Improve some URLs Fix test compatibility with 4.x components [Cache] cs fix
…sse)This PR was merged into the 3.4 branch.Discussion----------[HttpKernel] Fix s-maxage=3 transient test| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | NA| License | MIT| Doc PR | NAsometime the http server returns a `s-maxage=3` header (https://travis-ci.org/symfony/symfony/jobs/569326531)This PR fixes tests to allow both 2 and 3Commits-------f019b52 Fix s-maxage=3 transient test
* 3.4: [Intl] use strict comparisons Fix s-maxage=3 transient test
This PR was merged into the 3.4 branch.Discussion----------Replace warning by isolated test| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony#32844| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Failing test introduced in PHP 7.4 (fatal error) were skiped with a warning exception.This PR un tests is isolated process in order to correctly flag the test without stoping the test suite.I kept a comment to the original bug in order to easily remove themeCommits-------9c45a8e Replace warning by isolated test
* 3.4: Replace warning by isolated test
…ith/without return typehint (jderusse)This PR was merged into the 4.3 branch.Discussion----------[VarDumper] Fix test patern to handle callstack with/without return typehint| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony#32844| License | MIT| Doc PR | NAThe TestCase::tearDownAfterClass methods does not always have the same signature which change the output of the reflection. This use another methods for testingCommits-------feaadd1 Fix tst patern to handle callstack with/without return typehint
fabpot commentedAug 9, 2019
Should probably target 3.4 as this is a bug fix. |
Toflar commentedAug 9, 2019
I can have a look at it again but I'd still appreciate it if this would be released with the next 4.3 as it is affecting my projects and already didn't make it in the last 4.2 😊 |
fabpot commentedAug 9, 2019
ok, I missed the comment for 3.4. Let's try to do it on 4.3 then. |
10329c7 tob22a726Comparefabpot commentedAug 9, 2019
Thank you@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 requiredCurrently, 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@aschemppCommits-------b22a726 [HttpFoundation] Clear invalid session cookie
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 (meaningsession_start()has sent theSet-Cookieheader) then theAbstractSessionHandlerwill 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), theAbstractSessionHandlerwill 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 theAbstractSessionHandler. 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:
PHPSESSIDcookie yet.PHPSESSIDassigned. Let's go for session ID1.2regenerated.AbstractSessionListenerpops in and calls->save()on your session handler.NativeSessionStoragecalls theStrictSessionHandler(in fact the abstract parent,AbstractSessionHandler) whichwrite()s the session data. In case the session data is empty, it will actuallydestroy()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);willnot returnnull. That is because after regeneration we actually do have aSet-Cookie: PHPSESSID=2header present.PHPSESSID=1cookie 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
SessionUtilsclass so I'd prefer to leave that fix to somebody who feels more comfortable with that code base 😄/cc@aschempp