[Security] Don't destroy the session on buggy php releases. #13286
Conversation
|
Are we positive Debian has not backported the php fix to their PHP 5.4.4 version? :) |
|
@andrerom Good point. Since the bug does not have a security impact, I doubt that there is a backport. Same with RHEL/CentOS php 5.4.9. But I don't have either distribution as a VM right now to verfiy this. In general, I'm not a friend of checking against the php version, but I can't see a different way of checking at runtime weather the php distribution is affected or not in this particullar case. |
|
RHEL/CentOS uses 5.4.16 according to distrowatch |
|
👍 |
| @@ -39,6 +39,10 @@ public function testUnsupportedStrategy() | |||
|
|
|||
| public function testSessionIsMigrated() | |||
| { | |||
| if (PHP_VERSION_ID >= 50400 && PHP_VERSION_ID < 50411) { | |||
| $this->markTestSkipped('We must not destroy the old session on php 5.4.0 - 5.4.10.'); | |||
There was a problem hiding this comment.
the message should rather be cannot.
and instead of having 2 tests where one is always skipped while the other runs, shouldn't we have a single test with the version check in it ?
There was a problem hiding this comment.
Well, but we can destroy the session. We would just run into trouble afterwards. 😉 Okay, I will change the message.
Yes, I can merge those tests again. I made two tests for the following reasons:
- I'd like to keep logic (
if/else) out of tests. - They are testing different behavior.
- The second test can simply be dropped as soon as php 5.4 is not supported anymore.
There was a problem hiding this comment.
Keeping two tests is fine by me.
@derrabus Can you update this PR so that I can merge it today and start releasing 2.6.2? Thank you very much.
|
I have updated the test. |
|
Thank you @derrabus. |
…. (derrabus) This PR was squashed before being merged into the 2.3 branch (closes #13286). Discussion ---------- [Security] Don't destroy the session on buggy php releases. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13269, #13283 | License | MIT | Doc PR | none See #13269 for the discussion. This workaround avoids destroying the old session after login on the migrate strategy when running under a php version that we know to be broken. Corresponding php bug: https://bugs.php.net/bug.php?id=63379 Commits ------- 5d0b527 [Security] Don't destroy the session on buggy php releases.
|
Should we not also consider the Session::invalidate code as well? It makes an explicit call to Session::migrate: public function invalidate($lifetime = null)
{
$this->storage->clear();
return $this->migrate(true, $lifetime);
} |
See #13269 for the discussion. This workaround avoids destroying the old session after login on the migrate strategy when running under a php version that we know to be broken.
Corresponding php bug: https://bugs.php.net/bug.php?id=63379