Просмотр исходного кода

Warn during session regenerate fail (#9311)

Might provide more logs for e.g. https://github.com/FreshRSS/FreshRSS/discussions/9305
Alexandre Alapetite 6 часов назад
Родитель
Сommit
ad4723cb1f

+ 25 - 6
app/Controllers/authController.php

@@ -152,7 +152,15 @@ class FreshRSS_auth_Controller extends FreshRSS_ActionController {
 			);
 			if ($ok) {
 				// Set session parameter to give access to the user.
-				Minz_Session::regenerateID('FreshRSS');
+				try {
+					Minz_Session::regenerateID('FreshRSS');
+				} catch (RuntimeException $e) {
+					Minz_Log::error("Session could not be regenerated during login for user={$username}, ip_address={$ip_address}: {$e->getMessage()}");
+					header('HTTP/1.1 500 Internal Server Error');
+					Minz_Request::setBadNotification(_t('install.session.nok'));
+					Minz_Request::forward(['c' => 'auth', 'a' => 'login'], false);
+					return;
+				}
 				Minz_Session::_params([
 					Minz_User::CURRENT_USER => $username,
 					'passwordHash' => FreshRSS_Context::userConf()->passwordHash,
@@ -213,10 +221,17 @@ class FreshRSS_auth_Controller extends FreshRSS_ActionController {
 				)) {
 				Minz_Request::setBadNotification(_t('feedback.auth.login.invalid'));
 			} else {
-				Minz_Session::regenerateID('FreshRSS');
-				Minz_Session::_param('lastReauth', time());
-				Minz_Request::forward($redirect, true);
-				return;
+				try {
+					Minz_Session::regenerateID('FreshRSS');
+					Minz_Session::_param('lastReauth', time());
+					Minz_Request::forward($redirect, true);
+					return;
+				} catch (RuntimeException $e) {
+					Minz_Log::error("Session could not be regenerated during reauthentication! {$e->getMessage()}");
+					Minz_Session::_param('lastReauth', 0);
+					header('HTTP/1.1 500 Internal Server Error');
+					Minz_Request::setBadNotification(_t('install.session.nok'));
+				}
 			}
 		}
 		FreshRSS_View::prependTitle(_t('gen.auth.reauth.title') . ' · ');
@@ -231,7 +246,11 @@ class FreshRSS_auth_Controller extends FreshRSS_ActionController {
 			invalidateHttpCache();
 			FreshRSS_Auth::removeAccess();
 			Minz_Session::_param('csrf', false);
-			Minz_Session::regenerateID('FreshRSS');
+			try {
+				Minz_Session::regenerateID('FreshRSS');
+			} catch (RuntimeException $e) {
+				Minz_Log::error('Session could not be regenerated during logout! ' . $e->getMessage());
+			}
 			Minz_Request::good(
 				_t('feedback.auth.logout.success'),
 				[ 'c' => 'index', 'a' => 'index' ],

+ 7 - 1
app/Controllers/userController.php

@@ -198,7 +198,13 @@ class FreshRSS_user_Controller extends FreshRSS_ActionController {
 					return;
 				}
 
-				Minz_Session::regenerateID('FreshRSS');
+				try {
+					Minz_Session::regenerateID('FreshRSS');
+				} catch (RuntimeException $e) {
+					Minz_Log::error('Session could not be regenerated during password change! ' . $e->getMessage());
+					Minz_Request::bad(_t('install.session.nok'), ['c' => 'user', 'a' => 'profile']);
+					return;
+				}
 			}
 
 			if (FreshRSS_Context::systemConf()->force_email_validation && empty($email)) {

+ 18 - 9
lib/Minz/Session.php

@@ -15,8 +15,7 @@ class Minz_Session {
 
 	public static function lock(): bool {
 		if (!self::$volatile && !self::$locked) {
-			session_start();
-			self::$locked = true;
+			self::$locked = session_start();
 		}
 		return self::$locked;
 	}
@@ -209,26 +208,36 @@ class Minz_Session {
 
 	/**
 	 * Regenerate a session id.
+	 *
+	 * @throws RuntimeException if the session could not be regenerated (e.g. unwritable session storage)
 	 */
 	public static function regenerateID(string $name): void {
-		if (self::$volatile || self::$locked) {
+		if (self::$volatile) {
 			return;
 		}
+		if (self::$locked) {
+			throw new RuntimeException('Session is locked!');
+		}
 		// Ensure that regenerating the session won't send multiple cookies so we can send one ourselves instead
 		ini_set('session.use_cookies', '0');
-		session_name($name);
-		session_start();
-		session_regenerate_id(true);
+		if (session_name($name) === false || !session_start()) {
+			throw new RuntimeException("Session {$name} could not be started!");
+		}
+		if (!session_regenerate_id(delete_old_session: true)) {
+			throw new RuntimeException('Session could not be regenerated!');
+		}
 		session_write_close();
 		$newId = session_id();
 		if ($newId === false) {
-			Minz_Error::error(500);
-			return;
+			throw new RuntimeException('Session ID could not be retrieved!');
 		}
 		$params = session_get_cookie_params();
 		$params['expires'] = $params['lifetime'] > 0 ? time() + $params['lifetime'] : 0;
 		unset($params['lifetime']);
-		setcookie($name, $newId, $params);
+		if (!setcookie($name, $newId, $params)) {
+			throw new RuntimeException('Failed to set session cookie!');
+		}
+		return;
 	}
 
 	public static function deleteLongTermCookie(string $name): void {

+ 38 - 0
tests/app/Minz/SessionTest.php

@@ -0,0 +1,38 @@
+<?php
+declare(strict_types=1);
+
+final class SessionTest extends \PHPUnit\Framework\TestCase {
+
+	private string $originalSavePath;
+
+	#[\Override]
+	protected function setUp(): void {
+		$this->originalSavePath = (string)ini_get('session.save_path');
+	}
+
+	#[\Override]
+	protected function tearDown(): void {
+		ini_set('session.save_path', $this->originalSavePath);
+	}
+
+	public function testRegenerateIDOnHealthyStorage(): void {
+		$previous = $_SESSION ?? [];
+
+		Minz_Session::regenerateID('FreshRSS');
+
+		$_SESSION['probe'] = 'ok';
+		self::assertSame('ok', $_SESSION['probe']);
+		$_SESSION = $previous;
+		session_write_close();
+	}
+
+	public function testRegenerateIDOnBrokenStorage(): void {
+		$save_path = sys_get_temp_dir() . '/frss_test_sessions_' . bin2hex(random_bytes(4));
+		self::assertNotFalse(mkdir($save_path, 0700));
+		$broken_path = $save_path . '/missing_subdir';
+		ini_set('session.save_path', $broken_path);
+
+		$this->expectException(RuntimeException::class);
+		Minz_Session::regenerateID('FreshRSS');
+	}
+}