Fix invalid session regeneration on wrong session ID

[fredericgboutin-yapla]

Fix for #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 the start() method only really apply on an already opened session.

[falkenhawk]

@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?

[fredericgboutin-yapla]

I really hoped there would be some tests already covering my fix 😅 but yes sure.

[falkenhawk]

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.

[fredericgboutin-yapla]

this condition is adding to the difficulty to properly unit-test

[fredericgboutin-yapla]

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...

[fredericgboutin-yapla]

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,

1. there's no session_id already defined - in "real life" this happens on a brand new user, w/o any cookies
2. After skipping ID check, Zend starts a new session, PHP gets its session_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#L1559
3. Zend is happy a session was created (and stored) despite having an ID that did NOT pass the check requirement since it was empty prior to calling session_start
4. the next time Zend is asked to "start" a session, check validation fails, it then "schedule" a 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.

[fredericgboutin-yapla]

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...