Open fredericgboutin-yapla opened 1 year ago
@fredericgboutin-yapla could you try to add a test case to https://github.com/zf1s/zf1/blob/master/tests/Zend/Session/SessionTest.php covering the issue, please?
I really hoped there would be some tests already covering my fix 😅 but yes sure.
I really hoped there would be some tests already covering my fix 😅 but yes sure.
Nothing broke in existing tests with your change that's true, but a test should be added to cover the bug - which would fail without the fix and would pass after the fix.
there's already a test named testInvalidPreexistingSessionIdDoesNotPreventRegenerationOfSid
that is playing with the mecanism I'm trying to fix.
Looking at it, I guess the problem is that it does not let PHP figure the session_id
by itself through the PHPSESSID
cookie.
Since I cannot call setCookie
in this context, I tried replacing session_id('xxx');
with $_COOKIE['PHPSESSID'] = 'xxx';
and I think I reproduce the issue with the assert returning Failed asserting that '' matches PCRE pattern "/^[0-9a-v]+$/".
Now let's try to put all this together...
Hum... I think I got a bit out of my way in my last comment.
The current test is alright. The problem is really when,
session_id
already defined - in "real life" this happens on a brand new user, w/o any cookiessession_id
from the cookie and then sets SID
to an empty string - see https://github.com/php/php-src/blob/PHP-7.0.33/ext/session/session.c#L1559session_start
regenerateId
which does not happen because it bails out early when a session is already started.So what do we do from here? Well... I guess we have to make the getId()
and _checkId()
statements at the beginning of the start
function actually work when no session_id is currently defined.
Now about the SID
constant...
The more I read about session management, the more I realize how old Zendframework 1 is since it doesn't support session strict mode - https://www.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode - nor the session ID validation in session handlers.
I now really doubt the original purpose of Zend when using the SID
constant to determine when a "session has already been started". Because, normally / by default, session management is done "using only cookies" - https://www.php.net/manual/en/session.configuration.php#ini.session.use-only-cookies - and not through URL parameters anymore (for security reasons). So it means, looking at the code of PHP 7.0.33, that the SID
constant will either not exist OR always being set an empty string.
Long story short, we should most probably drop SID
constant usage and simply rely on session_status()
to know if a session was already started or not - https://www.php.net/manual/en/function.session-status.php
😩 I'm growing tired I think.
We are successfully using a workaround like this,
$sessionId = $_COOKIE['PHPSESSID'] ?? false;
if ($sessionId) {
session_id($sessionId);
}
Zend_Session::start(...);
So maybe we could modify getId
like,
public static function getId()
{
$id = session_id();
if ($id === '' && isset($_COOKIE[session_name()])) {
return $_COOKIE[session_name()];
}
return $id;
}
And NOT forcing a re-generation when creating a new session with an invalid ID,
// Force a regenerate after session is started
if (self::$_sessionStarted) {
self::$_regenerateIdState = -1;
}
It seems to work really well.
Now to test this. 🤔 We must NOT call session_id(...)
at all and rely exclusively on cookies to start a new session...
Fix for https://github.com/zf1s/zf1/issues/165
Zend have a logic in place to create a valid session ID from the orignal value, MD5-ing it. This logic doesn't properly kick in the aforementioned situation of the issue but with this fix it does.
The fix does NOT prevent an initial write through the Session handler. This means that session with invalid keys are created into the database, file or redis instance in use for session storage.
In order to fix that, I'm afraid we would need to refactor the code to always md5 invalid entries pro-actively and since
getId()
is empty at that moment, I'm really not sure how we should to that.Note that the comment
// Check to see if we've been passed an invalid session ID
at the beginning of thestart()
method only really apply on an already opened session.