session: Follow-up session store protection (part 1.1)
Why:
- Reviews for part 1 of the work that needed some improvements.
What:
- The patch improves part 1 of the patch and did some cleanup in
places necessary. Applied some nit from code review comments etc.
- Added `setLogger()` in the SessionStore interface for logging and
monitoring.
Bug: T394075
Change-Id: I9f9623ab26a96a5987ae799af627b15793ca1db4
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index ee5f875..737219c 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -2188,7 +2188,8 @@
$mainConfig = $services->getMainConfig();
return new SingleBackendSessionStore(
- $objectCacheFactory->getInstance( $mainConfig->get( MainConfigNames::SessionCacheType ) )
+ $objectCacheFactory->getInstance( $mainConfig->get( MainConfigNames::SessionCacheType ) ),
+ LoggerFactory::getInstance( 'session' ),
);
},
diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php
index e164983..8f8779b 100644
--- a/includes/session/SessionBackend.php
+++ b/includes/session/SessionBackend.php
@@ -296,6 +296,7 @@
*/
public function resetId() {
if ( $this->provider->persistsSessionId() ) {
+ $oldSessionInfo = $this->getSessionInfo();
$oldId = (string)$this->id;
$restart = $this->usePhpSessionHandling && $oldId === session_id() &&
PHPSessionHandler::isEnabled();
@@ -330,11 +331,6 @@
'reason' => 'ID reset',
] );
- // Get session info object of the old session ID
- $oldSessionInfo = new SessionInfo(
- SessionInfo::MIN_PRIORITY, [ 'id' => $oldId, 'idIsSafe' => true ]
- );
-
// Delete the data for the old session ID now
$this->sessionStore->delete( $oldSessionInfo );
}
@@ -410,8 +406,7 @@
] );
// Delete the session data, so the local cache-only write in
// self::save() doesn't get things out of sync with the backend.
- $info = $this->getSessionInfo();
- $this->sessionStore->delete( $info );
+ $this->sessionStore->delete( $this->getSessionInfo() );
$this->autosave();
}
@@ -868,9 +863,8 @@
}
$flags = $this->persist ? 0 : CachedBagOStuff::WRITE_CACHE_ONLY;
- $info = $this->getSessionInfo();
$this->sessionStore->set(
- $info,
+ $this->getSessionInfo(),
[
'data' => $this->data,
'metadata' => $metadata,
diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php
index 661280b..575d33d 100644
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -147,7 +147,6 @@
$this->setHookContainer( $hookContainer );
$this->objectFactory = $objectFactory;
$this->sessionStore = $sessionStore;
-
$this->proxyLookup = $proxyLookup;
$this->urlUtils = $urlUtils;
$this->userNameUtils = $userNameUtils;
@@ -515,14 +514,8 @@
}
}
- // https://www.php.net/manual/en/session.configuration.php#ini.session.gc-divisor
- // Doing this here because of how Session::gc() works in PHP session handler. The
- // only difference here is that this is done at the end of the request rather than
- // the beginning. But we want to preserve behavior for 1 in every 100 requests.
- if ( random_int( 1, 100 ) === 1 ) {
- // Do any garbage collection if we have expired entries
- $this->sessionStore->shutdown();
- }
+ // Do any garbage collection if we have expired entries
+ $this->sessionStore->shutdown();
}
/**
@@ -599,18 +592,17 @@
*/
private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) {
$blob = $this->sessionStore->get( $info );
- $oldInfo = $info;
// If we got data from the store and the SessionInfo says to force use,
// "fail" means to delete the data from the store and retry. Otherwise,
// "fail" is just return false.
if ( $info->forceUse() && $blob !== false ) {
- $failHandler = function () use ( $oldInfo, &$info, $request ) {
+ $failHandler = function () use ( &$info, $request ) {
$this->logSessionWrite( $request, $info, [
'type' => 'delete',
'reason' => 'loadSessionInfo fail',
] );
- $this->sessionStore->delete( $oldInfo );
+ $this->sessionStore->delete( $info );
return $this->loadSessionInfoFromStore( $info, $request );
};
} else {
diff --git a/includes/session/SessionStore.php b/includes/session/SessionStore.php
index bfc1928..eca4624 100644
--- a/includes/session/SessionStore.php
+++ b/includes/session/SessionStore.php
@@ -2,6 +2,8 @@
namespace MediaWiki\Session;
+use Psr\Log\LoggerAwareInterface;
+
/**
* This is a session store abstraction layer, which can be used to read
* and write sessions to configured backend(s). Backends can be single or
@@ -12,21 +14,13 @@
* the relevant store to fetch, write, or delete content from. Sessions
* can be strongly persistent or weakly persistent based on their type.
*
- * Authenticated sessions have to persist longer based on a TTL, while
- * anonymous sessions are not guaranteed to persist for the duration of
- * the TTL. They'll be regenerated if not available in the backing store.
- *
- * Garbage collection of anonymous sessions is mainly done by the backing
- * store while for authenticated sessions that are expired, it's done when
- * the request is about to be completed by the SessionManager.
- *
* @ingroup Session
* @since 1.45
*/
-interface SessionStore {
+interface SessionStore extends LoggerAwareInterface {
/**
- * Retrieves the session data for a given session ID. Can
+ * Retrieves the session data for a given session ID. Should
* return false if the session is not found.
*
* @param SessionInfo $sessionInfo
@@ -53,9 +47,7 @@
public function delete( SessionInfo $sessionInfo ): void;
/**
- * Perform various cleanup and instrumentation activities on the
- * session store object like: garbage collection (delete expired
- * entries), do logging and monitoring, etc.
+ * Will be called during shutdown.
*
* @return void
*/
diff --git a/includes/session/SingleBackendSessionStore.php b/includes/session/SingleBackendSessionStore.php
index ba4ffbf..79f43a4 100644
--- a/includes/session/SingleBackendSessionStore.php
+++ b/includes/session/SingleBackendSessionStore.php
@@ -21,7 +21,8 @@
namespace MediaWiki\Session;
-use MediaWiki\Logger\LoggerFactory;
+use Psr\Log\LoggerAwareTrait;
+use Psr\Log\LoggerInterface;
use Wikimedia\ObjectCache\BagOStuff;
use Wikimedia\ObjectCache\CachedBagOStuff;
@@ -35,15 +36,22 @@
*/
class SingleBackendSessionStore implements SessionStore {
+ use LoggerAwareTrait;
+
private CachedBagOStuff $store;
public function __construct(
BagOStuff $store,
+ LoggerInterface $logger
) {
+ $this->setLogger( $logger );
$this->store = $store instanceof CachedBagOStuff ? $store : new CachedBagOStuff( $store );
- LoggerFactory::getInstance( 'session' )->debug(
- 'SessionManager using store ' . get_class( $this->store )
- );
+ $this->logger->debug( 'SessionManager using store ' . get_class( $this->store ) );
+ }
+
+ /** @inheritDoc */
+ public function setLogger( LoggerInterface $logger ): void {
+ $this->logger = $logger;
}
/**
@@ -87,7 +95,13 @@
* @inheritDoc
*/
public function shutdown(): void {
- $this->store->deleteObjectsExpiringBefore( wfTimestampNow() );
+ // https://www.php.net/manual/en/session.configuration.php#ini.session.gc-divisor
+ // Doing this here because of how Session::gc() works in PHP session handler. The
+ // only difference here is that this is done at the end of the request rather than
+ // the beginning. But we want to preserve behavior for 1 in every 100 requests.
+ if ( random_int( 1, 100 ) === 1 ) {
+ $this->store->deleteObjectsExpiringBefore( wfTimestampNow() );
+ }
}
/**
diff --git a/tests/phpunit/includes/session/BotPasswordSessionProviderTest.php b/tests/phpunit/includes/session/BotPasswordSessionProviderTest.php
index 777c7af..bd4a2e0 100644
--- a/tests/phpunit/includes/session/BotPasswordSessionProviderTest.php
+++ b/tests/phpunit/includes/session/BotPasswordSessionProviderTest.php
@@ -63,12 +63,6 @@
$this->configHash = $configHash;
}
- $this->overrideConfigValues( [
- MainConfigNames::CookiePrefix => 'wgCookiePrefix',
- MainConfigNames::EnableBotPasswords => true,
- MainConfigNames::SessionProviders => $sessionProviders,
- ] );
-
$manager = new SessionManager(
new MultiConfig( [ $this->config, $this->getServiceContainer()->getMainConfig() ] ),
new NullLogger,
@@ -81,6 +75,8 @@
$this->getServiceContainer()->getSessionStore()
);
+ $this->setService( 'SessionManager', $manager );
+
return $manager->getProvider( BotPasswordSessionProvider::class );
}
diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
index 542f190..1d45495 100644
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -7,7 +7,9 @@
use MediaWiki\MainConfigNames;
use MediaWiki\Session\PHPSessionHandler;
use MediaWiki\Session\SessionManager;
+use MediaWiki\Session\SingleBackendSessionStore;
use MediaWikiIntegrationTestCase;
+use Psr\Log\LogLevel;
use TestLogger;
use UnexpectedValueException;
use Wikimedia\ScopedCallback;
@@ -136,7 +138,7 @@
$services->getProxyLookup(),
$services->getUrlUtils(),
$services->getUserNameUtils(),
- $services->getSessionStore()
+ new SingleBackendSessionStore( new TestBagOStuff(), $logger )
);
PHPSessionHandler::install( $manager );
$wrap = TestingAccessWrapper::newFromObject( $staticAccess->instance );
@@ -169,6 +171,10 @@
$_SESSION['AuthenticationSessionTest'] = $rand;
$expect = [ 'AuthenticationSessionTest' => $rand ];
session_write_close();
+ $this->assertSame( [
+ [ LogLevel::DEBUG, 'SessionManager using store MediaWiki\Tests\Session\TestBagOStuff' ],
+ [ LogLevel::WARNING, 'Something wrote to $_SESSION!' ],
+ ], $logger->getBuffer() );
// Screw up $_SESSION so we can tell the difference between "this
// worked" and "this did nothing"
diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php
index ed9050f7..8642c5c 100644
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -587,7 +587,7 @@
public function testSave() {
$user = static::getTestSysop()->getUser();
- $this->store = new SingleBackendSessionStore( new TestBagOStuff() );
+ $this->store = new SingleBackendSessionStore( new TestBagOStuff(), new NullLogger() );
$testData = [ 'foo' => 'foo!', 'bar', [ 'baz', null ] ];
$builder = $this->getMockBuilder( DummySessionProvider::class )
@@ -889,7 +889,7 @@
public function testRenew() {
$user = static::getTestSysop()->getUser();
- $this->store = new SingleBackendSessionStore( new TestBagOStuff() );
+ $this->store = new SingleBackendSessionStore( new TestBagOStuff(), new NullLogger() );
$testData = [ 'foo' => 'foo!', 'bar', [ 'baz', null ] ];
// Not persistent, expiring
diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php
index 5c2cd55..f8898ff 100644
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -81,7 +81,7 @@
$this->getServiceContainer()->getProxyLookup(),
$this->getServiceContainer()->getUrlUtils(),
$this->getServiceContainer()->getUserNameUtils(),
- new SingleBackendSessionStore( $this->store )
+ new SingleBackendSessionStore( $this->store, $this->logger ),
);
}
@@ -649,33 +649,6 @@
// Session already exists
$expectId = 'expected-----------------------3';
- $sessionStore = $pmanager->sessionStore;
- $blob = [ 'metadata' => [
- 'provider' => 'MockProvider2',
- 'userId' => 0,
- 'userName' => null,
- 'userToken' => null,
- ]
- ];
- $blob += [
- 'data' => [],
- 'metadata' => [],
- ];
- $blob['metadata'] += [
- 'userId' => 0,
- 'userName' => null,
- 'userToken' => null,
- 'provider' => 'DummySessionProvider',
- ];
- $expiry = $this->getServiceContainer()->getMainConfig()->get( MainConfigNames::ObjectCacheSessionExpiry );
- $info3 = new SessionInfo( SessionInfo::MIN_PRIORITY, [
- 'provider' => $provider2,
- 'id' => 'empty3--------------------------',
- 'persisted' => true,
- 'userInfo' => UserInfo::newAnonymous(),
- 'idIsSafe' => true,
- ] );
- $sessionStore->set( $info3, $expectId, $blob, $expiry );
$this->store->setSessionMeta( $expectId, [
'provider' => 'MockProvider2',
'userId' => 0,