Page MenuHomePhabricator

Session manager breaks installer
Closed, ResolvedPublic

Description

Going to installer:

If you don't have php compiled with mysql support, you get an exception about no valid db driver for mysql. The backtrace suggests that mw-config/index.php calls includes/WebStart.php, which calls SessionManager::getGlobalSession(), which calls SessionManager->generateSessionId(), which calls BagOStuff->get(string), which eventually goes to SqlBagOStuff->getDB(integer), which explodes.

If you do have mysql support compiled in, no exception, but you quickly run into "Your session data was lost! Check your php.ini and make sure session.save_path is set to an appropriate directory. " (I imagine because there is no db set up yet).

Thus preventing me from running the installer.

Event Timeline

Bawolff created this task.Feb 7 2016, 10:36 PM
Bawolff updated the task description. (Show Details)
Bawolff raised the priority of this task from to Needs Triage.
Bawolff added subscribers: Bawolff, Anomie.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptFeb 7 2016, 10:36 PM

Change 269066 had a related patch set uploaded (by Anomie):
Don't try to auto-create users when MW_NO_SESSION is defined

https://gerrit.wikimedia.org/r/269066

If you don't have php compiled with mysql support, you get an exception about no valid db driver for mysql. The backtrace suggests that mw-config/index.php calls includes/WebStart.php, which calls SessionManager::getGlobalSession(), which calls SessionManager->generateSessionId(), which calls BagOStuff->get(string), which eventually goes to SqlBagOStuff->getDB(integer), which explodes.

More specifically, from Setup.php line 799, which is the auto-creation. Installation works fine for me with https://gerrit.wikimedia.org/r/269066.

If you do have mysql support compiled in, no exception, but you quickly run into "Your session data was lost! Check your php.ini and make sure session.save_path is set to an appropriate directory. " (I imagine because there is no db set up yet).

I can't reproduce this, an install works fine for me. The installer sets MW_NO_SESSION which avoids using SessionManager for sessions.

Johsthao closed this task as a duplicate of T126250: <spam>.Feb 8 2016, 6:24 PM
matmarex reopened this task as Open.Feb 8 2016, 6:32 PM

Change 269066 merged by jenkins-bot:
Don't try to auto-create users when MW_NO_SESSION is defined

https://gerrit.wikimedia.org/r/269066

Anomie set Security to None.

Even after that patch, I still get "Your session data was lost! Check your php.ini and make sure session.save_path is set to an appropriate directory. " from the installer, which I do not get when I go back to pre-session manager (by pre-session manager, I mean git checkout 8aa6c9f43e9b)

Anomie added a comment.Feb 9 2016, 4:44 PM

You'll probably have to start debugging that yourself, since I can't manage to reproduce it locally. As I already noted, the installer sets MW_NO_SESSION which should avoid using SessionManager for PHP session handling. You might start by adding code to immediately die with a backtrace in MediaWiki\Session\PHPSessionHandler::install() to see if it's being activated somehow anyway.

It appears to be from User::loadDefaults and User::loadFromSession, which happen due to trying to fetch current language

SessionManager.php line 141 calls wfBacktrace()
SessionManager.php line 86 calls MediaWiki\Session\SessionManager->__construct()
WebRequest.php line 664 calls MediaWiki\Session\SessionManager::singleton()
User.php line 1114 calls WebRequest->getSession()
User.php line 386 calls User->loadDefaults()
User.php line 5089 calls User->load()
User.php line 2707 calls User->loadOptions()
RequestContext.php line 368 calls User->getOption()
StubObject.php line 204 calls RequestContext->getLanguage()
StubObject.php line 160 calls StubUserLang->_newObject()
ParserOptions.php line 595 calls StubObject->_unstub()
Installer.php line 408 calls ParserOptions->__construct()
WebInstaller.php line 138 calls Installer->__construct()
overrides.php line 68 calls WebInstaller->__construct()
index.php line 44 calls InstallerOverrides::getWebInstaller()
index.php line 39 calls wfInstallerMain()

Most obvious patch would be

diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php
index e61e2d2..7470cfd 100644
--- a/includes/installer/Installer.php
+++ b/includes/installer/Installer.php
@@ -359,7 +359,7 @@ abstract class Installer {
 	 * Constructor, always call this from child classes.
 	 */
 	public function __construct() {
-		global $wgMessagesDirs, $wgUser;
+		global $wgMessagesDirs, $wgUser, $wgLang;
 
 		// Disable the i18n cache
 		Language::getLocalisationCache()->disableBackend();
@@ -385,6 +385,8 @@ abstract class Installer {
 
 		// Having a user with id = 0 safeguards us from DB access via User::loadOptions().
 		$wgUser = User::newFromId( 0 );
+		// Determining $wgLang initiates a session.
+		$wgLang = Language::factory( RequestContext::getMain()->getRequest()->getVal( 'uselang', 'en' ) );
 
 		$this->settings = $this->internalDefaults;

But that seems a little fragile. Maybe $this->getRequest()->getSession() should watch for MW_NO_SESSION, and return some sort of dummy object in that case, but that seems a little weird too.

Anomie added a comment.Feb 9 2016, 5:55 PM

$webRequest->getSession() should be fine, unless something in the installer is trying to actually use it for session handling instead of using $_SESSION. I don't think there is any such thing.

What's more likely to cause problems for the installer is MediaWiki\Session\PHPSessionHandler::install() that actually messes with PHP's built-in session handling, which is why I suggested you look for that instead of MediaWiki\Session\SessionManager::__construct().

MediaWiki\Session\PHPSessionHandler::install() is never called, but sessions are still messed up. I honestly have no idea how they are messed up, as I can't seem to find anything that actually calls a session_* function, that is actually run.

Wait, I see why I'm confused. The session cookie is being overriden with a different cookie from the CSS file, and I was only ever looking at what got run on the main request.

So from the CSS what happens

string(1218) "<ul>
<li>SessionBackend.php line 212 calls wfBacktrace()</li>
<li>Session.php line 90 calls MediaWiki\Session\SessionBackend->resetId()</li>
<li>SessionManager.php line 975 calls MediaWiki\Session\Session->resetId()</li>
<li>SessionManager.php line 189 calls MediaWiki\Session\SessionManager->getSessionFromInfo()</li>
<li>WebRequest.php line 664 calls MediaWiki\Session\SessionManager->getSessionForRequest()</li>
<li>User.php line 1174 calls WebRequest->getSession()</li>
<li>User.php line 384 calls User->loadFromSession()</li>
<li>User.php line 5087 calls User->load()</li>
<li>User.php line 2705 calls User->loadOptions()</li>
<li>RequestContext.php line 368 calls User->getOption()</li>
<li>StubObject.php line 204 calls RequestContext->getLanguage()</li>
<li>StubObject.php line 160 calls StubUserLang->_newObject()</li>
<li>ParserOptions.php line 595 calls StubObject->_unstub()</li>
<li>Installer.php line 410 calls ParserOptions->construct()</li>
<li>WebInstaller.php line 138 calls Installer->
construct()</li>
<li>overrides.php line 68 calls WebInstaller->__construct()</li>
<li>index.php line 44 calls InstallerOverrides::getWebInstaller()</li>
<li>index.php line 39 calls wfInstallerMain()</li>
</ul>
"
string(78) "SessionBackend "{session}" metadata dirty due to ID reset (formerly "{oldId}")"

Anomie added a comment.Feb 9 2016, 6:32 PM

Unless PHPSessionHandler::isEnabled() is returning true there (so $restart is true), which it shouldn't be if MediaWiki\Session\PHPSessionHandler::install() is never called, I can't see any way that that SessionBackend::resetId() being called could be the problem.

tstarling added a subscriber: tstarling.EditedFeb 10 2016, 9:25 AM

PHPSessionHandler is not involved. Installer::overrideConfig() sets wgCookiePrefix to mw_installer, which unfortunately corresponds with the override of session_name() in WebInstaller::startSession(). User::loadFromSession() indirectly calls SessionManager::getSessionForRequest(), and getSessionInfoForRequest() iterates through the provider list without removing CookieSessionProvider from it, despite MW_NO_SESSION being set. The ID is retrieved from the cookie but isIdSafe() is false, so it's reset, which leads to a Set-Cookie header being sent with a new ID, destroying the previous installer session.

So it's a comedy of errors:

  • SessionManager and WebInstaller trying to use the same session cookie despite MW_NO_SESSION being set;
  • User::loadFromSession() being called despite the hack on Installer.php line 387 supposedly preventing that;
  • RequestContext::getLanguage() trying to get a language from the User despite the hack on mw-config/index.php line 73 supposedly preventing that.

Even if a different cookie is used, that still leaves you with a Set-Cookie header on every request, for a different random session ID each time, which seems inelegant. Maybe it's better to use a custom SessionProvider which knows a bit more about what is going on.

Change 269646 had a related patch set uploaded (by Tim Starling):
Suppress SessionManager sessions in the installer

https://gerrit.wikimedia.org/r/269646

Change 269649 had a related patch set uploaded (by Tim Starling):
In Installer, set the user to an anon in RequestContext, not just wgUser

https://gerrit.wikimedia.org/r/269649

Change 269654 had a related patch set uploaded (by Tim Starling):
In Installer, set the context language early to avoid loading from User

https://gerrit.wikimedia.org/r/269654

I tested each patch separately to confirm that they cause /mw-config/index.php?css=1 to stop sending a Set-Cookie header, so you can take your pick. I haven't fully tested the web installer with them enabled just yet.

Ah, finally I see why I couldn't reproduce it: the cookies being set by PHP's built-in session handling on my system aren't passing SessionManager::validateSessionId(), but apparently in your configurations they are. If I locally hack the validation I can reproduce this now.

Anomie closed this task as Resolved.Feb 10 2016, 3:05 PM
Anomie claimed this task.

The installer seems to work now with two of Tim's three patches merged (the third one needs one bit of cleanup), so marking this as Resolved.

Change 269649 merged by jenkins-bot:
In Installer, set the user to an anon in RequestContext, not just wgUser

https://gerrit.wikimedia.org/r/269649

Change 269654 merged by jenkins-bot:
In Installer, set the context language early to avoid loading from User

https://gerrit.wikimedia.org/r/269654

Change 269646 merged by jenkins-bot:
Suppress SessionManager sessions in the installer

https://gerrit.wikimedia.org/r/269646

Change 272815 had a related patch set uploaded (by Gergő Tisza):
Don't try to auto-create users when MW_NO_SESSION is defined

https://gerrit.wikimedia.org/r/272815

Change 272816 had a related patch set uploaded (by Gergő Tisza):
In Installer, set the context language early to avoid loading from User

https://gerrit.wikimedia.org/r/272816

Change 272823 had a related patch set uploaded (by Gergő Tisza):
Suppress SessionManager sessions in the installer

https://gerrit.wikimedia.org/r/272823

Change 272815 merged by jenkins-bot:
Don't try to auto-create users when MW_NO_SESSION is defined

https://gerrit.wikimedia.org/r/272815

Change 272816 merged by jenkins-bot:
In Installer, set the context language early to avoid loading from User

https://gerrit.wikimedia.org/r/272816

Change 272823 merged by jenkins-bot:
Suppress SessionManager sessions in the installer

https://gerrit.wikimedia.org/r/272823